Use Search API's display plugin to fix facets

Before #2772829 facets_search_api_query_alter is invoked just once:

1#0  facets_search_api_query_alter(Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search id' => 'search_api_views:search:page_1',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_display' => 'object (Drupal\\search_api\\Plugin\\search_api\\display\\ViewsPageDisplay)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
, , ) called at [/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:501]
#1  Drupal\Core\Extension\ModuleHandler->alter(search_api_query, Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search id' => 'search_api_views:search:page_1',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_display' => 'object (Drupal\\search_api\\Plugin\\search_api\\display\\ViewsPageDisplay)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
) called at [/web/modules/contrib/search_api/src/Query/Query.php:468]
#2  Drupal\search_api\Query\Query->preExecute() called at [/web/modules/contrib/search_api/src/Query/Query.php:420]
#3  Drupal\search_api\Query\Query->execute() called at [/web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php:440]
#4  Drupal\search_api\Plugin\views\query\SearchApiQuery->execute(Drupal\views\ViewExecutable Object ([storage] => Drupal\views\Entity\View Object ([*base_table] => ...

After #2772829 facets_search_api_query_alter is invoked twice:

1#0  facets_search_api_query_alter(Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
, , ) called at [/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:501]
#1  Drupal\Core\Extension\ModuleHandler->alter(search_api_query, Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
) called at [/web/modules/contrib/search_api/src/Query/Query.php:505]
#2  Drupal\search_api\Query\Query->preExecute() called at [/web/modules/contrib/search_api/src/Query/Query.php:457]
#3  Drupal\search_api\Query\Query->execute() called at [/web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php:426]
#4  Drupal\search_api\Plugin\views\query\SearchApiQuery->execute(Drupal\views\ViewExecutable Object ([storage] => Drupal\views\Entity\View Object ([*base_table] =>  ...

and

2#0  facets_search_api_query_alter(Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
, , ) called at [/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:501]
#1  Drupal\Core\Extension\ModuleHandler->alter(search_api_query, Index: solr
Keys: NULL
Searched languages: nl, und, zxx
Options: array (
    'conjunction' => 'AND',
    'search_api_view' => 'object (Drupal\\views\\ViewExecutable)',
    'search_api_base_path' => 'search',
    'skip result count' => true,
    'offset' => 0,
    'limit' => 11,
  )
) called at [/web/modules/contrib/search_api/src/Query/Query.php:505]
#2  Drupal\search_api\Query\Query->preExecute() called at [/web/modules/contrib/search_api/src/Query/Query.php:457]
#3  Drupal\search_api\Query\Query->execute() called at [/web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php:426]
#4  Drupal\search_api\Plugin\views\query\SearchApiQuery->execute(Drupal\views\ViewExecutable Object ([storage] => Drupal\views\Entity\View Object ([*base_table] => ...

At first sight it appeared that search_api_display is missing:

    'search_api_display' => 'object (Drupal\\search_api\\Plugin\\search_api\\display\\ViewsPageDisplay)',

But this is removed in #2772829:

-      // We only provide a display plugin for Views page displays.
-      // @todo figure out how to allow new displays for other views display
-      //   types to be added. Do we need a hook for this?
-      if ($display->getPluginId() == 'page') {
-        // Load the Search API display and attach it to the query.
-        $display_plugin_manager = \Drupal::service('plugin.manager.search_api.display');
-        $search_api_display = $display_plugin_manager->createInstance('views_page:' . $view->id() . '__' . $view->current_display);
-        $this->query->setOption('search_api_display', $search_api_display);
-      }

Before #2772829, results came from the static cache in fillFacetsWithResults():

    // Check if there are results in the static cache.
    $results = $this->searchApiQueryHelper->getResults($this->pluginId);

After #2772829 the static cache appears empty and the following part in fillFacetsWithResults() returns no results:

    // If our results are not there, execute the view to get the results.
    if (!$results) {
      // If there are no results, execute the view. and check for results again!
      $view = Views::getView($this->pluginDefinition['view_id']);
      $view->setDisplay($this->pluginDefinition['view_display']);
      $view->execute();
      $results = $this->searchApiQueryHelper->getResults($this->pluginId);
    }

The problem is here:

-    // Set the search ID, if it was not already set.
-    if ($this->query->getOption('search id') == get_class($this->query)) {
-      $this->query->setOption('search id', 'search_api_views:' . $view->storage->id() . ':' . $view->current_display);
-    }

The search ID should be set, if it was not already set.

@mkalkbrenner, I guess you'll need to commit that on search_api and then re-assign this to @boris to commit your patch on facets and fix tests there.

I committed @mkalkbrenner's patch

Last patch #8 from mpp works as expected! I've tested out and facets do display as expected!
@borisson_
looks like I do not see your commit in search_api history, could you please double check it and commit!
Thanks for all your effort!

fyi: Actually @borisson_ I see your commit:

