doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.86k stars 2.5k forks source link

Fix cloning entities #11475

Closed nicolas-grekas closed 3 weeks ago

nicolas-grekas commented 1 month ago

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 commented 1 month ago

PR green and ready.

beberlei commented 4 weeks ago

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 https://github.com/doctrine/orm/pull/11376 by re-running the tests from https://github.com/javer/doctrine-orm-lazy-ghost

nicolas-grekas commented 4 weeks ago

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 commented 3 weeks ago

Thanks @nicolas-grekas !