Eagerly loaded indexed associations use hydrated values instead of database values

[wmouwen]

Description

Update: bug appears to exists in 3.0.x as well

There is a breaking change between 2.16.x and 2.17.x where for eagerly loaded collections indexBy started looking at the hydrated set of values instead of the raw values returned from the database. This broke setups for all eager associations using either non-scalar fields (Uuid, other entities) or associations where the inverse field's database name didn't match the property name.

Affected versions

• 2.17
• 2.18
• 2.19
• 3.0
• 3.1

Caused by

• [GH-1569] Optimize eager fetch for collections to batch query #8391

Issues

• Doctrine ORM 2.17.x Update Causes TypeError with indexBy on Doctrine Type (custom UUID) #11149
• OneToMany::indexBy no longer works when refering a database column #11097

Related

• orderBy not being respected #11163
• Related Objects not Hydrated at all despite fetch:EAGER #11203

[wmouwen]

In order to load collections in a same way as other spots, I've chosen to reuse the existing method \Doctrine\ORM\UnitOfWork::loadCollection. This way the functionality of eager loaded collections is in line with both older versions (<= 2.16.x) and non-eager loads.

[greg0ire]

In case you want to run stuff locally, you can take a look at this guide.

[wmouwen]

In case you want to run stuff locally, you can take a look at this guide.

That's the fun part; it works on my machine, and the reason why the test EagerFetchCollectionTest has been altered. Apparently the values between my machine (PHP 8.3) and the failing GitHub actions (PHP 7.2) differ, which isn't weird considering that the eager fetch logic has been altered. Guess the next step is to have a good look at whether EagerFetchCollectionTest is still valid.

[wmouwen]

Forcing a run on PHP 7.2.34 with the same settings as the failing GitHub action (link), still passes the tests.

> docker run -it -v "$PWD":/app -w /app --rm php:7.2-cli vendor/bin/phpunit -c ci/github/phpunit/pdo_sqlite.xml --exclude-group performance,non-cacheable,locking_functional tests/Doctrine/Tests/ORM/Functional/EagerFetchCollectionTest.php
PHPUnit 8.5.35 by Sebastian Bergmann and contributors.


Using DB driver Doctrine\DBAL\Driver\PDO\SQLite\Driver
....                                                                4 / 4 (100%)

Time: 54 ms, Memory: 12.00 MB

OK (4 tests, 12 assertions)

I'm a bit clueless as to what is causing the difference in behavior. I have chosen to disable parts of the broken tests, that way we can see if it is the only class to fail and narrow down the search.

[greg0ire]

@wmouwen I just had a hunch that your issue was caused by the second level cache. If you look more closely at the failing action in your link above, you will notice that there's a successful "Run PHPUnit" step, followed by a failing "Run PHPUnit with Second Level Cache". You should be able to reproduce the issue with ENABLE_SECOND_LEVEL_CACHE=1 vendor/bin/phpunit.

[wmouwen]

@greg0ire I'm failing to find a way to get the raw database values of the correct indexBy column in \Doctrine\ORM\UnitOfWork::eagerLoadCollections.

Secondly, #11163 has been created for a different problem caused by the same PR #8391. As #8391 was an optimization, I would suggest reverting #8391 for now until a new optimization can be written without breaking changes.

cc @beberlei

[wmouwen]

Updated the test for 3.0.x and rebased. Still couldn't find a solution except for reverting the original changes and incurring a performance loss, help is welcome.

[greg0ire]

@mpdude , maybe you can help with this?

[wmouwen]

The below code fetches entities for associations of multiple entities at once.

orm/src/UnitOfWork.php

Line 2659 in b187bc8

$found = $this->getEntityPersister($targetEntity)->loadAll([$mappedBy => $entities]);

We can't inject an 'indexBy' property here. Even if we could, the result would be ambiguous, as what is unique for one entity does not have to be unique across multiple entities.

The ->loadAll() call hydrates the entities. This causes the variable $indexByProperty a few lines below to try and find a non-existent column.

orm/src/UnitOfWork.php

Lines 2672 to 2674 in b187bc8

	$indexByProperty = $targetClass->getReflectionProperty($mapping->indexBy);
	assert($indexByProperty !== null);
	$collectionBatch[$idHash]->hydrateSet($indexByProperty->getValue($targetValue), $targetValue);

The column locale_code in the testcases doesn't exist on the hydrated object.

There is no clear way to combine $targetClass (Doctrine\ORM\Mapping\ClassMetadata) and $mapping (Doctrine\ORM\Mapping\OneToManyAssociationMapping) to get the value for indexing from the hydrated object ($targetValue) given the database column name.

@beberlei @mpdude Does either of you know a way?

[beberlei]

Superseded by #11380