Problem/Motivation

See also
#2821191: Allow object-based plugin definitions to be processed in PluginDependencyTrait
#2818653: Allow object-based plugin definitions to be processed in DefaultPluginManager::findDefinitions()

ContextHandler can filter plugin definitions by context, but it only supports array-based plugin definitions with a context key containing context definitions. Now that some plugin definitions in core are class-based, we need to extend this functionality to work with those definitions.

Proposed resolution

Add a new interface for context-aware plugin definitions and allow ContextHandler to filter plugin definitions which implement it.

Remaining tasks

Patch, tests, commit.

User interface changes

None.

API changes

A new interface and trait will be added to the plugin context system.

Data model changes

None.

CommentFileSizeAuthor
#16 interdiff-2961822-6-16.txt644 bytesphenaproxima
#16 2961822-16.patch13.08 KBphenaproxima
#7 interdiff-2961822--7-FAIL-6.txt2.34 KBphenaproxima
#7 2961822-7-FAIL.patch10.61 KBphenaproxima
#6 interdiff-2961822-5-6.txt6.09 KBphenaproxima
#6 2961822-6.patch13.08 KBphenaproxima
#5 interdiff-2961822-3-5.txt6.24 KBphenaproxima
#5 2961822-5.patch6.85 KBphenaproxima
#3 interdiff-2961822-2-3.txt2.46 KBphenaproxima
#3 2961822-3.patch6.54 KBphenaproxima
#2 2961822-2.patch3.94 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Active » Needs review
FileSize
2961822-2.patch3.94 KB

Here's a first stab at defining the API for this. If this looks good, I'll update ContextHandler and write some tests.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs tests
FileSize
2961822-3.patch6.54 KB
interdiff-2961822-2-3.txt2.46 KB

Updated ContextHandler to work with the new interface. Still needs tests, though.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
  1. +++ b/core/lib/Drupal/Component/Plugin/Definition/ContextAwarePluginDefinition.php
@@ -0,0 +1,60 @@
+class ContextAwarePluginDefinition extends PluginDefinition implements ContextAwarePluginDefinitionInterface {

    I think this should be a trait instead of a class. 

  2. +++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -17,17 +18,47 @@ class ContextHandler implements ContextHandlerInterface {
-      if (!is_array($plugin_definition) || !isset($plugin_definition['context'])) {
+      if (!$this->pluginSupportsContext($plugin_definition)) {
         return TRUE;
       }
 
       // Check the set of contexts against the requirements.
-      return $this->checkRequirements($contexts, $plugin_definition['context']);
+      return $this->checkRequirements($contexts, $this->getContextDefinitions($plugin_definition));
...
+    if (is_array($plugin_definition)) {
+      return isset($plugin_definition['context']);
+    }
+    return $plugin_definition instanceof ContextAwarePluginDefinitionInterface;
...
+    if (is_array($plugin_definition)) {
+      return $plugin_definition['context'];
+    }
+    return $plugin_definition->getContextDefinitions();

    My original approach of the guard condition and simple return no longer makes sense with the more complex structure.

    I'd combine pluginSupportsContext and getContextDefinitions, and just have it return NULL if there aren't any.

    if (!$contexts = $this->getContextDefinitions($plugin_definition) {
  return TRUE;
}

    And for getContextDefinitions() it'd be something like (sorry for the pseudocode)

    $contexts = NULL
if is_array && isset $['context']
  return $['context'];

if instanceof
  return $->getContextDefinitions()
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2961822-5.patch6.85 KB
interdiff-2961822-3-5.txt6.24 KB

Done!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: -Needs tests
FileSize
2961822-6.patch13.08 KB
interdiff-2961822-5-6.txt6.09 KB

This should do the trick for tests -- I adjusted ContextHandlerTest::providerTestFilterPluginDefinitionsByContexts() so that all the plugin definitions are also duplicated as context-aware plugin definition objects.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2961822-7-FAIL.patch10.61 KB
interdiff-2961822--7-FAIL-6.txt2.34 KB

As requested by @tim.plunkett on Slack, here is a fail patch to prove that ContextHandler actually filters the plugin definitions correctly. The interdiff is what patch #6 adds in order to get the tests to pass.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

Change record written: https://www.drupal.org/node/2979920

Status: Needs review » Needs work

The last submitted patch, 7: 2961822-7-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review

Fail patch failed, as expected.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +blocker

This issue blocks #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Reviewed & tested by the community

#6 looks good, thanks for the CR

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -17,18 +18,37 @@ class ContextHandler implements ContextHandlerInterface {
+    elseif (is_array($plugin_definition) && isset($plugin_definition['context'])) {

ubernit: can just be an if, we returned above

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2961822-16.patch13.08 KB
interdiff-2961822-6-16.txt644 bytes

Nit fixed. Leaving at RTBC since I fully expect this one to pass.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Adding review credits

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

fixed on commit

diff --git a/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
index b66290f989..a43af4e3c6 100644
--- a/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
+++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
@@ -298,10 +298,14 @@ public function providerTestFilterPluginDefinitionsByContexts() {
         ->addContextDefinition('context2', new ContextDefinition('string')),
     ];
     // Context only satisfies two plugins.
-    $data[] = [TRUE, $plugins, [
-      'expected_array_plugin' => $plugins['expected_array_plugin'],
-      'expected_object_plugin' => $plugins['expected_object_plugin'],
-    ]];
+    $data[] = [
+      TRUE,
+      $plugins,
+      [
+        'expected_array_plugin' => $plugins['expected_array_plugin'],
+        'expected_object_plugin' => $plugins['expected_object_plugin'],
+      ],
+    ];

     return $data;
   }
@@ -567,6 +571,7 @@ public function testApplyContextMappingConfigurableAssignedMiss() {
 }

 interface TestConfigurableContextAwarePluginInterface extends ContextAwarePluginInterface, ConfigurablePluginInterface {
+
 }

Committed 1b66e53 and pushed to 8.6.x.

  • larowlan committed 1b66e53 on 8.6.x 
    Issue #2961822 by phenaproxima, tim.plunkett: Support object-based...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

published change record

Status: Fixed » Closed (fixed)

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