Fix broken changeset computation for entities loaded through fetch=EAGER + using inheritance

[mpdude]

#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:

orm/lib/Doctrine/ORM/UnitOfWork.php

Lines 762 to 764 in a616914

	if (! (isset($originalData[$propName]) || array_key_exists($propName, $originalData))) {
	continue;
	}

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?)

[mpdude]

@eXsio does this change fix it for you?

[eXsio]

@mpdude yup, that does it, the tests are green :)

[eXsio]

One thing I am wondering ( as a total ignorant, so I may be completely off here :) ) but is there a valid scenario when the registerManaged() is called twice for a single Entity instance? Shouldn't there be a validation or a warning (inside of that method) if such thing happen? Breaking the Change Detection seems to me like a worst error that can possibly be in any ORM, because it breaks the contract and has potential of effectively poisoning the client's data. There is no error, there is "only" data inconsistency. If not for our single behat test that failed, we would've been happy to release it into prod and acted only when our clients discovered data errors on their side.

Just a thought.

[derrabus]

I've fixed the PHPCS failure for you. ✌🏻