No way to handle 'optional' contexts without try/catch

This seems like a core bug in \Drupal\node\ContextProvider\NodeRouteContext::getAvailableContexts().
Why do we blindly state we'll have a context available, and not restrict it in some way to only show on node pages?

I agree that this is a core bug. Rather than throwing an exception, it should just not show the block.

It would have been easy to write a unit test for this, but someone refactored this code into a protected static method.

For the record, page manager has the same problem, pretty sure we need to fix it there too. Unless you did recently but last time I checked, it still exploded nicely :)

I also know that we are filtering, but we know that those two contexts (node and current user), *might* be available.

Might also be a caching problem, do we need to worry about that?

This is catching the exception, but shouldn't we not throw that in the first place?

Hm. We need some indication from the API that the context mapping failed. It could be a boolean return value, but that's an API change. This patch seems like the best we can do without changing the API.

Yeah, I find that very weird too. But yes, that would be an API change. See also #2521956: Missing contexts prevent caching of block access and #2375695-42: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed (comment #42 in case the link doesn't work).

Quoting myself from there:

* Started to refactor ContextHandler to explicitly check for required on the plugin context definition and check for a context value. Also throws an exception, but I really dislike that we are using exceptions there. Throwing an exception on /user when you have a block with a node type visibility condition is *bad*. I think that method should just return a boolean. What this refactoring allows is to still assign all context and pass through the cacheability metadata.. which gets me to the next point.

While we use exceptions for code flow in 404/403 pages because Symfony, and form API because we couldn't think of anything else, would really like to avoid any more cases. Exceptions should really be for actual errors and not just eaten - if this is valid behaviour, the code should not throw an exception, if it isn't, we shouldn't hit the condition in the first place.

Can add a new method if we have to, then it wouldn't be an API change.

Tim's patch is functionally replicating what we already do for conditions.

Eclipse

Looks like that was added in #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types but I don't see it discussed in that issue.

Here's a patch that takes @catch's suggestion in #11 of adding a new method. However, after attempting this exercise, I'd personally prefer Tim's patch in #6.

While it's possible to add a new method with out breaking BC, it's complicated by the fact that this is a service ('context.handler') that has an interface (ContextHandlerInterface). We can't simply add the new method to the existing interface, because then some contrib module which replaced that service with it's own class implementing that interface would cause a fatal error. So, we have to add a new interface (I called it ContextHandlerInterface2 for lack of a better name - please just ignore that the name is terrible).

And we can't require that any class that replaces the 'context.handler' service implements the new interface, because that would also be a BC break. So, code that uses that service needs to check if it implements the new interface before calling the new method (which I called applyContextMapping2() - again ignore the terrible name). This also means that we need to have a fallback that uses only methods from the original interface.

So, the net result is:

• We have to continue checking for the exception anyway in the case that the service doesn't implement the new interface
• Naming all this stuff is super hard and will probably make DX worse. To be honest, I could probably live with ContextHandlerInterface2 but I have no idea how to name the method...

While it'd be better not to use an exception here, I think those cons out-weigh the benefit.

So, like I said at the beginning, I think we should just go with #6 (which still needs tests).

