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

Ensure sort order is set on option text field for attributes using so… #463

Merged
merged 2 commits into from
Sep 5, 2017

Conversation

romainruaud
Copy link
Collaborator

…urce.

This is a fix for #450

See my comment explaining the problem : #450 (comment)

Imho, this fix is sufficient for 2.3.x branch.

But we should consider refactoring a bit the way we handle field name mapping in the collection for the next 2.4.x release. What I have in mind would be a FieldNameMapperInterface or something like this, which could handle these operations, and we could inject into the collection but also everywhere else we need to access the mapping (virtual categories, search API, and so on...) instead of duplicating a dirty "mapFieldName" function everywhere.

Let me know,

@romainruaud
Copy link
Collaborator Author

@afoucret any new about this one ?

@romainruaud
Copy link
Collaborator Author

romainruaud commented Aug 10, 2017

PR Updated :

  • inject proper field name for attributes directly via addSortFilterParameter

  • little change to prepareSortOrder function :

previously, the $sortField was erased because it was recomputed from $attribute even if found in _productLimitationFilters.

This was leading to the following issue : first "option_text_manufacturer" was properly pulled from _productLimitationFilters, but then overwritten by mapFieldName($attribute) where $attribute is the "real" attribute code without the "option_text_" prefix

@afoucret afoucret merged commit 9040bb6 into Smile-SA:2.3.x Sep 5, 2017
@romainruaud romainruaud deleted the fix_product-sorting branch September 8, 2017 10:44
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.

2 participants