Skip to content
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

Jump to bottom

ERATRANS-442: Fixed issue that makes sort applied to be overwritten by default bundle sort. #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ERATRANS-442: Fixed issue that makes sort applied to be overwritten by default bundle sort. #156

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
2 changes: 1 addition & 1 deletion src/ListExecutionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function executeList(ListPageConfiguration $configuration): ?ListExecutio
// If we have a specific sort, we use that first, followed by the default
// bundle sort. Otherwise, just the bundle sort.
$sort = $sort ? [$sort['name'] => $sort['direction']] : [];
if ($bundle_sort) {
if ($bundle_sort && empty($sort)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR but there are some problems:

  1. There is no test that proves the issue.
  2. The fix is not correct. The issue that happens is that the overridden sort (not the bundle one) is for the same field as the default bundle sort, in which case it gets incorrectly overridden by the default bundle sort. If the field used is different, it's fine and it works as expected. Because then you have 2 sorts applied onto the query and that's what needs to happen. So the fix is not exactly correct because like this, if there is a sort override of a different field, the bundle default sort no longer applies as secondary sort. So please fix it so that both get applied if it's for 2 different fields. And add test please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, we had an issue with the applied filters and default bundle in EFD.
I've created a PR for that. The case is when both sortings point to the same field, I'm not sure if is the same. I leave it here just in case:
#167

I haven't added tests yet

$sort[$bundle_sort['name']] = $bundle_sort['direction'];
}

Expand Down