Search API Autocomplete for Drupal 8

It's definitely planned, yes, but might still take a while. The first priority is to get a stable release of Search API itself, then I'll probably tackle the add-on modules according to their usage numbers. Or according to funding, if someone is interested enough in a port.

I created a contrib tracker issue so the status can be more easily tracked via the project page: #2700691: [search_api_autocomplete] Search API Autocomplete

I'm looking into porting the module now: https://github.com/dawehner/search_api_autocomplete

Quick startup report:

* You can create autocomplete configurations using the UI, both for views and page searches
* Autocompletion is provided for both the views and page searches
* A lot of things could be cleaned up still, of course
* The autocompletion of views act a bit weird, it seems to show too many results. Should be investigates

This is certainly in a state where testing/feedback would be highly appreciated.

Great! I'd be willing to test this out. Could you create a branch on d.o for 8.x?

Here is a patch relative to 7.x-1.x, but of course, I cannot create a branch against 8.x-1.x, because I'm not a maintainer or so.

Awesome work so far, thanks a lot!
I haven't tried it yet, but I've added the branch to the d.o project (along with one commit from me cleaning up a lot of small stuff) and done a review. Here is what I found and would still like to be fixed before creating an initial Alpha release:

• Though not strictly necessary, I try to create interfaces for all classes that are handed around as parameters, to allow proper type hinting. I see you already have one for the two plugin types, but please also add one for the entity class.
• We're generally trying to get rid of "magic arrays" as much as possible, so it would be great if you could change suggestions to use a small "bean" class for suggestions (maybe even a few helper methods would make sense on it). With, see above, an interface.
• There is a "display" plugin in the Search API which lists all known searches, of any type (supporting this plugin). So maybe you could get rid of AutocompleteTypeInterface::listSearches() in favor of that?
  I guess determining the autocomplete type plugin for those displays would then be a nissue, though. We actually did some thinking regarding letting display plugins support features, too, but that would again move the code away from this module and to the Search API and Pages modules – not what we want, I think. (Especially since the Search API maintainer is pretty strict about adding third-party integration code to the framework.)
  I would solve the problem of having to pick one display for all searches based on a view, by just creating one search config entity per display, not per view. (You can do that in any case, though – and might be a good idea, too. See #2711635: support multiple view displays for autocomplete.)
• What's weird in any case is that AutocompleteTypeInterface::listSearches() isn't static.
  However, I think using derivers to create one plugin per view (or display) and per page would be cleaner in any case.
• It's also a weird concept that there can at most be a single search entity per search type. I don't think that's a pattern used anywhere else. But I don't really see any way around that, this is almost certainly still the correct architecture. So, no criticism, just an observation. (And it was the same in D7, too, of course.)
• I'm pretty strict about coding standards in my module, especially documentation standards, and unfortunately those were ignored in a lot of the new code. I know it's a drag, but it really helps in the long term (and adding them later on is even more of a drag), so please add descriptions to all classes, methods, parameters, etc.
• I think the usual structure for new plugin types is something like the following (at least that's what I've been using in the Search API):
  

  - Annotation/
    - SearchApiAutocompleteSuggester.php
  - Plugin/
    - search_api_autocomplete/
      - suggester/
        - Server.php
  - Suggester/
    - SuggesterInterface.php
    - SuggesterManager.php
    - SuggesterPluginBase.php
  

  I'd find it helpful, because easier to navigate, if this module kept to the same structure. Or am I mistaken and the majority of Core uses a different standard?
  In any case, I think it's a good idea to add @see tags referencing the other three framework classes of the plugin type to each of the class description (so, not to the Server plugin in this example, but to all other classes).
  Using Suggester as the common prefix, not AutocompleteSuggester would be my personal preference here, but I'm not really against using the longer version either.

• The delay config setting, and maybe even scripts, too, should just become settings on the individual search entities, not hidden config values. But that can happen in a separate issue just as well.
• It seems most, or even all, of the Javascript doesn't really apply anymore. E.g., neither Drupal.jsAC nor Drupal.ACDB seem to exist anymore in Core, so overriding them will either not do anything or even produce errors.
• Is there any reason ConfigurablePluginInterface is not part of AutocompleteSuggesterInterface?
• Why is the entity ID "search_api_autocomplete_settings", not "…_search"?
• The forms, plugins and (if possible, not sure) the controller, as well as all other classes where it is possible, should use dependency injection to eliminate explicit usage of \Drupal:: or function calls wherever possible.
• Since it seems only small changes were necessary, I've already updated REAMDE.txt. However, it seems search_api_autocomplete.api.php still needs a complete overhaul.

If you'd be up to continuing work on this, you can either still continue in your Github branch or start working with patches here – your choice.
In any case, thanks a lot again!

@drunken monkey
Thank you for that great feedback. I'll try to create issues for the individual bits. It seems to be that you created a branch, but without pushing anything. I assume this was on purpose?

Some continues work on a couple of the excellent review points.

Is there any reason ConfigurablePluginInterface is not part of AutocompleteSuggesterInterface?

Sure, there is no reason, let's quickly fix that.

It seems most, or even all, of the Javascript doesn't really apply anymore. E.g., neither Drupal.jsAC nor Drupal.ACDB seem to exist anymore in Core, so overriding them will either not do anything or even produce errors.

The check at the beginning of the file: if (typeof Drupal.jsAC != 'undefined') { ensures we don't have any fatal. This file needs a complete rewrite as Drupal core now uses Jquery UI autocompletion instead of its own autocompletion, which is great!

Here is some more work, renaming a couple of classes/interfaces.

Thanks a lot for your continued work on this!

It seems to be that you created a branch, but without pushing anything. I assume this was on purpose?

I did, I pushed everything from your Github branch. Was there more in the patch?
In any case, it seems you're now not using the Github project anymore – but your patches also don't apply on the current dev version. Could you please either provide one or more patches against the current branch contents (d.o or Github – they're identical), or keep using your Github branch?

I therefore also couldn't review your latest changes, but I'll look at them later.

Ooops, cross-posted with myself.

I did, I pushed everything from your Github branch. Was there more in the patch?

Oh damnit, I pulled from my remote by accident and by that I haven't seen your changes. No worries, I'll create a interdiff now and continue to work on the github branch.

The delay config setting, and maybe even scripts, too, should just become settings on the individual search entities, not hidden config values. But that can happen in a separate issue just as well.

Resolved that.

I also moved some of the functionality out of the entity object, because, IMHO, those entity objects shouldn't contain too much logic, if possible.

On top of that I tried to have a look at the autocompletion JS changes. Its not obvious how we could alter the autocomplete object initialized by the core autocompletion object. Maybe we need to ship with our own JS file.

Ooops, cross-posted with myself.

Haha, that's kinda nice!

Committed those last two patches, thanks once again!
Are you now finished with this, at least as far as you're concerned, or is there still work to do? Since the changes are massive again, I think just reviewing the whole module once more when you're done (and when I have the time) makes more sense. (I'll also create an Alpha release, once you say it's usable.)

One thing I did already notice, though, is that you're using short array syntax. Which is great, and fine by me, but please also change the last few places where the old syntax is used (easy to find with git grep 'array(') to keep this consistent within the module. (In the Search API, we regrettable went with keeping this consistently the old syntax. :-/)

Thank you for your commit!

Are you now finished with this, at least as far as you're concerned, or is there still work to do? Since the changes are massive again, I think just reviewing the whole module once more when you're done (and when I have the time) makes more sense. (I'll also create an Alpha release, once you say it's usable.)

I have a bit of a time problem at the moment, so I'm not sure when I'll be next be able to look into it, but honestly, I haven't done manual testing since milano, so potentially some of the refactoring caused issues.

One thing I did already notice, though, is that you're using short array syntax. Which is great, and fine by me, but please also change the last few places where the old syntax is used (easy to find with git grep 'array(') to keep this consistent within the module. (In the Search API, we regrettable went with keeping this consistently the old syntax. :-/)

Fair point :)

I hope I'll find time in the time again to push it a bit more forward.
In case you have more review points, please point them here. The more they are, the more likely I will just work on a block of them :)

I pushed a couple of more commits today, see https://github.com/dawehner/search_api_autocomplete/commits/8.x-1.x

1. +++ b/search_api_autocomplete.api.php
   @@ -194,15 +194,15 @@ function hook_search_api_autocomplete_search_delete(\Drupal\search_api_autocompl
   + * @return array
   

   This should be SearchApiAutocompleteSearch[], I guess?

2. +++ b/src/Suggestion.php
   @@ -248,4 +248,26 @@ class Suggestion implements SuggestionInterface {
   +
   +
   

   Should be only one blank line.

2 small suggestions, but the rest looks great.

Woo!

Can we get #27 in?

Thanks a lot! Committed.

However, does hook_default_search_api_autocomplete_search() even still work? That was a hook provided by the Entity API to define default entities. With D8's config framework, I don't think it exists anymore.

Same thing for hook_search_api_autocomplete_types() and all the example_*() functions referenced there – now that autocomplete types are plugins, none of that applies anymore, right?

You can remove the entity hooks, too – they're all already documented as hook_ENTITY_TYPE_*() by Core.

So, really, I think only these three hooks need to be in there (as far as I can see):
- hook_search_api_autocomplete_suggestions_alter()
- hook_search_api_autocomplete_suggester_alter()
- hook_search_api_autocomplete_type_alter()

+    // @todo Get cacheability metadata from search_api and use
+    //   \Drupal\Core\Cache\CacheableJsonResponse instead.

We always assume no cacheability of any search data, and I don't think that will change (at least in the near future). So just set max-age to 0.

+ // @todo Get cacheability metadata from search_api and use
+ // \Drupal\Core\Cache\CacheableJsonResponse instead.
We always assume no cacheability of any search data, and I don't think that will change (at least in the near future). So just set max-age to 0.

You can use UncacheableDependencyTrait to make that easy to implement.

Thank you for committing the current progress.

However, does hook_default_search_api_autocomplete_search() even still work? That was a hook provided by the Entity API to define default entities. With D8's config framework, I don't think it exists anymore.

Same thing for hook_search_api_autocomplete_types() and all the example_*() functions referenced there – now that autocomplete types are plugins, none of that applies anymore, right?

You can remove the entity hooks, too – they're all already documented as hook_ENTITY_TYPE_*() by Core.

OH yeah, as written before, this documentation is totally outdated and doesn't work at all.

So, really, I think only these three hooks need to be in there (as far as I can see):
- hook_search_api_autocomplete_suggestions_alter()
- hook_search_api_autocomplete_suggester_alter()
- hook_search_api_autocomplete_type_alter()

That's a good list, thank you!

Should we close this issue and continue on in issues per todo from here?

Should we close this issue and continue on in issues per todo from here?

I'd totally say +1 for that.

I added #2865821: Add, test and document alter hooks, not sure if there are other todo's we can open already.

I've added #2877645: Overhaul "type" plugin system and #2877648: Review/port our Javascript overrides as further follow-ups.

In other news, I'm now starting work on this, so will probably/hopefully soon post a patch with a first overhaul of the module. I'd say we can close this issue after that and continue in separate issues for further tasks.

Here is a general overhaul of the module regarding code style, class names/structure, etc.
Would be great to get some tests or reviews. I fear the upgrade won't be completely painless, though, since some of the entity properties have changed. (The changes in service IDs and class names, on the other hand, should be resolved with a simple cache clear.) But that can't be helped, and must be expected of a module in Alpha phase.

Ah, I also forgot to actually change the current code usingoptions to use suggester_settings and type_settings instead, as appropriate.

This should fix both the test failure and the plugin configurations (for the most part).

Third time's the charm?

1. +++ b/config/schema/search_api_autocomplete.schema.yml
   @@ -1,17 +1,66 @@
   +        results:
   +          type: boolean
   +          label: 'Whether to show a predicted result count for suggestions'
   

   Not sure if 'results' is the best here, is 'show_count' better? I think it more clearly describes what the option does.

2. +++ b/search_api_autocomplete.module
   @@ -46,3 +47,75 @@ function search_api_autocomplete_entity_operation(EntityInterface $entity) {
   +  $search = Search::load($search_id);
   +  $controller = new AutocompleteController(\Drupal::service('renderer'));
   +  if (!$search || !$controller->access($search, \Drupal::currentUser())
   +      ->isAllowed()
   +  ) {
   +    return;
   +  }
   

   The formatting here looks kinds of wonky, can we change this?

   I think we'd benefit from doing something like:

   $search = ...
   // There's no searcher for this plugin id, so no need to continue.
   if (!$search) {
     return;
   }
   
   $controller = ...
   
   // The current user isn't allowed to autocomplete.
   if (!$controller->access->isAllowed()) {
     return;
   }
   
   
   

3. +++ b/src/Type/TypePluginBase.php
   @@ -0,0 +1,26 @@
   +/**
   + *
   + */
   +abstract class TypePluginBase extends PluginBase implements TypeInterface {
   

   Needs, at least a one-line description.

The patch is really big, so a comprehensive review is not so easy to do, but these are the things that stood out. Overall this looks good though.

Thanks for the review, and your great suggestions! Should all be implemented in the attached patch.

And yes, I know it's not easy to review. I'd just hoped I'd get a few people to just test whether it breaks anything for them, then commit and hope to come across all parts of the code again in separate issues in the future so we can spot any remaining issues, or bad decisions during this initial overhaul.
In other words: It doesn't have to be perfect right away, and it's OK if not even all of the changes here make sense. (I'd just like to avoid breaking anything that already works.)

And one more thing …

I tested the page has got the following error, renaming the type-hint to Search made everything work as expected.

TypeError: Argument 2 passed to Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend::getAutocompleteSuggestions() must be an instance of Drupal\search_api_autocomplete\Entity\SearchApiAutocompleteSearch, instance of Drupal\search_api_autocomplete\Entity\Search given, called in /vagrant/app/modules/contrib/search_api_autocomplete/src/Plugin/search_api_autocomplete/suggester/Server.php on line 85 in Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->getAutocompleteSuggestions() (line 1925 of /vagrant/app/modules/contrib/search_api_solr/src/Plugin/search_api/backend/SearchApiSolrBackend.php).

Thanks a lot for testing, good to hear!
For adapting the Solr module accordingly, see the appropriate patch in #38, or #2878601: Adapt to changes in Autocomplete module.

Anyways, as this seems to work well enough, I've now committed this so work can go on in the other issues. If we broke or missed something, we can always fix it later.
Thanks again to everyone here – especially to dawehner, of course!