Search API integration doesn't check/define feature support of backends

You're absolutely right. We should do this.

I'm not sure, but I think we should make support for "or" facets mandatory.
I included some code that should do this.
It's not tested but I think this is the right place to do this?

I think this does what @drunken monkey suggested, let's see what our tests think.

I think we can do the renames of the plugins in a later followup and see if we can implement the search api pages displays as well. I think otherwise these changes look good. Needs confirmation though.

The test fails here are because the base plugin changed from views_page to search_api. I'll do the find/replace later to fix the tests. Thanks for the patch - that looks great. I also think that we can now remove the facetsource plugin from search_api_page when this goes in. That sounds great!

Changed tests.

Looks like fixing the tests wasn't as easy as I expected it to be. I don't have any more time to look at it before sunday - so if anyone else has time to pick this up in the meanwhile - be my guest.

So, it looks like the tests are failing because we usually equate the search_id to facetsource-id. Because this is no longer the case, this is currently not working. I currently don't know yet how to map the search_id to the facet source id.

Maybe this works? I also added a processDefinition to the plugin manager so we are sure they have the display_id line in them.

Reverted #35 since that doesn't seem to help at all.

Patch no longer applied, reroll attached

Rerolled.

The reason why the core search tests are failing is because it looks like an invalid plugin is being created for the search api plugins. I don't understand why though.

Maybe we should remove the ::processDefinition in the plugin manager? That's not a solution though, that's just removing the hard fail.

Anyway, that's enough for the day.

Since I was ill over the last week I haven't had a lot of time to work on this. At least not as much as I wanted.

Anyway, attached is what I did today, this should fix the unit tests and on my local machine it looks like the core search tests are fixed now as well.

Fixes rest test in the wrong way.

fail: [Other] Line 129 of modules/facets/modules/rest_facets/src/Tests/RestIntegrationTest.php:
Value 'http://localhost/checkout/checkout/facets-rest?f[0]=type%3Aarticle' is equal to value 'http://localhost/checkout/facets-rest?f[0]=type%3Aarticle'.

The double checkout here is not working, so it looks like the changes I did in the QueryString Url processor broke things in fun ways. The tests don't seem to break when ran not in a subfolder.

I had some time to work on this on the plane over to iron camp. The rest test is green now, as well as all the unit tests. Hopefully there will be no other fails.

This also reduced complexity in a couple of places by a little bit.

Setting to needs review for the testbot.

UrlIntegrationTest is now green locally. The entire patch green by now?

So the patch is green - all this needs is a review.

This needs an upgrade path (just to help people already building on this).

Patch no longer applies and I screwed up my local branch. Needs a reroll.

Reroll attached, looking into fixing #70.

Looks like the patch I made in #73 doesn't work. Updating readme as requested in #70.

Reuploading for testbot.

Hey,

just asking: Do you keep in mind that CoreViewsFacets and at least potentially other modules, like your CoreSearchFacets module, do not use Search API and therefor do not depend on it?

I understand that after all Solr is your main focus, but in a perfect world, facets would provide its functionality independently of the underlying data source and handle those in separate handlers, right?

If you want any support in that direction, I am very willing to help.

@ChristianAdamski:

just asking: Do you keep in mind that CoreViewsFacets and at least potentially other modules, like your CoreSearchFacets module, do not use Search API and therefor do not depend on it?

I understand that after all Solr is your main focus, but in a perfect world, facets would provide its functionality independently of the underlying data source and handle those in separate handlers, right?

Yeah, Search API is the main focus. Not Solr specifically but also other search api backands (db, elasticsearch, ...). We try to keep in mind other facet sources can also be provided.
We don't (from memory) have any search api specific code other than in the FacetSource. Nothing in this issues creates a closer coupling with Search API.

