It seems the RenderedItem processor currently doesn't properly declare the view modes it uses as dependencies, and I guess it should?

CommentFileSizeAuthor
#5 2573997-5--rendered_item_processor_dependencies.patch1.49 KBdrunken monkey
#5 2573997-5--rendered_item_processor_dependencies--interdiff.txt1.47 KBdrunken monkey
#4 interdiff--2574633.txt2.93 KBborisson_
#4 fix_dependencies_of_the-2573997-4.patch9.72 KBborisson_
#2 fix_dependencies_of_the-2573997-2.patch9.52 KBborisson_
#2 fix_dependencies_of_the-2573997-2.txt3.17 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 commented
Status: Active » Needs review
FileSize
fix_dependencies_of_the-2573997-2.txt3.17 KB
fix_dependencies_of_the-2573997-2.patch9.52 KB

Attached is a patch that should implement this; it doesn't work and the test is based on the patch in #2574633: Properly react when a plugin's dependencies are removed.

The attached .txt file is an interdiff to that other issue.

Status: Needs review » Needs work

The last submitted patch, 2: fix_dependencies_of_the-2573997-2.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
fix_dependencies_of_the-2573997-4.patch9.72 KB
interdiff--2574633.txt2.93 KB

Still based on #2574633: Properly react when a plugin's dependencies are removed.

interdiff is towards that issue, no inderdiff to the previous patch though.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
FileSize
2573997-5--rendered_item_processor_dependencies--interdiff.txt1.47 KB
2573997-5--rendered_item_processor_dependencies.patch1.49 KB

The patch in #4 doesn't seem to actually contain the RenderedItem changes anymore. Which is probably why the tests pass – I otherwise don't think the code would correctly handle removed plugin dependencies, as explained in the other issues.
So, I think we should just leave the tests out for now and take care of all that in #2574633: Properly react when a plugin's dependencies are removed.

Regarding the actual code:

  1. +++ b/src/Plugin/search_api/processor/RenderedItem.php
@@ -281,4 +282,26 @@ class RenderedItem extends ProcessorPluginBase {
+    foreach ($view_modes as $entity => $definitions) {

    It's not $entity, it's $datasource_id – which is actually a pretty important difference. The current code will fail for any datasource that is not one of our own ContentEntity datasources. 

  2. +++ b/src/Plugin/search_api/processor/RenderedItem.php
@@ -281,4 +282,26 @@ class RenderedItem extends ProcessorPluginBase {
+        $configurations[] = $evm;

    What advantage does this have over just calling addDependency() here?

    However, more importantly, you're passing an object to addDependency() where a config dependency name is expected. I'm pretty sure you need to call $evm->getConfigDependencyName() there.

The attached patch would fix both of these issues, I think, and would be RTBC from my point of view (unless I've made a mistake in there). Testing dependency removal and making sure all processors declare appropriate dependencies would be two separate issues, I'd say (since this needs to happen for other processors, too).

borisson_’s picture

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

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

All right, we're on a roll here … !
Committed.
Thanks again!

Status: Fixed » Closed (fixed)

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