Commit order for removals has to consider SET NULL, not nullable

lib/Doctrine/ORM/UnitOfWork.php

@@ -28,6 +28,7 @@
 use Doctrine\ORM\Id\AssignedGenerator;
 use Doctrine\ORM\Internal\CommitOrderCalculator;
 use Doctrine\ORM\Internal\HydrationCompleteHandler;
+use Doctrine\ORM\Internal\TopologicalSort;
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Mapping\MappingException;
 use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter;
@@ -57,7 +58,6 @@
 use function array_map;
 use function array_merge;
 use function array_pop;
-use function array_reverse;
 use function array_sum;
 use function array_values;
 use function assert;
@@ -74,6 +74,7 @@
 use function reset;
 use function spl_object_id;
 use function sprintf;
+use function strtolower;
 
 /**
  * The UnitOfWork is responsible for tracking changes to objects during an
@@ -408,9 +409,6 @@ public function commit($entity = null)
 
         $this->dispatchOnFlushEvent();
 
-        // Now we need a commit order to maintain referential integrity
-        $commitOrder = $this->getCommitOrder();
-
         $conn = $this->em->getConnection();
         $conn->beginTransaction();
 
@@ -431,7 +429,7 @@ public function commit($entity = null)
                 // into account (new entities referring to other new entities), since all other types (entities
                 // with updates or scheduled deletions) are currently not a problem, since they are already
                 // in the database.
-                $this->executeInserts($this->computeInsertExecutionOrder($commitOrder));
+                $this->executeInserts($this->computeInsertExecutionOrder());
             }
 
             if ($this->entityUpdates) {
@@ -456,7 +454,7 @@ public function commit($entity = null)
             // Entity deletions come last. Their order only needs to take care of other deletions
             // (first delete entities depending upon others, before deleting depended-upon entities).
             if ($this->entityDeletions) {
-                $this->executeDeletions($this->computeDeleteExecutionOrder($commitOrder));
+                $this->executeDeletions($this->computeDeleteExecutionOrder());
             }
 
             // Commit failed silently
@@ -1265,14 +1263,11 @@ private function executeDeletions(array $entities): void
         }
     }
 
-    /**
-     * @param list<ClassMetadata> $commitOrder
-     *
-     * @return list<object>
-     */
-    private function computeInsertExecutionOrder(array $commitOrder): array
+    /** @return list<object> */
+    private function computeInsertExecutionOrder(): array
     {
-        $result = [];
+        $commitOrder = $this->getCommitOrder();
+        $result      = [];
         foreach ($commitOrder as $class) {
             $className = $class->name;
             foreach ($this->entityInsertions as $entity) {
@@ -1287,26 +1282,58 @@ private function computeInsertExecutionOrder(array $commitOrder): array
         return $result;
     }
 
-    /**
-     * @param list<ClassMetadata> $commitOrder
-     *
-     * @return list<object>
-     */
-    private function computeDeleteExecutionOrder(array $commitOrder): array
+    /** @return list<object> */
+    private function computeDeleteExecutionOrder(): array
     {
-        $result = [];
-        foreach (array_reverse($commitOrder) as $class) {
-            $className = $class->name;
-            foreach ($this->entityDeletions as $entity) {
-                if ($this->em->getClassMetadata(get_class($entity))->name !== $className) {
+        $sort = new TopologicalSort();
+
+        // First make sure we have all the nodes
+        foreach ($this->entityDeletions as $entity) {
+            $sort->addNode($entity);
+        }
+
+        // Now add edges
+        foreach ($this->entityDeletions as $entity) {
+            $class = $this->em->getClassMetadata(get_class($entity));
+
+            foreach ($class->associationMappings as $assoc) {
+                // We only need to consider the owning sides of to-one associations,
+                // since many-to-many associations can always be (and have already been)
+                // deleted in a preceding step.
+                if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) {
                     continue;
                 }
 
-                $result[] = $entity;
+                // For associations that implement a database-level cascade/set null operation,
+                // we do not have to follow a particular order: If the referred-to entity is
+                // deleted first, the DBMS will either delete the current $entity right away
+                // (CASCADE) or temporarily set the foreign key to NULL (SET NULL).
+                // Either way, we can skip it in the computation.
+                assert(isset($assoc['joinColumns']));
+                $joinColumns = reset($assoc['joinColumns']);
+                if (isset($joinColumns['onDelete'])) {
+                    $onDeleteOption = strtolower($joinColumns['onDelete']);
+                    if ($onDeleteOption === 'cascade' || $onDeleteOption === 'set null') {
+                        continue;
+                    }
+                }
+
+                $targetEntity = $class->getFieldValue($entity, $assoc['fieldName']);
+
+                // If the association does not refer to another entity or that entity
+                // is not to be deleted, there is no ordering problem and we can
+                // skip this particular association.
+                if ($targetEntity === null || ! $sort->hasNode($targetEntity)) {
+                    continue;
+                }
+
+                // Add dependency. The dependency direction implies that "$entity has to be removed before $targetEntity",
+                // so we can work through the topo sort result from left to right (with all edges pointing right).
+                $sort->addEdge($entity, $targetEntity, false);
             }
         }
 
-        return $result;
+        return $sort->sort();
     }
 
     /**

tests/Doctrine/Tests/ORM/Functional/Ticket/GH10566Test.php

@@ -0,0 +1,177 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\ORM\Mapping as ORM;
+use Doctrine\Tests\OrmFunctionalTestCase;
+use Generator;
+
+use function is_a;
+
+class GH10566Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->createSchemaForModels(
+            GH10566A::class,
+            GH10566B::class,
+            GH10566C::class
+        );
+    }
+
+    /**
+     * @dataProvider provideEntityClasses
+     */
+    public function testInsertion(string $startEntityClass): void
+    {
+        $a = new GH10566A();
+        $b = new GH10566B();
+        $c = new GH10566C();
+
+        $a->other = $b;
+        $b->other = $c;
+        $c->other = $a;
+
+        foreach ([$a, $b, $c] as $candidate) {
+            if (is_a($candidate, $startEntityClass)) {
+                $this->_em->persist($candidate);
+            }
+        }
+
+        // Since all associations are nullable, the ORM has no problem finding an insert order,
+        // it can always schedule "deferred updates" to fill missing foreign key values.
+        $this->_em->flush();
+
+        self::assertNotNull($a->id);
+        self::assertNotNull($b->id);
+        self::assertNotNull($c->id);
+    }
+
+    /**
+     * @dataProvider provideEntityClasses
+     */
+    public function testRemoval(string $startEntityClass): void
+    {
+        $a = new GH10566A();
+        $b = new GH10566B();
+        $c = new GH10566C();
+
+        $a->other = $b;
+        $b->other = $c;
+        $c->other = $a;
+
+        $this->_em->persist($a);
+        $this->_em->flush();
+
+        $aId = $a->id;
+        $bId = $b->id;
+        $cId = $c->id;
+
+        // In the removal case, the ORM currently does not schedule "extra updates"
+        // to break association cycles before entities are removed. So, we must not
+        // look at "nullable" for associations to find a delete commit order.
+        //
+        // To make it work, the user needs to have a database-level "ON DELETE SET NULL"
+        // on an association. That's where the cycle can be broken. Commit order computation
+        // for the removal case needs to look at this property.
+        //
+        // In this example, only A -> B can be used to break the cycle. So, regardless which
+        // entity we start with, the ORM-level cascade will always remove all three entities,
+        // and the order of database deletes always has to be (can only be) from B, then C, then A.
+
+        foreach ([$a, $b, $c] as $candidate) {
+            if (is_a($candidate, $startEntityClass)) {
+                $this->_em->remove($candidate);
+            }
+        }
+
+        $this->_em->flush();
+
+        self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_a WHERE id = ?', [$aId]));
+        self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_b WHERE id = ?', [$bId]));
+        self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_c WHERE id = ?', [$cId]));
+    }
+
+    public function provideEntityClasses(): Generator
+    {
+        yield [GH10566A::class];
+        yield [GH10566B::class];
+        yield [GH10566C::class];
+    }
+}
+
+/**
+ * @ORM\Entity
+ * @ORM\Table(name="gh10566_a")
+ */
+class GH10566A
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue()
+     *
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @ORM\OneToOne(targetEntity="GH10566B", cascade={"all"})
+     * @ORM\JoinColumn(nullable=true, onDelete="SET NULL")
+     *
+     * @var GH10566B
+     */
+    public $other;
+}
+
+/**
+ * @ORM\Entity
+ * @ORM\Table(name="gh10566_b")
+ */
+class GH10566B
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue()
+     *
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @ORM\OneToOne(targetEntity="GH10566C", cascade={"all"})
+     * @ORM\JoinColumn(nullable=true)
+     *
+     * @var GH10566C
+     */
+    public $other;
+}
+
+/**
+ * @ORM\Entity
+ * @ORM\Table(name="gh10566_c")
+ */
+class GH10566C
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue()
+     *
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @ORM\OneToOne(targetEntity="GH10566A", cascade={"all"})
+     * @ORM\JoinColumn(nullable=true)
+     *
+     * @var GH10566A
+     */
+    public $other;
+}