EntityManager::find() calls UnitOfWork::registerManaged() twice and that causes Change Detection to fail
#10880
Comments
|
+1 |
|
Can you provide the two code paths that ultimately lead to Do both calls come from the same Also, are the two links correct that refer to the UoW? |
|
I have tried your reproduce steps but cannot see a problem. Please provide stack traces for both invocations of |
|
The second invocation you mentioned can only come into play when there is something with associations going on. Does the entity loaded through the ParamConverter reference back to itself? |
The Entity that is found by the ParamConverter is the The re-assignment of the I realize that the Entity hierarchy is not the greatest, but this is a legacy code riddled with inheritance as decisions that seemed to be logical at the time. Having that said this was working flawlessly (the Project is 7 years old) up until 2.16.0. |
|
When is the Did I get it right that the ParamConverter is fetching a Are there any extra configuration settings like fetch=EAGER or so? |
|
@eXsio I've been trying to add a reproduce test in #10884, but still unsuccessful – the test passes. Feel free to open a PR against https://github.com/mpdude/doctrine2/tree/reproducer-10880 to show where it fails. For now, I'd assume the ParamConverter and Symfony or the FrameworkBundle don't have anything to do with it. We need a test that passes on 2.15.x and fails on 2.16.0 to proceed. |
|
OK, I will try to recreate this during the following week and submit a PR, thanks for trying :) |
|
Hello I've created a PR with a working reproduction: My Branch forked from your repo proves that it doesn't work: I've created a second branch to make sure that the exact same thing worked on 2.15.3 and, in fact, I was right: The whole thing seems to be about inheritance. If change the It magically starts to work on 2.16.0. The thing is, that in my Application I have multiple inheritors of the |
|
P.S. One Clarification: the After adding some Hope this helps. |
GH10880 reproduction on Doctrine 2.16.0 - proof that it doesn't work anymore
…ies loaded through fetch=EAGER + using inheritance doctrine#10880 reports a case where the changes from doctrine#10785 cause entity updates to be missed. Upon closer inspection, this change seems to be causing it: https://github.com/doctrine/orm/pull/10785/files#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4L2990-L3003 The code was changed to use `registerManaged()` instead, which basically does the same things, but (since doctrine#10785) also includes an additional check against duplicate entity instances. But, one detail slipped through tests and reviews: `registerManaged()` also updates `\Doctrine\ORM\UnitOfWork::$originalEntityData`, which is used to compute entity changesets. An empty array `[]` was passed for $data here. This will make the changeset computation assume that a partial object was loaded and effectively ignore all field updates here: https://github.com/doctrine/orm/blob/a616914887ea160db4158d2c67752e99624f7c8a/lib/Doctrine/ORM/UnitOfWork.php#L762-L764 I think that, effectively, it is sufficient to call `registerManaged()` only in the two cases where a proxy was created. Calling `registerManaged()` with `[]` as data for a proxy object is consistent with e. g. `\Doctrine\ORM\EntityManager::getReference()`. In the case that a full entity has to be loaded, we need not call `registerManaged()` at all, since that will already happen inside `EntityManager::find()` (or, more specifically, `UnitOfWork::createEntity()` called inside it). Note that the test case has to make some provisions so that we actually reach this case: * Load an entity that uses `fetch="EAGER"` on a to-one association * That association being against a class that uses inheritance (why's that?)
…ies loaded through fetch=EAGER + using inheritance doctrine#10880 reports a case where the changes from doctrine#10785 cause entity updates to be missed. Upon closer inspection, this change seems to be causing it: https://github.com/doctrine/orm/pull/10785/files#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4L2990-L3003 The code was changed to use `registerManaged()` instead, which basically does the same things, but (since doctrine#10785) also includes an additional check against duplicate entity instances. But, one detail slipped through tests and reviews: `registerManaged()` also updates `\Doctrine\ORM\UnitOfWork::$originalEntityData`, which is used to compute entity changesets. An empty array `[]` was passed for $data here. This will make the changeset computation assume that a partial object was loaded and effectively ignore all field updates here: https://github.com/doctrine/orm/blob/a616914887ea160db4158d2c67752e99624f7c8a/lib/Doctrine/ORM/UnitOfWork.php#L762-L764 I think that, effectively, it is sufficient to call `registerManaged()` only in the two cases where a proxy was created. Calling `registerManaged()` with `[]` as data for a proxy object is consistent with e. g. `\Doctrine\ORM\EntityManager::getReference()`. In the case that a full entity has to be loaded, we need not call `registerManaged()` at all, since that will already happen inside `EntityManager::find()` (or, more specifically, `UnitOfWork::createEntity()` called inside it). Note that the test case has to make some provisions so that we actually reach this case: * Load an entity that uses `fetch="EAGER"` on a to-one association * That association being against a class that uses inheritance (why's that?)
…ies loaded through fetch=EAGER + using inheritance doctrine#10880 reports a case where the changes from doctrine#10785 cause entity updates to be missed. Upon closer inspection, this change seems to be causing it: https://github.com/doctrine/orm/pull/10785/files#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4L2990-L3003 The code was changed to use `registerManaged()` instead, which basically does the same things, but (since doctrine#10785) also includes an additional check against duplicate entity instances. But, one detail slipped through tests and reviews: `registerManaged()` also updates `\Doctrine\ORM\UnitOfWork::$originalEntityData`, which is used to compute entity changesets. An empty array `[]` was passed for $data here. This will make the changeset computation assume that a partial object was loaded and effectively ignore all field updates here: https://github.com/doctrine/orm/blob/a616914887ea160db4158d2c67752e99624f7c8a/lib/Doctrine/ORM/UnitOfWork.php#L762-L764 I think that, effectively, it is sufficient to call `registerManaged()` only in the two cases where a proxy was created. Calling `registerManaged()` with `[]` as data for a proxy object is consistent with e. g. `\Doctrine\ORM\EntityManager::getReference()`. In the case that a full entity has to be loaded, we need not call `registerManaged()` at all, since that will already happen inside `EntityManager::find()` (or, more specifically, `UnitOfWork::createEntity()` called inside it). Note that the test case has to make some provisions so that we actually reach this case: * Load an entity that uses `fetch="EAGER"` on a to-one association * That association being against a class that uses inheritance (why's that?)
…GER + using inheritance (#10884) #10880 reports a case where the changes from #10785 cause entity updates to be missed. Upon closer inspection, this change seems to be causing it: https://github.com/doctrine/orm/pull/10785/files#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4L2990-L3003 The code was changed to use `registerManaged()` instead, which basically does the same things, but (since #10785) also includes an additional check against duplicate entity instances. But, one detail slipped through tests and reviews: `registerManaged()` also updates `\Doctrine\ORM\UnitOfWork::$originalEntityData`, which is used to compute entity changesets. An empty array `[]` was passed for $data here. This will make the changeset computation assume that a partial object was loaded and effectively ignore all field updates here: https://github.com/doctrine/orm/blob/a616914887ea160db4158d2c67752e99624f7c8a/lib/Doctrine/ORM/UnitOfWork.php#L762-L764 I think that, effectively, it is sufficient to call `registerManaged()` only in the two cases where a proxy was created. Calling `registerManaged()` with `[]` as data for a proxy object is consistent with e. g. `\Doctrine\ORM\EntityManager::getReference()`. In the case that a full entity has to be loaded, we need not call `registerManaged()` at all, since that will already happen inside `EntityManager::find()` (or, more specifically, `UnitOfWork::createEntity()` called inside it). Note that the test case has to make some provisions so that we actually reach this case: * Load an entity that uses `fetch="EAGER"` on a to-one association * That association being against a class that uses inheritance (why's that?)
BC Break Report
Summary
I have a Symfony 5.4 based Application that uses Framework Extra Bundle and its Param Converter feature. After upgrading from 2.15.3 to 2.16.0 Doctrine has stopped detecting Entity Changes in Entities loaded by the ParamConverter, due to the new logic introduced by this commit: 01a1432
After some snooping I've found out, that the
UnitOfWork::registerManaged()is actually called twice:from the line 2805: https://github.com/doctrine/orm/blame/2.16.x/lib/Doctrine/ORM/UnitOfWork.php#L2875
from the line 3038: https://github.com/doctrine/orm/blame/2.16.x/lib/Doctrine/ORM/UnitOfWork.php#L3038
The second call rewrites previously loaded/hydrated Entity and sets the
UnitOfWork::$originalEntityDataproperty to[]. This causes the change detection to fail, because the UoW is not able to correctly compare the Actual Data to the Original Data.The Framework Extra Bundle uses the
EntityManager::find()method under the hood, so I wouldn't blame the lib for the failure. The fact is that Doctrine has altered it's behavior significantly because of that change.Previous behavior
when an Entity wired by the ParamConverter was changed, Doctrine has correctly detected changes made in the Entity.
Current behavior
Doctrine doesn't detect changes in the Entities wired by the ParamConverter.
How to reproduce
I am not sure if this is the case for every such configuration, but I am positive that the
UnitOfWork::registerManagedmethod can be called twice for a singlecreateEntitymethod call.Suggested Fix
Any tampering with not calling the
UnitOfWork::registerManaged()a second time has caused the ORM to blow up in some other place. However, modifying theUnitOfWork::registerManaged()method itself to not allow to overwrite the Entity seems to have helped:P.S.
This is not the only regression caused by this change. I can also see multiple tickets that point to this change as a culprit. Don't mean to tell you how to do things, but this seems like a very impactful and drastic change for a minor release.
The text was updated successfully, but these errors were encountered: