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.

Sign up for GitHub

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

Jump to bottom

Fix UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet #9301

Merged
merged 2 commits into from
Aug 2, 2023
Merged

Fix UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet #9301

merged 2 commits into from
Aug 2, 2023

Conversation

olsavmic
Copy link
Contributor

Resolves #9300

@olsavmic olsavmic mentioned this pull request Dec 29, 2021
Closed
@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch from cc0549f to fa3916c Compare December 29, 2021 12:51
@beberlei
Copy link
Member

The test is not sufficient, as it does not show that this happens when just using the EntityManager public API. What you are doing might not be a valid use of the internal API of UoW.

@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch from e5c5f77 to 74888e6 Compare December 30, 2021 22:04
@olsavmic
Copy link
Contributor Author

The test is not sufficient, as it does not show that this happens when just using the EntityManager public API. What you are doing might not be a valid use of the internal API of UoW.

I've rewritten the test to use EntityManager instead of UoW. The issue is with the internal UoW state so the assert does check the UoW directly.

I can add additional test case that tests the side effect of not having the persistent collection present in the original data that would throw DUPLICATE INSERT in case the fix is not present if you'd prefer that, let me know :)

@olsavmic
Copy link
Contributor Author

I added a second test case showing how dangerous this bug is. Both test cases fail without the fix.

@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch 4 times, most recently from 1c25889 to b31a306 Compare December 30, 2021 23:29
@derrabus derrabus changed the base branch from 2.10.x to 2.11.x January 12, 2022 13:35
@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch from b31a306 to 442ba71 Compare January 13, 2022 12:26
@derrabus derrabus added the Bug label Jan 15, 2022
@olsavmic
Copy link
Contributor Author

olsavmic commented Aug 8, 2022

@derrabus @beberlei Any possibility of getting this reviewed? We mitigated the issue by removing all collection reassignments and using ->clear() instead as proposed by @xificurk but it still remains dangerous.

@olsavmic
Copy link
Contributor Author

olsavmic commented Aug 1, 2023

@beberlei @derrabus @mpdude

Can this please get a review or at least close this MR with a reason why it's not a welcomed change? Thanks!

@derrabus derrabus changed the base branch from 2.11.x to 2.16.x August 1, 2023 12:58
derrabus
derrabus reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Sorry, I wasn't aware of this PR. Can you rebase onto 2.16.x please? We've changed a lot in the UoW recently and I'd like to see if the CI still passes.

tests/Doctrine/Tests/Models/Issue9300/Issue9300Child.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php Outdated Show resolved Hide resolved
@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch from 442ba71 to 08e410e Compare August 2, 2023 11:14
@olsavmic @derrabus
Apply suggestions from code review
0752b24 
Co-authored-by: Alexander M. Turek <[email protected]>
@olsavmic olsavmic force-pushed the fix-actual-data-incomplete-after-flush branch from c800274 to 0752b24 Compare August 2, 2023 11:20
@olsavmic olsavmic requested a review from derrabus August 2, 2023 11:34
@olsavmic
Copy link
Contributor Author

olsavmic commented Aug 2, 2023

Thank you! It's rebased and review comments are fixed.

@derrabus derrabus added this to the 2.16.1 milestone Aug 2, 2023
@derrabus
Copy link
Member

derrabus commented Aug 2, 2023

Thank you!

@derrabus derrabus merged commit f50803c into doctrine:2.16.x Aug 2, 2023
derrabus added a commit that referenced this pull request Aug 7, 2023
@derrabus
Merge branch '2.16.x' into 2.17.x
58df407 
* 2.16.x:
  Turn identity map collisions from exception to deprecation notice (#10878)
  Add possibility to set reportFieldsWhereDeclared to true in ORMSetup (#10865)
  Fix UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet  (#9301)
  Add an UPGRADE notice about the potential changes in commit order (#10866)
  Update branch metadata (#10862)
nicolas-grekas pushed a commit to nicolas-grekas/doctrine-orm that referenced this pull request Aug 7, 2023
@olsavmic @derrabus @nicolas-grekas
Fix UnitOfWork->originalEntityData is missing not-modified collection…
6c65843 
…s after computeChangeSet (doctrine#9301)

* Fix original data incomplete after flush

* Apply suggestions from code review

Co-authored-by: Alexander M. Turek <[email protected]>

---------

Co-authored-by: Alexander M. Turek <[email protected]>
@github-actions github-actions bot mentioned this pull request Aug 9, 2023
Merged
@VincentChalnot
Copy link

VincentChalnot commented Oct 13, 2023 •
edited
Loading

The change made to UnitOfWork in this fix is breaking ManyToMany associations which are not persisted anymore: Collections are completely cleared on flush. (In my project at least)
I'm not sure what this is supposed to fix but the fact that it did not break any test is mind boggling.
Currently trying to find a way to reproduce this on a sample project.

EDIT: Nevermind this seems to be related to some custom code in my project, still I have a feeling that something is not right if a tiny change like this one can break a major feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xificurk xificurk xificurk left review comments

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
2.16.1
Development

Successfully merging this pull request may close these issues.

UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet
5 participants
@olsavmic @beberlei @derrabus @VincentChalnot @xificurk