Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cascade detach should detach lazy collections too #11717

Open
wants to merge 2 commits into
base: 2.20.x
Choose a base branch
from
+50 −26
Open

Cascade detach should detach lazy collections too #11717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75fa53c
cascade detach should detach lazy collections too
goetas Nov 15, 2024
56ce570
something is odd with sqlite
goetas Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions src/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?php

Check failure on line 1 in src/UnitOfWork.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm

UnusedBaselineEntry 

src/UnitOfWork.php:0:0: UnusedBaselineEntry: Baseline for issue "PossiblyUndefinedMethod" has 1 extra entry. (see https://psalm.dev/316)

declare(strict_types=1);

Expand Down Expand Up @@ -2390,29 +2390,27 @@

$visited[$oid] = $entity; // mark visited

switch ($this->getEntityState($entity, self::STATE_DETACHED)) {
case self::STATE_MANAGED:
if ($this->isInIdentityMap($entity)) {
$this->removeFromIdentityMap($entity);
}
$state = $this->getEntityState($entity, self::STATE_DETACHED);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make this changes because "child" objects should be detached first, and then the parents (otherwise the IDs are not available for some DBs as sqlite)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this is going to break some people event listeners :)

We should add this the UPGRADE notes to warn people. We don't guarantee the order so its not a BC break, but a heads up is always nice.

if (! $noCascade && $state === self::STATE_MANAGED) {
$this->cascadeDetach($entity, $visited);
}

unset(
$this->entityInsertions[$oid],
$this->entityUpdates[$oid],
$this->entityDeletions[$oid],
$this->entityIdentifiers[$oid],
$this->entityStates[$oid],
$this->originalEntityData[$oid]
);
break;
case self::STATE_NEW:
case self::STATE_DETACHED:
return;
if ($state !== self::STATE_MANAGED) {
return;
}

if (! $noCascade) {
$this->cascadeDetach($entity, $visited);
if ($this->isInIdentityMap($entity)) {
$this->removeFromIdentityMap($entity);
}

unset(
$this->entityInsertions[$oid],
$this->entityUpdates[$oid],
$this->entityDeletions[$oid],
$this->entityIdentifiers[$oid],
$this->entityStates[$oid],
$this->originalEntityData[$oid]
);
}

/**
Expand Down Expand Up @@ -2548,11 +2546,7 @@
$relatedEntities = $class->reflFields[$assoc['fieldName']]->getValue($entity);

switch (true) {
case $relatedEntities instanceof PersistentCollection:
// Unwrap so that foreach() does not initialize
$relatedEntities = $relatedEntities->unwrap();
// break; is commented intentionally!

// in order to detach the entities in the collection, initialization is needed (no unwrap)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented in the docs, because it could lead to the operation becoming very expensive.

It should also be documented in upgrade notes.

You could also implement a shortcut here, if no entity of $assoc entity is in the identity map, then loading the collection from database will not yield any benefit.

For both lazy and extra lazy you could also make a query to load only the IDs, then check if they are in the identity map to detech. This would make the query + loading cheaper. The reaosn is that the entities loaded here are immediately going out of scope and garbage collected anyways.

case $relatedEntities instanceof Collection:
case is_array($relatedEntities):
foreach ($relatedEntities as $relatedEntity) {
Expand Down
34 changes: 32 additions & 2 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/LazyEagerCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,36 @@ public function testRefreshRefreshesBothLazyAndEagerCollections(): void
self::assertSame('12345', $ph->data);
self::assertSame('6789', $ad->data);
}

public function testCascadeDetachBothLazyAndEagerCollections(): void
{
$user = new LazyEagerCollectionUser();
$user->data = 'Guilherme';

$ph = new LazyEagerCollectionPhone();
$ph->data = '12345';
$user->addPhone($ph);

$ad = new LazyEagerCollectionAddress();
$ad->data = '6789';
$user->addAddress($ad);

$this->_em->persist($user);
$this->_em->persist($ad);
$this->_em->persist($ph);
$this->_em->flush();
$this->_em->clear();

$user = $this->_em->find(LazyEagerCollectionUser::class, $user->id);
$this->_em->detach($user);

$ph = $user->phones[0];
$ad = $user->addresses[0];

self::assertFalse($this->_em->contains($user));
self::assertFalse($this->_em->contains($ph));
self::assertFalse($this->_em->contains($ad));
}
}

/**
Expand All @@ -78,14 +108,14 @@ class LazyEagerCollectionUser
public $data;

/**
* @ORM\OneToMany(targetEntity="LazyEagerCollectionPhone", cascade={"refresh"}, fetch="EAGER", mappedBy="user")
* @ORM\OneToMany(targetEntity="LazyEagerCollectionPhone", cascade={"refresh", "detach"}, fetch="EAGER", mappedBy="user")
*
* @var LazyEagerCollectionPhone[]
*/
public $phones;

/**
* @ORM\OneToMany(targetEntity="LazyEagerCollectionAddress", cascade={"refresh"}, mappedBy="user")
* @ORM\OneToMany(targetEntity="LazyEagerCollectionAddress", cascade={"refresh", "detach"}, mappedBy="user")
*
* @var LazyEagerCollectionAddress[]
*/
Expand Down
Loading