From 8259a166811e51f5c564b6a64ff1cd6174c21dce Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Thu, 10 Aug 2023 18:24:58 +0200 Subject: [PATCH] Mitigate problems with `EntityManager::flush()` reentrance since 2.16.0 (Take 2) The changes from #10547, which landed in 2.16.0, cause problems for users calling `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 #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 https://github.com/doctrine/orm/pull/10906#issuecomment-1682417987. 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. #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 | 33 +++++-- lib/Doctrine/ORM/UnitOfWork.php | 36 +++++++- .../ORM/Functional/EntityListenersTest.php | 91 +++++++++++++++++++ .../ORM/Functional/Ticket/GH10869Test.php | 76 ++++++++++++++++ 4 files changed, 225 insertions(+), 11 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php diff --git a/docs/en/reference/events.rst b/docs/en/reference/events.rst index 0307429b864..c24940773a3 100644 --- a/docs/en/reference/events.rst +++ b/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 diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4c343f2ec93..f85a62ea79d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/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,24 @@ 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 +1284,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 +1310,20 @@ 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 */ diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php index 1a19639d7f6..908cb31059d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php +++ b/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,45 @@ 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]) { + /** @var array */ + private $trackedObjects; + + /** @var int */ + 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 +220,50 @@ 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]) { + /** @var UnitOfWork */ + private $uow; + + /** @var array */ + private $trackedObjects; + + /** @var int */ + 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); + } } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php new file mode 100644 index 00000000000..937784df3d3 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php @@ -0,0 +1,76 @@ +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 = 'test ' . $object->id; + $objectManager->flush(); + } + }); + + $this->_em->flush(); + $this->_em->clear(); + + self::assertSame('test ' . $entity1->id, $entity1->field); + self::assertSame('test ' . $entity2->id, $entity2->field); + + $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; +}