[DDC-3382] Allow orphan removal to be cancelled

[c960657]

I have a one-to-many relation with orphanRemoval=true.
If I remove an entity from the related collection and add it back, the entity is removed from the database.

$employee = $company->getEmployees()->first();
$company->getEmployees()->removeElement($employee);
$company->getEmployees()->add($employee);

$em->persist($company);
$em->flush();
// Now $employee is deleted from the database.

The expected behaviour is to leave the entity in the database, because it was present in the PersistentCollection when $em->persist() was called.

This has previously been suggested in DDC-3382, but it was rejected. This PR shows how small a change it is, so I hope the suggestion will be reconsidered in the light of this.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

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

We use Jira to track the state of pull requests and the versions they got
included in.

[DHager]

Hmmm, it would be nice to let entities to be safely "transferred" to another parent of the very same entity-type:

$employee = $oldCompany->getEmployees()->first();
$oldCompany->getEmployees()->removeElement($employee);
$newCompany->getEmployees()->add($employee);

However, isn't this change a little broader than that? It looks like the instant the child gets put in any PersistentCollection its removal is cancelled.

[c960657]

@DHager Isn't that the expected behaviour? If the entity is added to another PersistentCollection, it is no longer an orphan and should thus not be deleted?

[beberlei]

Very good catch.

[beberlei]

Waiting for @Ocramius ack to merge this to 2.5 branch

[DHager]

@c960657 : I guess I had the idea that orphanRemoval was also an implicit promise that the child was part of a subtree that could only be kept around by a parent of the appropriate type.

In that case, don't we also need to cancel the removal for orphans that are re-adopted directly by an entity, without involving a Collection? For example:

$company->getEmployees()->removeElement($person);
$company->setOwner($person);

I think there should be a test-case for that, and my guess is that it won't (yet) work.

[Ocramius]

Not OK for 2.5.1, as this behavior existed for a lot of versions before that, and even if buggy, a lot of users coded their entity logic according to that.

[Charlie-Lucas]

According to @Ocramius this pull request give me error when i try to update a collection with a new item. spl_object_hash() expects parameter 1 to be object, null given

[c960657]

@Charlie-Lucas Could you provide steps to reproduce this problem?

[Charlie-Lucas]

I have a basic relation OneToMany with OrphanRemoval=true.
My form can send post values like

myForm[rows][0][name]:Row1
myForm[rows][1][name]:Row2

When i persist the first time my form with the entityManager, i have no errors.
But when i try edit this collection with a new item:

myForm[rows][0][name]:Row1
myForm[rows][1][name]:Row2
myForm[rows][2][name]:Row3

I have no controller logic to handle my collection, just a simple persist of my entity.
The cancelOrphanRemoval method is call with null as argument.

[nea]

Hi everyone

We face an issue with the extensive use of collections and the newly added cancelOrphanRemoval function.

unset($this->orphanRemovals[spl_object_hash($entity)]);

If $entity is 'null' for any reason in add/set this will fail with an exception. A check before to validate $entity is not null is at least helpful before unset.

Thanks

[c960657]

Could you provide a stack trace for this error? It seems that something is setting or adding a null value to the persistent collection. That sounds like a bug to me.

[DHager]

@c960657 Can you create a test-case for when an entity is removed from a Collection but re-linked into the object-graph without involving another Collection? Ex:

$company->getEmployees()->removeElement($person);
$company->setOwner($person);

As I said before, I'm reasonably certain this will highlight a scenario that hasn't been covered yet, where $person will still be considered an orphan.

[whitewhidow]

Hi everyone
We face an issue with the extensive use of collections and the newly added cancelOrphanRemoval function.
unset($this->orphanRemovals[spl_object_hash($entity)]);
If $entity is 'null' for any reason in add/set this will fail with an exception. A check before to validate > $entity is not null is at least helpful before unset.
Thanks

I have this exact same issue me thinks

stack trace below:

