Overhaul the "type" plugins in the following ways:

  • Remove the TypeInterface::listSearches() method and instead just create one derivative type per search. (There would then be just at most one search entity per type plugin – and we wouldn't have to map search IDs to the types, making them potentially much shorter and "nicer".)
  • Allow form alter hooks to pass GET parameters to the autocomplete URL. Get rid of the hard-coded "fields" path element in the process.

Old proposal:

In D8, the Search API now provides a "search display" plugin type so modules can define the searches they provide. E.g., each (suppported) Views display of a search view gets its own display plugin, as does each search page provided by search_api_page.

We should definitely build on that somehow for the list of available searches we provide. For that, we should somehow link our own "type" plugins (which might need to be renamed) and the Search API "display" plugins. The question is: how?

Ideally, Search API display plugins would just implement an additional interface that tells us they support autocomplete. However, since most modules that provide searches won't (want to) depend on the Search API Autocomplete module, that isn't really possible. And display plugins also don't provide a supportsFeature() method like backends, which might also have been a very clean solution.

So, I have the following actual suggestions/options:

  1. Automatically match displays to autocomplete types if they have the same plugin ID. So, we'd just need derivers creating one plugin for each (supported) Views display and search page and they would automatically be connected to the search display plugin with the same ID.
    Downside: Would need to (at least partially) duplicate the deriver code from the displays, so little gain there.
  2. On the type plugin, have a matchDisplays() method (name up for discussion, of course) that gets a list of all available search displays and returns the ones it should be used for.
    Downside: Having type and display separately would need two properties on the config entity – or we'd have to do the matching each time we want to use autocomplete (though it should be fairly quick anyways).
  3. Do not use a plugin type for our "search types" at all, but instead use a hook or event to link search displays to "type controllers".
    Downside: Not using a plugin type seems "wrong", or at least "un-D8-y". Otherwise, this seems pretty reasonable, though.
  4. Add something like supportsFeature() to display plugins, too, in the Search API. Shouldn't be difficult to do and the Search API maintainer seems amenable to the idea.
    Downside: This would either move the autocomplete code for the displays to the modules defining the displays (so we couldn't just support views and pages by default, or easily adapt them to changes in the framework), or need some fairly complex feature support system to enable us to make other modules' display plugins support a feature from our own module.

Any opinions, or additional suggestions?
As you see, unfortunately, all my suggestions come with fairly large downsides, there is none jumping out as "ideal" right away.

In any case, to stay consistent with Search API displays, I'd provide one autocomplete search for each of them (so, also one for each Views display, not just one for each view). To make this less inconvenient to set up we could, e.g., provide a "copy settings" option that copies one display's settings to another.

CommentFileSizeAuthor
#9 2877645-9--overhaul_type_plugins.patch62.48 KBdrunken monkey
#9 2877645-9--overhaul_type_plugins--interdiff.txt5.13 KBdrunken monkey
#6 2877645-6--overhaul_type_plugins.patch60.7 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue summary: View changes

New suggestion:

Do not use a plugin type for our "search types" at all, but instead use a hook or event to link search displays to "type controllers".
Downside: Not using a plugin type seems "wrong", or at least "un-D8-y". Otherwise, this seems pretty reasonable, though.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue tags: +beta blocker
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for DBC commented
Title: How to link "search types" and Search API display plugins » Overhaul "type" plugin system
Assigned: Unassigned » drunken monkey
Issue summary: View changes

Hm, thinking about this some more, I actually don't really see a benefit in linking types to displays. The Search API "display" plugins help us spot searches at query time – however, in this module, we actually need to identify them when their form is displayed. The display plugins are no help there.
And then, we need to create a query for the type: again, something the display plugin can't help us with (though that's potentially a short-coming of them – e.g., Facets could have needed something like this, too).

Still, analogous to display plugins, I think we shouldn't have a TypeInterface::listSearches() method, but instead just create one derivative type per search. (Contrary to Search API display plugins, I wouldn't create one derivative per Views display, though, just one per view – we can then just add a "Enable for these displays" option to the Views type config.)

Finally, I've noticed that we could very easily enable the form alter code to pass arbitrary additional data to the autocomplete route (and, thus, the createQuery()) via GET parameters. We should use that to clean up some of the uglier code: pass the actual Views display (and maybe even the effective contextual filters?), get rid of the hard-coded "fields" route parameter, etc.

Overall, I like this proposal much better than any of my previous suggestions (while I still like the idea of a features system for displays, this would be a bit out of scope at this point). So, I'll just start with that, unless someone objects. (And, apparently, no-one has any better ideas to offer anyways?)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for DBC commented

