Problem/Motivation

Because of the way context mappings are currently handled, you cannot reuse a value to satisfy multiple required contexts.

Take for example, a plugin that needs two different user contexts.
If you were on a /user/{user} page, the contexts could be mapped to the user viewing the page, and the user whose page you are viewing, and that will work.
But if both contexts were mapped to the same user, we'll lose all but one of the mappings due to an array_flip().

Proposed resolution

Flip the context mapping handling so that it is always keyed by a unique value, not one that could be duplicated.

Remaining tasks

N/A

User interface changes

N/A

API changes

ContextAwarePluginInterface::getContextMapping() and ContextAwarePluginInterface::setContextMapping() are now flipped.
Before they were keyed by the Context object's name and the values were the plugin context definition names.
Now they are keyed by the plugin context definition names with the values of the Context object's names.

Similarly, the optional $mappings parameter for ContextHandlerInterface::applyContextMapping() is flipped.

CommentFileSizeAuthor
#8 2378585-interdiff.txt1.39 KBEclipseGc
#8 2378585-8.patch10.64 KBEclipseGc
#7 2378585-interdiff.txt1.32 KBEclipseGc
#7 2378585-7.patch10.51 KBEclipseGc
#3 2378585-context-3.patch10.26 KBtim.plunkett
#1 2378585-context-1-PASS.patch10.77 KBtim.plunkett
#1 2378585-context-1-FAIL.patch4.95 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
2378585-context-1-FAIL.patch4.95 KB
2378585-context-1-PASS.patch10.77 KB

The tests add a condition that takes two users as context.

The last submitted patch, 1: 2378585-context-1-FAIL.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
2378585-context-3.patch10.26 KB

Was accidentally running each test twice. Refactored that a bit (I was borrowing unnecessary parts from UserRoleConditionTest).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandler.php
@@ -73,22 +73,20 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
-    foreach ($contexts as $name => $context) {
...
+    foreach (array_keys($plugin->getContextDefinitions()) as $plugin_context_id) {

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -55,7 +55,7 @@ public function setContext($name, ComponentContextInterface $context) {
-    return isset($configuration['context_mapping']) ? array_flip($configuration['context_mapping']) : [];
+    return isset($configuration['context_mapping']) ? $configuration['context_mapping'] : [];

This is the crux of the patch. array_flip is unsafe here since the values may not be unique, and to work around that removal we iterate over the expected context names, instead of over the actual context objects.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Reviewed & tested by the community

This certainly addresses a bunch of my concerns. RTBC

Eclipse

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/lib/Drupal/Component/Plugin/ContextAwarePluginInterface.php
@@ -132,7 +132,7 @@ public function validateContexts();
-   *   'current_user' => 'user'.
+   *   'user' => 'current_user'.

@@ -143,7 +143,7 @@ public function getContextMapping();
-   *   'current_user' => 'user'.
+   *   'user' => 'current_user'.

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextHandlerInterface.php
@@ -70,9 +70,9 @@ public function getMatchingContexts(array $contexts, ContextDefinitionInterface
+   *   names. For example, if one of the $contexts is named 'current_user', but the
+   *   plugin expects a context named 'user', then this map would contain
+   *   'user' => 'current_user'.

    We've updated the names in #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types but we didn't update this help text. Let's do this here as we're changing it. current_user should be user.current_user 

  2. +++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
@@ -305,7 +305,7 @@ public function testApplyContextMappingConfigurableAssigned() {
-    $this->contextHandler->applyContextMapping($plugin, $contexts, array('name' => 'hit'));
+    $this->contextHandler->applyContextMapping($plugin, $contexts, ['hit' => 'name']);

@@ -333,7 +333,7 @@ public function testApplyContextMappingConfigurableAssignedMiss() {
-    $this->contextHandler->applyContextMapping($plugin, $contexts, array('name' => 'miss'));
+    $this->contextHandler->applyContextMapping($plugin, $contexts, ['miss' => 'name']);

    Unrelated change

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs work » Needs review
FileSize
2378585-7.patch10.51 KB
2378585-interdiff.txt1.32 KB

Updated the documentation per your request. The changes to the tests are absolutely related since the mapping going on there has been flip-flipped by this patch.

Eclipse

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
FileSize
2378585-8.patch10.64 KB
2378585-interdiff.txt1.39 KB

screwed up the 80 char limit.

Eclipse

tim.plunkett’s picture

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

Yeah for #6.2 it may look like I'm just changing the syntax, but I'm also flipping the key/value, because of the removed array_flip.

Nice catch on the docs update.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed fda020e and pushed to 8.0.x. Thanks!

  • alexpott committed fda020e on 8.0.x 
    Issue #2378585 by EclipseGc, tim.plunkett: Multiple context requirements...

Status: Fixed » Closed (fixed)

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