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.

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

allow custom order by #203

Merged
merged 4 commits into from
Jul 28, 2023
Merged

Conversation

Ar0010r
Copy link
Contributor

@Ar0010r Ar0010r commented Jun 20, 2023

Some people might wanna use more complex order by statements like this:

"sort": [
{
"prices.value": {
"order": "desc",
"nested": {
"path": "prices",
"filter": {
"term": {
"prices.shop_id": "6f8c648c-112e-43ad-a93d-7ca72ea90aee"
}
}
}
}
}
]

So I think it wise to allow adding SyntaxInterface objects to the ScoutSearchCommandBuilder's $sort array, and not just Sort objects.

Thanks

@Jeroen-G
Copy link
Owner

Does Elastic support all types of queries in a sort?

@Ar0010r
Copy link
Contributor Author

Ar0010r commented Jun 20, 2023

not sure about all types of queries, but some of them it certainly accepts. Using this feature in my current project.

Also, I have fixed failing tests.
Locally i have : OK (205 tests, 330 assertions), after I run "vendor/bin/phpunit"

Thanks for the answer

@Jeroen-G
Copy link
Owner

This change allows all queries, if it's only a few I'd rather have it typed more strictly.

Thanks for contributing btw!

@Jeroen-G
Copy link
Owner

Can you find a list of the allowed queries?

@Ar0010r
Copy link
Contributor Author

Ar0010r commented Jun 20, 2023

I have checked the list of allowed options in sort section, and after my research I dont think that its possible to create one interface that will cover all cases.

Here is the list : https://www.elastic.co/guide/en/elasticsearch/reference/current/sort-search-results.html

So I think that we can stick with SyntaxInterface solution.

What do you think about it ?

@Jeroen-G
Copy link
Owner

Oh boy there are some funky things.
Yeah let's stick with this.

Regarding your tests: phpunit is only for the unit tests, there is also infection PHP for mutation tests and those fail.

@Jeroen-G
Copy link
Owner

make mutations and make tests

@Ar0010r
Copy link
Contributor Author

Ar0010r commented Jun 20, 2023

Im working on fixing mutations, just dont know how to proceed. Investigating

@Jeroen-G
Copy link
Owner

It usually means a case is not tested.

@Ar0010r
Copy link
Contributor Author

Ar0010r commented Jul 28, 2023

Hello again ! I was able to adjust test so infection runs with no errors
Please take a look once you available

Thanks

Screenshot 2023-07-28 at 00 28 21 Screenshot 2023-07-28 at 00 29 10

@Jeroen-G Jeroen-G merged commit 5bca9bc into Jeroen-G:master Jul 28, 2023
6 checks passed
@artemsky
Copy link

Nested sort ability was broken by this PR :( #217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants