Problem/Motivation

\Drupal\Core\Plugin\Context\ContextHandlerInterface::applyContextMapping() applies a set of contexts to a given plugin.
If any of the contexts required by the plugin are not present in those passed to the method, an exception is thrown.

However, if the plugin already had that context set, there is no reason to throw the exception

Proposed resolution

If a context is "missing" from those being applied, only throw an exception if the plugin doesn't already have that context

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#19 3029625-context-19-PASS.patch3.2 KBtim.plunkett
#19 3029625-context-19-FAIL.patch1.67 KBtim.plunkett
#18 3029625-revert.patch3.61 KBtim.plunkett
#16 3029625-missing-context.patch1.66 KBdpi
#7 3029625-context-7-interdiff.txt752 bytestim.plunkett
#7 3029625-context-7.patch3.61 KBtim.plunkett
#2 3029625-context-2-PASS.patch3.59 KBtim.plunkett
#2 3029625-context-2-FAIL.patch1.14 KBtim.plunkett

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3029625-context-2-FAIL.patch1.14 KB
new 3029625-context-2-PASS.patch3.59 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Related issues: +#3001313: Field blocks in the layout builder do not have view mode suggestions

The last submitted patch, 2: 3029625-context-2-FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: 3029625-context-2-PASS.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 3029625-context-7.patch3.61 KB
new 3029625-context-7-interdiff.txt752 bytes

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented
Status: Needs review » Reviewed & tested by the community

Works as expected, coming from #3001313: Field blocks in the layout builder do not have view mode suggestions.

alexpott’s picture

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

Committed and pushed 8c90cbfdf7 to 8.8.x and 35ddee690b to 8.7.x. Thanks!

  • alexpott committed 8c90cbf on 8.8.x 
    Issue #3029625 by tim.plunkett: Do not throw an exception when a context...

  • alexpott committed 35ddee6 on 8.7.x 
    Issue #3029625 by tim.plunkett: Do not throw an exception when a context...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

This commit broke https://www.drupal.org/project/entity_route_context

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

I see this after updating to beta1.

Drupal\Component\Plugin\Exception\ContextException: The @entity_route_context.entity_route_context:canonical_entity:node context is not a valid context. in Drupal\Component\Plugin\ContextAwarePluginBase->getContextDefinition() (line 92 of core/lib/Drupal/Component/Plugin/ContextAwarePluginBase.php).
Drupal\Core\Plugin\ContextAwarePluginBase->getContextDefinition('@entity_route_context.entity_route_context:canonical_entity:node') (Line: 53)
Drupal\Core\Plugin\ContextAwarePluginBase->getContext('@entity_route_context.entity_route_context:canonical_entity:node') (Line: 114)
Drupal\Core\Plugin\Context\ContextHandler->applyContextMapping(Object, Array) (Line: 87)
dpi’s picture

dpi
Location Perth, Australia
commented

I dont think the error has to do with the Entity Route Context project directly, in this case the error is caused by a DSfield with an optional context value, which happens to not be satisfied with values. For other pages with the DSField, the context is applied properly without error.

It looks to me like ($context = $plugin->getContext($context_id)) from the patch expects either an object or FALSE, but actually it returns an object or throws an exception.

We should swallow the exception allowing for the remaining else/elseif's to execute.

dpi’s picture

dpi
Location Perth, Australia
commented
Status: Fixed » Needs review
StatusFileSize
new 3029625-missing-context.patch1.66 KB

Should we continue in this issue?

Kinda ugly patch

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Needs tests

A bug fix added a new bug I'd suggest a revert.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 3029625-revert.patch3.61 KB

Let's get this reverted.
First have to make sure nothing depends on this change (I can't remember if I landed anything that was blocked on this).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Needs tests
StatusFileSize
new 3029625-context-19-FAIL.patch1.67 KB
new 3029625-context-19-PASS.patch3.2 KB

Here's a fix, and though I really don't like the continue, it fixes the flow so we can have a try/catch instead of one long if/elseif/elseif/elseif/elseif/else

+++ b/core/tests/Drupal/Tests/Core/Plugin/ContextHandlerTest.php
@@ -378,8 +379,9 @@ public function testApplyContextMappingMissingRequired() {
     // No context, so no cacheability metadata can be passed along.
-    $plugin->expects($this->never())
-      ->method('getContext');
+    $plugin->expects($this->once())
+      ->method('getContext')
+      ->willReturn(new Context($context_definition));

@@ -413,8 +415,9 @@ public function testApplyContextMappingMissingNotRequired() {
     // No context, so no cacheability metadata can be passed along.
-    $plugin->expects($this->never())
-      ->method('getContext');
+    $plugin->expects($this->once())
+      ->method('getContext')
+      ->willReturn(new Context($context_definition));

Idk how I got away with this. I specifically hid the problem with this change...

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Minor release beta testing

Tagging. Thanks, for the tests and the patch. Fix looks good to me.

The last submitted patch, 19: 3029625-context-19-FAIL.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Let's commit the follow-up patch then.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Minor release beta testing

Let's do this the right way, and close with a critical follow-up: #3046243: Regression: Optional context values may throw exceptions if unsatisfied

Status: Fixed » Closed (fixed)

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