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

Closed
havvg opened this Issue Apr 30, 2012 · 9 comments

Comments

Projects
None yet
5 participants
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.

Member

webmozart commented Apr 30, 2012

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.

Contributor

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.

Member

stof commented Apr 30, 2012

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

Contributor

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.

Member

webmozart commented Apr 30, 2012

@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.

Contributor

jaugustin commented May 6, 2012

@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;
Member

webmozart commented May 6, 2012

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

Contributor

jaugustin commented May 6, 2012

sure

Owner

fabpot commented Apr 28, 2014

Closing this old issue.

fabpot closed this Apr 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment