Prioritize sorted orderBy item when already present in QueryBuilder#350
Prioritize sorted orderBy item when already present in QueryBuilder#350jonmldr wants to merge 1 commit into
Conversation
jonmldr
changed the title
|
Please add tests |
There was a problem hiding this comment.
Pull request overview
Adjusts the Doctrine ORM sortable OrderByWalker so the requested sort field is always placed first in the generated ORDER BY, even when the same field is already present in the query’s existing ordering.
Changes:
- Removes an existing matching
ORDER BYitem (foralias.field) before prepending the requested sort item. - Always prepends (
array_unshift) the requested sort item when anOrderByClausealready exists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($AST->orderByClause) { | ||
| $set = false; | ||
| foreach ($AST->orderByClause->orderByItems as $item) { | ||
| $orderByItems = &$AST->orderByClause->orderByItems; | ||
| foreach ($orderByItems as $orderByIndex => $item) { | ||
| if ( | ||
| $item->expression instanceof PathExpression && | ||
| $item->expression->identificationVariable === $alias && | ||
| $item->expression->field === $field | ||
| ) { | ||
| $item->type = $direction; | ||
| $set = true; | ||
| unset($orderByItems[$orderByIndex]); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| if (!$set) { | ||
| array_unshift($AST->orderByClause->orderByItems, $orderByItem); | ||
| } | ||
|
|
||
| array_unshift($orderByItems, $orderByItem); | ||
| } else { |
There was a problem hiding this comment.
This change alters behavior when the sorted field is already present in an existing ORDER BY (it now removes the existing item and prepends the sorted one). There doesn’t appear to be test coverage for the “existing ORDER BY contains the same field” scenario in the Doctrine ORM sortable tests; please add/adjust a test to assert (1) the sorted field becomes the first ORDER BY item and (2) no duplicate ORDER BY entry remains.
| if ( | ||
| $item->expression instanceof PathExpression && | ||
| $item->expression->identificationVariable === $alias && | ||
| $item->expression->field === $field | ||
| ) { |
There was a problem hiding this comment.
The duplicate-removal logic only matches existing ORDER BY items where the expression is a PathExpression (alias.field). Sorts that use a result/scalar alias (e.g., the existing test case that sorts by counter) or other expression types won’t be de-duplicated, so you can still end up with duplicate ORDER BY entries even though the PR description says duplicates are removed when already present. If de-duplication is intended broadly, extend the match to handle non-PathExpression expressions when $alias === false (and/or other AST expression node types).
|
|
||
| break; |
There was a problem hiding this comment.
The loop stops after unsetting the first matching ORDER BY item. If the query already contains the same alias/field multiple times (e.g., repeated addOrderBy calls), additional duplicates will remain and you'll still end up with multiple ORDER BY entries for the same expression. Consider removing all matching items (don’t break; keep scanning) before unshifting the new item, or filtering the array to exclude matches.
| break; |
This MR ensures that a sorted orderBy item is always prioritized as the first in the list, especially when it is already present in the QueryBuilder. This ensures that the query prioritizes the sorted property first, as intended.
The change guarantees that
array_unshiftis always used. When the sorted orderBy item is already present in the QueryBuilder, it removes the duplicate item instead of modifying the direction of the existing one.