Repository: doctrine/orm (php)

Issue:
2.16.0 - Issue with persisting entities
<!--
Before reporting a BC break, please consult the upgrading document to make sure it's not an expected change: https://github.com/doctrine/orm/blob/2.9.x/UPGRADE.md
-->

### BC Break Report

<!-- Fill in the relevant information below to help triage your issue. -->

|    Q        |   A
|------------ | ------
| BC Break    | yes
| Version     | 2.16.0

#### Summary

I have a case in our test suite where entities are not persisted properly anymore. This worked fine on 2.15.

The insert statement does not bind any parameters and fails:

```
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' at line 1

/var/www/app/symfony/vendor/doctrine/dbal/src/Driver/PDO/Statement.php:121
/var/www/app/symfony/vendor/doctrine/dbal/src/Driver/Middleware/AbstractStatementMiddleware.php:69
/var/www/app/symfony/vendor/doctrine/dbal/src/Logging/Statement.php:98
/var/www/app/symfony/vendor/doctrine/dbal/src/Driver/Middleware/AbstractStatementMiddleware.php:69
/var/www/app/symfony/vendor/symfony/doctrine-bridge/Middleware/Debug/Statement.php:68
/var/www/app/symfony/vendor/doctrine/dbal/src/Statement.php:190
/var/www/app/symfony/vendor/doctrine/dbal/src/Statement.php:249
/var/www/app/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php:280
/var/www/app/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:1177
/var/www/app/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:442
/var/www/app/symfony/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:403
``` 

The logs (see `parameters: []`):

```
10:11:57 DEBUG     [doctrine] Executing statement: INSERT INTO widget_settings (hash, enabled, language, orientation, position, widget_content, tooltip_content, header_color, detail_urls, left_title, use_left_title, right_title, use_right_title, logo, use_logo, is_primary, booking_btn_color, widget_name, booking_urls, review_config_id, account_id, parent_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) (parameters: [], types: [])
``` 

#### How to reproduce

I cannot provide a small reproducer yet. I had a look and somehow inside the `BasicEntityPersister` the call to `prepareInsertData($entity)` with that entity returns an empty array.

The setup part of the test code that triggers the error looks like this:

```php
        $account = $this->fixtures->createAccount();
        $this->em->persist($account);
        $this->em->persist($this->fixtures->createWidgetSettings($account, false));
        $this->em->persist($this->fixtures->createWidgetSettings($account, false));

        $this->em->flush();
``` 

The issue seems to come from the second `$this->em->persist($this->fixtures->createWidgetSettings($account, false));` line :thinking: 

@mpdude does this information help at all to pinpoint the issue? I can try to provide a small reproducer


Pull request description:
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.

Fixes #10869.


Patch:
diff --git a/docs/en/reference/events.rst b/docs/en/reference/events.rst
index 0307429b8..c24940773 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 4c343f2ec..f85a62ea7 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -1164,13 +1164,13 @@ class UnitOfWork implements PropertyChangedListener
      */
     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 @@ class UnitOfWork implements PropertyChangedListener
                 $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 @@ class UnitOfWork implements PropertyChangedListener
      */
     private function executeDeletions(): void
     {
-        $entities = $this->computeDeleteExecutionOrder();
+        $entities         = $this->computeDeleteExecutionOrder();
+        $eventsToDispatch = [];
 
         foreach ($entities as $entity) {
             $oid       = spl_object_id($entity);
@@ -1295,9 +1310,20 @@ class UnitOfWork implements PropertyChangedListener
             }
 
             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> */


Test patch:
diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
index 1a19639d7..908cb3105 100644
--- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
+++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
@@ -4,12 +4,18 @@ declare(strict_types=1);
 
 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 @@ class EntityListenersTest extends OrmFunctionalTestCase
         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<object> */
+            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 @@ class EntityListenersTest extends OrmFunctionalTestCase
         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<object> */
+            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 000000000..937784df3
--- /dev/null
+++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10869Test.php
@@ -0,0 +1,76 @@
+<?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;
+
+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 = '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;
+}