[INCOMPLETE] Since v2.19.5 $em->remove() needs explicit $em->flush() call to reflect current object state

[mislavjakopovic]

BC Break Report

Q	A
BC Break	yes
Version	2.19.5

Summary

The problem arose couple days ago when we updated our project doctrine/orm 2.19.4 => 2.19.5.
For the purposes of explaining I will simplify examples here. Let's assume we have a following entity:

class Parent
{
    #[ORM\Id]
    protected ?int $id = null;
    
    #[ORM\OneToMany(mappedBy: 'parent', targetEntity: Child::class)]
    protected Collection $children;
}

Now we want to keep only children with even IDs and apply some more logic as for example change their names.
Below follows what we do in our service layer.

Previous Behavior

$parent = $em->findOneById(1);
$children = $parent->getChildren(); // ids: [1, 2, 3, 4, 5, 6, 7, 8 ...]

foreach ($children as $child) {
    // Delete children with odd ids
    if ($child->getId() % 2) {
        $entityManager->remove($child);
    }
}

// After these remove() calls identityMap IS UPDATED
// and subsequent calls to $parent->getChildren() only provide us child entities with even IDs
$evenChildren = $parent->getChildren(); // ids: [2, 4, 6, 8 ...]

// Apply some more logic on children entities
$this->handleChangeNamesOfChildren($evenChildren);

// Since all logic has been done, save our changes to database
$em->flush();

Current Behavior

//...

$children = $parent->getChildren(); // ids: [1, 2, 3, 4, 5, 6, 7, 8 ...]

foreach ($children as $child) {
    // Delete children with odd ids
    if ($child->getId() % 2) {
        $entityManager->remove($child);
    }
}

// After these remove() calls identityMap IS NOT UPDATED
// and subsequent calls to $parent->getChildren() provide us ALL child entities
$evenChildren = $parent->getChildren(); // <-- this line is now incorrect, ids: [1, 2, 3, 4, 5, 6, 7, 8 ...]

//...

Workarounds

We've managed to find two possible ways how to fix this problem.

First one being that we access entity manager's UnitOfWork and do a direct call toward removeFromIdentityMap,
but since this function is marked as internal, this doesn't seem like a good solution.

//...
foreach ($parent->getChildren() as $child) {
    // Delete children with odd ids
    if ($child->getId() % 2) {
        $entityManager->remove($child);
        $unitOfWork = $this->entityManager->getUnitOfWork();
        $unitOfWork->removeFromIdentityMap($entry);
    }
}
//...

Second approach is to just do explicit call on $entityManager->flush()

//...
foreach ($parent->getChildren() as $child) {
    // Delete children with odd ids
    if ($child->getId() % 2) {
        $entityManager->remove($child);
        $entityManager->flush();
    }
}
//...

Another approach might be to manually re-set the fields of our parent again to array containing only odd children, but with this we're losing already implemented functionality and elegance which comes from having entity manager.

Solution

With keeping the current ORM code, obvious solution would be to refactor our app code to have flush() call after every remove(). However, with this approach having entity manager and flush seems to start to lose sense.

The changes we make via entity manager should be reflected inside an identityMap and therefore
accessible in entity manager straight away.

It is my understanding that flush was always there for committing those in memory changes to database.
Not for refreshing current state of our objects. With current approach, think of it like this, would we ever introduce having flush call behind every persist?

for($i = 0; $i < 10; $i++) {
    $entity = new Entity();
    $entityManager->persist($entity);
    $entityManager->flush();
}

Root Cause

#11428

In this pull request a change was introduced where we would remove entity from identityMap
only upon executing deletion queries (UnitOfWork::executeDeletions) rather than straight away (UnitOfWork::scheduleForDelete)

I do agree with points raised there by @xificurk.
Every developer using Doctrine has met with rogue Proxy object sooner or later due to issues he tried to fix there (and I'd like to really thank him for that), but I believe that solution should be more discussed, looked at closer and potentially reverted.

[xificurk]

@mislavjakopovic Could you please provide a test case to demonstrate the BC break? I think there is some important detail missing from the example snippets you've provided, because removing (or not) entity from identity map should not affect the contents of loaded collection.

[mislavjakopovic]

@xificurk thank you for your reply. I will check it (might take some time as its quite complex) and try to come up with a test case repo.

[mislavjakopovic]

After going through code I found that the issue is more complex.

Closing this one in favor of #11448

@xificurk I'd appreciate if you can take a look.