From 1620137fc271d009ecb211dde8ba047f27a3decc Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Mon, 5 Jun 2023 17:22:46 +0000 Subject: [PATCH] Defer removing removed entities from to-many collections until after transaction commit --- lib/Doctrine/ORM/UnitOfWork.php | 120 +++++++------ .../Tests/ORM/Functional/GH10752Test.php | 157 ++++++++++++++++++ .../ManyToManyBasicAssociationTest.php | 9 + .../ORM/Functional/ManyToManyEventTest.php | 14 +- 4 files changed, 239 insertions(+), 61 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/GH10752Test.php diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index bc8af25fbff..5982947b247 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -24,7 +24,6 @@ use Doctrine\ORM\Event\PreRemoveEventArgs; use Doctrine\ORM\Event\PreUpdateEventArgs; use Doctrine\ORM\Exception\ORMException; -use Doctrine\ORM\Exception\UnexpectedAssociationValue; use Doctrine\ORM\Id\AssignedGenerator; use Doctrine\ORM\Internal\CommitOrderCalculator; use Doctrine\ORM\Internal\HydrationCompleteHandler; @@ -240,6 +239,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> + */ + private $pendingCollectionElementRemovals = []; + /** * The EntityManager that "owns" this UnitOfWork instance. * @@ -474,8 +484,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 +504,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 +934,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. @@ -929,15 +955,6 @@ private function computeAssociationChanges(array $assoc, $value): void $state = $this->getEntityState($entry, self::STATE_NEW); - if (! ($entry instanceof $assoc['targetEntity'])) { - throw UnexpectedAssociationValue::create( - $assoc['sourceEntity'], - $assoc['fieldName'], - get_debug_type($entry), - $assoc['targetEntity'] - ); - } - switch ($state) { case self::STATE_NEW: if (! $assoc['isCascadePersist']) { @@ -960,29 +977,21 @@ 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; } - break; + $coid = spl_object_id($value); + $this->visitedCollections[$coid] = $value; - case self::STATE_DETACHED: - // Can actually not happen right now as we assume STATE_NEW, - // so the exception will be raised from the DBAL layer (constraint violation). - throw ORMInvalidArgumentException::detachedEntityFoundThroughRelationship($assoc, $entry); + if (! isset($this->pendingCollectionElementRemovals[$coid])) { + $this->pendingCollectionElementRemovals[$coid] = []; + } - default: - // MANAGED associated entities are already taken into account - // during changeset calculation anyway, since they are in the identity map. + $this->pendingCollectionElementRemovals[$coid][$key] = true; + break; } } - - if ($value instanceof PersistentCollection && $value->isDirty()) { - $coid = spl_object_id($value); - - $this->collectionUpdates[$coid] = $value; - $this->visitedCollections[$coid] = $value; - } } /** @@ -2627,23 +2636,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', diff --git a/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php b/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php new file mode 100644 index 00000000000..58d9ba2eeb5 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/GH10752Test.php @@ -0,0 +1,157 @@ +setUpEntitySchema([ + GH10752Order::class, + GH10752Promotion::class, + ]); + } + + public function testThrowExceptionWhenRemovingPromotionThatIsInUse(): void + { + if (! $this->_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) { + self::markTestSkipped('Platform does not support foreign keys.'); + } + + $order = $this->createOrder(); + $promotion = $this->createPromotion(); + + $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 = $this->createOrder(); + $promotion = $this->createPromotion(); + + $order->addPromotion($promotion); + + $this->_em->persist($order); + $this->_em->persist($promotion); + $this->_em->flush(); + + $this->_em->clear(); + + $promotion = $this->_em->find(GH10752Promotion::class, $promotion->getId()); + $this->_em->remove($promotion); + + $this->expectException(ForeignKeyConstraintViolationException::class); + $this->_em->flush(); + } + + private function createOrder(): GH10752Order + { + return new GH10752Order(); + } + + private function createPromotion(): GH10752Promotion + { + return new GH10752Promotion(); + } +} + +/** + * @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 getId(): ?int + { + return $this->id; + } + + public function getPromotions(): Collection + { + return $this->promotions; + } + + public function addPromotion(GH10752Promotion $promotion): void + { + if (! $this->promotions->contains($promotion)) { + $this->promotions->add($promotion); + } + } + + public function removePromotion(GH10752Promotion $promotion): void + { + if ($this->promotions->contains($promotion)) { + $this->promotions->removeElement($promotion); + } + } +} + +/** + * @ORM\Entity + */ +class GH10752Promotion +{ + /** + * @ORM\Id + * @ORM\GeneratedValue + * @ORM\Column(type="integer") + * + * @var int + */ + private $id = null; + + public function getId(): ?int + { + return $this->id; + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php index 71d63b75152..20fd7163f8a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php +++ b/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 @@ -232,8 +235,14 @@ public function testRemoveGroupWithUser(): void } $this->_em->flush(); + + // 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()); } diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php index 03d547e6b72..d9436acb23f 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyEventTest.php @@ -30,21 +30,22 @@ 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(); @@ -52,10 +53,11 @@ public function testListenerShouldBeNotifiedWhenNewCollectionEntryRemoved(): voi $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);