Specialized filter for users and terms

Thanks a lot for the suggestion and the patch! Having a dedicated filter for users makes sense, of course.
However, there were several errors in your patch regarding use cases other than the one on d.o – for instance, non-exposed filters and exposed operators. Also, the code style and comments didn't completely follow the coding standards.
A revised patch is attached, please test/review!

While working on this, I also noticed an unrelated bug in our base filter handler: #2111273: Fix Javascript states for exposed filter operator. (Probably won't concern the d.o use case, though.)

Ah, you're right, thanks!
However, I guess it'd be even better to map the UIDs to the names in the summary. I also noticed that that conversion didn't take place for exposed filters with defaults in the live preview. It's really tiring how many cases you have to accomodate in Views …
Anyways, improved patch attached.

Thanks a lot for creating this new patch with a common base class! As you say, this also adds additional value for other contrib modules, and it definitely makes the code cleaner.

Actually, I would have thought even more code could be generalized – e.g., if you take the ID and label column from the entity_keys entity info setting, you could likely make most of the label <=> ID conversion code common. Just the vocabulary for terms and the anonymous user would have to be added, but the base class could already take that into account. (Also, users don't have the label set as an entity key, but rather use a label callback, so that would also have to be added manually somewhere. Maybe the two columns could be retrieved in init(), making overriding easy. Or we could use a dedicated method, of course.)

Speaking of anonymous users, though: Why does the code just use the literal string "Anonymous" instead of the anonymous variable with a translated default? Isn't that very confusing for sites in other languages, or who override the name? Or am I missing some behind-the-scenes magic?

So a bunch of code in SearchApiViewsHandlerFilterTaxonomyTerm can likely still be removed, which would be great.

I don't know, how would you mean we could do that? Do you mean removing the option to have a select list from the filter handler? That could of course make sense, yes …
Though, as said, providing this functionality in the base class, and thus also offering it for users (which could make sense on sites with a small set of possible authors), could also be a nice feature. And even for tagging term fields, a select list could make sense if the site is small.
Also, the code in the options filter handler doesn't allow you to use a select list without hierarchy, and also doesn't support autocomplete. So I'd say using this new handler for all taxonomy term references would probably be the better option.
The only "feature" the options handler has compared to the new one is caching of the options list, which could be a very small performance gain. (But by the way, how many of the issue fields are term references? If several are, a performance comparison between the two options might make sense – we are searching for performance data, and d.o would be perfect for that.)

Further comments/nitpicking:

1. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +    $options['limit'] = array('default' => TRUE, 'bool' => TRUE);
   

   I don't see any config form for "limit". How is that set, and what does it do?

2. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +    if ($this->options['limit']) {
   +      // We only do this when the form is displayed.
   +      if (empty($this->definition['vocabulary'])) {
   +        $first_vocabulary = reset($vocabularies);
   +        $this->definition['vocabulary'] = $first_vocabulary->machine_name;
   +      }
   +
   +      if (empty($this->definition['vocabulary'])) {
   +        $form['vocabulary'] = array(
   +          '#type' => 'radios',
   +          '#title' => t('Vocabulary'),
   +          '#options' => $options,
   +          '#description' => t('Select which vocabulary to show terms for in the regular options.'),
   +          '#default_value' => $this->definition['vocabulary'],
   +        );
   +      }
   +    }
   

   I don't understand this bit at all. Apart from the fact that I don't understand the "limit" option – doesn't the first if block mean that the second one will only be executed if there are no vocabularies defined – making the whole element useless?

3. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +      '#dependency' => array('radio:options[type]' => array('select')),
   

   As far as I know, and according to #1595022: Convert dependency to states, #dependency shouldn't be used anymore in favor of #states (though I admit the syntax for that is more complicated).

4. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +    if ($missing && !empty($this->options['error_message'])) {
   +      form_error($form, format_plural(count($missing), 'Unable to find term: @terms', 'Unable to find terms: @terms', array('@terms' => implode(', ', array_keys($missing)))));
   +    }
   +    elseif ($missing && empty($this->options['error_message'])) {
   +      $tids = array(0);
   +    }
   +
   

   A nested if would make the code considerably more readable here.

5. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +    elseif ($missing && empty($this->options['error_message'])) {
   +      $tids = array(0);
   +    }
   

   Is that really the best behavior? Shouldn't we just add 0, instead of replacing all found TIDs with just that one?
   If the operator is "=", this will show an empty result – which is probably exactly the expected behavior. However, for "<>", all other entered terms will be ignored, too!

6. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +    $form['error_message'] = array(
   +      '#type' => 'checkbox',
   +      '#title' => t('Display error message'),
   +      '#default_value' => !empty($this->options['error_message']),
   +    );
   

   This should have some description.

7. +++ b/contrib/search_api_views/includes/handler_filter_taxonomy_term.inc
   @@ -0,0 +1,310 @@
   +      $form['value'] = array(
   +        '#type' => 'select',
   +        '#title' => $this->options['limit'] ? t('Select terms from vocabulary @voc', array('@voc' => $vocabulary->name)) : t('Select terms'),
   +        '#multiple' => TRUE,
   +        '#options' => $options,
   +        '#size' => min(9, count($options)),
   +        '#default_value' => $default_value,
   +      );
   

   Completely setting $form['value'] here overwrites/removes general improvements like the #states for the "empty" operators and removing the title in the exposed form (though Views admittedly does that automatically anyways).

   Also, the taxonomy term filter should also (like the user filter) only use ids_to_strings() to set the default value if it actually used – i.e., not if the exposed form has already been submitted. It's probably only a minor performance improvement, but still better than nothing.

All of the comments I knew how to fix are contained in the attached patch, along with some documentation fixes/beautification. (I guess we don't really need interdiffs, you'll probably also use a dedicated branch, right? Otherwise, I can of course continue providing them.)

Great, thanks!
What I noticed, though, is that you're using reduce_value_options() in the term filter, where it isn't available. I've now added it there, it's pretty trivial anyways.

Do you still want to work on the two mentioned issues (making ID <=> label conversions more generic and either removing or generalizing the "select list" option), or would you prefer we just ensure everything works now and commit it?
Especially the former would be hard to do later, though, so we'd have to be sure we don't want that.

I don't see a huge benefit to making ID <=> label conversions more generic. Maybe it will emerge as search_api_views adds more entity types, but that's a separate issue.

I guess you're right. I thought that it would be harder to implement, then, as contrib modules might rely on the generic class already – but since the method is abstract now, all subclasses must implement it from scratch anyways. If we later on add a generic implementation there, subclasses won't notice it unless they choose to use it.
So, nothing against leaving it like this for now.

As for the select/autocomplete option, I don't really know either. Maybe someone who's using this will weigh in. Otherwise, I'll just commit one of them next Monday or so and hope for the best. ;)

In any case, thanks a lot again for your work!

Any chance you could commit this earlier? Today or tomorrow? This issue is kind of blocking other our before-launch to-dos.

Damn, I'm sorry, I didn't check the issues on the weekend. Please send me a mail in such situations – for d.o I'm more than willing to make an exception to the "no nagging about issues via mail" rule.
Anyways, I committed the patch in #9 now – having the select list available, too, might be useful for some users, and it gives us the option to always use this filter handler instead of the options list-based one for taxonomy terms.

Thanks again for your work, Neil, and sorry I blocked you guys!

No worries, and thanks a lot for all your help with this issue!

I have some problems with this change with an exposed filter of type "All parent terms (indexed)". After updating from search api 1.4 to the latest recommended release, the filter suddenly stopped to work.

Debugging the code in SearchApiViewsHandlerFilterTaxonomyTerm::value_form() shows that the base for loading the terms, $this->definition['vocabulary'], is empty. That worked well before i updated. Seems like this new term filter is buggy.

Oh, damn, I can't believe I overlooked that … Thanks a lot for reporting this!

Does the attached patch fix this for you? It attempts to retrieve the field information of the containing entity for the parent and parents_all properties. If that's not available, no vocabulary is saved for the filter and the functionality gets reduced accordingly (only a plain select list available). Later, we could also implement our own taxonomy term callback for that case (i.e., use all vocabularies) – but that's not necessary right away, I think. We'd have to see if having a term reference without a taxonomy defined (or defined some way we can't detect it) is really a use case.

