Mitigate problems with EntityManager::flush() reentrance since 2.16…

….0 (Take 2)

The changes from doctrine#10547, which landed in 2.16.0, cause problems for users that call `EntityManager::flush()` from within `postPersist` event listeners.

* When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and UoW::$entityInsertions being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869.

* The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment).

This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed.

This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant.

Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.

docs/en/reference/events.rst

@@ -705,13 +705,21 @@ not directly mapped by Doctrine.
 -  The ``postUpdate`` event occurs after the database
    update operations to entity data. It is not called for a DQL
    ``UPDATE`` statement.
--  The ``postPersist`` event occurs for an entity after
-   the entity has been made persistent. It will be invoked after the
-   database insert operation for that entity. A generated primary key value for
-   the entity will be available in the postPersist event.
+-  The ``postPersist`` event occurs for an entity after the entity has
+   been made persistent. It will be invoked after all database insert
+   operations for new entities have been performed. Generated primary
+   key values will be available for all entities at the time this
+   event is triggered.
 -  The ``postRemove`` event occurs for an entity after the
-   entity has been deleted. It will be invoked after the database
-   delete operations. It is not called for a DQL ``DELETE`` statement.
+   entity has been deleted. It will be invoked after all database
+   delete operations for entity rows have been executed. This event is
+   not called for a DQL ``DELETE`` statement.
+
+.. note::
+
+    At the time ``postPersist`` is called, there may still be collection and/or
+    "extra" updates pending. The database may not yet be completely in
+    sync with the entity states in memory, not even for the new entities.
 
 .. warning::
 
@@ -720,6 +728,19 @@ not directly mapped by Doctrine.
     cascade remove relations. In this case, you should load yourself the proxy in
     the associated ``pre*`` event.
 
+.. warning::
+
+    Making changes to entities and calling ``EntityManager::flush()`` from within
+    ``post*`` event handlers is strongly discouraged, and might be deprecated and
+    eventually prevented in the future.
+
+    The reason is that it causes re-entrance into ``UnitOfWork::commit()`` while a commit
+    is currently being processed. The ``UnitOfWork`` was never designed to support this,
+    and its behavior in this situation is not covered by any tests.
+
+    This may lead to entity or collection updates being missed, applied only in parts and
+    changes being lost at the end of the commit phase.
+
 .. _reference-events-post-load:
 
 postLoad

lib/Doctrine/ORM/UnitOfWork.php

@@ -1164,13 +1164,13 @@ public function recomputeSingleEntityChangeSet(ClassMetadata $class, $entity)
      */
     private function executeInserts(): void
     {
-        $entities = $this->computeInsertExecutionOrder();
+        $entities         = $this->computeInsertExecutionOrder();
+        $eventsToDispatch = [];
 
         foreach ($entities as $entity) {
             $oid       = spl_object_id($entity);
             $class     = $this->em->getClassMetadata(get_class($entity));
             $persister = $this->getEntityPersister($class->name);
-            $invoke    = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist);
 
             $persister->addInsert($entity);
 
@@ -1197,10 +1197,18 @@ private function executeInserts(): void
                 $this->addToEntityIdentifiersAndEntityMap($class, $oid, $entity);
             }
 
+            $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist);
+
             if ($invoke !== ListenersInvoker::INVOKE_NONE) {
-                $this->listenersInvoker->invoke($class, Events::postPersist, $entity, new PostPersistEventArgs($entity, $this->em), $invoke);
+                $eventsToDispatch[] = ['class' => $class, 'entity' => $entity, 'invoke' => $invoke];
             }
         }
+
+        // Defer dispatching `postPersist` events to until all entities have been inserted and post-insert
+        // IDs have been assigned.
+        foreach ($eventsToDispatch as $event) {
+            $this->listenersInvoker->invoke($event['class'], Events::postPersist, $event['entity'], new PostPersistEventArgs($event['entity'], $this->em), $event['invoke']);
+        }
     }
 
     /**
@@ -1270,7 +1278,8 @@ private function executeUpdates(): void
      */
     private function executeDeletions(): void
     {
-        $entities = $this->computeDeleteExecutionOrder();
+        $entities         = $this->computeDeleteExecutionOrder();
+        $eventsToDispatch = [];
 
         foreach ($entities as $entity) {
             $oid       = spl_object_id($entity);
@@ -1295,9 +1304,14 @@ private function executeDeletions(): void
             }
 
             if ($invoke !== ListenersInvoker::INVOKE_NONE) {
-                $this->listenersInvoker->invoke($class, Events::postRemove, $entity, new PostRemoveEventArgs($entity, $this->em), $invoke);
+                $eventsToDispatch[] = ['class' => $class, 'entity' => $entity, 'invoke' => $invoke];
             }
         }
+
+        // Defer dispatching `postRemove` events to until all entities have been removed.
+        foreach ($eventsToDispatch as $event) {
+            $this->listenersInvoker->invoke($event['class'], Events::postRemove, $event['entity'], new PostRemoveEventArgs($event['entity'], $this->em), $event['invoke']);
+        }
     }
 
     /** @return list<object> */

tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php

@@ -4,12 +4,18 @@
 
 namespace Doctrine\Tests\ORM\Functional;
 
+use Doctrine\ORM\Event\PostPersistEventArgs;
+use Doctrine\ORM\Event\PostRemoveEventArgs;
 use Doctrine\ORM\Event\PreFlushEventArgs;
 use Doctrine\ORM\Event\PreUpdateEventArgs;