-    $search_id = $query->getOption('search id');
+    $search_id = $query->getSearchId();

but that doesn't fix facets disappear!

instead PATCH from #8 does make it work for me
Thanks

@borisson_
looks like I do not see your commit in search_api history, could you please double check it and commit!

I have no commit rights on search_api, I only committed the facets patch.

@edutrul, both patches are required:
The one in facets is needed as search API added in #2772829 a different method to get the search id "getSearchId".
The one in search_api is needed as the search id should always be set.

#8 Makes it work for me. It think this should be committed first and the inconsistent search_id generation cleanup then be addressed in a separate issue?

Unfortunately, as a by-product of #2772829: Make "search id" a query property instead of option we had to change the search ID assigned to Views displays, since they now have to correspond to the search display plugin ID. The patch in #8 would just revert the search ID to the previous one, thus breaking that functionality. It's therefore definitely not the correct fix here.

The problem seems to be that the Facets module still isn't using Search API display plugins to determine the available search pages, but has hard-coded logic to translate Search API Views displays to facet sources – which are, however, still using the old search IDs, as used before #2772829: Make "search id" a query property instead of option.
A quick fix would be to update the facet source plugin IDs to reflect the new format for search view page displays.
The correct fix, I think, would be to have a general Search API facet source class (not Views-specific) and use the list of existing search display plugins to derive all facet sources from that. I thought there was already an issue for that, but I can't find it right now.

This is the quickfix mentioned in #18.

Looks like the "quick fix" is not really that quick and we should do this "The right way", I hope to be able to figure this out today.

This is the work I did on this yesterday.

Fixes some of the core search issues.

It looks like search_api_facets option is not set on the query.

With #29 facets are displayed when I recreate all the facets.

When I export the facet config it changes these 2 keys:

dependencies:
  config:
-    - views.view.search
+    - views.view.search__page_1

This causes an [ERROR] Configuration objects (facets.facet.myfacet) provided by my_search have unmet dependencies

-facet_source_id: 'search_api_views:search:page_1'
+facet_source_id: 'views_page:search__page_1'

So I updated all facet_source_id to 'views_page:search__page_1' and removed the dependencies to 'views.view.search__page_1'.

It looks like this doesn't support REST displays or block displays yet, which is probably the reason for the remaining failures.

I'm giving up for today, this has defeated me.

Tested the patch from #32 in combination with search_api-8.x-1.0-beta1 and search_api_solr-8.x-1.0-alpha6 in a views page display.
The facets are working and not disappearing.

Awesome, thanks for testing that Sam.

That means that the page are back to working as expected. So we "just" have to fix behavior for blocks and rest pages, the Search API portion of that work is being done in #2799475: Support block and rest displays in the display deriver

Currently failing on search-api-beta1:
- Drupal\rest_facets\Tests\RestIntegrationTest (still failing)
- Drupal\facets\Tests\IntegrationTest::testFramework (fixed with Search API patch)
- Drupal\facets\Tests\IntegrationTest::testBlockView (still failing)
- Drupal\facets\Tests\IntegrationTest::testOnViewRemoval (still failing) - needs to fixed in search api, too much caching on the deriver - I think.
- Drupal\facets\Tests\UrlIntegrationTest::testUrlIntegration (fixed with Search API patch)

This means that the page things are all working as they're supposed to. We need to fix the other displays though, I'm not sure if that's to be fixed here or in search api. Trying to continue work on this.

I'm giving up on this for today, hopefully I can figure it out tomorrow.

This fixes Drupal\facets\Tests\IntegrationTest::testBlockView by disabling "Hide facet when facet source is not rendered".

So Drupal\rest_facets\Tests\RestIntegrationTest and Drupal\facets\Tests\IntegrationTest::testOnViewRemoval still need to fixed.

In #36, I said:

- Drupal\rest_facets\Tests\RestIntegrationTest (still failing)
- Drupal\facets\Tests\IntegrationTest::testFramework (fixed with Search API patch)
- Drupal\facets\Tests\IntegrationTest::testBlockView (still failing)
- Drupal\facets\Tests\IntegrationTest::testOnViewRemoval (still failing) - needs to fixed in search api, too much caching on the deriver - I think.
- Drupal\facets\Tests\UrlIntegrationTest::testUrlIntegration (fixed with Search API patch)

I fixed the RestIntegrationTest. testBlockView was fixed w/ the previous patch, so still the OnViewRemoval test to fix.

So I just ran the IntegrationTest again on the plane and I think everything is ok now. However we still need the search api patch. Not sure how to proceed from here. Running all tests again on my local machine to confirm that everything is resolved.

This should fix the facet source integration test and now all tests are green again locally.
I feel quite happy with this, I'll review this patch again and I'll see if nothing slipped in that's not needed.

I don't think that we can commit this just yet though - since the search api patch needed to make this work is not in a tagged release yet.

Some cleanup of the patch. Now I'm going to try to reach @drunken monkey to see what to do next.