In any case, thanks again for reporting, and sorry I committed this so hastily, breaking your site!

Can confirm the patch in #18 fixes the problem with the empty $definition['vocabulary']. Unfortunately there is still something different from the behavior of the corresponding filter in search api 7.x-1.4.

I'm using the filter as exposed and the form should also work when there is no value for that filter submitted. This was working fine in 1.4. But In the current version there is something wrong in SearchApiViewsHandlerFilterTaxonomyTerm::validate_entity_strings() now. I just tried to make a few changes quickly to see if i could make it work and came up with this:

  protected function validate_entity_strings(array &$form, array $values) {
    if (empty($values)) {
      return array();
    }

    $tids = array();
    $names = array();
    $missing = array();
    foreach ($values as $value) {
      $missing[strtolower($value)] = TRUE;
      $names[] = $value;
    }

    if (!$names) {
      return FALSE;
    }

    $query = db_select('taxonomy_term_data', 'td');
    $query->innerJoin('taxonomy_vocabulary', 'tv', 'td.vid = tv.vid');
    $query->fields('td');
    $query->condition('td.tid', $names);
    if (!empty($this->definition['vocabulary'])) {
      $query->condition('tv.machine_name', $this->definition['vocabulary']);
    }
    $query->addTag('term_access');
    $result = $query->execute();
    foreach ($result as $term) {
      unset($missing[strtolower($term->tid)]);
      $tids[] = $term->tid;
    }

    if ($missing) {
      if (!empty($this->options['error_message'])) {
        form_error($form, format_plural(count($missing), 'Unable to find term: @terms', 'Unable to find terms: @terms', array('@terms' => implode(', ', array_keys($missing)))));
      }
      else {
        // Add a bogus TID which will show an empty result for a positive filter
        // and be ignored for an excluding one.
        $tids[] = 0;
      }
    }

    return $tids;
  }

