Child deletion issue in many-to-one form

[pimpreneil]

Hi,

I am experiencing a very problematic issue on a form submission involving a many-to-one relationship:

Here is what I want to achieve:
I have a company with users attached to it. I want to be able to add / update / delete users in the company form.

Schema:

<database name="default" namespace="Acme\Bundle\BlogBundle\Model">
    <table name="company">
        <column name="id" type="integer" required="true" primaryKey="true" autoIncrement="true" />
        <column name="name" type="varchar" required="false" primaryString="true" />
    </table>                                           
    <table name="user">
        <column name="id" type="integer" required="true" primaryKey="true" autoIncrement="true" />
        <column name="company_id" type="integer" required="true" />
        <column name="firstname" type="varchar" required="false" primaryString="true" />

        <foreign-key foreignTable="company" onDelete="CASCADE" onUpdate="CASCADE">
            <reference local="company_id" foreign="id"/>
        </foreign-key>
    </table>
</database>

Controller:

...
public function companySaveAction(Request $request)
{
    $company = CompanyQuery::create()->findOne();
    $formObj = $this->createForm('company_type', $company);
    $formObj->handleRequest($request);

    if ($formObj->isValid()) {
        $company->save();

        return $this->redirect($this->generateUrl('acme_blog_company'));
    }

    return array('form' => $formObj->createView());
}
...

Form type:

...
public function buildForm(FormBuilderInterface $builder, array $options)
{
    $builder->add('name');
    $builder->add('users', 'collection', array(
        'type' => 'user_type',
        'allow_add' => true,
        'allow_delete' => true,
        'by_reference' => false
    ));
    $builder->add('save', 'submit');
}
...

My issue is that on first form submission, everything works fine: The users are created properly and the company name is updated as it should.
But if I submit my form again (without deleting the users, just changing their name), they get deleted.
After that, I can add new users, they get created and if I submit the form again, they disappear, etc...

Here is a sample of the request parameters sent to the controller (we are here only working in a company edition scenario to avoid confusion):

First request:

company_type[name]:Main company
company_type[users][0][firstname]:user 1
company_type[users][1][firstname]:user 2
company_type[save]:
company_type[_token]:...

=> All the users are created just fine

Second request:

company_type[name]:Main company
company_type[users][0][firstname]:user 1 edited
company_type[users][1][firstname]:user 2 edited
company_type[save]:
company_type[_token]:...

=> All the users are gone. Note: I have the same result, even if I don't change the names (I have the issue even if I simply resend the same form).

This issue seems to be linked to the fact that in the second request, the company.removeUser method is called => the users are removed instead of being updated.

[pimpreneil]

After some deeper inspection, I think that I have found the origin of the problem:

This row https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L467 checks if the child object(s) already linked to the parent are equal to the ones we are storing instead. If there is a match, then the object is skipped (not added nor removed).

When using doctrine with the same setup, the two objects are equal:

object(Acme\Bundle\BlogBundle\Entity\User)[475]
  private 'id' => int 3
  private 'firstname' => string 'name edited' (length=11)
  ...

object(Acme\Bundle\BlogBundle\Entity\User)[475]
  private 'id' => int 3
  private 'firstname' => string 'name edited' (length=11)
  ...

Whereas when using Propel, the two objects differ:

object(Acme\Bundle\BlogBundle\Model\User)[464]
  ...
  protected 'id' => int 54
  protected 'firstname' => string 'name' (length=4)
  ...

object(Acme\Bundle\BlogBundle\Model\User)[444]
  ...
  protected 'id' => int 54
  protected 'firstname' => string 'name edited' (length=11)
  ...

And even when there are no modifications, the handle is still different and the condition is still not matched:

object(Acme\Bundle\BlogBundle\Model\User)[464]
  ...
  protected 'id' => int 57
  protected 'firstname' => string 'name edited' (length=4)
  ...

object(Acme\Bundle\BlogBundle\Model\User)[444]
  ...
  protected 'id' => int 57
  protected 'firstname' => string 'name edited' (length=11)
  ...

[pyguerder]

We have encountered a similar problem (collection emptied on update) and provided a minimal Symfony2 project showing this problem: https://github.com/spyrit/MinimalS2P2
There is also a Propel1 branch with (almost) the same code without this problem.
Regards.

[gharlan]

Same issue here..

And the child elements get deleted regardless of the allow_delete option of the collection type.

[gharlan]

Hmm, it seems that removing the by_reference option (or setting it to true) resolves the issue!

[gharlan]