+++ b/core/modules/block/src/BlockViewBuilder.php
@@ -144,8 +146,23 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
+      if ($handler instanceof ContextHandlerInterface2) {
+        $mapping_succeeded = $handler->applyContextMapping2($plugin, $contexts);
+      }
+      else {
+        try {
+          $handler->applyContextMapping($plugin, $contexts);
+        } catch (ContextException $e) {
+          $mapping_succeeded = FALSE;
+        }

Really we don't need the instance check we just wrap them both in try catch and one just won't trigger it.

Which sort of gets to the heart of by hesitance at this approach. Since we have to support both this interface doesn't really buy us anything. I think we should go ahead with tim's patch mostly as is (should document what's going on and why we basically throw the exception away).

I'd also like to follow up with improving the context interface to allow reporting of missing contexts(this could be a separate interface allowing us to compose them and provide BC). This would allow us to skip throwing exceptions when its implemented and when the interface is implemented we can do more sane logic like this instead of relying on exceptions.

// Skip rendering is context is missing.
if (!empty($context->missingContext())) {
  $build = [];
}

or in an admin interface:

$missing = $context->missingContext();
if (!empty($missing)) {
  foreach ($missing as $context) { /* Display message about missing context and steps to setting it up */ }
}

+++ b/core/modules/block/src/BlockViewBuilder.php
@@ -144,8 +146,23 @@ protected static function buildPreRenderableBlock($entity, ModuleHandlerInterfac
+      if ($handler instanceof ContextHandlerInterface2) {
+        $mapping_succeeded = $handler->applyContextMapping2($plugin, $contexts);
+      }
+      else {
+        try {
+          $handler->applyContextMapping($plugin, $contexts);
+        } catch (ContextException $e) {
+          $mapping_succeeded = FALSE;
+        }

Really we don't need the instance check we just wrap them both in try catch and one just won't trigger it.

Which sort of gets to the heart of by hesitance at this approach. Since we have to support both this interface doesn't really buy us anything. I think we should go ahead with tim's patch mostly as is (should document what's going on and why we basically throw the exception away).

I'd also like to follow up with improving the context interface to allow reporting of missing contexts(this could be a separate interface allowing us to compose them and provide BC). This would allow us to skip throwing exceptions when its implemented and when the interface is implemented we can do more sane logic like this instead of relying on exceptions.

// Skip rendering is context is missing.
if (!empty($context->missingContext())) {
  $build = [];
}

or in an admin interface:

$missing = $context->missingContext();
if (!empty($missing)) {
  foreach ($missing as $context) { /* Display message about missing context and steps to setting it up */ }
}

Really we don't need the instance check we just wrap them both in try catch and one just won't trigger it.

It's a different method, though, because we can't change the signature on the original method. This might have been hard to catch because the method names are so similar (applyContextMapping() vs applyContextMapping2())?

Which sort of gets to the heart of by hesitance at this approach. Since we have to support both this interface doesn't really buy us anything. I think we should go ahead with tim's patch mostly as is (should document what's going on and why we basically throw the exception away).

In any case, I completely agree with your conclusion. :-)

The patch that @phenaproxima references above will also fix this issue (confirmed while doing testing on a CTools patch for a block that takes context).

Lets close this as a duplicate then.

Which sort of gets to the heart of by hesitance at this approach. Since we have to support both this interface doesn't really buy us anything.

It gets us two things:

1. We're not creating then eating the exception as part of the code flow, which on 99.9% of sites would be the case since the context handler probably won't be different from the core one.

2. When we get to 9.x we can either drop the old interface or rename the new one back over the old interface. Either way modules would either have to do nothing to port, or maybe deal with a rename, and then the if/else just becomes dead code then.

Also it might be possible to handle the instanceof check with a new service definition using the new interface, not sure about that though - it's not something we've really dealt with yet.

I've committed #2635238: Contexts not mapped in time to use them in BlockInterface::access() but I still think we should fix this at the API level so that eventually calling code doesn't have to jump through hoops. So re-opening for the remaining discussion here.

So, I'm not sure of the importance of this issue any longer. Looking at the issue summary it's demonstrably false since the ContextDefinition system leveraged by plugins absolutely does have handling for optional contexts, they just simply must be defined as such in the plugin definition. The \Drupal\condition_test\Tests\OptionalContextConditionTest actually has test coverage for this use case and the \Drupal\condition_test\Plugin\Condition\OptionalContextCondition condition plugin shows how this is defined. A block plugin context will work the same way since they're all built on the same API. In short, optional contexts are absolutely supported by the plugin system and block specifically. For the sake of saving other looking it up:

/**
 * Provides a condition with an optional node context.
 *
 * The context type entity:node is used since that would allow to also use this
 * for web tests with the node route context.
 *
 * @Condition(
 *   id = "condition_test_optional_context",
 *   label = @Translation("Optional context"),
 *   context = {
 *     "node" = @ContextDefinition("entity:node", label = @Translation("Node"), required = FALSE),
 *   }
 * )
 */

The "required = FALSE" portion of the ContextDefinition annotation will set a plugin's contexts to optional and it will no longer throw an exception on missing contexts.

Eclipse

Why aren't we using that for the NodeRouteContext then?

@catch: I'm not sure I entirely understand how the issue you're describing is different than #2521956: Missing contexts prevent caching of block access?

As @EclipseGC points out, there is the idea of optional contexts built into the context system. What's missing is a way to distinguish a mapping failure between (a) a missing context (ie. no context object was passed in at all) and (b) an empty context (a context object was passed in, but doesn't have a value and is required). This is what #2521956 is attempting to address.

So, if the bug described here is the same as #2521956, we should work on that there, and maybe make this issue just be about switching from an exception to a return value (and the backcompat stuff necessary to make that happen)?

NodeRouteContext extracts the node object from the route upcasting and makes it available in the array of runtime contexts to blocks and their visibility conditions. If a block (or condition) sets a mapping that states it should use the node context provided by NodeRouteContext, then it an attempt will be made to map it from the contexts array to the appropriate plugin(s). However, if those plugins state that they REQUIRE a node context (instead of saying "oh I can figure it out without a node, but I'll use one if you have one") then they cannot operate if the context is missing and will throw an exception if the context is not currently available (i.e. you're not on a node/{node} page).

So, case in point, the node_type condition cannot determine the bundle of the node you're currently visiting if no node is available. Therefore, if it is executed without a node, it will throw an exception. Plugins are built to be aware of what contexts are available during configuration time, but core is not smart enough to know this, so it's really a failure of the block UI that we have to catch these exceptions since they are not intended to be part of typical runtime. Since core has no notion of pages or what's available on them, core resorts to interpreting these exceptions as "access false". As core becomes more "page aware", we will be able to predict what plugins can be executed on a given page and only allow for those plugins to be configured. This is true of blocks, conditions and any other plugin we might introduce into the page-rendering process.

Also, conditions return boolean values, not access checks. This is because conditions are not intended for exclusive use in access, i.e. rules makes use of them too and so can many other systems (pathauto can also use them in the contrib work we're doing). To this end they would not simply return some default if there's missing required contexts as that does not properly represent the evaluation they claim to do.

I hope that explains what's going on here.

Eclipse

The problem is that if you configure a node type visibility condition, then it is *not* optional. It's not the provider that decides if a context or optional or not, it's the plugin that requires it. That a provided context can even define that is weird and before #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed, we even checked the provided context for required/optional instead of the plugin that receives it.

NodeRouteContext doesn't always give you a context value but it does always give you a context with cacheabliity metadata, so it is doing it correctly.

I ran into a quite related issue with Rules (again). The problem is that there is no way to check for a set context value without having to use try/catch while doing so. While this matches exactly the issue title, it's not necessarily directly related as the exception is thrown at a different place: the Context class. Thus I opened #2677162: Context::getContextValue() violates ContextInterface by throwing an unexpected exception for it.

It's not the provider that decides if a context or optional or not, it's the plugin that requires it.

Right. The 'required' value of available context can tell you though whether it could be unset.

I faced the similar issue using panels #2679124: Missing contexts throw an exception.

Ok, as outlined in the issue summary of #2677162: Context::getContextValue() violates ContextInterface by throwing an unexpected exception you absolutely can determine if a context has a value. As outlined in this issue the plugin is responsible for setting whether a context is required or optional, and there is support for optional contexts which will not throw an exception. Though berdir points out we used to check for the required-ness of contexts defined by providers, that was simply wrong and probably my fault for forgetting which context object/definition I was dealing with at the time.

Best as I can tell, this all functions as expected after a handful of fixes that have landed in 8.0.x and I don't see any reason this issue should remain open.

Eclipse