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

Comments

tim.plunkett created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 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
commented
Issue tags: +Needs tests
StatusFileSize
new 2961822-3.patch6.54 KB
new 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
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
commented
StatusFileSize
new 2961822-5.patch6.85 KB
new interdiff-2961822-3-5.txt6.24 KB

Done!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: -Needs tests
StatusFileSize
new 2961822-6.patch13.08 KB
new 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
commented
StatusFileSize
new 2961822-7-FAIL.patch10.61 KB
new 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
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
commented
Status: Needs work » Needs review

Fail patch failed, as expected.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
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
commented
Issue summary: View changes
phenaproxima’s picture

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

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

#6 looks good, thanks for the CR

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
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
commented
StatusFileSize
new 2961822-16.patch13.08 KB
new 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
commented

Adding review credits

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
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
commented

published change record

Status: Fixed » Closed (fixed)

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