If we do have a place where we the coupling is too tight, could you open up a new issue? this issue is already a nightmare and I want to keep the suffering for this specifically as short as possible (even though it looks like I probably won't get to fix this before my vacation between christmas and new years).

Reroll.

Reroll + small changes for new tests.

go testbot, go!

I fixed the kernel tests.

I think all tests are green now.

Cleanup + comments + small changes. I'm not going to touch this (or commit anything else) until we get reviews in.

I'd also call the variable $facet_source_id (additional underscore), but that's of course even more unrelated. (Also, it's used the other way in lots of other places.)

Yeah, we've been using facetsource_id in a bunch of places. Keeping that as-is for now.

Missing "i" – but also, the whole file wildly mixes snake_case and camelCase for variables, which I'd try to avoid. Your choice, though, I guess.

Yeah, we should go trough everything and fix all those things. Not trying to worry about that in this issue. But I agree that I should try harder to keep style consistent.

Just using the derivative ID is of course an idea to avoid the "double colon" problem, but it will necessarily lead to collisions when, e.g., a view and a search page have the same ID. I think you'll have to make sure to deal with duplicates in any case, so you might as well use the whole display ID and just "sanitize" it.

Also, we got rid of the sorting in our derivers in the Search API, since they served no real purpose. I'd suggest doing the same here.

I removed the sorting in the derivers, but I'm not sure what you mean with just using the display ID?

Apart from this, I just added more information about the search_api_facets feature to README.txt. This should now really suffice as base information for someone who wants to add that to their backend plugin.

That looks great, thanks!

Also, is the "Needs upgrade path" still valid? Seems there is now an upgrade function.

You're right, that "Needs upgrade path" is no longer valid. Thanks!

EDIT: quick paste went wrong. The check for instanceof Url, that part of the code in the patch...
Can this be removed? Need to check if getPath can return something else

This fixes #96 and adds/improves docs a little.

Docs cleanup. Removed the strpos checking in the form and used instanceof checks instead.

1. +++ b/facets.install
   @@ -0,0 +1,28 @@
   + * Rename old search api facet sources to the new naming scheme.
   

   Perhaps a description what this update actually is trying to and trying to change (and why) instead of what is literally does would be a good change here.

2. +++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
   @@ -0,0 +1,343 @@
   +    // Check if there are results in the static cache.
   

   Not sure if this comment makes a lot of sense. How do you know the searchApiQueryHelper takes it from static cache? If that function ever changes this comment is no longer valid. Maybe better to say if the results have been populated already and if not, we should populate those in here.

3. +++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
   @@ -0,0 +1,343 @@
   +      // The getPluginId is "views_rest:search_content__rest_export_1" and the
   +      // derivative id is "views_page:search_content__rest_export_1".
   +      // It looks like another plugin is creating the display that we need to
   +      // load so we have to fix that by finding the results from the correct id.
   

   I don't get this comment at all. What does this mean and what display do "we" want to load? Why is views_rest and views_page even included here? All I understand is that we get allResults from the searchApiDisplay and get the derivativeId from whatever we have in our hands (object) at that moment. Confused :)

4. +++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
   @@ -0,0 +1,343 @@
   +          $results = $result_set;
   

   Can we not have multiple items that go into $results? If so, why don't we add a break here so that the foreach loops stops after one assignment to $results.

5. +++ b/src/Plugin/facets/facet_source/SearchApiDisplay.php
   @@ -0,0 +1,343 @@
   +      // END.
   

   Why is this even relevant to add?

6. +++ b/src/Plugin/facets/facet_source/SearchApiDisplayDeriver.php
   @@ -0,0 +1,57 @@
   +      // If 'index' is not set on the plugin, we can't load the index to check
   

   Missing dot to mark the end of a phrase.

7. +++ b/src/Plugin/facets/facet_source/SearchApiDisplayDeriver.php
   @@ -0,0 +1,57 @@
   +      // if the 'search_api_facets' feature is available on the backend.
   

   This comment should be repositioned to where the logic actually is residing.

Thanks for that, I hope the improved comments make it somewhat clearer what's going on here.

Per your request I also took a stab at splitting up the patch into multiple smaller pieces, but I figure the only way to do that is to actually create multiple issues and that split this patch out over those issues. If we don't get consensus over this issue I'll do that on monday to make reviews afterwards easier. For now I'm going to try to figure out .3 here.

1. Yep, it was already improved in #98 but I think it's even better now.
2. You're right, I know this, but the code can't possible know that. Thanks! I hope the current comment improves this.
3. Yeah, this is very confusing.

   The problem happens when there's a facet built on the non-standard display of a view.
   So for example there's a view (aaaaa) that has a page-display and rest-display.
   I have 1 facet / display.

       if ($results === NULL) {
         $all_results = $this->searchApiQueryHelper->getAllResults();
         $derivative_id = $this->getDisplay()->getDerivativeId();
   
         foreach ($all_results as $plugin_id => $result_set) {
           if (strpos($plugin_id, $derivative_id) !== FALSE) {
             $results = $result_set;
           }
         }
       }
   

   When I request the page display everything works as it should even without this codeblock. When looking at the rest display though, without that codeblock the facet is empty.
   This is because $this->getDisplay()->getPluginId() is views_rest:aaaaa__rest_export_1. That's correct.
   However, there are no results in the searchApiQueryHelper for that id. There are results in the searchApiQueryHelper for views_page:aaaaa__rest_export_1.

   So the code there actually just gets aaaaa__rest_export_1 ($this->getDisplay()->getDerivativeId();) and checks if any of the results in the result cache have that as part of their plugin id (the strpos check). This is a horrible way to do this and very error-prone.

   I think what's actually going on is that the results get saved with the wrong ID in search api. I'll try figuring out how exactly that is going on.

4. Yeah, I could add that, but we might be able to remove the entire block (see .3)
5. No, that's only for me to see what part of the code I needed to flag to reviewers :)
6. Added a dot.
7. Removed this line, there was already a comment to the same effect 10 lines down in the file.

About .3: If I change
$this->query->setSearchId('views_page:' . $view->id() . '__' . $view->current_display); to $this->query->setSearchId('views_rest:' . $view->id() . '__' . $view->current_display); in \Drupal\search_api\Plugin\views\query\SearchApiQuery::init to entire if ($results === NULL) {} is no longer needed. This means we either have to override the search ID from the facets or fix this in search api.

I think this should be fixed from in search api?

Not sure if that made sense to do, but I opened up #2839981: Improve search id correctness in views integration in search api. Chasing that for a bit.

If #2839981: Improve search id correctness in views integration lands, we can simplify a little bit here by removing the part that confused Nick in #99. (See attached patch - this won't pass tests but it IS correct.)

The fails in Drupal\Tests\facets\Functional\UrlIntegrationTest are weird, but fixed by restoring the version of that test from 8.x-1.x. The failures in Drupal\Tests\facets\Functional\FacetSourceTest I can't reproduce locally.

Expecting 1 failure.

I have no idea why these tests are failing. They aren't failing on my machine. I'll set up a local drupalci to see why these tests are failing.

I have no idea why these tests are failing. They aren't failing on my machine. I'll set up a local drupalci to see why these tests are failing.

Hah, that was easy to say. I'm giving up on trying to set up drupalci locally for the day. Still no idea how to reproduce those testfailures.

I can reproduce the two test fails in #107 on my local vagrant box (CentOS Linux release 7.1.1503). I could have a look at it the upcoming weekend, if that would help.

@dermario: oh good to know that it's reproducable on centos, I'll try setting up a vagrant box on thursday. Thanks!

Morning! Trying both patches #107 and #111 on both facets alpha 7 and dev, with search_api beta 4 gives me the following unexpected error / WSOD:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "views_page:search_api_publications__overview" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('views_page:search_api_publications__overview') (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('views_page:search_api_publications__overview', Array) (Line: 84)
Drupal\Component\Plugin\PluginManagerBase->createInstance('views_page:search_api_publications__overview') (Line: 388)
Drupal\facets\FacetManager\DefaultFacetManager->updateResults('views_page:search_api_publications__overview') (Line: 210)
Drupal\facets\FacetManager\DefaultFacetManager->processFacets('views_page:search_api_publications__overview') (Line: 295)
Drupal\facets\FacetManager\DefaultFacetManager->build(Object) (Line: 82)
Drupal\facets\Plugin\Block\FacetBlock->build() (Line: 123)
Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder->buildRegions(Array, Array) (Line: 162)
Drupal\panels\Plugin\DisplayBuilder\StandardDisplayBuilder->build(Object) (Line: 328)
Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant->build() (Line: 34)
Drupal\page_manager\Entity\PageVariantViewBuilder->view(Object, 'full') (Line: 96)
Drupal\Core\Entity\Controller\EntityViewController->view(Object, 'full')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

"search_api_publications" is the machine name of my view here, while "overview" is the machine name of my display. The view only contains this display, which is a block display.

@vegardjo: oh - that looks like the update hook didn't work as planned? I'll try to fix that as well. Updated IS with todo's

Oh, sorry, didn't notice there was an update hook there. However, after patching aplha7 with #111 again, and doing the db update, I still get the same error.

I set up a new drupalvm vagrant box and tried the UrlIntegraionTest and FacetSourceTest again (w/ #107 applied) and I can't get them to fail. This is an ubuntu box though so that might not be it. not sure how easy it is to set up a similar env w/ CentOS. It's too late to do that today so that'll have to be for another day.

There is a check for whether the source is an instance of SearchApiFacetSourceInterface in ListItemProcessor and this patch is causing a bug that doesnt render fields with allowed_values settings. Submitting a patch for the same!

Thanks,
Sukanya

Sorry, missed to remove the unused use statement for SearchApiFacetSourceInterface. Adding a corrected patch and an interdiff for the same.

Ok, so I tried this again w/ centos yesterday and it took me a while to set up a new box. I didn't see any errors though in the time I had to test this. I'd suggest to go back to #107 for now and let's try to get that one green. Once we manage that we can have a look at #110/ #111.

Since #118 introduces new failures and nothing failed on the tests for that part yet - let's delegate that to a followup? This is issue is big enough as-is.

Unassigning @StryKaizer for now, as a review is not yet needed before we get this green :)

@borisson_ Sorry to hear that the test did not fail. I will investigate in that problem this weekend (when i manage it to pass the xdebug cookie to the test itself).

Since I can't reproduce the failure (and I made my laptop crash 3 times with trying to run all the tests on a centos vagrant box and I can't get the testbot to run locally) I'm just guessing right now. This is 107 + a guess

Reupload of #107. I still have no idea how to fix these fails.

Sorry for not replying at the weekend, @borisson_ i didn't know that you try to go the CentOS way :-/ BIIG SORRY!!
I also investigated into that issue and somehow i couldn't reproduce it on CentOS any more. So i tried to reactivate some of my older linux machines at home to install a testbot on it. That did not work out as they are 32 Bit and Docker requires 64 Bit. So i set up a local Virtualhost machine and hacked as much as i could to make it running somehow. My Mac even went out of hdd-space. Somehow it works today - running the tests with sudo and hacky frwrites to STDERR inside the tests gave me a hint for the fail in FacetSourceTest. Please see the attached interdiff.

I don't think that this patch is the final solution for one of the fails in #107 but it maybe points us to the right directory. Lets see what the testbot says.

Sorry for the naming - mess. This one should be correct.

Sorry for not replying at the weekend, @borisson_ i didn't know that you try to go the CentOS way :-/ BIIG SORRY!!

Sorry for the naming - mess. This one should be correct.

No need to say you're sorry. I'm very grateful for all of the help you've provided here so far. Very curious about what the tests will say about those patches.

I am pretty sure that the tests in #107 fail due a different sort order of the facet sources on our testbots. On our local machines the sort order is like:

      <tbody>
                      <tr class="facet-source odd">
                      <td class="facets-type">Facet source</td>
                      <td>search_api:search_api_test_view__block_1</td>
                      <td><a href="/admin/config/search/facets/facet-sources/search_api%3Asearch_api_test_view__block_1/edit">Configure</a></td>
                  </tr>
                      <tr class="facet-source even">
                      <td class="facets-type">Facet source</td>
                      <td>search_api:search_api_test_view__page_1</td>
                      <td><a href="/admin/config/search/facets/facet-sources/search_api%3Asearch_api_test_view__page_1/edit">Configure</a></td>
                  </tr>
          </tbody>

On our testbots it is:

Type         Title                                   Operations
Facet source search_api:search_api_test_view__page_1 Configure
Facet source search_api:search_api_test_view__block_1 Configure

I attached another simple fix to proof my assumption. By applying these patches on our local machine the tests will fail there. So we need to find a failsafe solution for:

 $this->clickLink('Configure');

 $this->clickLink('Configure', 1);

If that's the problem - restoring the uasort might help as well. Let's try that?

Seems like the fails are fixed now :-) My local testbot agrees with the changes in #132.