+use Doctrine\ORM\Events;
+use Doctrine\ORM\UnitOfWork;
 use Doctrine\Persistence\Event\LifecycleEventArgs;
 use Doctrine\Tests\Models\Company\CompanyContractListener;
 use Doctrine\Tests\Models\Company\CompanyFixContract;
+use Doctrine\Tests\Models\Company\CompanyPerson;
 use Doctrine\Tests\OrmFunctionalTestCase;
+use PHPUnit\Framework\Assert;
 
 /** @group DDC-1955 */
 class EntityListenersTest extends OrmFunctionalTestCase
@@ -96,6 +102,42 @@ public function testPostPersistListeners(): void
         self::assertInstanceOf(LifecycleEventArgs::class, $this->listener->postPersistCalls[0][1]);
     }
 
+    public function testPostPersistCalledAfterAllInsertsHaveBeenPerformedAndIdsHaveBeenAssigned(): void
+    {
+        $object1 = new CompanyFixContract();
+        $object1->setFixPrice(2000);
+
+        $object2 = new CompanyPerson();
+        $object2->setName('J. Doe');
+
+        $this->_em->persist($object1);
+        $this->_em->persist($object2);
+
+        $listener = new class ([$object1, $object2]) {
+            private $trackedObjects;
+            public $invocationCount = 0;
+
+            public function __construct(array $trackedObjects)
+            {
+                $this->trackedObjects = $trackedObjects;
+            }
+
+            public function postPersist(PostPersistEventArgs $args): void
+            {
+                foreach ($this->trackedObjects as $object) {
+                    Assert::assertNotNull($object->getId());
+                }
+
+                ++$this->invocationCount;
+            }
+        };
+
+        $this->_em->getEventManager()->addEventListener(Events::postPersist, $listener);
+        $this->_em->flush();
+
+        self::assertSame(2, $listener->invocationCount);
+    }
+
     public function testPreUpdateListeners(): void
     {
         $fix = new CompanyFixContract();
@@ -175,4 +217,46 @@ public function testPostRemoveListeners(): void
         self::assertInstanceOf(CompanyFixContract::class, $this->listener->postRemoveCalls[0][0]);
         self::assertInstanceOf(LifecycleEventArgs::class, $this->listener->postRemoveCalls[0][1]);
     }
+
+    public function testPostRemoveCalledAfterAllRemovalsHaveBeenPerformed(): void
+    {
+        $object1 = new CompanyFixContract();
+        $object1->setFixPrice(2000);
+
+        $object2 = new CompanyPerson();
+        $object2->setName('J. Doe');
+
+        $this->_em->persist($object1);
+        $this->_em->persist($object2);
+        $this->_em->flush();
+
+        $listener = new class ($this->_em->getUnitOfWork(), [$object1, $object2]) {
+            private $uow;
+            private $trackedObjects;
+
+            public $invocationCount = 0;
+
+            public function __construct(UnitOfWork $uow, array $trackedObjects)
+            {
+                $this->uow            = $uow;
+                $this->trackedObjects = $trackedObjects;
+            }
+
+            public function postRemove(PostRemoveEventArgs $args): void
+            {
+                foreach ($this->trackedObjects as $object) {
+                    Assert::assertFalse($this->uow->isInIdentityMap($object));
+                }
+
+                ++$this->invocationCount;
+            }
+        };
+
+        $this->_em->getEventManager()->addEventListener(Events::postRemove, $listener);
+        $this->_em->remove($object1);
+        $this->_em->remove($object2);
+        $this->_em->flush();
+
+        self::assertSame(2, $listener->invocationCount);
+    }
 }

tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php

@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\ORM\Event\PostPersistEventArgs;
+use Doctrine\ORM\Events;
+use Doctrine\ORM\Mapping as ORM;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+use function sprintf;
+
+class GH10869Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->setUpEntitySchema([
+            GH10869Entity::class,
+        ]);
+    }
+
+    public function testPostPersistListenerUpdatingObjectFieldWhileOtherInsertPending(): void
+    {
+        $entity1 = new GH10869Entity();
+        $this->_em->persist($entity1);
+
+        $entity2 = new GH10869Entity();
+        $this->_em->persist($entity2);
+
+        $this->_em->getEventManager()->addEventListener(Events::postPersist, new class {
+            public function postPersist(PostPersistEventArgs $args): void
+            {
+                $object = $args->getObject();
+
+                $objectManager = $args->getObjectManager();
+                $object->field = sprintf('test %s', $object->id);
+                $objectManager->flush();
+            }
+        });
+
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $entity1Reloaded = $this->_em->find(GH10869Entity::class, $entity1->id);
+        self::assertSame($entity1->field, $entity1Reloaded->field);
+
+        $entity2Reloaded = $this->_em->find(GH10869Entity::class, $entity2->id);
+        self::assertSame($entity2->field, $entity2Reloaded->field);
+    }
+}
+
+/**
+ * @ORM\Entity
+ */
+class GH10869Entity
+{
+    /**
+     * @ORM\Id
+     * @ORM\GeneratedValue
+     * @ORM\Column(type="integer")
+     *
+     * @var ?int
+     */
+    public $id;
+
+    /**
+     * @ORM\Column(type="text", nullable=true)
+     *
+     * @var ?string
+     */
+    public $field;
+}