From 0808234578e8fc984b18fee63a533996b28a95cf Mon Sep 17 00:00:00 2001
From: Matthias Pigulla <mp@webfactory.de>
Date: Fri, 22 Dec 2023 11:46:47 +0100
Subject: [PATCH] Make sure in-memory collections have removed entities unset
 before the `postRemove` event is dispatched. This is related to #11098,
 although by itself probably not going to fix the regression.

#### Background

When an entity is removed from the database, the UoW also takes care of removing that entity from all in-memory collections. #10763 moved this cleanup and taking snapshots of updated collections to a very late stage during the commit phase, in order to avoid other side effects.

Now, part of the issue in #11098 is that `postRemove` event listeners will be called at a point where the database-level `DELETE` has happened (although the transaction has not yet been committed), but the in-memory collections have not yet been updated.

#### Suggested change

This PR moves the code part that unsets removed entities from in-memory collections and takes collection snapshots (makes collections "clean") from after transaction commit to before the `postRemove` event. That brings the in-memory update closer to the database-level execution.

In the case of a transaction failure/abort, this leaves us with updated and snapshotted in-memory collections, but no database-level updates. But, at this point, probably things got inconsistent anyways, the EM will be closed and we need not be too worried about the state.
---
 lib/Doctrine/ORM/UnitOfWork.php               | 14 ++++---
 .../ORM/Functional/EntityListenersTest.php    | 42 +++++++++++++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
index b3285a8845c..58eb8130200 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -469,6 +469,8 @@ public function commit($entity = null)
             // (first delete entities depending upon others, before deleting depended-upon entities).
             if ($this->entityDeletions) {
                 $this->executeDeletions();
+            } else {
+                $this->finalizeCollectionUpdates();
             }
 
             // Commit failed silently
@@ -490,7 +492,12 @@ public function commit($entity = null)
         }
 
         $this->afterTransactionComplete();
+        $this->dispatchPostFlushEvent();
+        $this->postCommitCleanup($entity);
+    }
 
+    private function finalizeCollectionUpdates(): void
+    {
         // Unset removed entities from collections, and take new snapshots from
         // all visited collections.
         foreach ($this->visitedCollections as $coid => $coll) {
@@ -499,13 +506,8 @@ public function commit($entity = null)
                     unset($coll[$key]);
                 }
             }
-
             $coll->takeSnapshot();
         }
-
-        $this->dispatchPostFlushEvent();
-
-        $this->postCommitCleanup($entity);
     }
 
     /** @param object|object[]|null $entity */
@@ -1317,6 +1319,8 @@ private function executeDeletions(): void
             }
         }
 
+        $this->finalizeCollectionUpdates();
+
         // Defer dispatching `postRemove` events to until all entities have been removed.
         foreach ($eventsToDispatch as $event) {
             $this->listenersInvoker->invoke(
diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
index 908cb31059d..2dc10efe29f 100644
--- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
+++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
@@ -4,6 +4,7 @@
 
 namespace Doctrine\Tests\ORM\Functional;
 
+use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\ORM\Event\PostPersistEventArgs;
 use Doctrine\ORM\Event\PostRemoveEventArgs;
 use Doctrine\ORM\Event\PreFlushEventArgs;
@@ -12,6 +13,7 @@
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\Persistence\Event\LifecycleEventArgs;
 use Doctrine\Tests\Models\Company\CompanyContractListener;
+use Doctrine\Tests\Models\Company\CompanyEmployee;
 use Doctrine\Tests\Models\Company\CompanyFixContract;
 use Doctrine\Tests\Models\Company\CompanyPerson;
 use Doctrine\Tests\OrmFunctionalTestCase;
@@ -266,4 +268,44 @@ public function postRemove(PostRemoveEventArgs $args): void
 
         self::assertSame(2, $listener->invocationCount);
     }
+
+    public function testPostRemoveCalledAfterAllInMemoryCollectionsHaveBeenUpdated(): void
+    {
+        $contract = new CompanyFixContract();
+        $contract->setFixPrice(2000);
+
+        $engineer = new CompanyEmployee();
+        $engineer->setName('J. Doe');
+        $engineer->setSalary(50);
+        $engineer->setDepartment('tech');
+
+        $contract->addEngineer($engineer);
+        $engineer->contracts = new ArrayCollection([$contract]);
+
+        $this->_em->persist($contract);
+        $this->_em->persist($engineer);
+        $this->_em->flush();
+
+        $this->_em->getEventManager()->addEventListener([Events::postRemove], new class ($contract) {
+            /** @var CompanyFixContract */
+            private $contract;
+
+            public function __construct(CompanyFixContract $contract)
+            {
+                $this->contract = $contract;
+            }
+
+            public function postRemove(): void
+            {
+                Assert::assertEmpty($this->contract->getEngineers()); // Assert collection has been updated before event was dispatched
+                Assert::assertFalse($this->contract->getEngineers()->isDirty()); // Collections are clean at this point
+            }
+        });
+
+        $this->_em->remove($engineer);
+        $this->_em->flush();
+
+        self::assertEmpty($contract->getEngineers());
+        self::assertFalse($contract->getEngineers()->isDirty());
+    }
 }