8000 [2.1][Form] CollectionType / PropertyPath BC Break? · Issue #4158 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[2.1][Form] CollectionType / PropertyPath BC Break? #4158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
havvg opened this issue Apr 30, 2012 · 9 comments
Closed

[2.1][Form] CollectionType / PropertyPath BC Break? #4158

havvg opened this issue Apr 30, 2012 · 9 comments
Labels
BC Break Form

Comments

@havvg
Copy link
Contributor
havvg commented Apr 30, 2012

The Symfony\Component\Form\Util\PropertyPath has changes in its behavior.
My question is; whether this is intended and if so should be noted as a BC break, or whether the given object should be considered valid (maybe adding an option to force the use of the setter method):

Before

An object having the methods addOccupation and setOccupations (Propel model) worked by using setOccupations.

After

The same object is now broken, the PropertyPath::writeProperty throws an Exception due to the missing removeOccupation method. However, the setOccupations is still there, but not used anymore.
@webmozart
Copy link
Contributor

The question is how PropertyPath should behave in that case?

If we remove the exception and change it to use setOccupations unless both the add and remove method are found, it is very hard to spot why for example the add method is not used when the developer simply forgot to add the remove method as well.

@havvg
Copy link
Contributor Author
havvg commented Apr 30, 2012

That's the case, where/why I would suggest to add an option that's false by default; allowing one to force the usage of the setter, despite the fact there is only one (or even both) of the add and remove methods.

@stof
Copy link
Member
stof commented Apr 30, 2012

btw, shouldn't the by_reference option allow to change this ?

@havvg
Copy link
Contributor Author
havvg commented Apr 30, 2012

It used to be; as far as I can see this is not passed to the PropertyPath itself, only the PropertyPathMapper is using it to reference or clone the value to further determine whether an actual change is required.

@webmozart
Copy link
Contributor

@stof: Not anymore. Handling of add/remove has been moved into the PropertyPath class. Think of a form field with the following property path:

author.tags

The field maps to a collection. There are three possible approaches for modifying this collection:

  1. modify it by reference, i.e. by directly changing the collection (by_reference=true)
  2. modify it by copy, i.e. clone the collection and write it back using setTags() (by_reference=false)
  3. modify it by copy, i.e. clone the collection and merge it back using addTag()/removeTag() (also by_reference=false)

The question is how PropertyPath distinguishes between 2 and 3. Any solution to this problem necessarily introduces a new syntax that enforces usage of a setter/adder, for example

// enforce setter usage, use adders by default if present
author.s#tags
author.$tags
author.tags$

// enforce adder usage, use setter by default if present
author.@tags
author.tags/a

The problem is that we already introduced a special syntax for configuring custom singulars, which is

author.calculi|calculus

We must take care now not to introduce anything that will hurt us in the future, for example, by limiting the set of allowed property names.

@jaugustin
Copy link
Contributor

@bschussek @havvg I have a PR for Propel waiting to add remove method, but with that I got an issue with the property path :

The remove method work only for the first element, because when remove is called, the collection is modified and the foreach break.
to make it work I have to add this line in PropertyPath.php:495 :

<?php
$previousValue = is_object($previousValue)? clone $previousValue : $previousValue;

@webmozart
Copy link
Contributor

@jaugustin Could you please open a separate ticket for this? I'll look at this.

@jaugustin
Copy link
Contributor

sure

@webmozart webmozart mentioned this issue May 14, 2012
Closed
@oscargala oscargala mentioned this issue May 15, 2012
Closed
This was referenced Jul 6, 2012
Closed
Closed
@webmozart webmozart mentioned this issue Jul 22, 2012
Closed
@fabpot
Copy link
Member
fabpot commented Apr 28, 2014

Closing this old issue.

@fabpot fabpot closed this as completed Apr 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @havvg @webmozart @stof @jaugustin
0