Problem/Motivation

The fields property of the $query is updated every time there is a value in $this->configuration['fields']. And there always is, even when no boxes have been checked in the configuration form:

   fields:
     rendered_item: '0'
     tag_names: '0'
     title_1: '0'

This is a problem later on when determining full text search fields for autocomplete suggestion queries, as \Drupal\search_api\Backend\BackendPluginBase::getQueryFulltextFields() performs an array_intersect() on $query->getFullTextFields() against $query->getIndex()->getFulltextFields() if the former returns values. If the query does not return values, the index' full text fields are returned, which is the purpose of leaving it empty, as illustrated by the config field description (emphasis mine):

Select the fields which should be searched for matches when looking for autocompletion suggestions. Leave blank to use the same fields as the underlying search.

Proposed resolution

Add array_filter() calls in appropriate places to make sure the 'empty' values don't end up as fields in the $query object.

Remaining tasks

Review patch provided in my next comment.

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kekkis created an issue. See original summary.

kekkis’s picture

kekkis’s picture

Just realised that in addition to the implementation, test coverage might be nice. I will have to look into that. Or if anyone else has time, I believe the IS is pretty clear on what to test.

drunken monkey’s picture

You're right, this currently isn't working correctly. Thanks a lot for reporting!
However, the correct fix is to properly sanitize the config value before saving it, not sanitizing it when it is needed. I thought we're already doing that, but it seems our code for this was buggy. The attached patch should fix that – re-save the search after applying it and things should work fine.

kekkis’s picture

I tested your patch and the configuration was upated compared to the situation earlier, as expected:

-  fields:
-    rendered_item: '0'
-    tag_names: '0'
-    title_1: '0'
+  fields: {  }

I'm hesitant setting this to RTBC as, while this seems the correct fix, it seems to me that \Drupal\Tests\search_api_autocomplete\Kernel\SearchCrudTest::testCreate could use some updating with this. It seems there are no assertions even.

  • drunken monkey committed 6738421 on 8.x-1.x authored by kekkis
    Issue #2882969 by kekkis, drunken monkey: Fixed sanitization of plugin...
drunken monkey’s picture

Status: Needs review » Fixed

That would be the wrong test to put this in, but our test coverage is generally pretty poor at the moment. Let's fix that over in #2889437: Provide proper test coverage, instead of piecemeal.
So, since you say it's otherwise working: committed.
Thanks again!

ressa’s picture

I just tested the latest dev-version of Search API Autocomplete together with Search API, and can confirm that it works, good work!

ressa’s picture

With the very recent first official release of Search API Solr Search 8.x-1.0, and Search API getting ever more stable at 8.x-1.2, I am really looking to getting this improvement released in the next version of Search API Autocomplete, to complete this trio of Search API modules. Is a future release planned?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.