Regression in `Events::postRemove` event

[onEXHovia]

Bug Report

Q	A
BC Break	yes
Version	2.17.1

Summary

When updating orm with version 2.14.1 to 2.17.1, some events stopped working. These are quite old tests and they still worked.
The idea was that after deleting an entity, check if there are more related entities, if no, then delete the parent(owner).

Current behavior

Parent(owner) entity stopped being deleted.

How to reproduce

namespace App\EventListener;

use Doctrine\Common\EventSubscriber;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Events;
use Doctrine\Persistence\Event\LifecycleEventArgs;
use App\Entity\PullRequest\Comment;

final class NoteAutoRemoveListener implements EventSubscriber
{
    public function getSubscribedEvents(): array
    {
        return [
            Events::postRemove,
        ];
    }

    public function postRemove(LifecycleEventArgs $args): void
    {
        $entity = $args->getObject();
        $entityManager = $args->getObjectManager();

        if ($entity instanceof Comment) {
            $this->removeDiscussionNote($entity, $entityManager);
        }
    }

    private function removeDiscussionNote(Comment $comment, EntityManagerInterface $entityManager): void
    {
        $discussion = $comment->getNote();

        if (null !== $discussion && 0 === $discussion->getComments()->count()) {
            $entityManager->remove($discussion);
        }
    }
}

and test

namespace App\Tests\EventListener;

use Doctrine\ORM\EntityManagerInterface;
use App\Entity\PullRequest\Comment;
use App\Entity\PullRequest\DiscussionNote;
use App\Entity\PullRequest\Note;
use App\Entity\User;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;

final class NoteAutoRemoveListenerTest extends KernelTestCase
{
    private EntityManagerInterface $em;

    protected function setUp(): void
    {
        parent::bootKernel();

        $this->em = self::getContainer()->get(EntityManagerInterface::class);
    }

    public function testRemoveDiscussionNoteAfterRemoveComment(): void
    {
        $pullRequest = $this->createPullRequest(); // method create entity
        $this->em->persist($pullRequest);
        $this->em->flush();

        $comment = (new Comment())
            ->setBody('text')
            ->setUser($this->em->getRepository(User::class)->findOneBy([]))
            ->setPullRequest($pullRequest);

        $note = (new DiscussionNote())
            ->setUser($comment->getUser())
            ->setPullRequest($pullRequest)
            ->addComment($comment);

        $this->em->persist($note);
        $this->em->flush();
        $this->em->refresh($note);

        $note = $this->em->getRepository(Note::class)->find($note->getId());
        $this->assertSame(1, $note->getComments()->count());
        $this->assertNotNull($note->getId());

        $this->em->remove($comment);
        $this->em->flush();

        $this->assertNull($note->getId());
    }
}

[onEXHovia]

I looked at the dates, current behavior worked from version ^2.5 to 2.14.1, it may work in 2.15 or 2.16, I haven’t checked.

[beberlei]

@onEXHovia can you please check if it works on 2.16?

[beberlei]

Ah, I am pretty sure this is related to the commit order change, @mpdude could you chime in?

[onEXHovia]

@onEXHovia can you please check if it works on 2.16?

Last work version 2.15.5, bug occurs in the version 2.16.0 and next releases

[mpdude]

Will do

[mpdude]

It may have to do with #10915, which moved the postRemove event dispatch to the point where all pending deletions have been executed. Maybe before that, the event was dispatched earlier so that the additional remove() call would update the list of pending entity removals and would happen within the currently running commit phase, without requiring an extra flush() call.

But, I am pretty sure #10763 plays a role as well:

In 2.16.0, the DiscussionNote::$comments collection still contains the already-removed Comment instance at the point where the postRemove event is dispatched. #10763 moved the clean-up for in-memory collections to a later stage for reasons I will have to recap. #11132 tries to address that.

[mpdude]

@beberlei would you say this kind of event listener should work without an extra flush() call, even if that was the case previously?

[beberlei]

The post* events are documented to be used only for non persistence related things, see https://www.doctrine-project.org/projects/doctrine-orm/en/2.17/reference/events.html#postupdate-postremove-postpersist

That this worked before was pure luck.

Could you not trigger this logic in preRemove? Instead of calling EntityManager::remove you could use entityManager->getUnitOfWork()->scheduleForDeletion to break/control the recursion