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 cloning entities #11475

Merged
merged 1 commit into from
Jun 1, 2024
Merged

Fix cloning entities #11475

merged 1 commit into from
Jun 1, 2024

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented May 24, 2024

Fix #11455

When initializing a proxy, the current identifier of the entity was used to load from the DB.

This breaks eg when __clone sets the id to null before setting other properties to null also: that second "set" triggers initialization, yet because the id was just set to null before, the ORM triggers a SELECT with no WHERE (!?)

This is also happening when changing the id outside of __clone before setting any other properties - that's why the testPersistUpdate case needs to be updated: id 123 doesn't exist in the DB, yet this was shadowed by the $proxy->id = null; line, which triggered a SELECT with no WHERE, leading to the proxy being hydrated with whatever first row was returned by that SELECT.

Now, we consistently initialize the proxy with the identifier it was created with.

@nicolas-grekas nicolas-grekas changed the base branch from 3.2.x to 2.19.x May 24, 2024 06:42
@nicolas-grekas nicolas-grekas force-pushed the fix-clone branch 8 times, most recently from 04785aa to 20cc649 Compare May 27, 2024 12:49
@nicolas-grekas
Copy link
Member Author

PR green and ready.

@beberlei
Copy link
Member

Since the new closure is not self-referencing I assume this change is fine, but we should nevertheless check that this does not re-introduce the memory pressure that was fixed with #11376 by re-running the tests from https://github.com/javer/doctrine-orm-lazy-ghost

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 28, 2024

I ran the bench and raw numbers show a ~10% perf impact on it.
Yet this fixes a behavior that's clearly wrong so I'd advise merging and releasing, and later on figuring out how this could be optimized, if doable.

@greg0ire greg0ire added the Bug label Jun 1, 2024
@greg0ire greg0ire added this to the 2.19.6 milestone Jun 1, 2024
@greg0ire greg0ire merged commit 23b35e9 into doctrine:2.19.x Jun 1, 2024
58 checks passed
@greg0ire
Copy link
Member

greg0ire commented Jun 1, 2024

Thanks @nicolas-grekas !

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.

Regression related to Improve lazy ghost performance by avoiding self-referencing closure
4 participants