Woo! Tests are green again. That's awesome. Thanks so much for all your help here @dermario!

So, now we have to decide about #110:

When a search display plugin has the plugin ID views_page:test__test, you only use the test__test part for your facet source plugin ID. That seems dangerous, since you end up with potential conflicts where IDs were previously unique. E.g., if someone creates a search page with ID test__test – the search display plugin ID would be search_api_page:test__test, and thus unique, but for your facet sources either the Views page or the search page would get overwritten (since both would get the facet source plugin ID search_api:test__test).
(Admittedly, this example sounds far-fetched, but with other modules defining search display plugins, this might get more likely – e.g., when two displays (defined by different module) just have search as the derivative plugin ID.)
I'd therefore recommend using the whole plugin ID (views_page:test__test and search_api_page:test__test), including the base ID, for your facet source derivative IDs, and just sanitizing them to avoid the additional colon (e.g., replace the : with __).

I don't think we have to care about that right now. We can always do the same we did right now (change the ids, provide upgrade path).

I'm going to manually test the upgrade path again tonight so that question should get answered before we can commit this patch (and all the open patches in the queue).

It would be great if this patch could be applied soon. Can be very confusing that 2777217 can't be applied without this one.

I just tested the upgrade path and that works as expected. I'm happy with the current state of this patch. All we need is an "ok, go" from @Nick_vh and/or @StryKaizer

