Defer removing removed entities from to-many collections until after transaction commit

docs/en/reference/association-mapping.rst

@@ -881,6 +881,15 @@ Generated MySQL Schema:
     replaced by one-to-many/many-to-one associations between the 3
     participating classes.
 
+.. note::
+
+    For many-to-many associations, the ORM takes care of managing rows
+    in the join table connecting both sides. Due to the way it deals
+    with entity removals, database-level constraints may not work the
+    way one might intuitively assume. Thus, be sure not to miss the section
+    on :ref:`join table management <remove_object_many_to_many_join_tables>`.
+
+
 Many-To-Many, Bidirectional
 ---------------------------
 
@@ -1019,6 +1028,15 @@ one is bidirectional.
 The MySQL schema is exactly the same as for the Many-To-Many
 uni-directional case above.
 
+.. note::
+
+    For many-to-many associations, the ORM takes care of managing rows
+    in the join table connecting both sides. Due to the way it deals
+    with entity removals, database-level constraints may not work the
+    way one might intuitively assume. Thus, be sure not to miss the section
+    on :ref:`join table management <remove_object_many_to_many_join_tables>`.
+
+
 Owning and Inverse Side on a ManyToMany Association
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

docs/en/reference/working-with-objects.rst

@@ -286,17 +286,53 @@ as follows:
 After an entity has been removed, its in-memory state is the same as
 before the removal, except for generated identifiers.
 
-Removing an entity will also automatically delete any existing
-records in many-to-many join tables that link this entity. The
-action taken depends on the value of the ``@joinColumn`` mapping
-attribute "onDelete". Either Doctrine issues a dedicated ``DELETE``
-statement for records of each join table or it depends on the
-foreign key semantics of onDelete="CASCADE".
+During the ``EntityManager#flush()`` operation, the removed entity
+will also be removed from all collections in entities currently
+loaded into memory.
+
+.. _remove_object_many_to_many_join_tables:
+
+Join-table management when removing from many-to-many collections
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Regarding existing rows in many-to-many join tables that refer to
+an entity being removed, the following applies.
+
+When the entity being removed does not declare the many-to-many association
+itself (that is, the many-to-many association is unidirectional and
+the entity is on the inverse side), the ORM has no reasonable way to
+detect associations targeting the entity's class. Thus, no ORM-level handling
+of join-table rows is attempted and database-level constraints apply.
+In case of database-level ``ON DELETE RESTRICT`` constraints, the
+``EntityManager#flush()`` operation may abort and a ``ConstraintViolationException``
+may be thrown. No in-memory collections will be modified in this case.
+With ``ON DELETE CASCADE``, the RDBMS will take care of removing rows
+from join tables.
+
+When the entity being removed is part of bi-directional many-to-many
+association, either as the owning or inverse side, the ORM will
+delete rows from join tables before removing the entity itself. That means
+database-level ``ON DELETE RESTRICT`` constraints on join tables are not
+effective, since the join table rows are removed first. Removal of join table
+rows happens through specialized methods in entity and collection persister
+classes and take one query per entity and join table. In case the association
+uses a ``@JoinColumn`` configuration with ``onDelete="CASCADE"``, instead
+of using a dedicated ``DELETE`` query the database-level operation will be
+relied upon.
+
+.. note::
+
+    In case you rely on database-level ``ON DELETE RESTRICT`` constraints,
+    be aware that by making many-to-many associations bidirectional the
+    assumed protection may be lost.
+
+
+Performance of different deletion strategies
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Deleting an object with all its associated objects can be achieved
 in multiple ways with very different performance impacts.
 
