Facets are lost by Views on keyword search

Modified the if condition because you get Notice: Undefined index: view in search_integration_form_alter() when editing a facet.

i have a use case of needing views exposed filters to work with facet api, i tried this module but it didn't seem to work for me, actually it seemed to have no effect at all.

not sure if I'm missing something...

There's an existing issue with a sandbox that resolves this. #1865950: Automatically add existing facets to the Views exposed filters

Of course a patch would be better ;-)

Lets move this to Search API as the patch is for Views exposed filters that are querying a search_api index.

Here's the code in patch format.

@drunken monkey: What sort of tests would you like for this?

Hi!

I tested and it doesn't work for me.

In my test, $form_state['view']->base_field is empty.

Finally i get working, putting inside a module, and filtering by form id.

Regards,
JOINSO

After further testing I've discovered this has unintended consequences if there's more than one Search API form on the page at once - if there are any matching arguments it adds the values to all forms.

Updated the above patch slightly as it was not working with exposed filters in blocks (which I'd imagine would be fairly common with facets).

This replaces the check for a base_field of 'search_api_id' with a base_table starting with 'search_api_index_', which is the method used throughout the search_api module.

This issue also exists on Drupal 8 with SearchApi, Better Exposed Filters and the Facets module.

Patch #10 also works!

Did a little rewrite for Drupal 8:

/**
 * Implements hook_form_FORM_ID_alter() for views_exposed_form().
 *
 * Custom integration for FacetAPI. When a Views exposed filter is modified
 * on a search results page it will loose any facets which have been already
 * selected. This adds hidden fields for each facet so their values are
 * retained.
 */
function search_api_form_views_exposed_form_alter(&$form, &$form_state) {

  // Check if this form is a Search API form. This could be replaced with the
  // value of a custom Views setting like e.g. "Include facets".
  if ($view = $form_state->getStorage()['view']) {

    if (substr(key($form_state->getStorage()['view']->getBaseTables()), 0, 17) === 'search_api_index_') {

      if (\Drupal::moduleHandler()->moduleExists('facets')) {
        // Get query parameters.
        $query_parameters = Drupal::request()->query->all();
        // If any facet query parameters are provided.
        if (!empty($query_parameters['f'])) {
          // Iterate through facet query parameters.
          foreach ($query_parameters['f'] as $key => $value) {
            // Add hidden form field for facet parameter.
            $form['f[' . $key . ']'] = [
              '#type' => 'hidden',
              '#value' => $value,
              '#weight' => -1,
            ];
          }
        }
      }
    }
  }
}

Patch #10 worked for me.

Sorry for the late reply! This somehow seems to have slipped through the cracks.
In the future, if something like this happens and I don't reply at all in a new issue (or, newly in one of my queues, as in this case) for more than, say, two weeks, please feel free to ping me via my contact form or on IRC.

Anyways, to the issue at hand: There were a few code style problems in the code, which I addressed. Furthermore, this makes much more sense in the search_api_facetapi module (where you also don't need to check whether the Facet API is installed), so I moved it (not contained in the interdiff, since that would make it useless).

The largest problem with this, though, is that this changes current behavior in a way that some people will definitely want, but others won't. So, we definitely need to add an option for that, and default to the current behavior.
In the name of simplicity, I guess the option could be added directly to the Search API Views query plugin, to not make the code unnecessarily complicated. (There, of course, you would actually have to check whether search_api_facetapi is installed.)
With that addition, and since people have already reported this works correctly, I'd be happy to commit this to the module.

(@ #7: Our test coverage in D7 is rubbish anyways, so no need for tests for that. Different matter once we port this to D8, though.)

Patch in #16 works for me!
Thanks!

Thanks a lot for the patch @all. I used the one in #16 and works fine even for my rather complex setup.

I am using a panels page which contains the same search view twice, once as a map display and once as a list of results. Exposed filter (keywords) are exposed as a block and added to yet another pane in panels. Works fine now.

Adding a patch for D8, applies against Search API 8.x-1.6. It is working for me! Thanks.

As said, we'll definitely need an option for this before I'll think about committing it.
But thanks for your feedback!

#16 for D7 seems to work well on my side as well.
Thank you!

patch in #19 for D8 works for me as well

#19 works for me. @drunken monkey where did you envision the option being surfaced in the Drupal UI? I understand the intention but it's not totally clear to me where the option should be.

@drunken monkey where did you envision the option being surfaced in the Drupal UI? I understand the intention but it's not totally clear to me where the option should be.

That’s not completely clear, no. Probably in the view, and since we have best access that way, probably in the query settings (it would logically most likely belong into the exposed form settings, but we can’t really add it there cleanly). It could also be a hidden global config value for the start, that would at least be something. (Since we have no global config form in the UI, configuring it globally via the UI would be tricky. Adding a config form just for that would seem overkill.)

Adding this feature to Views itself would still seem to me to be the cleanest solution, but I understand why that’s unlikely to happen. Makes some sense to instead get it in here. (However, has someone at least tried what the Views maintainers would say to this feature? The exposed form plugin could just have an text field for the query parameters to preserve.)

Adding this into the Facet API/Facets module would also be an option. They’d have an even harder time getting the UI option into Views, sure – but we, on the other hand, have the problem that we can’t really know whether f will even be the correct query parameter. Also not ideal.

All in all, a pretty tricky feature to get right, as it’s spanning three different modules.

I created same issue a while back for views here https://www.drupal.org/project/drupal/issues/2992672 and uploaded a patch for review. The patch takes into consideration if you have facet pretty path module enable else it adds facets filters as hidden form fields to views exposed forms as we have here.

I created same issue a while back for views here https://www.drupal.org/project/drupal/issues/2992672 and uploaded a patch for review. The patch takes into consideration if you have facet pretty path module enable else it adds facets filters as hidden form fields to views exposed forms as we have here.

I don’t think the patch will have any chance in this form for Views – it’s much too specific to Facets, and even a specific configuration at that (since you can just use a different GET parameter instead of f). For Views, I’d definitely leave the GET parameters to preserve configurable – this would allow for a much larger range of use cases. (Would be more work, though, I admit.) But from my experience with Core, trying to get any feature requests in there is pretty much a waste of time anyways, unfortunately. (Usually, they won’t even accept bug fixes without a lot of lobbying.)

So, yes, considering this, and that the Facet API module is only minimally maintained at this point, I guess we’d have to get the feature in here, at least for D7. (For D8, we might talk with the Facets maintainers about this.)
So, if someone supplies a patch that makes this configurable at least in some way, we can commit it.

patch in #16 for D7 works for me

Here is an updated patch for D8

Make in_array check more strict

Minor adjustments because the in_array check was not working

Here is a patch to make it configurable

Adding the schema for the new config setting

Great work, thanks a lot!
This looks pretty good already.

There were just still some issues in the hook code regarding configurability (the query parameter isn’t always f and facet filters are prefixed with the URL alias of the facet, not the ID – but I don’t think we really need to check for those, anyways, just preserving the whole query parameter seems best) and some issues with coding standards, but those were easy to fix.
Revised patch attached, please test/review!

(Changing issue version, as the best patch is now for D8, not D7.)

Patch in #34 did not work for me. Debugging code shows this line $facet_source = FacetSource::load($facet_source_id);returning null. Also, will this work if you are using facets pretty paths module?

I had the same issue with patch #34. The issue is the ":" after "search_api" also needed converted to "__". I also had to load the facet source with entityTypeManager.

One additional issue is that the filter key would return NULL if no filter key exists. But if there is no filter key, then we need to use 'f'.

Ah, right, thanks!
I was confused by the “Machine name” column in the UI, which apparently just lists the label, which looks like an ID and can’t be changed.
I would call this sub-optimal UX, but I’m probably not one to talk.

Also, will this work if you are using facets pretty paths module?

No, since facet parameters aren’t located in a query parameter in that case. (Or maybe they are, internally?)
However, since they are instead part of the path, I’d have thought this would be default behavior when pretty paths are used anyways?
Otherwise, a separate solution is needed for that case, but I think it should better live in the Pretty Paths module.

Having read through all the entire thread I still feel that adding all this facet specific logic to search_api is far from ideal, but I think I've found a solution which we could make work and would make Search API more extensible in the process.

What I propose is the following:
in SearchApiQuery

  /**
   * {@inheritdoc}
   */
  public function defineOptions() {
    $optionDefinitions = [
      ....
    ];
    $options = $this->getOptions();
    foreach ($options as $option) {
      $optionDefinitions[$option->getName()] = $option->getDefinition();
    }

    return parent::defineOptions() + $optionDefinitions;
  }

  /**
   *  @return SearchApiViewOption[]
   */
  private function getOptions() {
    // Get options through the event dispatcher.
  }

  /**
   * {@inheritdoc}
   */
  public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    parent::buildOptionsForm($form, $form_state);

    .... 

    foreach ($this->getOptions() as $option) {
      $form[$option->getName()] = $option->getFormElement();
    }
  }

in search_api.module:

function search_api_config_schema_info_alter(&$definitions) {
  // Alter the schema definition based on the options added through event dispatcher.
  // This ensures that our schema definition stays correct.
}

The suggestion above would allow any module to provide additional options to the SearchApiQuery by implementing an event listener. search_api_form_views_exposed_form_alter() can then be moved to the facets module and become facets_form_views_exposed_form_alter and search_api would no longer have to know anything about the existence of the facets module

search_api would no longer have to know anything about the existence of the facets module

This is not possible to do, because of how facets are generated from their data.
https://cgit.drupalcode.org/search_api/tree/modules/search_api_db/src/Pl... (::getFacets in the database backend).

I think that means that we're implementing a whole lot of code for a goal that is not attainable.
On the other hand, this is something that would make Search API more flexible (which is good), I'm not sure where I sit on this one.

search_api would no longer have to know anything about the existence of the facets module

This is not possible to do, because of how facets are generated from their data.
https://cgit.drupalcode.org/search_api/tree/modules/search_api_db/src/Pl... (::getFacets in the database backend).

Let me clarify. I meant that we would not have to add any new knowledge of facets :)

Thanks for your suggestion, it does sound pretty interesting!
However, I think I’ll have to go with Joris here: As long as we don’t have multiple use cases for this functionality, the added complexity doesn’t seem worth the relatively small benefit of cleanly separating out facets functionality. Especially since this is the Views integration, where we’re generally not that shy about including module-specific code (compared to the framework part of the module, where I’m almost pedantic about this).
As it stands, it seems to me the code would even be cleaner with the current approach than with your suggested alternative, plus it would need additional (also more complex) code in the Facets module.

That being said, if there do appear more use cases for this, it could be worth considering again after all. (At which point we could also refactor the code for this issue to use the new system.) So maybe create a new issue with this suggestion and we’ll see whether other people speak up with their own ideas/use cases.
Note, though, there is also the Views “display extender” plugin which also covers use cases like that. It is a bit more cumbersome than the proposed query plugin-level support for additional options, but could already be used to (pretty cleanly) implement add-on functionality like this in related modules.

PS:

This is not possible to do, because of how facets are generated from their data.https://cgit.drupalcode.org/search_api/tree/modules/search_api_db/src/Pl... (::getFacets in the database backend).

That’s not in the Search API module, but in the Database Search module – an important distinction in this case. While I am always loathe to add module-specific code to any of the framework parts of the Search API, it’s far more reasonable in the Views integration (or, say, Drush) – and for the Database Search module, which contains 0% framework, it’s even less of a problem.

Ok, I've reviewed and manually tested the patch. it works like a charm.

I've researched the possibility of using the Decorator pattern to be able to move this code to the facets module, but that also requires quite a lot of code to work. Since I think it's important to deliver features and we can always refactor this later I'll mark this RTBC.

Patch in #36 works fine for me as well.

Alright, thanks a lot!
Committed.
Thanks again, everyone!

Moving back to D7 for backporting.

I had an issue with this patch as we are using the exposed form in a separate block. This highlighted to me inconsistencies between the two approaches (exposed form in view / in a block) so in the event of using the exposed form in a block the form does not get updated by the Ajax, and therefore the hidden fields are never added.

I have submitted a patch for review on the views module to add consistency to the two approaches (which would in turn allow this patch to work in both scenarios.

Following on from my previous comment; due to the fact that this patch is directly accessing the query variable on the view object, rather than using the getQuery method it doesn't work for exposing a form in a block.

By changing this the patch then works in both scenarios.

Not sure if this should be on a separate ticket, as the patch is not yet released.

The patch should only contain the diff as the previous patch is already committed.

The code looks good. Do we need to manually test since it's passing tests and it's only that one line? I'm going to RTBC in case not.

Rerolled #16 to work with Search API multi-index searches

@ andy_w: Ah, thanks a lot for that! Committed.

@ nbaosullivan: Patch doesn’t apply, please re-roll against latest HEAD.

8.x-1.x not working with Facet Pretty Paths — \Drupal::request()->query->get($filter_key, []) is empty.
Fulltext search filter exposed in block.

Setting #action to NULL as a quick hack works

    if ( 'facets_pretty_paths' == $facet_source->getUrlProcessorName()) {
      $form['#action'] = NULL;
    }

Here's just a fixed patch from https://www.drupal.org/project/search_api/issues/2378945#comment-12976026

Thanks for sorting that @minorOffense!

@scoff: given this is already committed for the module, please open a new issue for this new problem with this other contrib module. Thanks.

This still needs a query option, same as in D8.

Patch from #60 worked for me, so I added the config option under views query settings.

Thanks a lot for moving this along! Always a bit sad if an issue just lies around with a perfectly working patch just because no-one wants to add the config option (which is usually far less work than the rest, too).

Also looks very good already, just a few small remarks:

1. +++ b/contrib/search_api_facetapi/search_api_facetapi.module
   @@ -506,7 +506,6 @@ function search_api_facetapi_search_api_admin_index_fields_submit($form, &$form_
    }
   -
    /**
   

   Probably deleted by accident?

2. +++ b/contrib/search_api_facetapi/search_api_facetapi.module
   @@ -517,19 +516,24 @@ function search_api_facetapi_search_api_admin_index_fields_submit($form, &$form_
   +    $preserve_facets = !empty($form_state['view']->display_handler->default_display->options['query']['options']['preserve_facet_query_args'])
   +      && module_exists('facetapi');
   

   This looks like it would always use the default display’s config, making it impossible to handle this differently for different displays? (A niche use case, to be sure, but as it’s possible to configure this, just ignoring it could lead to a UX WTF.)

3. +++ b/contrib/search_api_views/includes/query.inc
   @@ -197,6 +197,9 @@ class SearchApiViewsQuery extends views_plugin_query {
   +      'preserve_facet_query_args' => array(
   +        'default' => FALSE,
   +      ),
   

   I think this also needs a 'bool' => TRUE?

4. +++ b/contrib/search_api_views/includes/query.inc
   @@ -243,6 +246,20 @@ class SearchApiViewsQuery extends views_plugin_query {
   +    }
   

   Is this even necessary, if we have a default for the setting?

4. Is this even necessary, if we have a default for the setting?

Which part?

Oops, sorry!

+++ b/contrib/search_api_views/includes/query.inc
@@ -243,6 +246,20 @@ class SearchApiViewsQuery extends views_plugin_query {
+    else {
+      $form['preserve_facet_query_args'] = array(
+        '#type' => 'value',
+        '#value' => FALSE,
+      );
+    }

This.

1. yes, that was an accident. Fixed.
2. Good point. This should make it work for different displays
3 & 4. Updated

I also updated the README.

Thanks so much! I agree! This is a useful patch and someone else already did the hard part.

Thanks a lot for the further improvements, great work! Getting very close now …
I tried making the code a bit more readable, the nested ifs in search_api_facetapi_form_views_exposed_form_alter() were becoming a bit unwieldy in my opinion. (Not quite sure whether the substr() trick with 'search_api_multi' is really an improvement over that, but it did shorten the whole thing a lot and I explained it in the comment.)

Regarding _search_api_preserve_views_facets():

• Facet API is a dependency of the Search Facets module, so no need for checking for that there.
• There is actually views_plugin_display::get_option() to get the effective (overridden or default) version of an option, which both makes the code more readable and more robust. Also, we need to use !empty again to avoid a notice for existing views.
• Also, comment lines should never exceed 80 characters in length – but since we don’t need to mention the Facet API check anymore, that was easy to fix.

Thank you! Works for me and looks much better now!

Awesome, good to hear. Thanks for reporting back!
Committed.
Thanks a lot again, everyone! (And sorry if I forgot someone in the commit message!)

It wasn't apparent to me until I looked at the patch that this is functionality that needs to be enabled by configuring the view. For anyone else that is confused: If you have a view based on a search index, you can enable this option by editing the view > Query settings > and checking "Preserve facets while using filters".

Thank you for this, it works great! I'm using search_api and search_api_solr for what it's worth.

@maskedjellybean
Great observation man! It helped a lot.
Thanks

Hi guys,
Congrats on the great work
I have this issue but the other way around, so not sure I have to post it here, my apologies in advance if this is not the right place.

I have a views page, with exposed text filter (exposed form in block) + multiple facets
When you type a keyword and submit, the results are updated of course, but not the facets (counting, excluding empty items...)

This happens only when Ajax is on (everything works great with Ajax turned off)

Did I miss something?

Thank you in advance for your time!

Maybe it will help someone. The exposed form wasn't updated by ajax, so when you apply the facet filter and after that want to search by keyword, facets are not taken into account in query.
This patch helped https://www.drupal.org/project/drupal/issues/3032353#comment-13032497

I have the same issue in Drupal 8. I applied the patch in #36, but no use.

Any other solution for this?

I have found a working solution on my Drupal 8:
https://www.drupal.org/project/drupal/issues/2992672#comment-14165890

+1 for #78. This is the patch for Drupal 9:
https://www.drupal.org/files/issues/2021-07-19/3032353-22.patch

For what it's worth I have Search API, Views, BEF, and Facets working nicely together without any patches in Drupal 9.2.7. I can share what I did with some specific important configuration notes.

1. Create search index
2. Create block view of search index with exposed filter "Search: Fulltext search (or)"
3. (Optional) Create two exposed sorts in this order: "Search: Relevance" (desc), "Content datasource: Title" (asc)
4. (Optional) Exposed form style: Better Exposed Filters > Include reset button, enable auto-submit, allow people to choose sort order, combine sort order with sort by
5. Under Advanced > Use AJAX: No
6. Under Advanced > Query settings > Enable Preserve facets while using filters
7. Under Advanced > Caching > None
8. Create Facets and place them in blocks
9. Create a layout and add the view block and facet blocks

#82 works for me.

Thank you @chrisck for #82. You've saved my day!