Mitigate problems with EntityManager::flush() reentrance since 2.16.0 (Take 2)
#10915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mpdude
force-pushed
the
post-events-later
branch
from
8fc29ec
to
5517ba0
Compare
dmaicher
approved these changes
Aug 21, 2023
greg0ire
reviewed
Aug 22, 2023
| ]); | ||
| } | ||
|
|
||
| public function testPostPersistListenerUpdatingObjectFieldWhileOtherInsertPending(): void |
There was a problem hiding this comment.
What exactly are we testing here? It's unclear to me. There is a listener that alters field, but then we don't check that field contains test, so we're not even sure it comes into play.
There was a problem hiding this comment.
Would have to check whether it’s a leftover from when I tried to reproduce things, or actually necessary to make the subsequent flush() call do anything.
There was a problem hiding this comment.
I guess it has to do with checking that the ID is available at the time the listener is called... I'll try to improve that.
mpdude
force-pushed
the
post-events-later
branch
from
8836f08
to
8d45f26
Compare
|
@greg0ire Better like this? |
|
Yes 👍 I trust you've already noticed the CS issues? |
mpdude
force-pushed
the
post-events-later
branch
from
8d45f26
to
5986709
Compare
….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.
mpdude
force-pushed
the
post-events-later
branch
from
5986709
to
8259a16
Compare
greg0ire
approved these changes
Aug 23, 2023
SenseException
approved these changes
Aug 24, 2023
|
Thanks @mpdude ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes from #10547, which landed in 2.16.0, cause problems for users calling
EntityManager::flush()from withinpostPersistevent listeners.When
UnitOfWork::commit()is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed andUoW::$entityInsertionsbeing empty. After return, the entity insertion order, an array computed at the beginning ofUnitOfWork::executeInserts(), still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as 2.16.0 - Issue with persisting entities #10869.The fixes made to the commit order computation may lead to a different entity insertion order than previously.
postPersistlistener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in Mitigate problems withEntityManager::flush()reentrance since 2.16.0 #10906 (comment).This PR suggests to address both issues by dispatching the
postPersistevent only after all new entities have their rows inserted into the database. Likewise, dispatchpostRemoveonly after all deletions have been executed.This solves the first issue because the sequence of insertions or deletions has been processed completely before we start calling event listeners. This way, potential changes made by listeners will no longer be relevant.
Regarding the second issue, I think deferring
postPersista bit until all entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions for a particular class had been processed - so, it was never an "immediate" event for every single entity. #10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after all insertions.Fixes #10869.