Search API views argument handler for all indexed taxonomy term fields.

@orakili,

You totally made my day! Thanks for this awesome patch! It has been a while since you created it, so I re-rolled it against the latest 1.x-dev. As for filtering taxonomy terms, it does that great. However, I tried to override the view title based on the taxonomy term ID that was present, and that didn't really work as I expected. Instead of the taxonomy term, I got "Search indexed taxonomy term fields for "75"".

So... I'm marking this as "needs work", but I think it's actually very close. Thanks again!

Well, I obviously don't know how to roll a patch. Trying again...

This patch includes all the changes (the previous patch only included the new file).

Updated the patch to print out a sensible title when using %1 as the title.

This feature saved my life. Not sure why you never updated it to needs review after #6, but it should have been (based on the reasoning why you didnt mark it needs review in #4) so

Patch in #6++ RTBC!

Great work.

Does this also work if the term is in an related field and this is also not the same entity type? For example i tried this patch with nodes and commerce products. If i try to load a term from the commerce product i get the error 400; Status: Bad Request from solr. The search api index is for nodes and related to commerce products by reference field.

@checker: search api has the limitation that each entity type requires a separate index. There are solutions for showing results (and facets) from multiple indexes in one single page, but that is a totally separate matter. Check out search_api_multi and search_api_multi_index_facets.

Thanks a lot for proposing this feature and also providing a patch (or, rather, even several revisions of it)!
However, I've found a few shortcomings, which I've adressed in the attached patch. Please test and review!

The problem checker mentions in #8 is one of them: it stems from you always assuming the Search API field identifier is also the field's Field API key – which it isn't for fields on related entities.

I also cached the term fields and vocabulary data in the Views definition, which should save time in the Views execution (and gives us the possibility to easily add a "Search these fields" option later), used term fields which don't specify a vocabulary (possibly because they aren't Field API fields) for all terms and remove the phrase from the title() method (cf. #1959088: Fix titles for contextual filters – which you brought me to finally create and fix, thanks).

Patch #10 looks good. Fix my problems in #8. This is a killer feature!

Please set the issue status to "reviewed & tested by the community" when you successfully test a patch. That way, I won't miss it.

Anyways, now I see that this has been successfully tested, I guess it's good to go.
Committed.

Thanks for your work, everyone!

damaged routine, when i add the field.
suddenly started to work
as soon as you activate the "override title feature" its not working.

Please elaborate on what you do and what “its not working” means.

the view does not load anything anymore. the data...its just an empty page here but may be caused by other stuff. consider that this may only happen here.

Since you also have other trouble with your search views, I suggest you first investigate yourself on your site and then report back if you find any underlying problems which aren't just incompatibilities or wrong configurations. (Or maybe report those, too, if they aren't documented well enough yet.)

this seems to be in the latest 1.6 and its working there.

OK, great.

Are there any plans to port this feature to Drupal 8? As per the instructions here: https://www.drupal.org/project/search_api/issues/2387007, I am changing the version to 8.x-1.x-dev, but I am unable to change the status of this issue to "Need port".

Posting my initial attempt at this patch.

Entity reference fields can be shared between bundles of an entity type, and each instance of the field can be configured to reference different vocabularies. I added some code to go through each bundle and make sure we grab every vocabulary that is referenced between bundles for that field.

This still needs some work:

• each bundle that references the same vocabulary gets a duplicate entry in the $vocabulary_fields. Doesnt appear to hurt the functionality AFAIK...but shouldnt be left like that. Maybe check if in_array before adding?
• I have not tested for entity reference fields that don't limit what vocabulary is selected

I also noticed in the D8 version some of the logic was separated into this function

// Add special fields.
      _search_api_views_data_special_fields($table);

Since we need to be able to look at the indexed fields, I left the functionality within search_api_views_data(). The only way I see we could move it to the new function is to load indexes again, or to pass the indexes as a parameter.

Thanks a lot for your work, looks pretty good already! (Also, my apologies for the long delay!)
I just cleaned up the code a bit and added better handling for some error conditions. (For instance, if an invalid taxonomy term ID is passed, we don’t want to leave the results unfiltered, but instead return no results.)
Also, I think handling of multiple taxonomy terms was broken before – should now be fixed.

Please see, test and review the attached patch and tell me what you think!

In theory, we should also add tests before committing this. However, it seems we currently don’t have any tests at all for our taxonomy-related Views plugins, so I created #3023142: Add tests for the taxonomy Views plugins as a follow-up instead.

