RFC: Prevent UnitOfWork::commit() reentrance?

[mpdude]

This PR prevents reentrant calls to UnitOfWork::commit() with a clear exception message.

Reentrance happens, for example, when users call EntityManager::flush() from within postPersist or similar event listeners. Since #10547, it causes strange-looking, non-helpful error messages (#10869).

Instead of treating this as a bug and trying to fix it, my suggestion is to fail with a clear exception message instead.

Reasons:

I assume the UoW was never designed to support reentrant flush() calls in the first place. So, trying to make (or keep) this working might be a rabbit hole.

The assumption is based on the following:

• Not a single test in the ORM test suite covers or even triggers flush() reentrance – otherwise, such a test would have failed with the changes suggested here.
• Documentation for e. g. preFlush or postFlush explicitly mentions that flush() must not be called again. I don't know why this is not also stated for e. g. postPersist. But why would it be safe to call flush() during postPersist, in the middle of a transaction, when there are reasons against calling it in postFlush, just before final cleanups are made?
• Last but not least, entity insertions, computed changesets etc. are kept in fields like UnitOfWork::$entityChangeSets or UnitOfWork::$originalEntityData. It's all to easy to mess up these states when we re-compute changesets mid-way through a flush() – and again, if that were anticipated, I'd assume to find any kind of test coverage.

TODO

• Review Mitigate problems with EntityManager::flush() reentrance since 2.16.0 #10906 – revert that change, maybe in 3.0?

[mpdude]

@beberlei maybe you recall if this was ever meant to work?

[derrabus]

Tests please. 🙂

[mpdude]

☝🏻 – sure, as soon as we agree we want to go down that road. Converting PR to draft for the time being.

[jmperruchini]

I modified the UnitOfWork file, I now have the following error, if I comment out $this->entityManager->flush();, I don't have an error but
I don't have a database record.

The UnitOfWork is currently in the commit phase. You must not call UnitOfWork::commit() or EntityManager::flush() at this time, since commit phases cannot safely be nested.

LogicException
in vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php (line 375)
in vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php -> commit (line 403)
EntityManager->flush() in src/Trace/Logger.php (line 42)

    if (isset($user)) {
        $log->setUtilisateur($user);
    }
    $log->setLibelleLog($libelle);
    $this->entityManager->persist($log);
    $this->entityManager->flush();
}

}

Logger->log('create', 'consultation', 7368, '127.0.0.1', '11/08/2023 14:24:26 : l'utilisateur Dr X (medecine physique et de readaptation - CEDEX) (ip : 127.0.0.1) a créé un objet dans le module consultation, id=7368 (action : create) - patient sélectionné : Mr X', object(SfGuardUser), object(Patient)) in src/Trace/Trace.php (line 183)

Should I wait for changes? or should I not try to create logs with postpersist? the problem is that I can't retrieve the id of newly created objects in my logs in onflush or other doctrine events!

[mpdude]

Discussed with @derrabus, @morozov , @beberlei and @greg0ire in-person that we do not want to deprecate flush() re-entrance in 2.x and prevent it in 3.0, since we consider it a legetimate use case to update or create new entities from event listeners, and for users it would feel natural to call flush() after that.

This of course means we need to spend some work on it:

• Add tests for this behavior
• Work out how event dispatching should work, e. g. will we call preFlush or similar events again?

One idea by @morozov was to have one class or data structure keeping track of what needs to be inserted/updated/deleted and in what order, and to make sure these orders remain valid when additional inserts/updates/deletes are added at a later time. A second class would in a way "traverse" or "work" through this structure until all operations have been performed.

That way, it might be possible to ensure that all flush() operations, including the re-entrant ones, happen in the same transaction.

If all that turns out to be not feasible, we can still prevent re-entrance in 4.0.

[mpdude]

@alcaeus told me at the current hackathon that the ODM chose to prevent reentrance and thus flush() cannot be called there from event handlers that during the commit phase.