Adder and remover sidenote #4253
Conversation
| Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | none There are cases when you only want either adder or remover, important to know both of them need to be found if any is to work.
| @@ -462,7 +462,8 @@ these new ``Tag`` objects easier (especially if you're using Doctrine, which | |||
| we talk about next!). | |||
|
|
|||
| .. caution:: | |||
|
|
|||
| Create **both** methods, otherwise none of them will be used. | |||
There was a problem hiding this comment.
Is this correct? If yes, the "and ” in the following paragraph has to be replaced by "or".
There was a problem hiding this comment.
Yes, this is correct, see https://github.com/symfony/symfony/blob/2.5/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L589
If one of them is missing, none is used.
I see 2.3 is throwing exceptions in such cases:
https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L409
but I guess this of behaviour has been considered unneeded as default set* and collection->removeElement are used in case of adder / remover absence.
Also the part "If no addTag and removeTag method is found, the form will..." is semantically and logically correct.
~(p^v) <=> ~p v ~v
There was a problem hiding this comment.
Hmm, isn't the bold and enough to know this text? If not (readers know things like this better than maintainers), I prefer to add this extra information into the first sentence of this caution. Something along the lines of "You have to create both the addTag and removeTag methods, otherwise the form will still use setTag even if by_reference is set to false."
Merged both notes into one, in accordance to discussion feedback.
|
I'd still add proposed note explicitly. Merged into one paragraph as suggested. Rationale: I think it's also because natural, intuitive expectation is adder and remover are separate overrides. |
|
@wouterj @weaverryan What do you think about this? From my point of view, this is ready to be merged. |
|
Hey @kshishkin! I think this is a way more clear than before. Thanks so much for this! |
This PR was merged into the 2.3 branch. Discussion ---------- Adder and remover sidenote | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | none There are cases when you only want either adder or remover, important to know both of them need to be found if any is to work. Commits ------- 59b932d Merging emphasized notes bdd8325 Adder and remover sidenote
There are cases when you only want either adder or remover, important to know both of them need to be found if any is to work.