So @StryKaizer convinced me that we should do #110 + #136.

I fixed the upgrade path and most of the tests that failed in #111. Let's see how many things I missed.

I don't think that test fail is our fault, as the test indicates that the patch didn't apply:

23:21:50 Checkout complete.
23:21:50 Fetch of https://www.drupal.org/files/issues/search_api_integration-2772745-139.patch to /var/lib/drupalci/workspace/jenkins-default-296910/ancillary/search_api_integration-2772745-139.patch complete.
23:21:50 Patch Error
23:21:50 The target patch directory /var/lib/drupalci/workspace/jenkins-default-296910/source/modules/contrib/facets is invalid.
23:21:50 PHP Warning:  implode(): Invalid arguments passed in /opt/drupalci_testbot/src/DrupalCI/Plugin/BuildTask/BuildStep/CodebaseAssemble/Patch.php on line 93

https://dispatcher.drupalci.org/job/default/296910/console

1 Error_ Patch failed to apply
1 Apply Patch
✗
search_api_integration-2772745-139.patch
Patch Failed to apply

Confirmed: #2842529: ECK patch test because eck module not checked out?

Ok, so #139 now fails with 2 failures and no longer is broken because of the testbot. I'll resolve those 2 fails after work.

I noticed a failure in the facet summary test locally that I didn't see online. Maybe we should investigate? I'll try to remember that.

