Just seen in a @todo comment: instead of having the "search ID" be just a query option (with, still, a lot of special handling various code), it would make more sense to just have it be a separate property, with getter and setter.

However, the question is whether we need this at all, now that we have display plugins? Instead of setting the search ID, why not just set the display plugin ID? Downside: no display plugin, no results cache. But would/could at least advocate the providing of display plugins by contrib modules.
(Still, though, there could be use cases for having searches cached that aren't displayed anywhere. So, maybe encourage the use of display plugin IDs as search IDs, but don't require it? And provide a method for retrieving a query's display plugin, if one matches?)

Input welcome!

CommentFileSizeAuthor
#6 2772829-6--search_id_overhaul.patch8.33 KBdrunken monkey
#3 2772829-3--search_id_overhaul.patch8.59 KBdrunken monkey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey created an issue. See original summary.

borisson_’s picture

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

I'd love to encourage contrib to work with display plugins instead, that makes sense.

(Still, though, there could be use cases for having searches cached that aren't displayed anywhere. So, maybe encourage the use of display plugin IDs as search IDs, but don't require it? And provide a method for retrieving a query's display plugin, if one matches?)

Yes, that sounds sensible. I don't see a lot of usecases to have searches cached that are not displayed tbh.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Active » Needs review
FileSize
2772829-3--search_id_overhaul.patch8.59 KB

Here is a straight-forward implementation of my suggestion, along with a short test.

Status: Needs review » Needs work

The last submitted patch, 3: 2772829-3--search_id_overhaul.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs work » Needs review
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
FileSize
2772829-6--search_id_overhaul.patch8.33 KB

Re-roll.

borisson_’s picture

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

This is a good change and I'd like to see this get in, however - if you don't mind. I'd like to get facet's tests green again first. This will probably require some refactoring in facets as well.

I'd also like to get feedback from people who maintain other contrib modules, so let's try to ping some people to have a look at this?

swentel’s picture

swentel CreditAttribution: swentel commented

Hmm, so by the looks of this patch, the only thing I'd have to change in search api page is basically removing the 'search id' line, that works for me :) Let me know if I'm interpreting this wrong.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented

I don't think that this patch changes anything for search_api_solr. So feel free to finalize your optimization.

drunken monkey’s picture

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

OK, thanks a lot for your input!
Then I guess I'll just wait for Joris to give his final OK after getting the tests green again.

  • drunken monkey committed 11c8f00 on 8.x-1.x 
    Issue #2772829 by drunken monkey: Made "search id" a query property...
drunken monkey’s picture

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

Got Joris' OK via IRC, so: committed.
Thanks again for reviewing, everyone!

Status: Fixed » Closed (fixed)

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