Skip to content

Prioritize sorted orderBy item when already present in QueryBuilder #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonmldr wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −7
Open

Prioritize sorted orderBy item when already present in QueryBuilder #350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/Knp/Component/Pager/Event/Subscriber/Sortable/Doctrine/ORM/Query/OrderByWalker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,21 +71,20 @@ public function walkSelectStatement(SelectStatement $AST): void
$orderByItem->type = $direction;

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
) {
Comment on lines 76 to 80

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
$item->type = $direction;
$set = true;
unset($orderByItems[$orderByIndex]);

break;
Comment on lines +82 to 83

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
break;

Copilot uses AI. Check for mistakes.
}
}
if (!$set) {
array_unshift($AST->orderByClause->orderByItems, $orderByItem);
}

array_unshift($orderByItems, $orderByItem);
} else {
Comment on lines 73 to 88

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
$AST->orderByClause = new OrderByClause([$orderByItem]);
}
Expand Down
Loading