[Form] by_reference talks about setters?!

[flip111]

There is a setting here called by_reference. It does actually control by reference or not, but then the explanation is that a setter is not called. What does that even have to do with a setter. You can keep by reference and do the setter at the same time.

$article->setTitle('...');
$author->setName('...');
$author->setEmail('...');
$article->setAuthor($author);

[weaverryan]

If look, the docs say this:

If by_reference is true, the following takes place behind the scenes when you call submit() (or handleRequest()) on the form:

$article->setTitle('...');
$article->getAuthor()->setName('...');
$article->getAuthor()->setEmail('...');

The key part is "following takes place behind the scenes when you call submit" - we're trying to describe to you what the form system does when you change this setting.

Does that make sense?

[flip111]

To me it doesn't, there is one option named after 1 behavior controlling 2 behaviors. This would make sense to me:

call_setter = true, by_reference = true

$article->setTitle('...');
$author->setName('...');
$author->setEmail('...');
$article->setAuthor($author);

call_setter = false, by_reference = true

$article->setTitle('...');
$article->getAuthor()->setName('...');
$article->getAuthor()->setEmail('...');

call_setter = true, by_reference = false

$article->setTitle('...');
$author = clone $article->getAuthor();
$author->setName('...');
$author->setEmail('...');
$article->setAuthor($author);

call_setter = false, by_reference = false

$article->setTitle('...');
$author = clone $article->getAuthor();
$author->setName('...');
$author->setEmail('...');
(new \ReflectionProperty::__construct('Namespace\Article', 'Author'))->setValue($article , $author);

My problem with the documentation in specific is that it's focused on the setter behavior which is not in line with the actual property name and behavior you can see in the code.

So, all that by_reference=false really does is force the framework to call the setter on the parent object.

No it doesn't ... it clones your object and your spl_object_hash changes. Which is very obvious from the CODE but not from the text.

The use case for the setter is explained very well, namely you want it to be called when filling the objects with the form data. There is no mention about what problem the cloning is suppose to solve. I showed you don't need to clone to call the setter anyway.

I made my point here i think, i can't do a PR for the docs because i don't understand the implementation. If you want to close it it's ok.

[weaverryan]

Hmm, the clone is indeed confusing - that was added later in #7569. @HeahDude can you help out? Basically, I wrote:

So, all that by_reference=false really does is force the framework to call the setter on the parent object.

Then later you added the clone in the code. Is the object really cloned? If so, why?

[flip111]

I think "by reference" was originally named by the fact that you work with a variable (property of the object). The not "by reference" would be calling explicit the setter. This is different than the PHP meaning of "by reference". I vaguely remember reading about this option some years ago already noticing this difference. Now you have heahdude doing a patch to make it "by reference" in the php sense. Which is how i would understand it as well (by the name by_reference), it's just poorly chosen and should be called use_setter and then revert patch #7569 it will be a BC break though.

[HeahDude]

Hmm, the clone is indeed confusing - that was added later in #7569. @HeahDude can you help out? Basically, I wrote:

So, all that by_reference=false really does is force the framework to call the setter on the parent object.
Then later you added the clone in the code. Is the object really cloned? If so, why?

@flip111 Yes this is intended and this is tied to cloning, due to how PHP references work.

What could be more explicit IMO is that by_reference is an option to set the behavior of nested object of the underlying data.

Indeed, when an article has an object author, its initial data comes from the article getter, but when (and if) an Author object is modified on submission, there is no need to call setAuthor() back to the article, because the author is modified by reference and the article already has that reference.

[HeahDude]

This is why we need to set by_reference false on collection, calling a setter would not make sense either, but if we want adder/remover to be called, we have to clone it so they are called.

[flip111]

@HeahDude

Yes this is intended and this is tied to cloning, due to how PHP references work.

What i'm saying was that by_reference did not mean php reference originally and it has been like that for years.

[HeahDude]

I don't get the issue, what exactly do you think that should be changed in the doc? Because changing the option name has no sense since this is how it works now.

The core feature should not be discussed here, but please note that even the root data can be cloned too if the by_reference option is false. I even use this in an application so that form->getData() returns a clone in the controller when valid instead of the initial data passed to the factory method.

This is a valid use case IMO and we should not break BC here without good reasons.

[weaverryan]

I'm also a little confused by what should be changes in the docs. @flip111 I don't mean that in a "your wrong" kind of way :). I think something in the docs is confusing, but I don't quite understand. Do you have a problem with the name of the feature itself (by_reference)? Or how it's explained? If it's how it's explained, could you give an example of what could change in the docs to make it more clear?

Cheers!

[flip111]

For the docs i really would like to change this line at least

So, all that by_reference=false really does is force the framework to call the setter on the parent object.

and if the #7569 stays like it is i would like to put more emphasizes that at least now by_reference has the same meaning as php reference regardless of that i didn't think it was like this in the past.