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.

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

RFC: Prevent UnitOfWork::commit() reentrance? #10900

Closed
wants to merge 1 commit into from

Commits on Aug 9, 2023

  1. Prevent UnitOfWork::commit() reentrance

    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 doctrine#10547, it causes strange-looking, non-helpful error messages (doctrine#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`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) or [`postFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) 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.
    mpdude committed Aug 9, 2023
    Configuration menu
    Copy the full SHA
    8b0c5ac View commit details
    Browse the repository at this point in the history