Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We have noticed, that the facet calculations are wrong. The bug can be observed when using OR facets on the following site:
- http://inventory.horizonwood.com.dev.us1.compact.amazee.io/
- Select Ash (6) from the Species facet
- Notice that the Thickness facet is now narrowed to 4 options (originally there were 9) and of those 4 options - the total results add up to 6. This operates as expected. Because there were only 6 results for Ash, any of the remaining facets should not exceed matches for more than 6 results.
- But if you look at the other facets, the Category, Quality, and Cutting Style options are not narrowed. In fact, "Figured" in the Category facet shows 108 results available.
The order in which the facets are returned from DefaultFacetManager::getEnabledFacets
is the order in which the facets are processed and the count is being added.
Comment | File | Size | Author |
---|---|---|---|
#31 | facet_calculations_are-2711697-31.patch | 11.21 KB | borisson_ |
Comments
Comment #2
StryKaizerNeeds test
Comment #3
borisson_I'll have a look at this, to try to write a test.
Comment #4
StryKaizerComment #5
borisson_I think this test proves what's wrong, so getting this to pass should be a good start.
Comment #8
borisson_The test failed earlier on
If I change getEnabledFacets in the DefaultFacetManager so it returns this in reverse order, that line passes but it creates a new failure, on line 543 of the test:
This means that the order in which the facets are returned from
loadMultiple
is the order in which the facets are processed and the count is being added.I removed that code from the patch, but added a more restrictive assertion in the test. Not sure how we should resolve this, but at least we have an idea to where this comes from.
Comment #11
borisson_Rerolled this, still no solution though.
Comment #14
borisson_Back to the one expected error. I have no idea how to even start on this.
Comment #17
dasjoI haven't had time to look at this, but from looking at the code, I'd start debugging at
FacetsQuery::execute()
at line$this->condition($this->conditions);
Comment #18
claudiu.cristeaThis, for me, fixes the problem. However, this needs a deep review because I'm not sure we have the same bug.
Comment #21
borisson_The test seems to disagree with your solution, and I can't see how that'd make any difference either.
Comment #22
marthinal CreditAttribution: marthinal commentedI'm trying to understand why the result is always the same in the case of some facets. Well, if we use the OR query operator by default... then we will build the unfiltered options:
Maybe I'm missing something... anyway let's try with this patch
Comment #23
StryKaizerFollowing patch should fix the issue for search api datasources. We should not calculate count ourself but rather delegate this to the datasource, as certain backends (e.g. solr) provide this automaticly.
Other data sources then search api should prolly be looked after to provide the correct count too. An example for sql-based sources can be found in search_api_db in Database.php::getFacets()
This patch includes test from #14
Comment #24
StryKaizerComment #27
claudiu.cristeaThis can be compacted to only 3 lines:
Comment #28
borisson_Attached interdiff fixes #27 and fixes the unit test that broke in #23. @StryKaizer is fixing the integration test meanwhile, so not uploading a full patch.
Comment #29
StryKaizerPatch with integrationfix and interdiff from #28
Comment #30
claudiu.cristeaNits. I will test the patch in the evening.
Space around if() parenthesis. See https://www.drupal.org/coding-standards#controlstruct
Remove this un-needed line. Just after:
Comment #31
borisson_Fixes nits from #30.
Comment #32
claudiu.cristeaIs working as expected. Thank you.
Comment #33
borisson_Committed, thanks for the reviews and the patches everyone.