-
 1. If an association is marked as ``CASCADE=REMOVE`` Doctrine ORM
    will fetch this association. If its a Single association it will
    pass this entity to

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -538,7 +538,7 @@ final protected function updateTable(
     protected function deleteJoinTableRecords(array $identifier, array $types): void
     {
         foreach ($this->class->associationMappings as $mapping) {
-            if ($mapping['type'] !== ClassMetadata::MANY_TO_MANY) {
+            if ($mapping['type'] !== ClassMetadata::MANY_TO_MANY || isset($mapping['isOnDeleteCascade'])) {
                 continue;
             }
 
@@ -574,10 +574,6 @@ protected function deleteJoinTableRecords(array $identifier, array $types): void
                 $otherKeys[] = $this->quoteStrategy->getJoinColumnName($joinColumn, $class, $this->platform);
             }
 
-            if (isset($mapping['isOnDeleteCascade'])) {
-                continue;
-            }
-
             $joinTableName = $this->quoteStrategy->getJoinTableName($association, $this->class, $this->platform);
 
             $this->conn->delete($joinTableName, array_combine($keys, $identifier), $types);

lib/Doctrine/ORM/UnitOfWork.php

@@ -240,6 +240,17 @@ class UnitOfWork implements PropertyChangedListener
      */
     private $visitedCollections = [];
 
+    /**
+     * List of collections visited during the changeset calculation that contain to-be-removed
+     * entities and need to have keys removed post commit.
+     *
+     * Indexed by Collection object ID, which also serves as the key in self::$visitedCollections;
+     * values are the key names that need to be removed.
+     *
+     * @psalm-var array<int, array<array-key, true>>
+     */
+    private $pendingCollectionElementRemovals = [];
+
     /**
      * The EntityManager that "owns" this UnitOfWork instance.
      *
@@ -474,8 +485,15 @@ public function commit($entity = null)
 
         $this->afterTransactionComplete();
 
-        // Take new snapshots from visited collections
-        foreach ($this->visitedCollections as $coll) {
+        // Unset removed entities from collections, and take new snapshots from
+        // all visited collections.
+        foreach ($this->visitedCollections as $coid => $coll) {
+            if (isset($this->pendingCollectionElementRemovals[$coid])) {
+                foreach ($this->pendingCollectionElementRemovals[$coid] as $key => $valueIgnored) {
+                    unset($coll[$key]);
+                }
+            }
+
             $coll->takeSnapshot();
         }
 
@@ -487,15 +505,16 @@ public function commit($entity = null)
     /** @param object|object[]|null $entity */
     private function postCommitCleanup($entity): void
     {
-        $this->entityInsertions               =
-        $this->entityUpdates                  =
-        $this->entityDeletions                =
-        $this->extraUpdates                   =
-        $this->collectionUpdates              =
-        $this->nonCascadedNewDetectedEntities =
-        $this->collectionDeletions            =
-        $this->visitedCollections             =
-        $this->orphanRemovals                 = [];
+        $this->entityInsertions                 =
+        $this->entityUpdates                    =
+        $this->entityDeletions                  =
+        $this->extraUpdates                     =
+        $this->collectionUpdates                =
+        $this->nonCascadedNewDetectedEntities   =
+        $this->collectionDeletions              =
+        $this->pendingCollectionElementRemovals =
+        $this->visitedCollections               =
+        $this->orphanRemovals                   = [];
 
         if ($entity === null) {
             $this->entityChangeSets = $this->scheduledForSynchronization = [];
@@ -916,6 +935,14 @@ private function computeAssociationChanges(array $assoc, $value): void
             return;
         }
 
+        // If this collection is dirty, schedule it for updates
+        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.
@@ -960,10 +987,18 @@ private function computeAssociationChanges(array $assoc, $value): void
                 case self::STATE_REMOVED:
                     // Consume the $value as array (it's either an array or an ArrayAccess)
                     // and remove the element from Collection.
-                    if ($assoc['type'] & ClassMetadata::TO_MANY) {
-                        unset($value[$key]);
+                    if (! ($assoc['type'] & ClassMetadata::TO_MANY)) {
+                        break;
                     }
 
+                    $coid                            = spl_object_id($value);
+                    $this->visitedCollections[$coid] = $value;
+
+                    if (! isset($this->pendingCollectionElementRemovals[$coid])) {
+                        $this->pendingCollectionElementRemovals[$coid] = [];
+                    }
+
+                    $this->pendingCollectionElementRemovals[$coid][$key] = true;
                     break;
 
                 case self::STATE_DETACHED:
@@ -976,13 +1011,6 @@ 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;
-        }
     }
 
     /**
@@ -2627,23 +2655,24 @@ public function getCommitOrderCalculator()
     public function clear($entityName = null)
     {
         if ($entityName === null) {
-            $this->identityMap                    =
-            $this->entityIdentifiers              =
-            $this->originalEntityData             =
-            $this->entityChangeSets               =
-            $this->entityStates                   =
-            $this->scheduledForSynchronization    =
-            $this->entityInsertions               =
-            $this->entityUpdates                  =
-            $this->entityDeletions                =
-            $this->nonCascadedNewDetectedEntities =
-            $this->collectionDeletions            =
-            $this->collectionUpdates              =
-            $this->extraUpdates                   =
-            $this->readOnlyObjects                =
-            $this->visitedCollections             =
-            $this->eagerLoadingEntities           =
-            $this->orphanRemovals                 = [];
+            $this->identityMap                      =
+            $this->entityIdentifiers                =
+            $this->originalEntityData               =
+            $this->entityChangeSets                 =
+            $this->entityStates                     =
+            $this->scheduledForSynchronization      =
+            $this->entityInsertions                 =
+            $this->entityUpdates                    =
+            $this->entityDeletions                  =
+            $this->nonCascadedNewDetectedEntities   =
+            $this->collectionDeletions              =
+            $this->collectionUpdates                =
+            $this->extraUpdates                     =
+            $this->readOnlyObjects                  =
+            $this->pendingCollectionElementRemovals =
+            $this->visitedCollections               =
+            $this->eagerLoadingEntities             =
+            $this->orphanRemovals                   = [];
         } else {
             Deprecation::triggerIfCalledFromOutside(
                 'doctrine/orm',

tests/Doctrine/Tests/ORM/Functional/GH10752Test.php

@@ -0,0 +1,125 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\Common\Collections\Collection;
+use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException;
+use Doctrine\ORM\Mapping as ORM;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+/**
+ * @group GH10752
+ */
+class GH10752Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->setUpEntitySchema([
+            GH10752Order::class,
+            GH10752Promotion::class,
+        ]);
+    }
+
+    public function testThrowExceptionWhenRemovingPromotionThatIsInUse(): void
+    {
+        if (! $this->_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) {
+            self::markTestSkipped('Platform does not support foreign keys.');
+        }
+
+        $order     = new GH10752Order();
+        $promotion = new GH10752Promotion();
+
+        $order->addPromotion($promotion);
+
+        $this->_em->persist($order);
+        $this->_em->persist($promotion);
+        $this->_em->flush();
+
+        $this->_em->remove($promotion);
+
+        $this->expectException(ForeignKeyConstraintViolationException::class);
+        $this->_em->flush();
+    }
+
+    public function testThrowExceptionWhenRemovingPromotionThatIsInUseAndOrderIsNotInMemory(): void
+    {
+        if (! $this->_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) {
+            self::markTestSkipped('Platform does not support foreign keys.');
+        }
+
+        $order     = new GH10752Order();
+        $promotion = new GH10752Promotion();
+
+        $order->addPromotion($promotion);
+
+        $this->_em->persist($order);
+        $this->_em->persist($promotion);
+        $this->_em->flush();
+
+        $this->_em->clear();
+
+        $promotion = $this->_em->find(GH10752Promotion::class, $promotion->id);
+        $this->_em->remove($promotion);
+
+        $this->expectException(ForeignKeyConstraintViolationException::class);
+        $this->_em->flush();
+    }
+}
+
+/**
+ * @ORM\Entity
+ */
+class GH10752Order
+{
+    /**
+     * @ORM\Id
+     * @ORM\GeneratedValue
+     * @ORM\Column(type="integer")
+     *
+     * @var int
+     */
+    private $id = null;
+
+    /**
+     * @ORM\ManyToMany(targetEntity="GH10752Promotion", cascade={"persist"})
+     * @ORM\JoinTable(name="order_promotion",
+     *      joinColumns={@ORM\JoinColumn(name="order_id", referencedColumnName="id", onDelete="CASCADE")},
+     *      inverseJoinColumns={@ORM\JoinColumn(name="promotion_id", referencedColumnName="id")}
+     * )
+     *
+     * @var Collection
+     */
+    private $promotions;
+
+    public function __construct()
+    {
+        $this->promotions = new ArrayCollection();
+    }
+
+    public function addPromotion(GH10752Promotion $promotion): void
+    {
+        if (! $this->promotions->contains($promotion)) {
+            $this->promotions->add($promotion);
+        }
+    }
+}
+
+/**
+ * @ORM\Entity
+ */
+class GH10752Promotion
+{
+    /**
+     * @ORM\Id
+     * @ORM\GeneratedValue
+     * @ORM\Column(type="integer")
+     *
+     * @var int
+     */
+    public $id = null;
+}

tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php

@@ -101,6 +101,9 @@ public function testManyToManyAddRemove(): void
         //$user->getGroups()->remove(0);
 
         $this->_em->flush();
+
+        self::assertFalse($user->getGroups()->isDirty());
+
         $this->_em->clear();
 
         // Reload same user
@@ -141,8 +144,13 @@ public function testManyToManyCollectionClearing(): void
 
         $user->groups->clear();
 
+        $this->getQueryLog()->reset()->enable();
         $this->_em->flush();
 
+        // Deletions of entire collections happen in a single query
+        $this->removeTransactionCommandsFromQueryLog();
+        self::assertQueryCount(1);
+
         // Check that the links in the association table have been deleted
         $this->assertGblancoGroupCountIs(0);
     }
