doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.93k stars 2.51k forks source link

Entity changes created by an onFlush subscriber are not executed during flush if no other changes were scheduled #7248

Open kalifg opened 6 years ago

kalifg commented 6 years ago

Bug Report

Q A
BC Break no
Version 2.5.6

Summary

If you call flush() when there are no changes (inserts, updates, deletes, etc) scheduled, no changes will be flushed — even if an onFlush subscriber adds them.

Current behavior

As described above, if you have an ORM with no registered changes, but you call flush expecting an onFlush subscriber to create some changes, those changes will not be executed.

The changes are registered with the ORM, and calling flush() a second time will execute them.

This is because of the early exit here

How to reproduce

We have an onFlush subscriber that scans the identity map for entities of a certain interface. It then performs updates on some of them. It's code that is used to process a file upload on a form, sending it to S3 and then adding the S3 path to the entity.

If the user doesn't change anything else in the form, the new S3 info will not be updated either (unless we use flush() twice). This code is paraphrased but I hope you get the idea.

class TrophySubscriber
{
    public function getSubscribedEvents()
    {
        return [
            'onFlush',
        ];
    }

    public function onFlush(OnFlushEventArgs $args)
    {
        $uow = $args->getEntityManager()->getUnitOfWork();

        foreach ($uow->getIdentityMap() as $class => $entities) {
            if (!is_a($class, Trophy::class, true)) {
                continue;
            }

            $this->updateTrophy($entity);
            $uow->recomputeSingleEntityChangeSet($em->getMetadataFactory()->getMetadataFor($class), $entity);

            // Unnecessary but something we tried before noticing the early exit
            $uow->scheduleForUpdate($entity);
        }
    }

    private function updateTrophy(Trophy $trophy)
    {
        $trophy->setImageHash(S3ImageService::$trophy->getImageFileName());
    }
}
class ContentController
{
    public function trophiesEditAction(Request $request, Trophy $trophy = null)
    {
        $form = new TrophyForm();
        $form->handleRequest($request);

        if ($form->isValid()) {
            $em = $this->getDoctrine()->getManager();
            $em->persist($trophy);
            $em->flush();
            // At this point an update will have been scheduled by TrophySubscriber, but not yet executed
            $em->flush();  // Required to flush the update scheduled by TrophySubscriber
    }
}

Expected behavior

I would expect one flush() to be sufficient; after dispatching the onFlush event UnitOfWork could check the change arrays again to make sure they are still empty.,

greentopsecret commented 6 years ago

@kalifg , did you consider to use preFlush event ?

kalifg commented 6 years ago

@kalifg , did you consider to use preFlush event ?

Thank you for taking the time to respond! That's a great idea and I think it will work for us. Any onFlush listener that does not depend on existing change sets should work just as well as a preFlush.

I still think this issue needs to be resolved through at least some documentation of the phenomenon if nothing else. The difference in behavior between the two ORM states could be confusing.