Bug #88876
closedPageRepository ignores multiple orderings when fetching subpages for pages
100%
Description
The PageRepository currently does not allow to define multiple orderings when fetching subpages for pages.
When I call either $pageRepository->getMenu(<uid>, '*', 'crdate DESC, sorting ASC')
or $pageRepository->getMenuForPages([<uids>], '*', 'crdate DESC, sorting ASC')
(for example), the last defined order field (in this case sorting
) takes precedence as it overrides all previously defined fields (in this case crdate
).
Affected code:
if (!empty($sortField)) {
$orderBy = QueryHelper::parseOrderBy($sortField);
foreach ($orderBy as $order) {
$res->orderBy(...$order);
}
}
The problem is that each $res->orderBy()
replaces the previous calls as the QueryBuilder replaces the queryPart
instead of adding it.
I would suggest to check if an orderBy
query part has been added previously and in this case to use $res->addOrderBy()
. This leads to the following code:
if (!empty($sortField)) {
$orderBy = QueryHelper::parseOrderBy($sortField);
foreach ($orderBy as $order) {
if (!$res->getQueryPart('orderBy')) {
$res->orderBy(...$order);
} else {
$res->addOrderBy(...$order);
}
}
}
What do you think?
Updated by Jonas Eberle over 5 years ago
I think multiple orderBys are an undocumented feature so far.
/*
...
* @param string $sortField The field to sort by. Default is "sorting"
...
*/
public function getMenuForPages(...
Is it used somewhere already with multiple ORDER BYs?
Updated by Elias Häußler over 5 years ago
Jonas Eberle wrote:
I think multiple orderBys are an undocumented feature so far.
[...]
That's true, it's currently not documented and it seems that is was meant to support only one sort field. But the implementation using the foreach loop shows that it might be a "feature" to support multiple fields. Also, the complexity should be pretty low to finally implement this.
Is it used somewhere already with multiple ORDER BYs?
It is currently not used with multiple sorting fields directly, but the AbstractMenuContentObject
class offers some methods calling the getMenu()
method of PageRepository
where it could be possible to achieve this.
Updated by Gerrit Code Review over 5 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61425
Updated by Gerrit Code Review over 5 years ago
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61371
Updated by Anonymous over 5 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 922bd357a792b40be7a6e7eb76dd3e8a11edb3dc.
Updated by Gerrit Code Review over 5 years ago
- Status changed from Resolved to Under Review
Patch set 2 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61371
Updated by Anonymous over 5 years ago
- Status changed from Under Review to Resolved
Applied in changeset 50be8ce6f6874c24e49caf6cf21aa84940d3125a.