That way it is working as expected again. Sorry i don't have time to make a correct patch. This is for sure not the correct solution but it might let you see what is going on actually. The main problem is that the current code in this method expects $values to be an array of term names. What i get is an array of tids though.

Would be great if you could have a look at this again.

Good to know the patch works – I now committed it to fix at least that part.

For the new problem: could you please describe in more detail what you're doing, what settings you're using and what goes wrong?

Ok so here is what i am trying to do. I have a "All parent terms (indexed)" filter that is exposed on a view that is items of a certain search index. The field is part of that index and enabled as well in the fields of the index.

Due to exposure of the filter i need it to accept empty values and the empty behavior should be to return all results for the view. Unfortunately what happens is that no results are shown, the autocomplete field is marked red and the message "Unable to find term:" gets displayed.

It is caused by the code in SearchApiViewsHandlerFilterTaxonomyTerm::validate_entity_strings() . That function expects the $values argument to be an array of term names. But instead what i get there is an array of tids which of course will cause that function to return nothing. Please let me know if you need any more details.

Sorry, I can't reproduce this. Everything seems to work fine. Maybe try to get to the bottom of why validate_entity_strings() receives IDs instead of names, which really shouldn't happen. However, I don't see why this should even be relevant to the problem you describe – if you have issues with empty form input, then the array passed to validate_entity_strings() will be empty anyways.

