Use output walkers when ordering by non-fetched relation

[Sajito]

I'm not quiet sure, if this is there correct template, but I'm submitting this as an improvement, instead of bugfix, because the existing tests and comments related to this PR (#2639) seem like the current behavior is intended.
For this reason I have not created a PR for my request, but might submit one later.

Description
At the moment the Method shouldDoctrinePaginatorUseOutputWalkers will return false, when ordering by a field, which is not being selected. This will lead to the error, which should be prevented in the last check of that method.

The main difference between the checks in the hasOrderByOnFetchJoinedToManyAssociation method and the validation in the LimitSubqueryWalker, is that the QueryChecker is looking at the select parts and potentially traversing the whole relation tree, while the LimitSubqueryWalker at the moment is only looking at the order by parts.

Proposal
The check in QueryChecker could be changed to be equivalent to the checks in LimitSubqueryWalker. Meaning we would look at the order by field and simply check if that relation is a to-many relation. This should be simple, performant and return the same result as the checks in the LimitSubqueryWalker. But it would break once doctrine actually fixes the TODO for complex order by.

Looking at the code:

core/src/Bridge/Doctrine/Orm/Util/QueryChecker.php

Lines 89 to 151 in 2062d41

	/**
	* Determines whether the QueryBuilder has ORDER BY on a column from a fetch joined to-many association.
	*/
	public static function hasOrderByOnFetchJoinedToManyAssociation(QueryBuilder $queryBuilder, ManagerRegistry $managerRegistry): bool
	{
	if (
	0 === \count($selectParts = $queryBuilder->getDQLPart('select')) ||
	0 === \count($queryBuilder->getDQLPart('join')) ||
	0 === \count($orderByParts = $queryBuilder->getDQLPart('orderBy'))
	) {
	return false;
	}
	$rootAliases = $queryBuilder->getRootAliases();
	$selectAliases = [];
	foreach ($selectParts as $select) {
	foreach ($select->getParts() as $part) {
	[$alias] = explode('.', $part);
	$selectAliases[] = $alias;
	}
	}
	$selectAliases = array_diff($selectAliases, $rootAliases);
	if (0 === \count($selectAliases)) {
	return false;
	}
	$orderByAliases = [];
	foreach ($orderByParts as $orderBy) {
	foreach ($orderBy->getParts() as $part) {
	if (false !== strpos($part, '.')) {
	[$alias] = explode('.', $part);
	$orderByAliases[] = $alias;
	}
	}
	}
	$orderByAliases = array_diff($orderByAliases, $rootAliases);
	if (0 === \count($orderByAliases)) {
	return false;
	}
	foreach ($orderByAliases as $orderByAlias) {
	$inToManyContext = false;
	foreach (QueryBuilderHelper::traverseJoins($orderByAlias, $queryBuilder, $managerRegistry) as $alias => [$metadata, $association]) {
	if ($inToManyContext && \in_array($alias, $selectAliases, true)) {
	return true;
	}
	if (null !== $association && $metadata->isCollectionValuedAssociation($association)) {
	$inToManyContext = true;
	}
	}
	}
	return false;
	}

We could alternatively remove the lines 95, 103-117 and replace the variable $selectAliases in line 140 by the result of $queryBuilder->getAllAliases().

This should increase the guessing success rate, with hopefully not so big performance impact.

What do you think?

[soyuka]

Why not. We could also add an option in operations to select the behavior manually. Please open a PR if you want and we'll discuss further.

[Sajito]

I've opened a PR with my proposed changes :)