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

Fix to-many collections left in dirty state after entities are removed by the UoW #10486

Merged
merged 1 commit into from
May 11, 2023

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Jan 31, 2023

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.

specdrum-agc
specdrum-agc previously approved these changes Feb 3, 2023
Copy link

@specdrum-agc specdrum-agc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK.

jacquesbh added a commit to jacquesbh/doctrine-orm that referenced this pull request Apr 4, 2023
…d by the UoW

@see doctrine#10486

Co-authored: Matthias Pigulla <mp@webfactory.de>
jacquesbh added a commit to jacquesbh/doctrine-orm that referenced this pull request Apr 4, 2023
…d by the UoW

@see doctrine#10486

Co-authored-by: Matthias Pigulla <mp@webfactory.de>
@FelixBehr
Copy link

Hi Guys,

this would be the solution we currently have in a project. Is there any chance one could review this asap?
Thanks for helping out.

@greg0ire greg0ire added the Bug label Apr 13, 2023
@mpdude mpdude changed the base branch from 2.14.x to 2.15.x May 9, 2023 23:31
@mpdude
Copy link
Contributor Author

mpdude commented May 9, 2023

@SenseException maybe you could review this?

SenseException
SenseException previously approved these changes May 10, 2023
Copy link
Member

@SenseException SenseException left a 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.

@greg0ire greg0ire added this to the 2.15.2 milestone May 11, 2023
@greg0ire greg0ire merged commit 8126882 into doctrine:2.15.x May 11, 2023
@greg0ire
Copy link
Member

Thanks @mpdude !

@mpdude
Copy link
Contributor Author

mpdude commented May 12, 2023

Thank you for fixing the remaining issues

@mpdude mpdude deleted the fix-to-many-update-on-delete branch May 12, 2023 05:13
@mpdude
Copy link
Contributor Author

mpdude commented May 12, 2023

@greg0ire could you close the fixed issues I referenced in the initial post? Thanks.

@greg0ire
Copy link
Member

greg0ire commented May 12, 2023

🤔 that's been done automatically, hasn't it? Everything in the first message is either merged or closed.

@mpdude
Copy link
Contributor Author

mpdude commented May 12, 2023

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.

@greg0ire
Copy link
Member

Yes, it has happened to me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants