As part of porting search_api_autocomplete we need to adapt \Drupal\search_api_db\Plugin\search_api\backend\Database::getAutocompleteSuggestions().

CommentFileSizeAuthor
#18 2747283-18.patch1.16 KBdawehner
#10 2747283-4.patch831 bytesdawehner
#2 2747283-2.patch1.25 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs review
FileSize
2747283-2.patch1.25 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2747283-2.patch, failed testing.

The last submitted patch, 2: 2747283-2.patch, failed testing.

borisson_’s picture

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

We can't implement a search api autocomplete interface in search api, does the interface need to be moved to search api?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs work » Postponed (maintainer needs more info)

What are you talking about?
Are you working on porting the Autocomplete module? Where is the issue? And what's the purpose of this issue?
Also, as Joris says, we can't just use some other module's interfaces without depending on it (and we should surely not depend on it).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Are you working on porting the Autocomplete module? Where is the issue? And what's the purpose of this issue?

Sorry for my quite non verbose issue :) But yes, i'm working on porting the search_api_autocomplete module, see https://www.drupal.org/node/2682949#comment-11282727

For that I just posted my current local hack. I guess the right way is to keep the function in here, but just don't add the interface, much like D7 worked?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Title: Adapt \Drupal\search_api_db\Plugin\search_api\backend\Database::getAutocompleteSuggestions as part of porting search_api_autocomplete » Adapt the DB backend's Autocomplete integration to the module's D8 version
Component: General code » Database backend
Status: Postponed (maintainer needs more info) » Active

For that I just posted my current local hack. I guess the right way is to keep the function in here, but just don't add the interface, much like D7 worked?

Yes, unfortunately I don't think there is any other option.
Otherwise we wouldn't need supportsFeature() either, but could just use interfaces.

But good to hear you're working on a port! Then I guess we can just leave this open until you have the necessary patch for the backend.

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
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs review
FileSize
2747283-4.patch831 bytes

Sorry again for my stupid first patch. This portion though is something we should still have.

borisson_’s picture

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

@dawehner, are you sure that that patch does something?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

@borisson_
Yes! Open up your IDE without the patch and browse to \Drupal\search_api_db\Plugin\search_api\backend\Database::getAutocompleteSuggestions. You'll see two unknown classes. This patch fixes that.

borisson_’s picture

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

Oh, okay! In that case, let's get this in.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I agree its funny how much this patch solves.

drunken monkey’s picture

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

Why does it need the interface imported, when we don't use it?
Also, you should create an interface for the entity, since type hinting should be done on interfaces wherever possible. (I'd also rename the class(es) to not include the module name as prefix, since those aren't needed anymore with namespaces. But I admit that the coding standards discussion is still ongoing.)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I'd also rename the class(es) to not include the module name as prefix,

I like this suggestion!

Also, you should create an interface for the entity, since type hinting should be done on interfaces wherever possible.

Well that itself is also a bit in discussion at the moment, but yeah its easy to add an interface ...

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Postponed

OK, then let's see what changes will be made to the autocomplete module yet and then apply them here. (Or, as discussed, maybe the implementation will get moved to the autocomplete module anyways and we just have to remove the code here.)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2747283-18.patch1.16 KB

Here is a small adaption.

borisson_’s picture

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

These are very small changes that are needed, so let's commit both patches. #10 + #18.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2747283-18.patch, failed testing.

borisson_’s picture

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

Oh, this should be rtbc, because the patch fails are not related to this issue, we have the same fails in #2777483: Unmet dependencies.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented

See https://www.drupal.org/node/2784849#comment-11533091

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs work » Active

These are very small changes that are needed, so let's commit both patches. #10 + #18.

OK, fine with me, too.
Committed.
Thanks, dawehner!

Should I leave this issue open, or will we open a new one in case further changes are needed?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Thank you for the commit! We are getting somewhere.

Should I leave this issue open, or will we open a new one in case further changes are needed?

I personally don't care. Do you have a preference?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Active » Fixed

I personally don't care. Do you have a preference?

Ah, let's just mark it as "fixed".

Status: Fixed » Closed (fixed)

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