Search API Autocomplete expects that a backend implements some functions. But since search_api_autocomplete is not a required dependency of any backend, there's no interface. I suggest to create that Interface within Search API to have better DX.

CommentFileSizeAuthor
#7 3238635-5--move_autocomplete_interface--search_api_autocomplete--step_2.patch6.04 KBdrunken monkey
#7 3238635-5--move_autocomplete_interface--search_api_autocomplete--step_1.patch2.67 KBdrunken monkey
#3 3238635-3--move_autocomplete_interface--search_api_autocomplete--step_2.patch3.57 KBdrunken monkey
#3 3238635-3--move_autocomplete_interface--search_api_autocomplete--step_1.patch1.21 KBdrunken monkey
#3 3238635-3--move_autocomplete_interface--search_api.patch4.93 KBdrunken monkey

Comments

mkalkbrenner created an issue. See original summary.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Title: Add an AutocompleteInterface » Move AutocompleteBackendInterface to Search API
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
StatusFileSize
new 3238635-3--move_autocomplete_interface--search_api.patch4.93 KB
new 3238635-3--move_autocomplete_interface--search_api_autocomplete--step_1.patch1.21 KB
new 3238635-3--move_autocomplete_interface--search_api_autocomplete--step_2.patch3.57 KB

Makes sense, thanks for the suggestion!
I normally try to keep the Search API module as free of other modules’ code as possible, but Autocomplete is popular enough to surely warrant this small addition (which also comes with more or less zero maintenance burden).

Attached are three patches: the one for Search API and step 1 for Autocomplete could be committed right away; step 2 for Autocomplete will have to wait until we can rely on a Search API version that includes the new interface.

Also, now that I think of it, we probably don’t want to remove that interface completely, but just deprecate it for now. (Would then also need a change notice – but that should probably wait until we move the issue to the Autocomplete project. Or should we create a separate issue there anyways?)

Finally, I took this chance to also add type hints to the interface’s sole method. Would you say that’s OK? After all, no-one will yet have implemented that interface, that would need to be an explicit change on the part of any module – and when they do that, they can also change the method signature.

(Side note: I ran the automated tests manually with all three patches applied, and they all passed, so shouldn’t be any hidden pitfalls.)

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Needs review » Reviewed & tested by the community

Thanks for working on this one.
I agree that the old interface could be deprecated first and be removed later. Together with the removal, composer.json should declare incompatibilities with older versions of Search API and Search API Solr.

  • drunken monkey committed 597366b on 8.x-1.x 
    Issue #3238635 by drunken monkey, mkalkbrenner: Added the...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Project: Search API » Search API Autocomplete
Status: Reviewed & tested by the community » Needs review

Thanks for reviewing!
OK then, committed the Search API patch and moving to Search API Autocomplete.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 3238635-5--move_autocomplete_interface--search_api_autocomplete--step_1.patch2.67 KB
new 3238635-5--move_autocomplete_interface--search_api_autocomplete--step_2.patch6.04 KB

Here are the updated patches. I expect we want to deprecate the interface in Search API Autocomplete right away and then remove it in step 2? (Not sure anymore, though. Also not sure when we should switch the doc comments to point to the new type – all a bit tricky in this case.)

In any case, I also already created a change record for Autocomplete. Might need to be adapted in case the deprecation won’t go in before the 1.6 release (planned for this week).

  • drunken monkey committed 69f1e32 on 8.x-1.x 
    Issue #3238635 by drunken monkey, mkalkbrenner: Deprecated...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: +Search API 1.22 dependency

Committed the first step. Keeping at “Needs review” for the second step – to be committed once we (want to) add a dependency on Search API 1.22 (to be released soon).

Regarding the composer.json incompatibility with older versions of Search API Solr: Is that really necessary? It seems the current version doesn’t use the interface, except in comments, so should be no harm there.
Otherwise: What should the conflict constraint be, <4.3?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

Regarding the composer.json incompatibility with older versions of Search API Solr: Is that really necessary?

I can't find it in composer.json. Can you provide a link?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

It isn’t there yet, sorry for the confusion. I was referring to your comment #4:

Together with the removal, composer.json should declare incompatibilities with older versions of Search API and Search API Solr.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

I the Interface has the same name and the backends leverage a use statement we can indeed avoid the declaration of an incompatbility.