[Form][Collection] Collection type is broken

[stof]

The collection type is broken when using it for the inversed side of a Doctrine relation as it modifies the collection instead of calling some setter in the class. As Doctrine does not track changes in the inversed side, the changes are not persisted.

The best practice for Doctrine is to call the setter of the owning side in the setter of the inversed side. But as the setter is not called, this has no effect for the forms. And there is currently no way to change this behavior in user-land forms.

I suggested in #1141 to allow defining the setter used. A collection type should then call addXXX and removeXXX on the object instead of modifying the collection to allow having additionnal logic when adding and removing elements.

[j]

I've posted around for this issue so many times and it's been a mystery to everyone. I noticed the setter on collections calls the wrong one (if I can remember correctly, it calls the setter of whatever the Data Class is set to in the collection type instead of working with an owning side to allow persist to work correctly). Also, since there isn't an owning side, it's very hard to modify and remove elements. I currently have to grab all matched elements from the table, remove them, then add new ones on 1:M / M:1 collections. It's a pain in the ___. :(

If this can be fixed, it would be a huge fix.

[stof]

@jstout24 it does not call any setter. It modify the collection directly.

[stof]

Note that the issue impacts the EntityType used with multiple set to true as it deals with a collection in this case.

[j]

@stof yep, I understand. My current project has about 3 forms with a collection type utilizing multiple => true... The form handling is a pain.

[j]

#1461 -- the right direction? =\

[stof]

@jstout24 no, the issue is different. #1461 is about creating the child field. This one is about binding the submitted values.

[j]

Gotcha, I barely looked at it before I headed to lunch.

[blauwers]

Related issue discussed here http://forum.symfony-project.org/viewtopic.php?f=23&t=35914

[beberlei]

I agree adding add and remove methods would help manage bi-directional relationships here.

[j]

+1

[tystr]

I'm glad this issue is being addressed :)

+1

[fabpot]

@stof: are you working on a patch?

[marcw]

In the case of the deletion of a relation, the Form component clearly does the job by unsetting the object in the PersistentCollection object. @beberlei Could Doctrine2 propagates the changes?

[beberlei]

@marcw no, its the inverse side of the association. No Propagation by design.

[fabpot]

see #1745

[acasademont]

@marcw did you find some time to work on another patch? May i help you? I'm also stuck with this issue because setters of the parent Entity are not being called.

[marcw]

Well... I thought that it was better for me to sleep a bit on it in order to find a creative solution. I think that it would the easiest solution would be to implement snapshots in Doctrine's ArrayCollection. @beberlei Is that possible or is it just plain wrong?

[stof]

@marcw The change has not to be done in Doctrine but in the Form component. The only way to trigger the logic implemented by the user to update the owning side from the inversed side is to call the setters of the entity instead of modifying the collection.

[marcw]

@stof As we already talk about this on IRC, I think you have a really good and deep understanding of this issue. Could you propose an implementation?

[stof]

@marcw The issue is that I don't know well enough how the binding works in the Form framework. I fear that this needs some very special handling that cannot be done as is.

[acasademont]

The furthest i could go is at the PropertyPathMapper class where there is a variable "isReference" that compares the form data with the ¿previous? data. But in case of that collection the previous data and the submitted one are the same (bizarre!) so the isReference var is always true. That leads to the $propertyPath->setValue never being called.

Any suggestions or workarounds to have my many to many forms working?

[marcw]

Do we have a solution there? http://sf.khepin.com/2011/08/inconsistent-behavior-of-symfony2s-form-framework/#comment-1172

[webmozart]

This behaviour is intentional. When the form framework modifies related objects, it obtains the object (for example through getTags()) and the modifies it by reference without writing it back using setTags(). If you want to enforce the setter to be called, you need to set the option "by_reference" of the "tags" field to false (the default is true).

Probably the default value of "by_reference" had better been false (to avoid these headaches), but this would be a non-BC change now.

Automatically calling a addTag() and removeTag() in the case of collections would be nice, but difficult to implement.

[keymaster]

Probably the default value of "by_reference" had better been false (to avoid these headaches), but this would be a
non-BC change now.

Has the form api been declared stable ?

[fabpot]

@keymaster: no

[jamhall]

Any update on this?

[acasademont]

