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

Lazy and eager collection refresh inconsistency #10065

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

goetas
Copy link
Member

@goetas goetas commented Sep 26, 2022

When there is a cascade={"refresh"}, EAGER collections are refreshed, LAZY one are not refreshed, even if initialized...

The issue lies in the fact that \Doctrine\ORM\UnitOfWork::cascadeRefresh is called after the entity holding that collection has been refreshed, thus the lazy collections in there that have been edited are not refreshed (because the initial entity refresh has marked them as not initialized).

Moving the cascadeRefresh before the entity is refreshed.. but that might break other things... i'm waiting for github actions to tell.

I fear that the same should be done for \Doctrine\ORM\UnitOfWork::cascadeDetach

@goetas
Copy link
Member Author

goetas commented Sep 26, 2022

first commit add the test, the second provides a fix for it

SenseException
SenseException previously approved these changes Sep 28, 2022
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.

Builds are green. Do you still expect BC breaks despite that?

@goetas
Copy link
Member Author

goetas commented Sep 29, 2022

i do not think that this would cause bc breaks.

one of my questions here is: should i do the same fore the detach cascade operation? i'm pretty sure it has the same buggy behavior

@goetas
Copy link
Member Author

goetas commented Oct 21, 2022

hi, are there any comment on this? is there an interest in merging it?

Copy link
Contributor

github-actions bot commented Nov 9, 2024

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Nov 9, 2024
@goetas
Copy link
Member Author

goetas commented Nov 9, 2024

I think that this is still relevant for 2.x...

@github-actions github-actions bot removed the Stale label Nov 10, 2024
@greg0ire
Copy link
Member

one of my questions here is: should i do the same fore the detach cascade operation? I'm pretty sure it has the same buggy behavior

Well then yes. Also, please rebase on 2.20.x 🙏

hi, are there any comment on this? is there an interest in merging it?

The fact that you did not break any test by changing the code makes me think it was not intentionally like this, and the patch looks sensible, so I'm OK with this on principle, will merge after my comments are addressed. Thanks for working on this.

@goetas goetas force-pushed the lazy-eager-collection-refresh branch from b42f949 to 76ff717 Compare November 14, 2024 19:11
@goetas goetas changed the base branch from 2.13.x to 2.20.x November 14, 2024 19:11
@goetas goetas closed this Nov 14, 2024
@goetas goetas reopened this Nov 14, 2024
@goetas goetas force-pushed the lazy-eager-collection-refresh branch from 76ff717 to 7d1b24f Compare November 14, 2024 19:16
@goetas
Copy link
Member Author

goetas commented Nov 14, 2024

@greg0ire thanks for looking again into this! I have rebased it and targeted 2.20.x as base branch.
Please let me know if anything else is needed. If this gets merged, I would like to do the same with cascade detach

@goetas
Copy link
Member Author

goetas commented Nov 14, 2024

The psalm failure seems unrelated to my change (it is failing in the base branch as well)

@greg0ire greg0ire merged commit 486e406 into doctrine:2.20.x Nov 14, 2024
74 of 75 checks passed
@greg0ire greg0ire added this to the 2.20.1 milestone Nov 14, 2024
@greg0ire greg0ire added the Bug label Nov 14, 2024
@greg0ire
Copy link
Member

Thanks @goetas , please go ahead with detach()

@goetas
Copy link
Member Author

goetas commented Nov 15, 2024

Created #11717 for the detach feature

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

Successfully merging this pull request may close these issues.

3 participants