@@ -212,28 +220,61 @@ public function testRetrieveManyToManyAndAddMore(): void
     /** @group DDC-130 */
     public function testRemoveUserWithManyGroups(): void
     {
-        $user   = $this->addCmsUserGblancoWithGroups(2);
+        $user   = $this->addCmsUserGblancoWithGroups(10);
         $userId = $user->getId();
 
         $this->_em->remove($user);
+
+        $this->getQueryLog()->reset()->enable();
+
         $this->_em->flush();
 
+        // This takes three queries: One to delete all user -> group join table rows for the user,
+        // one to delete all user -> tags join table rows for the user, and a final one to delete the user itself.
+        $this->removeTransactionCommandsFromQueryLog();
+        self::assertQueryCount(3);
+
         $newUser = $this->_em->find(get_class($user), $userId);
         self::assertNull($newUser);
     }
 
     /** @group DDC-130 */
     public function testRemoveGroupWithUser(): void
     {
-        $user = $this->addCmsUserGblancoWithGroups(2);
+        $user = $this->addCmsUserGblancoWithGroups(5);
+
+        $anotherUser           = new CmsUser();
+        $anotherUser->username = 'joe_doe';
+        $anotherUser->name     = 'Joe Doe';
+        $anotherUser->status   = 'QA Engineer';
+
+        foreach ($user->getGroups() as $group) {
+            $anotherUser->addGroup($group);
+        }
+
+        $this->_em->persist($anotherUser);
+        $this->_em->flush();
 
         foreach ($user->getGroups() as $group) {
             $this->_em->remove($group);
         }
 
+        $this->getQueryLog()->reset()->enable();
         $this->_em->flush();
+
+        // This takes 5 * 2 queries – for each group to be removed, one to remove all join table rows
+        // for the CmsGroup -> CmsUser inverse side association (for both users at once),
+        // and one for the group itself.
+        $this->removeTransactionCommandsFromQueryLog();
+        self::assertQueryCount(10);
+
+        // Changes to in-memory collection have been made and flushed
+        self::assertCount(0, $user->getGroups());
+        self::assertFalse($user->getGroups()->isDirty());
+
         $this->_em->clear();
 
+        // Changes have been made to the database
         $newUser = $this->_em->find(get_class($user), $user->getId());
         self::assertCount(0, $newUser->getGroups());
     }