Not that i know :(

On Wed, Sep 14, 2011 at 08:56, Flukey <
reply@reply.github.com>wrote:

Any update on this?

Reply to this email directly or view it on GitHub:
#1540 (comment)

[daFish]

I just came across this "behaviour" and its quite frustrating. If the form API hasn't been finalized, the fix for setting "by_reference" to false as the default is plausible unless there is an overall fix.

[Shreef]

I was talking to @stof today on IRC about this problem.

I just wanted to add that even if you set by_reference = False. the collection in the Entity will get updated the old way , then another call to the Entity's setXX() method will be made and the same data will get passed again.

In my case, This is a problem as I'm using this in a form that edits a pre-existing Entity that has sub-entities in a collection. Form replaces the old sub-entities in the collection with new ones using the data submitted, so this removes my old sub-entities and this isn't a valid logic.

[kmohrf]

@Shreef yupp. i can confirm this behaviour and it pretty much ruins my hope of updating doctrine relations from the inverse side as i will never have any chance of knowing which entities were assigned before in my object. huuuge bummer.

+1 for getting this fixed

[keymaster]

This is great news.

Is it feasible to back propagate this to 2.0x?

[stloyd]

@keymaster New features are never backported.

[keymaster]

if it is possible technically to back port it (it may not be, and then there is nothing to talk about), this one might be worth doing.

If we're saying it's not a bug, then the documentation for 2.0x needs to reflect a different way of processing forms from the inverse side.

[stof]

fixing the issue is part of the refactoring of the forms done for 2.1. I don't think it can be backported cleanly.

[blauwers]

Is there a recommended revision for testing the fix / form code? Or is head it?

On Feb 5, 2012, at 2:40 PM, Christophe Coevoet reply@reply.github.com wrote:

fixing the issue is part of the refactoring of the forms done for 2.1. I don't think it can be backported cleanly.

---

Reply to this email directly or view it on GitHub:
#1540 (comment)

[stof]

it is the master branch. But take care that many bundles are not updated yet for the changes done in Form and Validator (including DoctrineMongoDBBundle and SonataAdminBundle for instance)

[webmozart]

Actually it should be possible to backport this by backporting MergeCollectionListener, MergeDoctrineCollectionListener and the changes in ResizeFormListener. AFAIK this should not depend on the changes in the choice lists.

@stof: Do you have time to try this?

[RapotOR]

Is the CollectionType working fine with entities on master branch?
I dont know if I am doing something wrong but my collections are empty after bindRequest even if something is submitted.

Little example with AcmePizzaBundle: https://github.com/RapotOR/AcmePizzaBundle/blob/master/Controller/OrderController.php#L111
If I edit an order with 2 pizzas, and I submit it without changing data: $order->getItems()->count() == 0 on line 111 after the bindRequest.

It could be great to have unit tests for the CollectionType with entities in Doctrine bridge!

[stof]

@RapotOR @bschussek discovered a bug in the Doctrine ORM about cloning the persistent collections. This is the reason of the issue and should be fixed in Doctrine soon

[RapotOR]

@stof Ok! Thanks for the light!

[stof]

to be exact, this bug probably affects other doctrine projects too as they basically copied the way the ORM did things

[RapotOR]

@stof
Regarding the cloning, ok. I patched with __clone waiting the Doctrine fix.

When I add a new item to the collection (allow_add activated), there is a issue :

     Catchable Fatal Error: Argument 1 passed to Acme\PizzaBundle\Entity\Order::addItem() must be an instance of Acme\PizzaBundle\Entity\OrderItem, array given

The new item is an array and not OrderItem. (data_class is well defined).

What is your opinion regarding unit tests in doctrine bridge for CollectionType with entities?

[RapotOR]

Succeeded using:

    $builder->add('items', 'collection', array(
        'type'      => new OrderItem(),
        'prototype' => true,
        'allow_add' => true,
        'allow_delete' => true,
        'options' => array('data_class' => 'Acme\PizzaBundle\Entity\OrderItem'),
    ));

[maciej-pyszynski]

Current version (62100f1), still behave BAD.
It's cool you added addXXX and removeXXX methods on by_reference => false, BUT this should be a choice.
You consistently ignore one important use case - reordering.
Easy example: I got images, sortable by UI and I want save them in order which come with POST, in version 2.0 I was able to hold logic of ordering in setImages() method, but now it uses addImage and break the logic. I don't see any way to choose setImages() as setter.

If there is a way to set images directly by setImages(), please update the docs for collection type.

[acasademont]

setImages will be called if no addXXX nor removeXXX are present and by_reference is set to false :)

[maciej-pyszynski]

This is sill bad, since I have no option to directly set the setter. My class method setImages() use addImage() as implementation of DRY idea.

[acasademont]

So...i'm afraid you'll have to rename your addImage to addManualImage or something

[maciej-pyszynski]

This is just... awkward. Auto magic methods are cool, but user have to have control on them.
What my dev team members thought will be after spotting addManualImage() method? Probably: What the heck is manual image? Is it in business model anywhere?

To make SF user friendly you should allow to choose setter directly in options, not force them to give methods misleading names...

[stof]

guys, please open a new issue if you want to report an issue. We don't track comments on closed issues when we want to know what are the pending reports.

[maciej-pyszynski]

#4709

[mvrhov]

I'm going to assume that I don't have to track the changes myself anymore as outlined in the doctrine persistence section for collection in 2.1 and master.
Now the addFoo($foo) on oneTomany (aka inverse side) is pretty easy the code beloww and cascade persist option set on the inverse side works as expected.

public function addFoo($foo)
{
  $this->foo[] = $foo;
  $foo->setParent($this); 
}

But I'm really unable to figure out what's the removeFoo($foo) method supposed to look like

public function addFoo($foo)
{
  $foo->setParent(null); 
  $this->foo->removeElement($foo);
}

Doesn't work as expected. Because the record in the DB stays just the relation column is set to null. Cascade remove option on the inverse side doesn't seem to do anything

[stof]

@mvrhov Removing a relation is not meant to remove the element in all cases. The OneToMany is about the relation, not about the related element.
If you want that Doctrine removes the related element when the relation is removed, configure the orphan-removal on your doctrine association

[mvrhov]

Thanks. This should really be in the documentation..

[webmozart]

@mvrhov Would you mind opening a PR on symfony/symfony-docs fixing the documentation accordingly?