Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.To make the native facet range query the min and max will have already had to be set, and be known. But then the count will also be available with the results collected on the gaps - so the whole plugin becomes redundant.
This plugin also loops over the results, after it's QueryTypePluginBase has already looped over them. That plugin duplicates the behaviour of QueryTypeRangeBase - although for only one use - it, this version, does implement the query itself better.
Looking ahead it makes more sense to collapse all similar behaviour into a PluginBase used by all range types, this can be changed (once) when the native ranges lands.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | interdiff.txt | 513 bytes | ekes |
| #25 | use_native_facet_range-2872404-25.patch | 6.98 KB | ekes |
| #7 | fix_slider_processor_sort.patch | 767 bytes | agoradesign |
Comments
Comment #2
ekes CreditAttribution: ekes as a volunteer commentedAdding related issue, as this would also remove the need for looping over values.
Comment #3
borisson_I think that we should probably do a usort to sort by count in the processor. Is that a good idea or should we do #2872360: Distinguish between different facets types first and depend on the sorting of the results when they come back from the backend?
Comment #4
ekes CreditAttribution: ekes as a volunteer commentedTo make the native facet range query the min and max will have already had to be set, and be known. But then the count will also be available with the results collected on the gaps - so the whole plugin becomes redundant.
This plugin also loops over the results, after it's QueryTypePluginBase has already looped over them. That plugin duplicates the behaviour of QueryTypeRangeBase - although for only one use - it, this version, does implement the query itself better.
Summary: quick fix for the min/max, alone, put in a sort right at the beginning of this method.
Looking ahead it makes more sense to collapse all similar behaviour into a PluginBase used by all range types, this can be changed (once) when the native ranges lands.
Comment #5
borisson_Sounds like a solid plan to get started with improving this!
Comment #6
ekes CreditAttribution: ekes as a volunteer commentedSo everything from the Processors preQuery, and postQuery, can go into a query class extending the BaseQuery which has it's own equivalent pre and post method calls. This is also code that largely becomes unnecessary when using native ranges.
This also has the UX bonus of not having processors that are a requirement for specific widgets, so need to be enabled for it to work, but that don't work with other widgets.
There is another bit of code in the processor code though. The build method. This _also_ loops over all the results, and alters the query filters. I'm guessing this is specific to the widget, so could be pushed up to there.
Comment #7
agoradesign CreditAttribution: agoradesign commentedThe refactoring plan sounds great, but I think, we need a quick bugfix too. Here's a patch that takes care that the results are properly sorted. Of course that may have another negative impact on performance
Comment #8
borisson_#7 is a good temporary solution, I think I found/fixed a similar issue in: #2894637: Add testcoverage for range + slider widgets/processors. Or is this something else?
Comment #9
agoradesign CreditAttribution: agoradesign commentedTook a quick lookt at your patch: seems it is different. You changed the way how you get the min and max. The problem here is that the values in between need to be properly sorted from min to max
Comment #10
borisson_Setting this one back to active as it's work is now in #2894637: Add testcoverage for range + slider widgets/processors as well. Keeping this one for the bigger refactor.
Comment #11
borisson_Comment #12
borisson_I updated the title and description of this issue to properly point to what we actually want/need to do in this issue.
@ekes: can you extend the IS to point to how we should properly fix this (we can discuss this in Antwerp/Vienna) maybe we should do this in https://www.drupal.org/node/2896229#comment-12176243? Or at least take that in consideration here?
Comment #13
ekes CreditAttribution: ekes as a volunteer commentedSo before I try this out, Joachim's comments in https://www.drupal.org/node/2896229#comment-12176243 do almost certainly overlap.
Before to try and do this it would require some 'black magic' from the configuration to change the facet query, like #2863611: Support range facets. So it seems to me if there is now a concept of 'type' that can be supplied and used to optimise the way the query is made in the back end.
Comment #14
borisson_Comment #15
borisson_Wrong branch I diffed from.
Comment #16
ekes CreditAttribution: ekes as a volunteer commentedRelated:
#2872360: Distinguish between different facets types
#2863611: Support range facets
So like to use consistent strings the value set for ::getSupportedFeatures() like 'search_api_mlt', and probably should be declared 'search_api_location', this could be better 'search_api_range' rather than 'backend_range_grouping'
Comment #17
borisson_Comment #18
borisson_Ah, only the 2.35kb patch is valid here.
Comment #20
borisson_Fixes the unit tests, no idea how to fix the integration test yet. I'll need some help to figure out why that one is failing.
Comment #22
borisson_Comment #23
ekes CreditAttribution: ekes as a volunteer commentedWasn't being called with the query from fillFacetsWithResults()
Comment #24
ekes CreditAttribution: ekes as a volunteer commentedPerhaps #16 wasn't clear enough, but 'search_api_mlt' is already used (it's the more like this); so I was suggesting something like that. Proposal was search_api_range, but that's possibly confusing now that range is also used for from-to filters, so search-api_granular?
Comment #25
ekes CreditAttribution: ekes as a volunteer commentedAla
Comment #26
borisson_Woo! Great work @ekes. Thanks for explaining and debugging this and the performance bump this will lead to.