Skip to content

Commit

Permalink
Defer removing removed entities from to-many collections until after …
Browse files Browse the repository at this point in the history
…transaction commit
  • Loading branch information
mpdude committed Jun 5, 2023
1 parent 76b8a21 commit 1620137
Show file tree
Hide file tree
Showing 4 changed files with 239 additions and 61 deletions.
120 changes: 65 additions & 55 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<int, array<array-key, true>>
*/
private $pendingCollectionElementRemovals = [];

/**
* The EntityManager that "owns" this UnitOfWork instance.
*
Expand Down Expand Up @@ -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();
}

Expand All @@ -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 = [];
Expand Down Expand Up @@ -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.
Expand All @@ -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']) {
Expand All @@ -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;
}
}

/**
Expand Down Expand Up @@ -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',
Expand Down
157 changes: 157 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/GH10752Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
<?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 = $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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
}
Expand Down
Loading

0 comments on commit 1620137

Please sign in to comment.