-
-
Notifications
You must be signed in to change notification settings - Fork 835
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.
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
Fix relevance sort #2773
Fix relevance sort #2773
Conversation
[ci skip] [skip ci]
[ci skip] [skip ci]
@@ -85,12 +85,13 @@ protected function data(ServerRequestInterface $request, Document $document) | |||
|
|||
$filters = $this->extractFilter($request); | |||
$sort = $this->extractSort($request); | |||
$sortIsDefault = $this->sortIsDefault($request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename sortIsDefault
to isDefaultSorting
or maybe isSortingByDefault
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed feelings about this one. Any use of sorting describes whether or not any sort is being applied. But this variable should describe whether the sort currently being used (whatever that is, could be no sort at all) is the controller's default sort.
Alternatively, we could use userRequestedSort
or userSpecifiedSort
and invert everything, but that sounds very verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, we only want to know if we're falling back to the default sort.
Though I was more looking for just a isSomething
naming, since that's the usual convention for boolean variables/methods where possible. But isDefaultSort
might be weird.
Let's keep it as is then!
This was introduced in flarum/framework#2773, and allows us to more cleanly determine whether the sort requested is the default one, while taking extension modifications into account.
This was introduced in flarum/framework#2773, and allows us to more cleanly determine whether the sort requested is the default one, while taking extension modifications into account.
This was introduced in flarum/framework#2773, and allows us to more cleanly determine whether the sort requested is the default one, while taking extension modifications into account.
Fixes #2772
Changes proposed in this pull request:
QueryCriteria
that determines whether the sort provided is the controller's default sorttrue
iffsort
not in query params. Default it to falseConfirmed
composer test
).