[1] Symfony\Component\Debug\Exception\ContextErrorException: Warning: spl_object_hash() expects parameter 1 to be object, null given
    at n/a
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php line 2446

    at Symfony\Component\Debug\ErrorHandler->handleError('2', 'spl_object_hash() expects parameter 1 to be object, null given', '/home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php', '2446', array('entity' => null))
        in  line 

    at spl_object_hash(null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php line 2446

    at Doctrine\ORM\UnitOfWork->cancelOrphanRemoval(null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 475

    at Doctrine\ORM\PersistentCollection->set('2', null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 522

    at Doctrine\ORM\PersistentCollection->offsetSet('2', null)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 223

    at Symfony\Component\PropertyAccess\PropertyAccessor->readPropertiesUntil(object(PersistentCollection), object(PropertyPath), '1', true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 60

    at Symfony\Component\PropertyAccess\PropertyAccessor->getValue(object(PersistentCollection), object(PropertyPath))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php line 58

    at Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper->mapDataToForms(object(PersistentCollection), object(RecursiveIteratorIterator))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 936

    at Symfony\Component\Form\Form->add('2', 'sonata_type_admin', array('sonata_field_description' => object(FieldDescription), 'data_class' => 'Sam\PosBundle\Entity\ArticlePrice', 'delete' => false, 'property_path' => '[2]', 'delete_options' => array('type' => 'hidden', 'type_options' => array('mapped' => false, 'required' => false))))
        in /home/sam/git/PosBranch/vendor/sonata-project/core-bundle/Form/EventListener/ResizeFormListener.php line 163

    at Sonata\CoreBundle\Form\EventListener\ResizeFormListener->preSubmit(object(FormEvent))
        in /home/sam/git/PosBranch/vendor/sonata-project/core-bundle/Form/EventListener/ResizeFormListener.php line 118

    at Sonata\CoreBundle\Form\EventListener\ResizeFormListener->preBind(object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in  line 

    at call_user_func(array(object(ResizeFormListener), 'preBind'), object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in /home/sam/git/PosBranch/app/cache/dev/classes.php line 1826

    at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(array(object(BindRequestListener), 'preBind'), array(object(TrimListener), 'preSubmit'), array(object(CsrfValidationListener), 'preSubmit'), array(object(ResizeFormListener), 'preBind')), 'form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/app/cache/dev/classes.php line 1759

    at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/ImmutableEventDispatcher.php line 43

    at Symfony\Component\EventDispatcher\ImmutableEventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 556

    at Symfony\Component\Form\Form->submit(array(array('amount' => '11', 'articleSize' => '175'), array('amount' => '12', 'articleSize' => '176'), array('amount' => '13', 'articleSize' => '176')), true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 577

    at Symfony\Component\Form\Form->submit(array('name' => 'ONLY PEPPERONI', 'frontName' => 'only pepperoni', 'articleCategory' => '85', 'weight' => '1', 'isVisibleInFront' => '1', 'isPurchaseable' => '1', 'articlePrices' => array(array('amount' => '11', 'articleSize' => '175'), array('amount' => '12', 'articleSize' => '176'), array('amount' => '13', 'articleSize' => '176')), '_token' => 'XAwEGLnw0a2hWWdOVracSwckqM9mDCcvkqDllrLcBHg'), true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php line 116

    at Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler->handleRequest(object(Form), object(Request))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 499

    at Symfony\Component\Form\Form->handleRequest(object(Request))
        in /home/sam/git/PosBranch/vendor/sonata-project/admin-bundle/Controller/CRUDController.php line 407

    at Sonata\AdminBundle\Controller\CRUDController->editAction('154', object(Request))
        in  line 

    at call_user_func_array(array(object(CRUDController), 'editAction'), array('154', object(Request)))
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3109

    at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), '1')
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3071

    at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), '1', true)
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3222

    at Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel->handle(object(Request), '1', true)
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 2444

    at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
        in /home/sam/git/PosBranch/web/app_dev.php line 21

Any help would be greatly appreciated, as this is a mayor blocker for me atm, as i cant get past it (other then circumventing the orphanremoval code)

Let me know what steps i can take to help solve this issue.

[c960657]

The problem occurs because a non-object is added to the PersistentCollection:

at Doctrine\ORM\PersistentCollection->set('2', null)
    in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 522

at Doctrine\ORM\PersistentCollection->offsetSet('2', null)
    in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 223

I don't know if this is supposed to work, but I don't think so. The documentation states that a "PersistentCollection represents a collection of elements that have persistent state". I believe this excludes non-elements.

The below code would throw a similar error even before this change:

        $doors = $house->getDoors();
        $doors[1] = null;
        $doors->clear();

So I don't think the bug is in Doctrine 2 but probably in the Symfony form binding code.

That being said, it seems reasonable to modify to add() and set() to check for non-object values. Because of BC we have to just silently ignore these (this might change in a future major release). I will prepare a PR for that.

[whitewhidow]

yes i know its adding a null to the collection, i just cant seem to figure out where, and why, things are going wrong, causing it to end up with this null.

Only thing i can do atm is overwrite the PropertyAccessor->getValue function with

        if (!$propertyPath instanceof PropertyPathInterface) {
            $propertyPath = new PropertyPath($propertyPath);
        }

        try {
            $propertyValues = &$this->readPropertiesUntil(
                $objectOrArray,
                $propertyPath,
                $propertyPath->getLength(),
                $this->ignoreInvalidIndices
            );
        }catch(\Exception $e) {
            return;
        }
        return $propertyValues[count($propertyValues) - 1][self::VALUE];

To make things atleast function

[whitewhidow]

temp workaround:

in app/config/config.yml:

property_accessor.class: BBIT\BlogBundle\Component\PropertyAccess\MyPropertyAccessor

in BBIT\BlogBundle\Component\PropertyAccess\MyPropertyAccessor

<?php
namespace BBIT\BlogBundle\Component\PropertyAccess;

use \Symfony\Component\PropertyAccess\PropertyAccessor as BaseAccessor;

class MyPropertyAccessor extends BaseAccessor
{
    public function getValue($objectOrArray, $propertyPath)
    {
        try {
            return parent::getValue($objectOrArray, $propertyPath);
        }catch(\Exception $e) {
            return;
        }
    }
}

[grachevko]

When this mergin will be released? It is very necessary.

[vlastv]

Ping...

#4785

[Charlie-Lucas]

@vlastv you must use 2.5 version of doctrine not dev-master

[grachevko]

@Charlie-Lucas 2.5 version does not contain this pr.

[Charlie-Lucas]

@grachevko sorry i was wrong this PR is only merged on master