From a9513692cb2b23e39e0f574a45a2acc8bbc4f494 Mon Sep 17 00:00:00 2001
From: Matthias Pigulla <mp@webfactory.de>
Date: Tue, 31 Jan 2023 15:25:20 +0000
Subject: [PATCH] Fix to-many collections left in dirty state after entities
 are removed by the UoW

---
 lib/Doctrine/ORM/UnitOfWork.php               | 14 ++---
 .../ORM/Functional/ManyToManyEventTest.php    | 19 ++++++-
 tests/Doctrine/Tests/ORM/UnitOfWorkTest.php   | 57 +++++++++++++++++++
 3 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
index 1e949cab4ee..27a54c9efcc 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -916,13 +916,6 @@ private function computeAssociationChanges(array $assoc, $value): void
             return;
         }
 
-        if ($value instanceof PersistentCollection && $value->isDirty()) {
-            $coid = spl_object_id($value);
-
-            $this->collectionUpdates[$coid]  = $value;
-            $this->visitedCollections[$coid] = $value;
-        }
-
         // Look through the entities, and in any of their associations,
         // for transient (new) entities, recursively. ("Persistence by reachability")
         // Unwrap. Uninitialized collections will simply be empty.
@@ -983,6 +976,13 @@ private function computeAssociationChanges(array $assoc, $value): void
                     // during changeset calculation anyway, since they are in the identity map.
             }
         }
+
+        if ($value instanceof PersistentCollection && $value->isDirty()) {
+            $coid = spl_object_id($value);
+
+            $this->collectionUpdates[$coid]  = $value;
+            $this->visitedCollections[$coid] = $value;
+        }
     }
 
     /**
diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php
index ce661eb69e0..03d547e6b72 100644
--- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php
+++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php
@@ -28,7 +28,7 @@ protected function setUp(): void
         $evm->addEventListener(Events::postUpdate, $this->listener);
     }
 
-    public function testListenerShouldBeNotifiedOnlyWhenUpdating(): void
+    public function testListenerShouldBeNotifiedWhenNewCollectionEntryAdded(): void
     {
         $user = $this->createNewValidUser();
         $this->_em->persist($user);
@@ -44,6 +44,23 @@ public function testListenerShouldBeNotifiedOnlyWhenUpdating(): void
         self::assertTrue($this->listener->wasNotified);
     }
 
+    public function testListenerShouldBeNotifiedWhenNewCollectionEntryRemoved(): void
+    {
+        $user        = $this->createNewValidUser();
+        $group       = new CmsGroup();
+        $group->name = 'admins';
+        $user->addGroup($group);
+
+        $this->_em->persist($user);
+        $this->_em->flush();
+        self::assertFalse($this->listener->wasNotified);
+
+        $this->_em->remove($group);
+        $this->_em->flush();
+
+        self::assertTrue($this->listener->wasNotified);
+    }
+
     private function createNewValidUser(): CmsUser
     {
         $user           = new CmsUser();
diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
index 479f1168218..5e5fc29b7f9 100644
--- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
+++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
@@ -31,6 +31,7 @@
 use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Mocks\EntityPersisterMock;
 use Doctrine\Tests\Mocks\UnitOfWorkMock;
+use Doctrine\Tests\Models\CMS\CmsGroup;
 use Doctrine\Tests\Models\CMS\CmsPhonenumber;
 use Doctrine\Tests\Models\CMS\CmsUser;
 use Doctrine\Tests\Models\Forum\ForumAvatar;
@@ -866,6 +867,62 @@ public function testItThrowsWhenLookingUpIdentifierForUnknownEntity(): void
         $this->expectException(EntityNotFoundException::class);
         $this->_unitOfWork->getEntityIdentifier(new stdClass());
     }
+
+    public function testRemovedEntityIsRemovedFromManyToManyCollection(): void
+    {
+        $group       = new CmsGroup();
+        $group->name = 'test';
+        $this->_unitOfWork->persist($group);
+
+        $user       = new CmsUser();
+        $user->name = 'test';
+        $user->groups->add($group);
+        $this->_unitOfWork->persist($user);
+
+        $this->_unitOfWork->commit();
+
+        self::assertFalse($user->groups->isDirty());
+
+        $this->_unitOfWork->remove($group);
+        $this->_unitOfWork->commit();
+
+        // Test that the removed entity has been removed from the many to many collection
+        self::assertEmpty(
+            $user->groups,
+            'the removed entity should have been removed from the many to many collection'
+        );
+
+        // Collection is clean, snapshot has been updated
+        self::assertFalse($user->groups->isDirty());
+        self::assertEmpty($user->groups->getSnapshot());
+    }
+
+    public function testRemovedEntityIsRemovedFromOneToManyCollection(): void
+    {
+        $user       = new CmsUser();
+        $user->name = 'test';
+
+        $phonenumber              = new CmsPhonenumber();
+        $phonenumber->phonenumber = '0800-123456';
+
+        $user->addPhonenumber($phonenumber);
+
+        $this->_unitOfWork->persist($user);
+        $this->_unitOfWork->persist($phonenumber);
+        $this->_unitOfWork->commit();
+
+        self::assertFalse($user->phonenumbers->isDirty());
+
+        $this->_unitOfWork->remove($phonenumber);
+        $this->_unitOfWork->commit();
+
+        // Test that the removed entity has been removed from the one to many collection
+        self::assertEmpty($user->phonenumbers);
+
+        // Collection is clean, snapshot has been updated
+        self::assertFalse($user->phonenumbers->isDirty());
+        self::assertEmpty($user->phonenumbers->getSnapshot());
+    }
 }
 
 /** @Entity */