so, we also need #2799475: Support block and rest displays in the display deriver, I almost forgot about that. That one still has failing tests though.

1. +++ b/facets.module
   @@ -80,7 +80,7 @@ function facets_entity_presave(EntityInterface $entity) {
   +        $test = 'views_page:' . $entity->id() . '__' . $display['id'];
   

   Always good to put, in comments, the reason why this is a double underscore. Reading the code, whomever is making changes won't get why this was done.

2. +++ b/facets.module
   @@ -122,3 +122,25 @@ function facets_preprocess_block(&$variables) {
   + * Implements hook_entity_delete().
   

   Can we get a bit more comments here what this is doing exactly? I suppose it deletes facets from entities that are being deleted.

   Also, are there places where this could fail? We have to be very robust here as this could completely break a site.

3. +++ b/facets.module
   @@ -122,3 +122,25 @@ function facets_preprocess_block(&$variables) {
   +    foreach ($facet_source_plugin_manager->getDefinitions() as $plugin_id => $definition) {
   

   For example, check if there are definitions and if this is really an array.

4. +++ b/facets.module
   @@ -122,3 +122,25 @@ function facets_preprocess_block(&$variables) {
   +        $fm = \Drupal::getContainer()->get('facets.manager');
   

   Check if this will return the facetManager

5. +++ b/facets.module
   @@ -122,3 +122,25 @@ function facets_preprocess_block(&$variables) {
   +        $facets = $fm->getFacetsByFacetSourceId($plugin_id);
   

   Same, make sure this is really a list. And perhaps catch errors?

6. +++ b/facets.module
   @@ -122,3 +122,25 @@ function facets_preprocess_block(&$variables) {
   +    $facet_source_plugin_manager->clearCachedDefinitions();
   

   Also needs comments on the reason why this is needed.

7. +++ b/src/Plugin/facets/facet_source/SearchApiViewsDeriver.php
   @@ -22,54 +19,27 @@ class SearchApiViewsDeriver extends FacetSourceDeriverBase {
   +    foreach ($search_api_displays->getDefinitions() as $display) {
   

   should we check for errors here? Is there any way this can go wrong?

.7 shouldn't be able to fail. Other things are addressed.

Also attributing Nick for help on this during the sprint.

Also tagging this w/ dublin's tag.

I have tested patch 51 and have found a problem. With the patch, the facet list displays when I first visit a view page showing results from a Search API index, and continue to be displayed while facets are selected, but if I clear all facets then all the listed facets disappear. The facet list shows up again after doing a cache rebuild.

This has been tested with a clean Drupal 8.1.10 install with the latest dev versions of Search API, Search API Solr and Facets modules. The same behaviour can been seen using the database backend as well as the Solr backend.

I have tested patch 51 and have found a problem. With the patch, the facet list displays when I first visit a view page showing results from a Search API index, and continue to be displayed while facets are selected, but if I clear all facets then all the listed facets disappear. The facet list shows up again after doing a cache rebuild.

Not sure if we should try to resolve that in this issue. I'll try to reproduce this.

The problem I found is introduced with the patch - although the caching issue might be related to something else. I'm in Dublin and happy to help with this if I can.

The problem I found is introduced with the patch - although the caching issue might be related to something else. I'm in Dublin and happy to help with this if I can.

Oh, we're in the sprintroom right now, come and find us at the search api table!

The problem I found is introduced with the patch - although the caching issue might be related to something else. I'm in Dublin and happy to help with this if I can.

I've been trying to reproduce this, with a fresh 8.1 download w/ search_api's latest HEAD and database db and the patch in #51 applied.

I can't seem to do that.

Steps to reproduce the problem:
- Fresh Drupal 8.1.10 site install with latest dev versions of Search API and Facets
- Patch Facets with patch #51
- Enable search_api, search_api_database and facets
- Add tags taxonomy as a field to basic page and add the following tags: a, b, c
- Create a few basic pages and articles tagged with tags
- Create a new search API database server
- Create a new index, using content as the data source and the database server
- Add 'Title' and 'Tags » Taxonomy term » Name' to the index and index all content
- Create a new view with a page based on the search api index
- Add 'Content datasource: Title (indexed field)' and 'Content datasource: Tags » Taxonomy term » Name (indexed field)' to the view
- Create a new facet using the view as the source and 'Tags » Taxonomy term » Name' as the field
- Add the facet block to a region
- Rebuild the cache

The facet block will display, you can select facets and the view will be filtered, but if you remove all the facets then the facet block will disappear. The block will show again after rebuilding the caches.

Committed this as-is after running tests locally again. Hopefully the testbots will agree.

This works on my local machine, as well as Nick's, so no idea how to resolve this. Trying w/ debug info in the tests here.

The last test that failed on the bot is now green again locally. So now everything should green.

WOO! GREEN! WOOOOOOO

Was already committed.