Drop addFields() methods in favor of addField() returning $this

[mvorisek]

Improve method chaining. All addField methods are adding named fields, thus the result can be easily obtained with Model::getField method when needed.

also see the discussion below, especially the summary in https://github.com/atk4/data/pull/990#issuecomment-1150251646

[DarkSide666]

I didn't get what improvement it is to remove these handy short-hand methods? They are not to complex to have I guess.

[mkrecek234]

@DarkSide666 Same here - I use them a lot to save lines of code when defining Models.

[mvorisek]

I have not merged this change yet as I am not exactly sure if returning $this from addField is a good idea & consistent with other addXxx methods. Normally, addXxx methods returns the newly created object (like Field::addField currently or View::add). In 99% cases, the result of Field::addField is not used and returning $this is fine or even wanted, as storing a Field instance reference is unwanted because of possible cloning later. For justifiable cases, the result can be always obtained via getField getter.

The addFields (plural) method is in my sense redundant and mainly usefull because of the $this return for chaining. Although creating a model on runtime using a generic Model class ($m = new Model(); $m->addField();) is better to be replaced with specific class (class Xxx extends Model { /* define fields in init method */ }) where the chaining is not needed (nor wanted by CS, we are in $this scope), there are usecases in the atk4/* ecosystem (like creading a Model for a UI Form) where the Field::addField(s) chaining is usefull.

Given this reasoning, can you guys help me to understand your usage? Do you use addFields because of the chaining or because of a seed deduplication (if so, please provide an example, like if you dedup 1, 2 properties like type or required, I still consider this a negligible advance)?

[DarkSide666]

Yeah, addXxx should normally return newly created object (that's what I would normally expect from add method to return), so in addField case - return Field object. In addFields(array) case return $this of course.

I use addFields mostly not because of chaining, but to keep all field config in one place, one array. Of course I can live without such method, but ... I don't see anything bad to have it as shorthand method.

[mvorisek]

@DarkSide666, thanks for the answer. I want to simplify the API to be simple as possible, currently the addFields is misused like:

which only complicates static analysis and I would say readability as well.

For adding references or other field types (addExpression for ex.), plural method is not available neither.

I use addFields mostly not because of chaining, but to keep all field config in one place, one array.

Is your usecase/array comming from an external source? Would you need an extra foreach or you can simply rewrite your code to multiple $this->addField()?

When the addField will continue to return Field, would this complicate your code (you create a lot of inline models) or you use it mostly in Model::init() method?

@abbadon1334, @PhilippGrashoff, @mkrecek234 can you please desribe your typical usecase as well?

[mkrecek234]

@mvorisek I have two areas where i use addFields: 1) In Model::init to define a lot of fields quickly, so addFields(['name', 'created_date' => ['type' => 'date'], ...)

This saves having a lot lines of code with many addField calls.

2) When defining hasOne(...)->addFields(['created_date', 'created_by', ...])->addTitle();

This is very compact also to link and import hasOne fields quickly.

These are all use cases, and it is mostly to have readable compact code which I like a lot.

[mvorisek]

@DarkSide666 and @mkrecek234 thank you, you confirmed my feelings about Xxx::addFields().

⚠️ 1. almost all uses are in Model::init() where chaining for $this is not needed

⚠️ 2. chaining is however needed for references.

So until a special operator for chaining is implemented in php-src (or we add some method like function chain(\Closure $fx) { $fx($this); return $this; }, which would be elegant, but writing ->chain(fn (Model $m) => $m->addField('name', ['type' => ...])) seems to complicated for frequent use) Reference::addFields() must stay if type of Xxx::addField() should stay.

So the only (sensible) option is to change the return type of Xxx::addField() to $this. This would keep the Reference::addField() method chainable (and Field::addField() as well, but that is not (strictly) needed).

This is also how this PR is designed. It can be slighly inconsistent as most addXxx methods return not $this, but I belive it is only the first impression. View::add() adds an UNNAMED object/seed and it is strictly needs to return the resulting/added object. However, Field::addField accepts a name as 1st parameter, and the return can be easily obtained if needed. Using scalar names (preferably via hintable magic) is also IMHO prefered if possible, as when Field object is passed to some Model subelement, it results in badly linked object when cloned (see https://github.com/atk4/data/pull/711 for more).

So I am adding in review tag to keep this PR here for a while and please let me to know if the proposed change - drop Xxx::addFields() in favor of Xxx::addField() returning $this (to keep the chainability primary for Reference) would suite your usage well.

[DarkSide666]

Well, I can live with that of course :) But having addFields in Reference is even more usable like @mkrecek234 said above. And if it's there, then why not in Model itself?

Also returning Field object was sometimes used like $field = $this->addField(...) and then do something more with $field. Of course you can call $field = $this->getField(...) later on, but that's another code line which looks strange if used right after addField line ;)

[mvorisek]

Maybe someday, thank you guys for the feedback.