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 enum change set recomputation on single entity #10806

Merged
merged 1 commit into from Jul 6, 2023

Conversation

rmikalkenas
Copy link
Contributor

@rmikalkenas rmikalkenas commented Jun 28, 2023

The root cause is well explained in #10074 and #10277
These PRs mitigate the issue in computeChangeSet method, but in some cases recomputeSingleEntityChangeSet might also be called.
One of the cases where recomputeSingleEntityChangeSet might be called are in event listeners when you want to modify some other existing entity. Due to the fact that originalEntityData contains enum objects and ReflectionEnumProperty::getValue() returns value of enum - comparison of values are always falsy, resulting to update column value even though its exactly the same. In case nothing changed in entity it can even result to additional queries to DB if entity contains enum attribute

@greg0ire
Copy link
Member

@HypeMC @michnovka please review

@michnovka
Copy link
Contributor

michnovka commented Jun 28, 2023 via email

@rmikalkenas
Copy link
Contributor Author

Give me until weekend to have a look. Also @rmikalkenas , could you please explain in few words what this does and why is it needed?

On Wed, Jun 28, 2023, 14:12 Grégoire Paris @.> wrote: @HypeMC https://github.com/HypeMC @michnovka https://github.com/michnovka please review — Reply to this email directly, view it on GitHub <#10806 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6JI7Y3AEQWU4WIAPVT2H3XNQNT5ANCNFSM6AAAAAAZW4UO5A . You are receiving this because you were mentioned.Message ID: @.>

Updated my initial message

@rmikalkenas
Copy link
Contributor Author

@greg0ire is there anything I could help so that this PR gets merged and released as patch version?

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2023

I'll try to review it soon, unless another maintainer beats me to it.

@greg0ire
Copy link
Member

greg0ire commented Jul 5, 2023

The commit message should allow us to understand this change without access to the internet.

Please improve your commit message according to the contributing guide.

The root cause is well explained in #10074 and #10277

Both links contains list to other issues… do I need to read 4 pages to understand this? The rest of the PR description is good, as it gives some insight on what this might be about.

…enum attributes.

Due to the fact that originalEntityData contains enum objects and
ReflectionEnumProperty::getValue() returns value of enum,
comparison of values are always falsy, resulting to update
columns value even though it has not changes.
@rmikalkenas
Copy link
Contributor Author

@greg0ire thanks for your time reviewing the PR 🙏
Updated the code based on comments. Let me know if more changes are needed

@greg0ire greg0ire added this to the 2.15.4 milestone Jul 6, 2023
@greg0ire greg0ire added the Bug label Jul 6, 2023
@greg0ire greg0ire merged commit 0b9060c into doctrine:2.15.x Jul 6, 2023
41 checks passed
@greg0ire
Copy link
Member

greg0ire commented Jul 6, 2023

Thanks @rmikalkenas !

@aurimasrimkusnfq
Copy link

aurimasrimkusnfq commented Jul 19, 2023

When do you plan to release version 2.15.4?

@aurimasrimkusnfq
Copy link

@greg0ire ^

@derrabus
Copy link
Member

Soon.

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.

None yet

5 participants