One problem I noticed, however (even though I've made sure this is solved before committing) is that the default value of the "Value" field in the admin UI will again not always be properly set – i.e., IDs instead of names.
Views never ceases to shock me – it seems $form_state['input'] is sometimes even populated for new forms! I really haven't got any clue left on how to properly determine when the default value will be used, so the attached patch would change this to have the IDs be always transformed to names. The performance improvement was probably negligible anyways, since setting a default value for an exposed filter isn't often done anyways, I'd guess.

Ah, now I stumbled across it by accident. Would have been important to know you have "Allow multiple selections" disabled.
The attached patch should fix this, please test!

Thanks for your effort drunken monkey. I just checked your patch and can confirm that it solved the problem with the empty value of the field. Unfortunately when i try to select a term using autocompletion, the view doesn't display any results at all now.

It just displays the message "Unable to find term: 123". And yes, i have "Allow multiple selections" disabled on that filter.

Hope it helps

Good to hear that part works, thanks for testing!
I cannot reproduce the new error you mention, though. It's with the same setup and field as before, and the filter exposed? With the patch, everything really seems to work now for me.
Did you maybe enter a term from another vocabulary, or something?

I came across this issue when trying to solve an issue that was introduced after updating to 7.x-1.9 and was wondering if you could advise if this issue may be related to the issue as i've detailed in https://drupal.org/node/2141741

thanks!

Have you tried with the latest dev version of the Search API, too?

Yes I am running 7.x-1.9+33-dev

That doesn't seem to be related to this issue at all. I'll comment over in #2141741: Use case with taxonomy/term/% issue with search api 7.x-1.9and facet api.

@ s_leu: Could you reproduce your problem with a newly configured view? If so, please provide details and reproducible steps. Otherwise, we should finally close this issue.

Here is my observation for term fields:

1- "Term reference" fields:
- handled by "SearchApiViewsHandlerFilterOptions".
- terms are provided as checkboxes in filter configuration.
- no autocomplete option.

2- "Entity Reference"(Taxonomy term) fields:
- handled by "SearchApiViewsHandlerFilterTaxonomyTerm".
- terms (all terms in the site!) are provided in a multiple-select in filter configuration.
- no autocomplete option.

Attached patch:
- makes "Term reference" fields to be handled by "SearchApiViewsHandlerFilterTaxonomyTerm"
- correctly prepopulates "vocabulary" option for "Entity Reference" so extra options(autocomplete,etc.) work as expected.

ps: Not sure if there is an open issue on it, but i think providing a vocabulary selection UI would be nice besides/instead of prepopulating it in hook_views_data().

Thanks for the patch, the suggestion makes sense.
The decision to use the options filter for non-autocomplete term reference fields was a deliberate one, so I'd first need to know whether there is larger consensus that these should be handled by the term-specific filter instead.
Also, if this is the case, we still should just move the if for taxonomy terms up in the chain (probably along with users, for consistency), instead of moving the one for options down. Otherwise, options also become less preferable than, e.g., dates.

We could also consider a different approach, of just adding autocomplete to the options filter as well – should be well possible and could also be handy in other situations. However, it would of course be more work, so I understand if no-one wants to do that. (We'd probably have to abstract our entity filter handler again to also handle other autocomplete-able values.)

+++ b/contrib/search_api_views/search_api_views.views.inc
@@ -221,9 +216,20 @@ function _search_api_views_add_handlers($id, array $field, EntityMetadataWrapper
+        $table[$id]['filter']['vocabulary'] = key($field_info['settings']['handler_settings']['target_bundles']);

Will the 'target_bundles' key always only contain a single value? Otherwise, we should only do this when there is only a single value. Or change $table[$id]['filter']['vocabulary'] to an array, but that would of course be a larger change.

Also, should this maybe also be fixed for the search_api_views_taxonomy_term contextual filter for "Entity reference" fields? It seems that would have the same problem.

Attached is also a re-roll of #25 – however, part of this already seems to have been fixed by #2198791: Indexed Content: Author as Exposed Filter: empty field treated as zero (Anonymous), so I'd first need to know if anyone still has problems with this in the latest version of this module?

Will the 'target_bundles' key always only contain a single value? Otherwise, we should only do this when there is only a single value. Or change $table[$id]['filter']['vocabulary'] to an array, but that would of course be a larger change.

Target bundles may contain multiple values. It's unlikely but possible. Limiting to single valued target_bundles would cover most of the sites i guess. There is also another scenario where the handler(term source) is the views module, which may be considered later together with the multiple bundles.

Also, should this maybe also be fixed for the search_api_views_taxonomy_term contextual filter for "Entity reference" fields? It seems that would have the same problem.

Yes, the same vocabulary discovery logic should be added there too.

OK, then please add these two fixes, too.

- Moved the term-if up keeping the options-if in its place as you suggested.

- Moved the vocabulary discovery code to a function.

- Fixed the two issues.

Fixed interdiff.

Thanks, looks good. Just had to fix the comments a bit.
However, as said, I don't want to change the handler type of non-autocomplete term reference fields without enough consensus on the issue, as this will probably break at least some, if not a lot of, views containing such filters. Does the rest of the fix make sense without that? Or will term-typed entity reference fields then still not be recognized? In the former case, I could commit the attached patch – for the if-reordering I'd need some more feedback here.

This works for Entity reference fields.

Term reference fields however needs the if-reordering. I understand your concerns about breaking existing configurations but in my opinion if there were many people using the term filter as options list some of them would have already come here and request dropdown and autocomplete widgets because that's the way a term field works in normal views. I think we should fix this. The sooner the better.

I understand your concerns about breaking existing configurations but in my opinion if there were many people using the term filter as options list some of them would have already come here and request dropdown and autocomplete widgets because that's the way a term field works in normal views.

On the other hand, you are the first to complain about it the other way.
I have committed the patch from #39 now. To avoid overloading this issue even more, please create a new one for the re-ordering issue, and we can have a "vote" there.