In the build method of DefaultFacetsSummaryManager, all facets are built and results for all facets are retrieved instead of just building and getting results for the facets which were selected for the current facet summary.

If I understand correctly, this should only be the case for the facets which were actually selected for that facets summary.

So

    foreach ($facets as $facet) {
      // For clarity, process facets is called each build.
      // The first facet therefor will trigger the processing. Note that
      // processing is done only once, so repeatedly calling this method will
      // not trigger the processing more than once.
      $this->facetManager->processFacets($facetsource_id);
      $this->facetManager->build($facet);
    }

should become

    $facets_config = $facets_summary->getFacets();
    foreach ($facets as $facet) {
      // Do not build facets which are not part of this facet summary.
      if (!isset($facets_config[$facet->id()])) {
        continue;
      }
      // For clarity, process facets is called each build.
      // The first facet therefor will trigger the processing. Note that
      // processing is done only once, so repeatedly calling this method will
      // not trigger the processing more than once.
      $this->facetManager->processFacets($facetsource_id);
      $this->facetManager->build($facet);
    }

and

    foreach ($facets as $facet) {
      $facet_results = $facet->getResults();
      $show_count = $facets_config[$facet->id()]['show_count'];
      if (!$show_count) {
        foreach ($facet_results as $facet_result_id => $facet_result) {
          $facet_results[$facet_result_id]->setCount(NULL);
        }
      }
      $results = array_merge($facet_results, $results);
    }

should become

     foreach ($facets as $facet) {
      // Do not get results for facets which are not part of this facet summary.
      if (!isset($facets_config[$facet->id()])) {
        continue;
      }
      $facet_results = $facet->getResults();
      $show_count = $facets_config[$facet->id()]['show_count'];
      if (!$show_count) {
        foreach ($facet_results as $facet_result_id => $facet_result) {
          $facet_results[$facet_result_id]->setCount(NULL);
        }
      }
      $results = array_merge($facet_results, $results);
    }

See attached patch.
(I marked this as bug report but this could be considered a feature rather than bug.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jummonk created an issue. See original summary.

borisson_’s picture

Issue tags: +Needs tests

I'd probably wouldn't do it like that but instead do something like this, personal preference though but this reduces nesting and makes sure that if we ever add another loop that we wouldn't have to do the same thing again.

$facets_config = $facets_summary->getFacets();
$facets = array_filter($facets, function($item) use ($facets_config) { return (isset($facets_config[$item->id])); }
foreach ($facets as $facet) {
...
}

In any case this will also need a test.

jummonk’s picture

You're right. See modified patch in attachment. Had to call method id() on $item though as you cannot access private class properties directly in anonymous functions.
(The test will be for when I get Webtests running on my local machine which unfortunately is not the case currently.)

borisson_’s picture

Status: Active » Needs review
Issue tags: -Needs tests
FileSize
5 KB
4.09 KB
5.36 KB

Adds Integration test.

borisson_’s picture

FileSize
578 bytes
5.38 KB

  • borisson_ committed 7fbabdf on 8.x-1.x
    Issue #2841357 by borisson_, jummonk: Facet summary manager builds...
borisson_’s picture

Status: Needs review » Fixed

Committed, thanks @jummonk!

Status: Fixed » Closed (fixed)

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