With one plugin per search, though, the label "type" for the plugin type doesn't really make sense anymore, though. It's more like "display", or "search" – both confusing due to name conflicts, though. Or maybe "form", since that's what we're actually looking for? After all, there doesn't actually need to be a search, we just need a form, really. "search_form"?
Does "search" even make sense for the entity type, thinking of it, or would it be better to rename that, too? Maybe just "settings" or "config"? Or AutocompleteSettings/AutocompleteConfig for the class name, to not be that confusing to read in code?

PS: See #2897538: Naming things.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for DBC commented
Status: Active » Needs review
FileSize
2877645-6--overhaul_type_plugins.patch60.7 KB

OK, can't get the integration test to pass atm, but otherwise this patch is more or less done. Good enough to review, definitely, at least.
One of those patches where you'll have to re-create all your autocomplete settings, though, I'm afraid. Better first delete all of them, then apply the patch and clear the cache.

Anyways, feedback welcome! Also, whether you think my basic approach here makes sense.

Status: Needs review » Needs work

The last submitted patch, 6: 2877645-6--overhaul_type_plugins.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented

Only a few very basic remarks, this looks great!

  1. +++ b/src/Entity/Search.php
@@ -15,7 +15,15 @@
+ *   label_collection = @Translation("Autocomplete searches"),
+ *   label_singular = @Translation("autocomplete search"),

    Is there a reason why the collection label is the only one with a starting upper case? 

  2. +++ b/src/Form/IndexOverviewForm.php
@@ -285,6 +217,62 @@ public function buildForm(array $form, FormStateInterface $form_state, IndexInte
+    $suggesters = array_map(function ($suggester_info) {
+      return $suggester_info['class'];
+    }, $this->suggesterManager->getDefinitions());
+    $filter_suggesters = function ($suggester_class) use ($search) {
+      return $suggester_class::supportsSearch($search);
+    };
+    $available_suggesters = array_filter($suggesters, $filter_suggesters);

    I don't really understand why only one of these uses a named function, and the other one uses a lambda. I guess it doesn't really matter either way. Great use of the array_ functions here though! 

  3. +++ b/src/Form/IndexOverviewForm.php
@@ -285,6 +217,62 @@ public function buildForm(array $form, FormStateInterface $form_state, IndexInte
+    if ($available_suggesters) {

    I think it'd be better to revert this if.

    It's very readable now, but if we get more code in here, the early return should make the code easier to follow.

    if (count($available_suggesters) === 0) {
return $search;
}

... // rest of code.

  4. +++ b/src/Plugin/search_api_autocomplete/type/Views.php
@@ -67,59 +78,50 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      throw new SearchApiAutocompleteException("Could not create search query for view '$views_label'.");

    Not sure if this is the best message in the exception here, it looks like the error is because the view is not based on a search api index? Should we mention that to make debugging easier?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for DBC commented
Status: Needs work » Needs review
FileSize
2877645-9--overhaul_type_plugins--interdiff.txt5.13 KB
2877645-9--overhaul_type_plugins.patch62.48 KB

Thanks a lot for your review!

I also managed to fix the test fails – see the attached patch.

Is there a reason why the collection label is the only one with a starting upper case?

Yes: this is how Core does it. (See, e.g., Node.) I expect it just depends on which context the labels are supposed to be used in.

I don't really understand why only one of these uses a named function, and the other one uses a lambda. I guess it doesn't really matter either way. Great use of the array_ functions here though!

Simple reason: line length. I could use a variable for the second function, too, though, for the sake of consistency.

I think it'd be better to revert this if.

It's very readable now, but if we get more code in here, the early return should make the code easier to follow.

I don't think it's likely there'll be more code there: if we have any suggesters available, we set one, otherwise we can't. Nothing else should depend on that. (Otherwise, we can always change it later.) And on the other hand, having just one return point in a method also makes the code cleaner. So, I don't think reversing this would be a good idea.

Not sure if this is the best message in the exception here, it looks like the error is because the view is not based on a search api index? Should we mention that to make debugging easier?

I had to change that part around again anyways, but also took the opportunity to improve the exception message in that case, thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Dazzle commented
Status: Needs review » Reviewed & tested by the community

Not sure if you're expecting other reviews as well, but for me this looks good. I only had small things to say earlier and you explained them all. With green tests now as well, this looks good to me.

Great work!

  • drunken monkey committed a063c2b on 8.x-1.x 
    Issue #2877645 by drunken monkey, borisson_: Overhauled the "type"...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for DBC commented
Status: Reviewed & tested by the community » Fixed

Good to hear, thanks a lot for reviewing! Committed.
At this stage, I think I'd rather commit things quickly and wait for people to complain. ;)

Status: Fixed » Closed (fixed)

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