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

Change all sort directions to lowercase #1333

Closed

Conversation

imadphp
Copy link

@imadphp imadphp commented Feb 25, 2019

Uppercase sort direction causes Elasticsearch (v2.4.x) to default to asc.

Lowercase sort direction does not work in Elasticsearch
@rbayet
Copy link
Collaborator

rbayet commented Feb 25, 2019

Hello @abbatis,

We were wondering with @romainruaud which version of ElasticSearch you were using because the issue is indeed not present on a 5.6 or 6.x ElasticSearch.

Which version of ElasticSuite are you using ?
Be aware that the (@romainruaud all ? or only recent ?) ElasticSuite 2.6.x versions are not supposed to be compatible with ElasticSearch 2.x where this issue is present...

@abbatis I don't have a problem per se of merging the PR, but to benefit it, you would need to upgrade to a version imposing a newer version of ElasticSearch where the issue is non-existent, so ...

Regards

@imadphp
Copy link
Author

imadphp commented Feb 25, 2019

Hi @rbayet,

Thank you for your reply. We are currently using elasticsuite 2.5.x together with elasticsearch 2.4.x. Our codebase currently does not support newer elasticsearch versions, so upgrading is not really an option for now.

I have already fixed this in our own codebase by extending the \Fulltext\Collection class. I created this PR rather to help other people who struggle with the same issue. So if it does not help anyone, please feel free to close it.

Might be a good approach anyway to just strtolower the sort direction for the sake of consistency.

@romainruaud
Copy link
Collaborator

It's nearly useless since Elasticsearch does not complain anymore about sort orders in uppercase since 5.x.

Versions < 2.6.x of Elasticsuite are not part of our Open Source support anymore. So merging this on 2.6.x (which require ES >5) will have no effect.

I close this one, but thank you for proposing it.

Anyway, you should think about updating your ES (and probably your Magento too), since ES 2.4 is EOL for more than one year now.

Regards

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