Orm: Using remove in preFlush event results in spl_object_hash conflict

[develancer]

Bug Report

| Q | A
|------------ | ------
| BC Break | no
| Version | 2.6.4

Summary

We have had a number of reported bugs related to spl_object_hash conflicts in the past, and it seems the problem is not eliminated yet. For example, EntityManager's (and UnitOfWork's) remove method cannot be safely used in PreFlush.

Current behavior

If the remove() is called on an entity within a PreFlush event, it is garbage collected but its hash is still remembered by the EntityManager. Creating another entity afterwards (with a subsequent flush) can result in hash conflict and ORM error:

A managed+dirty entity TestEntity@000000001d515bca000000005d0092d6 can not be scheduled for insertion.

How to reproduce

Entity specification:

/**
 * @Table(name="test_table")
 * @Entity
 * @HasLifecycleCallbacks
 */
class TestEntity
{
    /**
     * @Id
     * @Column(name="id", type="integer", nullable=false)
     */
    private $id;

    /**
     * @Column(name="value", type="integer", nullable=false)
     */
    private $value = 0;

    public function __construct($id)
    {
        $this->id = (int) $id;
    }

    /** @PreFlush */
    public function preFlush(PreFlushEventArgs $args)
    {
        // prevents empty records from storing in the database
        if (!$this->value) {
            $args->getEntityManager()->remove($this);
        }
    }
}

Core of the unit test:

public function runTestForPreFlush(EntityManagerInterface $em)
{
    $this->createEmptyEntity($em, 1);
    $em->flush();
    $this->createEmptyEntity($em, 2);
    $em->flush();
}

private function createEmptyEntity(EntityManagerInterface $em, $id)
{
    $entity = new TestEntity($id);
    $em->persist($entity);
}

Expected behavior

Both flush operations should perform without error, and none of the entities should be saved into the database.

[SenseException]

"Expected behavior" misses more detailed description. Do you expect that none of the entities in the example will be saved into the database? What is your use case?

[develancer]

@SenseException Exactly. I have just fixed the description, thanks. However, the main problem here is the “managed+dirty” error.

The use case is that for each base record we have a number of optional additional records in auxiliary tables, and if any of such records does not exist, all fields are assumed to have default values. Therefore, if a user changes all fields in any given record back to defaults, the record doesn't have to exist anymore and can be deleted to save space.

[piotrkardasz]

In my opinion is mishmash logic. If EntityManager is step before flush you would like to flush and remove in the same instance. Why you don't check the value property before persist?

[develancer]

@piotrkardasz It may as well be, but the real question is: is it allowed by the documentation? In my opinion, it is, as the documentation states only that flush itself should not be called inside the preFlush event. Therefore, we should either:

• fix this bug, or
• update the documentation and state that remove cannot be safely called within preFlush event listeners.

[Ocramius]

Does moving this to onFlush change the behavior at all? I remember DoctrineExtensions doing something like that for soft-deletes...

[piotrkardasz]

I created the same model as you presented and it work correctly as you described in Expected behavior Could you explain how to reproduce your case?

[develancer]

@piotrkardasz Attached you'll find a dockerized test case docker-example.zip. You can build it with docker build -t docker-example . and then run it as docker run docker-example.

If you need an interactive PHP shell for debugging, docker run -it docker-example -a will do.

[develancer]

@Ocramius onFlush also fails — see attached docker-example-onflush.zip.

[piotrkardasz]

@develancer You should call method flush once.

e.g.

for ($i = 0; $i <10; $i++)
{
createEmptyEntity($em, $i);
}
$em->flush();

[develancer]

@piotrkardasz :) I hope you understand that's clearly not the point. Most of the complicated database workflows will utilize multiple small transactions instead of a single large one.

This workflow is correct and it adheres to the docs. Unless you find some part of the documentation that states “You should call method flush once.” ;-)

[beberlei]

This happens because you call remove while computeChangeSet is in progress. The removal however doesn't stop that operation, and data is then entered into originalEntityData and other stateful properties inside the UnitOfWork, causing this problem.

I believe this is not fixable, calling remove() at this point will not work. You need to do this in an EventListener that listens to preFlush instead of a lifecycle event.

class PreventInsertListener
{
    public function onPreFlush($args)
    {
        $entityManager = $args->geTentityManager();
        foreach ($entityManager->getUnitOfWork()->getScheduledEntityInsertions() as $entity) {
             $entityManager->remove($entity);
        }
    }
}

This happens slightly earlier and will work.

[develancer]

@beberlei Shouldn't we mention it in the docs, then?

[biozshock]

For those who landed here from search. After update from 2.6.3 to 2.7.0 the ORM throws this exception when you issue flush() from preFlush listener. I know it's forbidden in docs, but we ended up with the flush after some major refactoring.