We have noticed, that the facet calculations are wrong. The bug can be observed when using OR facets on the following site:

  1. http://inventory.horizonwood.com.dev.us1.compact.amazee.io/
  2. Select Ash (6) from the Species facet
  3. 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.
  4. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dasjo created an issue. See original summary.

StryKaizer’s picture

Needs test

borisson_’s picture

Assigned: Unassigned » borisson_

I'll have a look at this, to try to write a test.

StryKaizer’s picture

Issue tags: +beta blocker
borisson_’s picture

Status: Active » Needs review
FileSize
2.07 KB

I think this test proves what's wrong, so getting this to pass should be a good start.

Status: Needs review » Needs work

The last submitted patch, 5: facet_calculations_are-2711697-5.patch, failed testing.

The last submitted patch, 5: facet_calculations_are-2711697-5.patch, failed testing.

borisson_’s picture

Title: Facet calculations are wrong (alphabetical order/hierarchy) » Facet calculations are in the wrong order.
Issue summary: View changes
Status: Needs work » Needs review
FileSize
893 bytes
2.21 KB

The test failed earlier on

fail: [Other] Line 527 of modules/facets/src/Tests/IntegrationTest.php:
"grape (3)" not found

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:

"item (1)" found
   public function getEnabledFacets() {
-    return $this->facetStorage->loadMultiple();
+    $facets = $this->facetStorage->loadMultiple();
+    arsort($facets);
+    return $facets;
   } 

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.

Status: Needs review » Needs work

The last submitted patch, 8: facet_calculations_are-2711697-8.patch, failed testing.

The last submitted patch, 8: facet_calculations_are-2711697-8.patch, failed testing.

borisson_’s picture

Assigned: borisson_ » Unassigned
Status: Needs work » Needs review
FileSize
2.2 KB

Rerolled this, still no solution though.

Status: Needs review » Needs work

The last submitted patch, 11: facet_calculations_are-2711697-11.patch, failed testing.

The last submitted patch, 11: facet_calculations_are-2711697-11.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
672 bytes
2.25 KB

Back to the one expected error. I have no idea how to even start on this.

Status: Needs review » Needs work

The last submitted patch, 14: facet_calculations_are-2711697-14.patch, failed testing.

The last submitted patch, 14: facet_calculations_are-2711697-14.patch, failed testing.

dasjo’s picture

I 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);

claudiu.cristea’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3.36 KB
1.12 KB

This, for me, fixes the problem. However, this needs a deep review because I'm not sure we have the same bug.

Status: Needs review » Needs work

The last submitted patch, 18: 2711697-18.patch, failed testing.

The last submitted patch, 18: 2711697-18.patch, failed testing.

borisson_’s picture

The test seems to disagree with your solution, and I can't see how that'd make any difference either.

marthinal’s picture

Status: Needs work » Needs review
Issue tags: +neworleans2016
FileSize
2.34 KB
720 bytes

I'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:

          if ($query_operator === 'OR') {
            $unfiltered_results = $this->facet->getUnfilteredResults();
            $field_identifier = $this->facet->getFieldIdentifier();

            foreach ($unfiltered_results[$field_identifier] as $unfiltered_result) {
              if ($unfiltered_result['filter'] === $result['filter']) {
                $count = $unfiltered_result['count'];
              }
            }
          }

Maybe I'm missing something... anyway let's try with this patch

StryKaizer’s picture

Following 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

StryKaizer’s picture

Status: Needs review » Needs work

The last submitted patch, 23: facet_calculations_are-2711697-23.patch, failed testing.

The last submitted patch, 23: facet_calculations_are-2711697-23.patch, failed testing.

claudiu.cristea’s picture

+++ b/src/Tests/IntegrationTest.php
@@ -455,6 +455,56 @@ class IntegrationTest extends WebTestBase {
+    $this->drupalGet('admin/config/search/facets/keywords/edit');
+    $edit = [
+      'widget' => 'links',
+      'widget_configs[show_numbers]' => '1',
+    ];
+    $this->drupalPostForm(NULL, $edit, $this->t('Save'));
+    $this->drupalGet('admin/config/search/facets/type/edit');
+    $edit = [
+      'widget' => 'links',
+      'widget_configs[show_numbers]' => '1',
+    ];
+    $this->drupalPostForm(NULL, $edit, $this->t('Save'));

This can be compacted to only 3 lines:

$edit = ['widget' => 'links', 'widget_configs[show_numbers]' => '1'];
$this->drupalPostForm('admin/config/search/facets/keywords/edit', $edit, $this->t('Save'));
$this->drupalPostForm('admin/config/search/facets/type/edit', $edit, $this->t('Save'));
borisson_’s picture

FileSize
1.86 KB

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.

StryKaizer’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
11.21 KB

Patch with integrationfix and interdiff from #28

claudiu.cristea’s picture

Nits. I will test the patch in the evening.

  1. +++ b/facets.module
    @@ -30,12 +30,13 @@ function facets_help($route_name, RouteMatchInterface $route_match) {
    +  if($query->getIndex()->getServerInstance()->supportsFeature('search_api_facets')){
    

    Space around if() parenthesis. See https://www.drupal.org/coding-standards#controlstruct

  2. +++ b/src/QueryType/QueryTypeInterface.php
    @@ -10,8 +10,6 @@ interface QueryTypeInterface {
        *
    

    Remove this un-needed line. Just after:

    /**
     * Adds facet info to the query using the backend native query object.
    
borisson_’s picture

FileSize
1 KB
11.21 KB

Fixes nits from #30.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Is working as expected. Thank you.

borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks for the reviews and the patches everyone.

  • borisson_ committed 9d94805 on 8.x-1.x
    Issue #2711697 by borisson_, StryKaizer, claudiu.cristea, marthinal:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.