No, then it is not possible to delete a child.

[Maxooo]

To sum up the situation, for each collection the Symfony PropertyAccessor test the inside objects equality to know if it has to keep it or remove it from the collection.

    // Symfony/Component/PropertyAccess/PropertyAccessor.php
    /**
     * Adjusts a collection-valued property by calling add*() and remove*()
     * methods.
     *
     * @param object             $object       The object to write to
     * @param string             $property     The property to write
     * @param array|\Traversable $collection   The collection to write
     * @param string             $addMethod    The add*() method
     * @param string             $removeMethod The remove*() method
     */
    private function writeCollection($object, $property, $collection, $addMethod, $removeMethod)
    {
        // At this point the add and remove methods have been found
        // Use iterator_to_array() instead of clone in order to prevent side effects
        // see https://github.com/symfony/symfony/issues/4670
        $itemsToAdd = is_object($collection) ? iterator_to_array($collection) : $collection;
        $itemToRemove = array();
        $propertyValue = &$this->readProperty($object, $property);
        $previousValue = $propertyValue[self::VALUE];
        // remove reference to avoid modifications
        unset($propertyValue);

        if (is_array($previousValue) || $previousValue instanceof \Traversable) {
            foreach ($previousValue as $previousItem) {
                foreach ($collection as $key => $item) {
                    if ($item === $previousItem) {
                        // Item found, don't add
                        unset($itemsToAdd[$key]);

                        // Next $previousItem
                        continue 2;
                    }
                }

                // Item not found, add to remove list
                $itemToRemove[] = $previousItem;
            }
        }

        foreach ($itemToRemove as $item) {
            $object->{$removeMethod}($item);
        }

        foreach ($itemsToAdd as $item) {
            $object->{$addMethod}($item);
        }
    }

If the object is in the collection (of the persisted objects) keep it (continue 2), but if it's not, remove it $itemToRemove[] = $previousItem;. This assertion is correct and it's working for Doctrine and Propel1.
In fact, the strict equality operator === test if both objects make reference to the same memory space. However, this is not the case as we have to use the by_reference option (to make propel works) that clone the original object.

// Symfony/Component/Form/Form.php
public function setData($modelData)
{
     //...
     if (is_object($modelData) && !$this->config->getByReference()) {
         $modelData = clone $modelData;
     }
}

A simple equality comparison == make it works but we cannot ask to Symfony to change this part of the code which otherwise is correct. So, we have to find what is the difference between Propel1 and 2 and fix it. Easy to say...

[pimpreneil]

Well, after a deep look into propel1, it seems that the behavior was to let Symfony call the removeUser for all the entities already in the DB and the addUser for all the submitted entities. So exactly what we encounter on propel2.

The code and the behavior of the generated addUser and removeUser is pretty much the same on Propel1 and 2, so I guess that the key of the solution must be in the object's save procedure (so on the propel side)

[pimpreneil]

After a comparison of generated classes between propel2 and propel1, I have found the workaround that was used in propel1: in my Company.addUser generated class, there is a condition that unschedules the user from deletion:

if (!in_array($l, $this->collUsers->getArrayCopy(), true)) { // only add it if the **same** object is not      already associated
    $this->doAddUser($l);

    if ($this->usersScheduledForDeletion and $this->usersScheduledForDeletion->contains($l)) {
        $this->usersScheduledForDeletion->remove($this->usersScheduledForDeletion->search($l));
    }
}

Here is the full method in propel1:

public function addUser(User $l)
{
    if ($this->collUsers === null) {
        $this->initUsers();
        $this->collUsersPartial = true;
    }

    if (!in_array($l, $this->collUsers->getArrayCopy(), true)) { // only add it if the **same** object is not already associated
        $this->doAddUser($l);

        if ($this->usersScheduledForDeletion and $this->usersScheduledForDeletion->contains($l)) {
            $this->usersScheduledForDeletion->remove($this->usersScheduledForDeletion->search($l));
        }
    }

    return $this;
}

Here is the corresponding code in propel2:

public function addUser(ChildUser $l)
{
    if ($this->collUsers === null) {
        $this->initUsers();
        $this->collUsersPartial = true;
    }

    if (!$this->collUsers->contains($l)) {
        $this->doAddUser($l);
    }

    return $this;
}

And simply by replacing propel2's method content by propel1's, it solves the problem. I am working on a PR... 😄

[Maxooo]

Very good job, you got it !

[gharlan]

So this is fixed and can be closed, right?

[pyguerder]

IMHO yes

[pimpreneil]

yup