-
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
Lazy and eager collection refresh inconsistency #10065
Lazy and eager collection refresh inconsistency #10065
Conversation
first commit add the test, the second provides a fix for it |
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.
Builds are green. Do you still expect BC breaks despite that?
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 |
hi, are there any comment on this? is there an interest in merging it? |
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. |
I think that this is still relevant for 2.x... |
Well then yes. Also, please rebase on 2.20.x 🙏
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. |
b42f949
to
76ff717
Compare
76ff717
to
7d1b24f
Compare
@greg0ire thanks for looking again into this! I have rebased it and targeted 2.20.x as base branch. |
The psalm failure seems unrelated to my change (it is failing in the base branch as well) |
Thanks @goetas , please go ahead with |
Created #11717 for the detach feature |
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