-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix to-many collections left in dirty state after entities are removed by the UoW #10486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK.
…d by the UoW @see doctrine#10486 Co-authored: Matthias Pigulla <mp@webfactory.de>
…d by the UoW @see doctrine#10486 Co-authored-by: Matthias Pigulla <mp@webfactory.de>
Hi Guys, this would be the solution we currently have in a project. Is there any chance one could review this asap? |
@SenseException maybe you could review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests fail with the old code, so your fix seems to be what's needed. Once the build is green (CS fails) this is 👍. Please check the Coding Style.
eb7e540
to
a951369
Compare
Thanks @mpdude ! |
Thank you for fixing the remaining issues |
@greg0ire could you close the fixed issues I referenced in the initial post? Thanks. |
🤔 that's been done automatically, hasn't it? Everything in the first message is either merged or closed. |
Sorry, you’re right. I had the impression that did not work in other cases when a PR opened by me claims to close issues raised by others. |
Yes, it has happened to me as well. |
Current situation
When an entity is removed from the EM, the UoW updates collections that contain that entity, removing it from the collections as well.
The collections are left in a dirty state afterwards with outdated snapshots, which leads to errors e. g. when the Entity Manager is flushed again (see #10485).
Suggested fix
This PR fixes the problem by moving a check further down, so that also collections updated by the UoW itself (by removing the entity in question) are captured as "processed" by the current commit phase.
Tests added
It seems tests for this feature were completely missing, so I added them.
Also added a second test case for the feature added in #256 that the event for many-to-many collections is not only generated for added, but also for removed entities.
Closes #10485, fixes #10483, fixes #10060, closes #10061.