The patch in #24 causes a fatal error: Error: Call to a member function getFields() on null in _search_api_views_data_special_fields() (line 485 of modules/contrib/search_api/search_api.views.inc). The reason is the $index variable is not available in that function as mentioned in the last bit of my comment #22.

Other than that, the code looks good to me.

Please use the dev version of the module as the base for patching. The parameter was added just recently.

After updating to the dev version, this patch works for me. Thanks!

This patch doesn't contain any test coverage, which I think should be added for this new functionality. Besides that I've found the following issues with the patch itself.

1. +++ b/search_api.views.inc
   @@ -498,7 +498,76 @@ function _search_api_views_data_special_fields(array &$table, IndexInterface $in
   +    if (!isset($settings['target_type'])
   +        || $settings['target_type'] !== 'taxonomy_term') {
   +      continue;
   +    }
   

   Splitting this into

   if (!isset(..)) {
     continue;
   }
   if ($settings['target_type']...) {
     continue;
   }
   

   would improve the readability of the code in my opinion (Personal preference, not a must change)

2. +++ b/search_api.views.inc
   @@ -498,7 +498,76 @@ function _search_api_views_data_special_fields(array &$table, IndexInterface $in
   +    $entity_type = $datasource->getEntityTypeId();
   

   This variable should actually be named $entity_type_id because $entity_type could be interpreted as an entity type object instead of just an identifier.

3. +++ b/search_api.views.inc
   @@ -498,7 +498,76 @@ function _search_api_views_data_special_fields(array &$table, IndexInterface $in
   +  if ($vocabulary_fields) {
   

   if ($vocabulary_fields !== []) { would make this much more expressive.

4. +++ b/src/Plugin/views/argument/SearchApiAllTerms.php
   @@ -0,0 +1,92 @@
   + * Note: The plugin annotation below is not misspelled. Due to dependency
   + * problems, the plugin is not defined here but in
   + * search_api_views_plugins_argument_alter().
   

   Idea, might not be feasible: Wouldn't it be possible to actually use the correct annotation here and remove the plugin in search_api_views_plugins_argument_alter() if the taxonomy module is not enabled? That would remove all knowledge of dependency handling from the plugin itself.

5. +++ b/src/Plugin/views/argument/SearchApiAllTerms.php
   @@ -0,0 +1,92 @@
   +    $vocabulary_fields += ['' => []];
   

   This line feels like magic to me and could probably do with a comment.

#28.3 I agree with this, but @drunken monkey doesn't mind this being type-juggled. :)

I'm not sure if .4 will work, I believe we have tried this before, but I can't seem to find the correct issue, do you remember this @drunken monkey?

28.3: Me and my team try to be as type-safe as possible to reduce the likelihood of bugs caused by type-juggling. That said, in this case it's just a matter of preference really because at the moment this wouldn't cause any issues :)

28.4: I think it would be worth investigating this, but want to propose to do so in a follow up issue to prevent scope creep.

Thanks a lot for the thorough review! I really could get used to this …

This patch doesn't contain any test coverage, which I think should be added for this new functionality.

See my reasoning in #23.

This variable should actually be named $entity_type_id because $entity_type could be interpreted as an entity type object instead of just an identifier.

I dislike long variable names (they also make things less readable), and context (and/or the IDE) will usually make it clear that this is just the string ID, not the object. We also use $entity_type like this in numerous other places in this module (source: git grep '\$entity_type =').
But I guess you’re right, it’s just 3 characters and could really add a bit of clarity.

Idea, might not be feasible: Wouldn't it be possible to actually use the correct annotation here and remove the plugin in search_api_views_plugins_argument_alter() if the taxonomy module is not enabled? That would remove all knowledge of dependency handling from the plugin itself.

As Joris said, this would indeed make sense, but unfortunately doesn’t work. When discovering all available plugins, the plugin manager will already load class files, which leads to a fatal error if Taxonomy-dependent plugins are reached and the Taxonomy module is not enabled. Since the alter hook is only invoked after this discovery (which makes sense, of course), it’s useless in this case.
See #2777483: Unmet dependencies for details.

I was actually pretty glad I finally thought of the current solution (#2917399-10: Missing / broken handler when adding a filter for a field) – before that, you had to install an additional sub-module just for the taxonomy term Views plugins.

#28.3 I agree with this, but @drunken monkey doesn't mind this being type-juggled. :)

You know me so well! <3

Looks good to me :)

Great to hear, thanks again for reviewing!
Committed.
Thanks again, everyone!

(Ah, shoot, realized too late that some of the patches here were for D7, so now credits in the commit are a bit off. Sorry, ericchew, should have been your main credit, of course!)