As discussed in #2772745-19: Search API integration doesn't check/define feature support of backends, search displays should be able to define their dependencies. We can either do this by adding the DependentPluginInterface right to the search display interface, or by just implementing the interface with those plugin classes that we define. The former would probably be better as it would motivate developers to also add proper dependency information for their plugins. (Also, in the plugin base class, we could already add the index by default – I guess in any case, even if not adding the interface right to ours.)

One problem with the Views search displays is that I guess there's no way to depend on an actual Views display, which we'd actually need – we can just add the view itself as a dependency, and live with this maybe causing trouble sometimes. (Unless someone more clever than me comes up with a solution.)

CommentFileSizeAuthor
#9 2831436-9--display_plugin_dependencies.patch5.96 KBdrunken monkey
#9 2831436-9--display_plugin_dependencies--interdiff.txt991 bytesdrunken monkey
#8 add_dependency-2831436-8.patch6.25 KBborisson_
#8 interdiff.txt1.29 KBborisson_
#6 2831436-6--display_plugin_dependencies.patch4.68 KBdrunken monkey
#6 2831436-6--display_plugin_dependencies--interdiff.txt4.68 KBdrunken monkey
#4 add_dependency-2831436-4.patch1.61 KBborisson_
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_ as a volunteer and commented

We can either do this by adding the DependentPluginInterface right to the search display interface, or by just implementing the interface with those plugin classes that we define.

I'd say we go for the first option. More strictness is a good idea there I think.

One problem with the Views search displays is that I guess there's no way to depend on an actual Views display, which we'd actually need – we can just add the view itself as a dependency, and live with this maybe causing trouble sometimes. (Unless someone more clever than me comes up with a solution.)

I've broken my head over that a few times already. I have no idea how we can do that, so I guess for now we should just add a dependency on the view.

borisson_’s picture

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

I'll have a look at this issue over the weekend, this is something I'd love to see included in the next release. We should probably ping someone with more views experience about a way to do dependency on a views-display.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Active » Needs review
FileSize
add_dependency-2831436-4.patch1.61 KB

This should get a test as well, and I haven't tested it. Started on this too close to heading out for beers.

Hopefully I can find some time to work on this on sunday.

Status: Needs review » Needs work

The last submitted patch, 4: add_dependency-2831436-4.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue tags: +Needs tests
FileSize
2831436-6--display_plugin_dependencies--interdiff.txt4.68 KB
2831436-6--display_plugin_dependencies.patch4.68 KB

Thanks for starting on this!
The base class shouldn't really have the code to deal with a view dependency, though.
And while asking a Views expert can't hurt, I really don't think it's possible at this point. (At least without introducing something like fake display config entities.)
Here is my suggestion.

You're right, though, it still needs tests in any case. But thanks again for the work so far!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs work » Needs review

I like that, let's see how the bot feels about the patch in #6

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
FileSize
interdiff.txt1.29 KB
add_dependency-2831436-8.patch6.25 KB
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
2831436-9--display_plugin_dependencies--interdiff.txt991 bytes
2831436-9--display_plugin_dependencies.patch5.96 KB

Looks good, thanks!
If the bot agrees with this slightly changed version, I guess I can commit it.

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 » Fixed

Committed. Thanks again!

Status: Fixed » Reviewed & tested by the community
borisson_’s picture

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

hah, cool. I didn't know about assertContains! Thanks Thomas!

borisson_’s picture

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

Back to fixed per #10.

drunken monkey’s picture

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

hah, cool. I didn't know about assertContains! Thanks Thomas!

I thought as much, yeah. It's a bit confusing, since it doesn't have Array in the name and can also be used to check for substring matches. (And there's no second, dedicated method for either of them.)

Status: Fixed » Closed (fixed)

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