Thanks for a great module! I am new to facets, so there is still a lot to explore, but I am loving it so far...
I have a "problem" and am not sure whether I am missing a setting or that this is expected behaviour, a bug or maybe a future feature request.
Situation
A View with an exposed search form with keyword search to search a Search API database index with facet filters.
Problem
When searching for a keyword after using a facet filter, the facet filter is ignored by Views, the Facet API's query parameters are not being sent to the server.
Can be tested at ie. the Commerce Kickstart demo.
Am I missing a setting, is this expected behaviour, a bug or maybe a future feature request?
Temporary fix
My quick fix is to add facet filters as hidden form fields to Views exposed forms as in:
/**
* Implements hook_form_FORM_ID_alter().
*/
function MYMODULE_form_views_exposed_form_alter(&$form, &$form_state) {
/**
* SITUATION: Search forms with facet filters and keywords search.
* PROBLEM: When searching for a keyword after using a facet filter, the
* facet filter is ignored by Views.
* SOLUTION: Add facet filters as hidden form fields to Views exposed forms.
*/
// Check if this form is a Search API form. This could be replaced with the
// value of a custom Views setting like ie. "Include facets".
if (isset($form_state['view']) && $form_state['view']->base_field === 'search_api_id') {
// Get quary parameters.
$query_parameters = drupal_get_query_parameters();
// 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 . ']'] = array(
'#type' => 'hidden',
'#value' => $value,
'#weight' => -1,
);
}
}
}
}
Temporary fix in Drupal 8
Check comments for another example too.
function go_utility_form_alter(&$form, FormStateInterface $form_state, $form_id) {
// Check if this form is a Search API form. This could be replaced with the
// value of a custom Views setting like ie. "Include facets".
$view_names = array('my_view_name');
$view = $form_state->getStorage('view');
if ($form_id == 'views_exposed_form' && in_array($view['view']->id(), $view_names)) {
// Reference: https://www.drupal.org/node/2378945
// SITUATION: Search API forms with facet filters and Views exposed form.
// PROBLEM: When using a Views exposed form after using a facet filter, the
// facet filter is ignored by Views.
// SOLUTION: Add facet filters as hidden form fields to Views exposed forms
// if any facet query parameters are provided.
$query = \Drupal::request()->query->all();
if (!empty($query['f'])) {
// Iterate through facet query parameters.
foreach ($query['f'] as $key => $value) {
// Add hidden form field for facet parameter.
$form['f[' . $key . ']'] = array(
'#type' => 'hidden',
'#value' => $value,
'#weight' => -1,
);
}
}
}
}
Thanks a lot in advance for your time and efforts!
Comment | File | Size | Author |
---|---|---|---|
#69 | 2378945-69--views_retain_facets.patch | 4.4 KB | drunken monkey |
| |||
#16 | 2378945-16--views_retain_facets.patch | 1.95 KB | drunken monkey |
|
Comments
Comment #1
cristiroma CreditAttribution: cristiroma commentedModified the if condition because you get
Notice: Undefined index: view in search_integration_form_alter()
when editing a facet.Comment #2
Colin @ PCMarket CreditAttribution: Colin @ PCMarket commentedi 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...
Comment #3
lmeurs CreditAttribution: lmeurs commented@Colin: As far as I know Views exposed filters do not work with facets (yet). I read somewhere that the maintainer does not have the intention to integrate this Views exposed filters with facets, but that the API offers enough possibilities for a third party to develop this kind of functionality.
The snippet from above just adds hidden form fields for each active facet to any Views exposed form to prevent active facets from being reset on a new text search.
Comment #4
DamienMcKennaThere'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 ;-)
Comment #5
DamienMcKennaLets move this to Search API as the patch is for Views exposed filters that are querying a search_api index.
Comment #6
DamienMcKennaHere's the code in patch format.
Comment #7
DamienMcKenna@drunken monkey: What sort of tests would you like for this?
Comment #8
JOINSO CreditAttribution: JOINSO commentedHi!
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
Comment #9
DamienMcKennaAfter 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.
Comment #10
JPHuxley CreditAttribution: JPHuxley at CTI Digital commentedUpdated 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.
Comment #11
ndf CreditAttribution: ndf at Dx Experts for Triquanta commentedThis 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:
Comment #12
Kristen PolComment #13
Kristen PolComment #14
Kristen PolComment #15
cmseasy CreditAttribution: cmseasy commentedPatch #10 worked for me.
Comment #16
drunken monkeySorry 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.)
Comment #17
timlie CreditAttribution: timlie as a volunteer commentedPatch in #16 works for me!
Thanks!
Comment #18
broonThanks 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.
Comment #19
lpeabody CreditAttribution: lpeabody commentedAdding a patch for D8, applies against Search API 8.x-1.6. It is working for me! Thanks.
Comment #20
drunken monkeyAs said, we'll definitely need an option for this before I'll think about committing it.
But thanks for your feedback!
Comment #21
herved CreditAttribution: herved commented#16 for D7 seems to work well on my side as well.
Thank you!
Comment #22
jnrfred CreditAttribution: jnrfred as a volunteer and at Acro Commerce commentedpatch in #19 for D8 works for me as well
Comment #23
froboy#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.
Comment #24
drunken monkeyThat’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.
Comment #25
jnrfred CreditAttribution: jnrfred as a volunteer and at Acro Commerce commentedI 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.
Comment #26
drunken monkeyI 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.
Comment #27
cmseasy CreditAttribution: cmseasy commentedpatch in #16 for D7 works for me
Comment #28
chr.fritschHere is an updated patch for D8
Comment #30
chr.fritschMake in_array check more strict
Comment #31
chr.fritschMinor adjustments because the in_array check was not working
Comment #32
chr.fritschHere is a patch to make it configurable
Comment #33
chr.fritschAdding the schema for the new config setting
Comment #34
drunken monkeyGreat 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.)
Comment #35
jnrfred CreditAttribution: jnrfred at Acro Commerce commentedPatch 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?Comment #36
bdlangton CreditAttribution: bdlangton commentedI 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'.
Comment #37
drunken monkeyAh, 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.
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.
Comment #38
legolasboHaving 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
in search_api.module:
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 becomefacets_form_views_exposed_form_alter
and search_api would no longer have to know anything about the existence of the facets moduleComment #39
borisson_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.
Comment #40
legolasboLet me clarify. I meant that we would not have to add any new knowledge of facets :)
Comment #41
drunken monkeyThanks 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.
Comment #42
drunken monkeyPS:
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.
Comment #43
legolasboOk, 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.
Comment #44
jnrfred CreditAttribution: jnrfred at Acro Commerce commentedPatch in #36 works fine for me as well.
Comment #46
drunken monkeyAlright, thanks a lot!
Committed.
Thanks again, everyone!
Moving back to D7 for backporting.
Comment #47
lmeurs CreditAttribution: lmeurs commentedThat's great news! Maybe I (as creator of this issue and providing an initial solution) will be credited for the D7 version... :-)
Comment #48
andy_w CreditAttribution: andy_w at Numiko commentedI 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.
Comment #49
andy_w CreditAttribution: andy_w at Numiko commentedFollowing 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.
Comment #50
andy_w CreditAttribution: andy_w at Numiko commentedNot sure if this should be on a separate ticket, as the patch is not yet released.
Comment #52
andy_w CreditAttribution: andy_w at Numiko commentedThe patch should only contain the diff as the previous patch is already committed.
Comment #53
Kristen PolThe 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.
Comment #54
nbaosullivan CreditAttribution: nbaosullivan commentedRerolled #16 to work with Search API multi-index searches
Comment #55
nbaosullivan CreditAttribution: nbaosullivan commentedComment #58
drunken monkey@ andy_w: Ah, thanks a lot for that! Committed.
@ nbaosullivan: Patch doesn’t apply, please re-roll against latest HEAD.
Comment #59
scoff CreditAttribution: scoff commented8.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
Comment #60
minorOffense CreditAttribution: minorOffense at Coldfront Labs Inc. commentedHere's just a fixed patch from https://www.drupal.org/project/search_api/issues/2378945#comment-12976026
Comment #61
nbaosullivan CreditAttribution: nbaosullivan commentedThanks for sorting that @minorOffense!
Comment #62
DamienMcKenna@scoff: given this is already committed for the module, please open a new issue for this new problem with this other contrib module. Thanks.
Comment #63
drunken monkeyThis still needs a query option, same as in D8.
Comment #64
capysara CreditAttribution: capysara commentedPatch from #60 worked for me, so I added the config option under views query settings.
Comment #65
drunken monkeyThanks 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:
Probably deleted by accident?
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.)
I think this also needs a
'bool' => TRUE
?Is this even necessary, if we have a default for the setting?
Comment #66
capysara CreditAttribution: capysara commentedWhich part?
Comment #67
drunken monkeyOops, sorry!
This.
Comment #68
capysara CreditAttribution: capysara commented1. 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.
Comment #69
drunken monkeyThanks a lot for the further improvements, great work! Getting very close now …
I tried making the code a bit more readable, the nested
if
s insearch_api_facetapi_form_views_exposed_form_alter()
were becoming a bit unwieldy in my opinion. (Not quite sure whether thesubstr()
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()
: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.Comment #70
drunken monkeyComment #71
capysara CreditAttribution: capysara commentedThank you! Works for me and looks much better now!
Comment #72
drunken monkeyAwesome, good to hear. Thanks for reporting back!
Committed.
Thanks a lot again, everyone! (And sorry if I forgot someone in the commit message!)
Comment #75
maskedjellybeanIt 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.
Comment #76
amd.miri@maskedjellybean
Great observation man! It helped a lot.
Thanks
Comment #77
Bill.B CreditAttribution: Bill.B commentedHi 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!
Comment #78
allaprishchepa CreditAttribution: allaprishchepa commentedMaybe 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
Comment #79
AswathyAjish CreditAttribution: AswathyAjish commentedI have the same issue in Drupal 8. I applied the patch in #36, but no use.
Any other solution for this?
Comment #80
mecmartini CreditAttribution: mecmartini as a volunteer and at Soulweb Solutions Ltd for European Commission and European Union Institutions, Agencies and Bodies commentedI have found a working solution on my Drupal 8:
https://www.drupal.org/project/drupal/issues/2992672#comment-14165890
Comment #81
john.oltman CreditAttribution: john.oltman commented+1 for #78. This is the patch for Drupal 9:
https://www.drupal.org/files/issues/2021-07-19/3032353-22.patch
Comment #82
chrisck CreditAttribution: chrisck commentedFor 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.
Comment #83
mizage@gmail.com CreditAttribution: mizage@gmail.com commented#82 works for me.
Comment #84
lmeurs CreditAttribution: lmeurs commentedGreat news that this issue has been fixed for D8 as well! But rather unfortunate that I (as creator of this issue and providing the initial solution) again am not credited... ;-(
Comment #85
SKAUGHTComment #86
vlad.dancerThank you @chrisck for #82. You've saved my day!