Also testOnViewDisplayRemoval found a legit bug. Hurray!

Green tests again, that's great! Do we need a test for the upgrade path as well?

I tested this on a project we are working right now and found a bug. Everything works fine, except for the facet blocks we have inserted via panels. Here the getPath() function returns the views path instead of the path from the current page. Any Idea as to how we fix this?

I just found that the patch changes SearchApiDisplay->getPath() from:

  public function getPath() {
    $display = View::load($this->pluginDefinition['view_id'])->getDisplay($this->pluginDefinition['view_display']);
    switch ($display['display_plugin']) {
      case 'page':
        $view = Views::getView($this->pluginDefinition['view_id']);
        $view->setDisplay($this->pluginDefinition['view_display']);
        return '/' . $view->getDisplay()->getPath();

      case 'block':
      default:
        $current_path = \Drupal::service('path.current')->getPath();
        if (\Drupal::moduleHandler()->moduleExists('path')) {
          return \Drupal::service('path.alias_manager')->getAliasByPath($current_path);
        }
        else {
          return $current_path;
        }
    }
  }

to:

  public function getPath() {
    // The implementation in search api tells us that this is a url object only
    // if a path is defined, and null if that isn't done. This means that we
    // have to check for this + create our own Url object if that's needed.
    if ($this->getDisplay()->getUrl() instanceof Url) {
      return $this->getDisplay()->getUrl();
    }

    return Url::createFromRequest($this->request);
  }

Which is the cause of this issue. Is there a reason for this change or can it be changed back to account for the cases where the block is not on the url of the view page?

Br Jesper

i created this fix for our own project to solve the issue for now. Hope you can use it.

Br Jesper

Looks like that is a bug in Search API, not facets. That's where the path is generated.

One of the pro's of this patch is that we don't have to duplicate changes to displays for facets and search_api_sorts (and others). The patch in #144 reintroduces a (small) place where we'll have to keep things in line again.

@jespermb is it possible to fix this in search api instead?

Sure i will look into that. Thanks for the reply.

Br Jesper

#2842971: Handle path for blocks makes #145 obsolete.

@jespermb: can you confirm that the upgrade path worked?

Yes it worked as expected.

I tried to update a customer project from alpha7 to the latest dev + #142 locally and run into an issue.

Facets source id before updb: views_page:search_results__search_results
Facets source id after updb: search_api:views_page__:search_results__search_results

When visiting admin/config/search/facets/section_facet/settings i get:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "search_api:views_page__:search_results__search_results" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

I just took a screenshot of a debug session i did:

I could investigate tomorrow. Today i am to blind to the see the problem ;-)

"search_api:views_page__:search_results__search_results" should be
"search_api:views_page__search_results__search_results"

The problem is the extra :.

Thank you @borisson_ for your confirmation. This patch fixes the upgrade path for me. I also modified the comment a bit.

I tested the upgrade path again locally and noticed that facet summaries were not upgraded. They are now.

Great work all, impressive patch ;-)

• Lets move the summary upgrade path to the summary submodule
• Lets change the upgrade documentation to a short sentence, e.g. "Convert Search API facet sources to use Search API display plugin system."

So, something like this?

Current patch breaks facets which are rendered without being on the actual search api view page.

Lets fix this, and write a test for this which fails without the that fix.

Other than that, upgrade functionally working on my projects.

It looks like this means we don't have sufficient coverage for only_visible_when_facet_source_is_visible.
So we should write a new test that covers that.