[Doctrine][Pagination] Complex OrderBy item must use output walker

[teohhanhui]

The results are wrong for example when ordering by a result variable.

https://github.com/doctrine/doctrine2/blob/e6c434196c8ef058239aaa0724b4aadb0107940b/lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryWalker.php#L141

[soyuka]

Needs a change in our QueryChecker?

Feel free to continue on #1301

[teohhanhui]

The bigger issue is that it might be unsafe in many cases to not use the output walkers, but we do not know. The only symptom may be incorrect paginated results.

[ambroisemaupate]

Hello,

I’ve got the same issue with this DQL (Cannot select distinct identifiers from query with LIMIT and ORDER BY on a column from a fetch joined to-many association. Use output walkers) :

SELECT o FROM AppBundle\Entity\Event o 
INNER JOIN o.dates dates_a1 
WHERE o.superEvent IS NULL 
ORDER BY dates_a1.doorTime DESC

This is generated for this request: {{url}}/events?order[dates.doorTime]=DESC

I’ve found that method QueryChecker::hasOrderByOnToManyJoin($queryBuilder, $this->managerRegistry) returns false but it should return true.

[soyuka]

9f9da0c last fix on the subject. Please help improving this!

[rlanting-framna]

I’ve found that method QueryChecker::hasOrderByOnToManyJoin($queryBuilder, $this->managerRegistry) returns false but it should return true.

It returns false because on QueryChecker.php#L153 $metadata is the joined entity and on QueryChecker.php#L154 $relationship is 'o.joinedEntity'. There is never a field that has a dot in it, and even if there would be, it checks on the wrong metadata.

I did a quick fix with the old code to see whether everything would work and it did. Basically did this:

...
if (false !== strpos($relationship, '.')) {
    list($parentAlias, $association) = explode('.', $relationship);

    $parentMetadata = QueryJoinParser::getClassMetadataFromJoinAlias($parentAlias, $queryBuilder, $managerRegistry);
    if ($parentMetadata->isCollectionValuedAssociation($association)) {
        return true;
    }
} else {
...

This does not handle deeper associations, but I don't know whether that is needed. If @soyuka could look into this, that would be awesome.

[ambroisemaupate]

@rlantingmove4mobile fix works for me.

[soyuka]

@ambroisemaupate would you mind sending a PR with your referenced commit?