Skip to content

Commit

Permalink
Make sure in-memory collections have removed entities unset before th…
Browse files Browse the repository at this point in the history
…e `postRemove` event is dispatched. This is related to #11098, although by itself probably not going to fix the regression.

#### Background 

When an entity is removed from the database, the UoW also takes care of removing that entity from all in-memory collections. #10763 moved this cleanup and taking snapshots of updated collections to a very late stage during the commit phase, in order to avoid other side effects.

Now, part of the issue in #11098 is that `postRemove` event listeners will be called at a point where the database-level `DELETE` has happened (although the transaction has not yet been committed), but the in-memory collections have not yet been updated.

#### Suggested change

This PR moves the code part that unsets removed entities from in-memory collections and takes collection snapshots (makes collections "clean") from after transaction commit to before the `postRemove` event. That brings the in-memory update closer to the database-level execution.

In the case of a transaction failure/abort, this leaves us with updated and snapshotted in-memory collections, but no database-level updates. But, at this point, probably things got inconsistent anyways, the EM will be closed and we need not be too worried about the state.
  • Loading branch information
mpdude committed Jan 2, 2024
1 parent c288647 commit 6d72e24
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 4 deletions.
13 changes: 9 additions & 4 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,8 @@ public function commit($entity = null)
// (first delete entities depending upon others, before deleting depended-upon entities).
if ($this->entityDeletions) {
$this->executeDeletions();
} else {
$this->finalizeCollectionUpdates();
}

// Commit failed silently
Expand All @@ -490,7 +492,12 @@ public function commit($entity = null)
}

$this->afterTransactionComplete();
$this->dispatchPostFlushEvent();
$this->postCommitCleanup($entity);
}

private function finalizeCollectionUpdates(): void
{
// Unset removed entities from collections, and take new snapshots from
// all visited collections.
foreach ($this->visitedCollections as $coid => $coll) {
Expand All @@ -502,10 +509,6 @@ public function commit($entity = null)

$coll->takeSnapshot();
}

$this->dispatchPostFlushEvent();

$this->postCommitCleanup($entity);
}

/** @param object|object[]|null $entity */
Expand Down Expand Up @@ -1317,6 +1320,8 @@ private function executeDeletions(): void
}
}

$this->finalizeCollectionUpdates();

// Defer dispatching `postRemove` events to until all entities have been removed.
foreach ($eventsToDispatch as $event) {
$this->listenersInvoker->invoke(
Expand Down
42 changes: 42 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/EntityListenersTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Doctrine\Tests\ORM\Functional;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ORM\Event\PostPersistEventArgs;
use Doctrine\ORM\Event\PostRemoveEventArgs;
use Doctrine\ORM\Event\PreFlushEventArgs;
Expand All @@ -12,6 +13,7 @@
use Doctrine\ORM\UnitOfWork;
use Doctrine\Persistence\Event\LifecycleEventArgs;
use Doctrine\Tests\Models\Company\CompanyContractListener;
use Doctrine\Tests\Models\Company\CompanyEmployee;
use Doctrine\Tests\Models\Company\CompanyFixContract;
use Doctrine\Tests\Models\Company\CompanyPerson;
use Doctrine\Tests\OrmFunctionalTestCase;
Expand Down Expand Up @@ -266,4 +268,44 @@ public function postRemove(PostRemoveEventArgs $args): void

self::assertSame(2, $listener->invocationCount);
}

public function testPostRemoveCalledAfterAllInMemoryCollectionsHaveBeenUpdated(): void
{
$contract = new CompanyFixContract();
$contract->setFixPrice(2000);

$engineer = new CompanyEmployee();
$engineer->setName('J. Doe');
$engineer->setSalary(50);
$engineer->setDepartment('tech');

$contract->addEngineer($engineer);
$engineer->contracts = new ArrayCollection([$contract]);

$this->_em->persist($contract);
$this->_em->persist($engineer);
$this->_em->flush();

$this->_em->getEventManager()->addEventListener([Events::postRemove], new class ($contract) {
/** @var CompanyFixContract */
private $contract;

public function __construct(CompanyFixContract $contract)
{
$this->contract = $contract;
}

public function postRemove(): void
{
Assert::assertEmpty($this->contract->getEngineers()); // Assert collection has been updated before event was dispatched
Assert::assertFalse($this->contract->getEngineers()->isDirty()); // Collections are clean at this point
}
});

$this->_em->remove($engineer);
$this->_em->flush();

self::assertEmpty($contract->getEngineers());
self::assertFalse($contract->getEngineers()->isDirty());
}
}

0 comments on commit 6d72e24

Please sign in to comment.