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..474695c7362 --- /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; +}