@@ -243,7 +284,13 @@ public function testDereferenceCollectionDelete(): void
         $user         = $this->addCmsUserGblancoWithGroups(2);
         $user->groups = null;
 
+        $this->getQueryLog()->reset()->enable();
         $this->_em->flush();
+
+        // It takes one query to remove all join table rows for the user at once
+        $this->removeTransactionCommandsFromQueryLog();
+        self::assertQueryCount(1);
+
         $this->_em->clear();
 
         $newUser = $this->_em->find(get_class($user), $user->getId());
@@ -528,4 +575,15 @@ public function testMatching(): void
 
         self::assertFalse($user->groups->isInitialized(), 'Post-condition: matching does not initialize collection');
     }
+
+    private function removeTransactionCommandsFromQueryLog(): void
+    {
+        $log = $this->getQueryLog();
+
+        foreach ($log->queries as $key => $entry) {
+            if ($entry['sql'] === '"START TRANSACTION"' || $entry['sql'] === '"COMMIT"') {
+                unset($log->queries[$key]);
+            }
+        }
+    }
 }

tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php

@@ -30,32 +30,34 @@ protected function setUp(): void
 
     public function testListenerShouldBeNotifiedWhenNewCollectionEntryAdded(): void
     {
-        $user = $this->createNewValidUser();
+        $user        = $this->createNewValidUser();
+        $group       = new CmsGroup();
+        $group->name = 'admins';
+
         $this->_em->persist($user);
+        $this->_em->persist($group);
         $this->_em->flush();
         self::assertFalse($this->listener->wasNotified);
 
-        $group       = new CmsGroup();
-        $group->name = 'admins';
         $user->addGroup($group);
-        $this->_em->persist($user);
         $this->_em->flush();
 
         self::assertTrue($this->listener->wasNotified);
     }
 
-    public function testListenerShouldBeNotifiedWhenNewCollectionEntryRemoved(): void
+    public function testListenerShouldBeNotifiedWhenCollectionEntryRemoved(): void
     {
         $user        = $this->createNewValidUser();
         $group       = new CmsGroup();
         $group->name = 'admins';
         $user->addGroup($group);
 
         $this->_em->persist($user);
+        $this->_em->persist($group);
         $this->_em->flush();
         self::assertFalse($this->listener->wasNotified);
 
-        $this->_em->remove($group);
+        $user->getGroups()->removeElement($group);
         $this->_em->flush();
 
         self::assertTrue($this->listener->wasNotified);