[Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper #3315

Merged
merged 3 commits into from Feb 10, 2012

Conversation

Projects
None yet
6 participants
Contributor

webmozart commented Feb 9, 2012

Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3293, #1499
Todo: -

Travis Build Status

This fixes CollectionType to properly use adders and removers. Apart from that, adders and removers now work with custom property paths. PropertyPath was extended for a method getParent.

$this->allowAdd = $allowAdd;
$this->allowDelete = $allowDelete;
- $this->useAccessors = $useAccessors;
+ $this->mergeStrategy = $mergeStrategy ?: self::MERGE_NORMAL | self::MERGE_INTO_PARENT;
@stof

stof Feb 9, 2012

Member

why using a binary flag here ? using both strategy at the same time does not make any sense (and MERGE_INTO_PARENT will win)

@webmozart

webmozart Feb 9, 2012

Contributor

It makes sense and is used in ChoiceType. MERGE_INTO_PARENT is not applied if adders/removers are not found, in this case MERGE_NORMAL is applied as fallback (if allowed).

.../Component/Form/Extension/Core/EventListener/MergeCollectionListener.php
+ */
+ private $dataSnapshot;
+
+ public function __construct($allowAdd = false, $allowDelete = false, $mergeStrategy = true, $addMethod = null, $removeMethod = null)
@vicb

vicb Feb 9, 2012

Contributor

getMethod ?

@webmozart

webmozart Feb 9, 2012

Contributor

Nope. The getter follows the normal conventions used by PropertyPath.

@webmozart

webmozart Feb 9, 2012

Contributor

Changed this now so that the property is being used instead of replicating its code here.

+
+ if (null !== $this->dataSnapshot && !is_array($this->dataSnapshot) && !($this->dataSnapshot instanceof \Traversable && $this->dataSnapshot instanceof \ArrayAccess)) {
+ throw new UnexpectedTypeException($this->dataSnapshot, 'array or (\Traversable and \ArrayAccess)');
+ }
}
public function onBindNormData(FilterDataEvent $event)
@vicb

vicb Feb 9, 2012

Contributor

What about splitting this methods in smaller chunks ?

@webmozart

webmozart Feb 9, 2012

Contributor

You really think this would improve readability?

@vicb

vicb Feb 9, 2012

Contributor

I think it could help.

@webmozart

webmozart Feb 10, 2012

Contributor

I won't invest any more time on this now. There are more important things.

Contributor

webmozart commented Feb 10, 2012

@fabpot Ready to merge.

fabpot added a commit that referenced this pull request Feb 10, 2012

merged branch bschussek/issue3293 (PR #3315)
Commits
-------

da2447e [Form] Fixed MergeCollectionListener when used with a custom property path
b56502f [Form] Added getParent() to PropertyPath
7e5104e [Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper (as happening in CollectionType)

Discussion
----------

[Form] Fixed MergeCollectionListener for the case that the form's data is updated by the data mapper

Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3293, #1499
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3293)

This fixes CollectionType to properly use adders and removers. Apart from that, adders and removers now work with custom property paths. PropertyPath was extended for a method `getParent`.

---------------------------------------------------------------------------

by bschussek at 2012-02-10T07:42:13Z

@fabpot Ready to merge.

@fabpot fabpot merged commit da2447e into symfony:master Feb 10, 2012

Contributor

webmozart commented Feb 10, 2012

By the way, there still is a bug when operating on PersistentCollections which is caused by Doctrine.

See http://www.doctrine-project.org/jira/browse/DDC-1643

+
+ // Enable support for adders/removers unless "by_reference" is disabled
+ // (explicit calling of the setter is desired)
+ if ($options['by_reference']) {
@Tobion

Tobion Feb 10, 2012

Member

I simply don't get what by_reference really means. And the name is really irritating in this context.
The doc says:

If the underlying value of a field is an object and this option is set to true, then the resulting object won't actually be set when binding the form. For example, if you have a protected author field on your underlying object which is an instance of some Author object, then if by_reference is false, that Author object will be updated with the submitted data, but setAuthor will not actually be called on the main object. Since the Author object is a reference, this only really makes a difference if you have some custom logic in your setAuthor method that you want to guarantee will be run. In that case, set this option to false.´

by_reference = true -> "resulting object won't actually be set"
by_reference = false -> "setAuthor will not actually be called"
by_reference = false -> setAuthor is not called but "that you want to guarantee will be run" (or do they mean: that you will ensure yourself that it is run (i.e. passive vs. active))

To me all three examples are contradictory. I comment this code because it astonished me that "by_reference" must be true to allow setters?

@stof

stof Feb 10, 2012

Member

by_reference => true means that the object is modified by reference, and so does not need to be set again (you modified the instance so the change is already there)

@webmozart

webmozart Feb 10, 2012

Contributor

The docs are indeed confusing. Things are much more simple.

Let's look at a simple form:

$builder = $this->createFormBuilder($article);
$builder
    ->add('title', 'text')
    ->add(
        $builder->create('author', 'form', array('by_reference' => ?))
            ->add('name', 'text')
            ->add('email', 'email')
    )

If by_reference is true, binding looks like this:

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

Note that setAuthor is not called. The author is modified by reference.

If we set by_reference to false, binding looks like this:

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

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

@Tobion

Tobion Feb 10, 2012

Member

Thanks for the examples. They should be included in the doc because it's much more understandable.

But again, at this line of code by_reference must be true to call the setters in the MergeCollectionListener. So the other way round?

@webmozart

webmozart Feb 10, 2012

Contributor

Because this line decides if

  1. the setter is used (if by_reference is false), or
  2. the add/remove-method is used (otherwise)

So, (1)

$tags = $article->getTags();
$tags[] = ...
$tags[] = ...
$article->setTags($tags);

vs. (2)

$article->addTag(...)
$article->addTag(...)
@Tobion

Tobion Feb 10, 2012

Member

Ok that makes sense. I will make a PR with your examples for the docs.

Is it possible to explain this modification?
The $originalData doesn't contain any item to be deleted, right?

Case 1: Item is added
This case is done by unsetting $itemsToAdd that exist in dataSnapshot. Remaining data will be added.

Case 2: Item is kept
This case is managed when unsetting $itemsToAdd because followed by continue 2;.

Case 3: Item is removed
$itemsToDelete[$originalKey] = $originalItem; should be enough, no?

@doctrinebot doctrinebot referenced this pull request in doctrine/doctrine2 Dec 6, 2015

Closed

DDC-1643: PersistentCollection should support clone #2285

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