diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4c343f2ec93..57adb604ca3 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1380,20 +1380,6 @@ private function computeDeleteExecutionOrder(): array continue; } - // 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 @@ -1403,9 +1389,53 @@ private function computeDeleteExecutionOrder(): array continue; } + $optionalDependency = false; + + // Check if there is a database-level cascade operation that would affect + // $entity when $targetEntity is removed. + assert(isset($assoc['joinColumns'])); + $joinColumns = reset($assoc['joinColumns']); + if (isset($joinColumns['onDelete'])) { + $onDeleteOption = strtolower($joinColumns['onDelete']); + // SET NULL: Deletion of $targetEntity would NULL out the association + // at $entity. This is unproblematic: We don't care (with regard to + // that particular association) whether $targetEntity is deleted first, + // since the cascading operation would stop at $entity and leave it in + // place. Skip this association. + if ($onDeleteOption === 'set null') { + continue; + } + + // CASCADE deletes: Deletion of $targetEntity means that $entity + // will be removed by the DBMS at the same time. So, when there + // are other entities that depend upon and need to be removed + // before $entity, they transitively also need to be removed + // before $targetEntity. That means we need to consider this + // as a dependency edge. + // + // There may be cases (see GH-10912) where we have circular + // ON DELETE CASCADE constraints at the RDBMS level, but it + // seems DBMSes can deal with this just fine. In order not to + // bail out with a CircularReferenceException, treat this kind + // of association as "optional" - this means cycles can be broken + // when necessary. As long as there are no cycles, the order + // is honored. + // + // I am not sure this 100% correct, considering the following case: + // A has a not-nullable FK to B + // B has a not-nullable FK to C + // A has a FK to C with CASCADE DELETE, and vice versa + // C has a FK to A with CASCADE DELETE. + // This is unsolvable, since deleting A or C will immediately remove the + // other one as well; but we also require to remove A _before_ B _before_ C. + if ($onDeleteOption === 'cascade') { + $optionalDependency = true; + } + } + // 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); + $sort->addEdge($entity, $targetEntity, $optionalDependency); } } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php new file mode 100644 index 00000000000..b8f7ffecba8 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php @@ -0,0 +1,166 @@ +setUpEntitySchema([ + GH10912User::class, + GH10912Profile::class, + GH10912Room::class, + ]); + } + + public function testIssue(): void + { + $user = new GH10912User(); + $profile = new GH10912Profile(); + $room = new GH10912Room(); + + $user->rooms->add($room); + $user->profile = $profile; + $profile->user = $user; + $room->user = $user; + + $this->_em->persist($room); + $this->_em->persist($user); + $this->_em->persist($profile); + $this->_em->flush(); + + /* + * This issue is about finding a special deletion order: + * $user and $profile cross-reference each other with ON DELETE CASCADE. + * So, whichever one gets deleted first, the DBMS will immediately dispose + * of the other one as well. + * + * $user -> $room is the unproblematic (irrelevant) inverse side of + * a OneToMany association. + * + * $room -> $user is a not-nullable, no DBMS-level-cascade, owning side + * of ManyToOne. We *must* remove the $room _before_ the $user can be + * deleted. And remember, $user deletion happens either when we DELETE the + * user (direct deletion), or when we delete the $profile (ON DELETE CASCADE + * propagates to the user). + * + * In the original bug report, the ordering of fields in the entities was + * relevant, in combination with a cascade=persist configuration. + * + * But, for the sake of clarity, let's put these features away and create + * the problematic sequence in UnitOfWork::$entityDeletions directly: + */ + $this->_em->remove($profile); + $this->_em->remove($user); + $this->_em->remove($room); + + $queryLog = $this->getQueryLog(); + $queryLog->reset()->enable(); + + $this->_em->flush(); + + $queries = array_values(array_filter($queryLog->queries, static function ($entry) { + return strpos($entry['sql'], 'DELETE') === 0; + })); + + self::assertCount(3, $queries); + + // $room deletion is the first query + // we do not care about the order of $user vs. $profile, so do not check them. + self::assertSame('DELETE FROM GH10912Room WHERE id = ?', $queries[0]['sql']); + + // The EntityManager is aware that all three entities have been deleted (sanity check) + $im = $this->_em->getUnitOfWork()->getIdentityMap(); + self::assertEmpty($im[GH10912Profile::class]); + self::assertEmpty($im[GH10912User::class]); + self::assertEmpty($im[GH10912Room::class]); + } +} + +/** @ORM\Entity */ +class GH10912User +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user") + * + * @var Collection + */ + public $rooms; + + /** + * @ORM\OneToOne(targetEntity=GH10912Profile::class) + * @ORM\JoinColumn(onDelete="cascade") + * + * @var GH10912Profile + */ + public $profile; + + public function __construct() + { + $this->rooms = new ArrayCollection(); + } +} + +/** @ORM\Entity */ +class GH10912Profile +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity=GH10912User::class) + * @ORM\JoinColumn(onDelete="cascade") + * + * @var GH10912User + */ + public $user; +} + +/** @ORM\Entity */ +class GH10912Room +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH10912User::class, inversedBy="rooms") + * @ORM\JoinColumn(nullable=false) + * + * @var GH10912User + */ + public $user; +}