From ff5ce3e677fce355ca686f0fdee44edd9936bf35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20=C5=A0upka?= Date: Tue, 8 Feb 2022 10:53:09 +0100 Subject: [PATCH] Compute particular commit order for deletions This fixes a bug which arises in some cases when trying to delete two entities which have a circular reference through other entities. UnifOfWork computed the order which was not correct for deletions and it failed with ForeignKeyConstraintViolationException. To fix that, this adds a new particular function getCommitOrderForDeletions() to compute the right order. This function is similar to getCommitOrder(), but it operates only with entities set to be deleted. This also adds test for this issue. --- lib/Doctrine/ORM/UnitOfWork.php | 48 ++++- psalm-baseline.xml | 2 +- .../ORM/Functional/Ticket/GH9376Test.php | 204 ++++++++++++++++++ 3 files changed, 251 insertions(+), 3 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH9376Test.php diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index a07abb49c7f..729e1447690 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -445,8 +445,9 @@ public function commit($entity = null) // Entity deletions come last and need to be in reverse commit order if ($this->entityDeletions) { - for ($count = count($commitOrder), $i = $count - 1; $i >= 0 && $this->entityDeletions; --$i) { - $this->executeDeletions($commitOrder[$i]); + $commitOrderForDeletions = $this->getCommitOrderForDeletions(); + for ($count = count($commitOrderForDeletions), $i = $count - 1; $i >= 0 && $this->entityDeletions; --$i) { + $this->executeDeletions($commitOrderForDeletions[$i]); } } @@ -1331,6 +1332,49 @@ private function getCommitOrder(): array return $calc->sort(); } + /** + * Gets the commit order for deletions. + * + * @return list + */ + private function getCommitOrderForDeletions(): array + { + $calc = $this->getCommitOrderCalculator(); + + $newNodes = []; + + foreach ($this->entityDeletions as $entity) { + $class = $this->em->getClassMetadata(get_class($entity)); + + if ($calc->hasNode($class->name)) { + continue; + } + + $calc->addNode($class->name, $class); + + $newNodes[] = $class; + } + + // Calculate dependencies for new nodes + while ($class = array_pop($newNodes)) { + foreach ($class->associationMappings as $assoc) { + if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) { + continue; + } + + $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); + + $joinColumns = reset($assoc['joinColumns']); + + if ($calc->hasNode($targetClass->name)) { + $calc->addDependency($targetClass->name, $class->name, (int) empty($joinColumns['nullable'])); + } + } + } + + return $calc->sort(); + } + /** * Schedules an entity for insertion into the database. * If the entity already has an identifier, it will be added to the identity map. diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 259c44495ff..82b05f45c0b 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -3453,7 +3453,7 @@ $class $collectionToDelete $collectionToUpdate - $commitOrder[$i] + $commitOrderForDeletions[$i] ! is_object($object) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9376Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9376Test.php new file mode 100644 index 00000000000..0adaf976875 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH9376Test.php @@ -0,0 +1,204 @@ +_schemaTool->createSchema([ + $this->_em->getClassMetadata(GH9376GiftVariant::class), + $this->_em->getClassMetadata(GH9376OrderGiftVariant::class), + $this->_em->getClassMetadata(GH9376Order::class), + $this->_em->getClassMetadata(GH9376ProductPartner::class), + $this->_em->getClassMetadata(GH9376Product::class), + $this->_em->getClassMetadata(GH9376Gift::class), + ]); + } + + protected function tearDown(): void + { + $this->_schemaTool->dropSchema([ + $this->_em->getClassMetadata(GH9376GiftVariant::class), + $this->_em->getClassMetadata(GH9376OrderGiftVariant::class), + $this->_em->getClassMetadata(GH9376Order::class), + $this->_em->getClassMetadata(GH9376ProductPartner::class), + $this->_em->getClassMetadata(GH9376Product::class), + $this->_em->getClassMetadata(GH9376Gift::class), + ]); + + parent::tearDown(); + } + + public function testIssueRemove(): void + { + $product = new GH9376Product(); + $gift = new GH9376Gift($product); + $giftVariant = new GH9376GiftVariant($gift); + + $this->_em->persist($product); + $this->_em->persist($gift); + $this->_em->persist($giftVariant); + $this->_em->flush(); + $this->_em->clear(); + + $persistedGiftVariant = $this->_em->find(GH9376GiftVariant::class, 1); + $this->_em->remove($persistedGiftVariant); + + $persistedGift = $this->_em->find(GH9376Gift::class, 1); + $this->_em->remove($persistedGift); + + $this->_em->flush(); + $this->_em->clear(); + + self::assertEmpty($this->_em->getRepository(GH9376Gift::class)->findAll()); + self::assertEmpty($this->_em->getRepository(GH9376GiftVariant::class)->findAll()); + } +} + +/** + * @Entity + */ +class GH9376GiftVariant +{ + /** + * @var int + * @Id + * @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH9376Gift::class) + * @ORM\JoinColumn(nullable=false) + * + * @var GH9376Gift + */ + public $gift; + + public function __construct(GH9376Gift $gift) + { + $this->gift = $gift; + } +} + +/** + * @Entity + */ +class GH9376OrderGiftVariant +{ + /** + * @var int + * @Id @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH9376GiftVariant::class) + * @ORM\JoinColumn(nullable=true) + * + * @var GH9376GiftVariant|null + */ + public $giftVariant; +} + +/** + * @Entity + */ +class GH9376Order +{ + /** + * @var int + * @Id @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity=GH9376OrderGiftVariant::class, cascade={"persist"}) + * + * @var GH9376OrderGiftVariant|null + */ + public $orderGiftVariant; +} + +/** + * @Entity + */ +class GH9376ProductPartner +{ + /** + * @var int + * @Id @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity=GH9376Order::class) + * @ORM\JoinColumn(nullable=true) + * + * @var GH9376Order|null + */ + public $order = null; +} + +/** + * @Entity + */ +class GH9376Product +{ + /** + * @var int + * @Id @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity=GH9376ProductPartner::class) + * @ORM\JoinColumn(nullable=true) + * + * @var GH9376ProductPartner|null + */ + public $productPartner = null; +} + +/** + * @Entity + */ +class GH9376Gift +{ + /** + * @var int + * @Id @Column(type="integer") + * @GeneratedValue + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH9376Product::class) + * @ORM\JoinColumn(nullable=false) + * + * @var GH9376Product + */ + public $product; + + public function __construct(GH9376Product $product) + { + $this->product = $product; + } +}