While working on Rules, I repeatedly ran into the issue that Context::getContextValue() throws a ContextException when no value is set and the context is marked as required. So far, we've been successfully working around that, however this turns out to be really nasty when during configuration a contextual plugin (action or condition) often needs to access possibly not yet configured context values.

Problem

$plugin->getContextValue('type') might thrown an ContextExecption, although it's not documentd as such. This is due to Context::getContextValue() doing so, but this has no documentation for the exception either.

Conditions in Rules often have code like:

    if ($type = $this->getContextValue('type')) {
      // do sometthing.
    }

This works fine as long as the value is there. But thanks to the exception it fails if it isn't unless you do:

    if ($this->getContext('type')->hasContextValue() && $type = $this->getContextValue('type')) {
      // do sometthing.
    }

That's quite longer and no one would do it, as the API docs do not mention any exceptions. So until this is fixed, people will have to run into the exception first.

Proposed resolution

Make getContextValue work as documented and just return the value, without throwing an exception.

Imo, this is the better fix than documenting the existing exception as checking whether some required context is set, is something that belongs into the context handler mapping code which prepares a plugin for execution. And actually, there it is. That way, access a context value during configuration time is safe.

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

-

Comments

fago created an issue. See original summary.

fago’s picture

StatusFileSize
new706 bytes

Here is the API docs of ContextInterface::getContextValue:


  /**
   * Gets the context value.
   *
   * @return mixed
   *   The currently set context value, or NULL if it is not set.
   */
  public function getContextValue();

Let's make it follow that.

klausi’s picture

Title: Getting a context values might throw an unexpected exception » Context::getContextValue() violates ContextInterface by throwing an unexpected exception
Status: Needs review » Needs work
Issue tags: +Plugin Context, +Needs tests

Makes sense, this should come with a test case. We can probably extend existing context test cases somewhere.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Priority: Normal » Major
Related issues: +#2521956: Missing contexts prevent caching of block access

This came up again. Bumping to major.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
StatusFileSize
new2.09 KB
new2.24 KB
new2.93 KB

Here is any attempt at a test. I used $this->any() because sometimes the method is not called. If this needs to be exact that will have to change.

The last submitted patch, 13: 2677162-13-fail.patch, failed testing. View results

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Tests coverage looks good to me.

The last submitted patch, 13: 2677162-13-fail.patch, failed testing. View results

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Plugin/Context/Context.php
@@ -68,10 +68,6 @@ public function getContextValue() {
         $this->setContextValue($default_value);
       }
-      elseif ($definition->isRequired()) {
-        $type = $definition->getDataType();
-        throw new ContextException("The '$type' context is required and not present.");
-      }
       return $default_value;
     }

Are there cases where we're catching this exception but shouldn't be now that could be cleaned up?

eclipsegc’s picture

Yes, there are a number of locations in which we try/catch this very behavior both in core and contrib.

Core uses this in the:

  • ContextHandler (all contextual plugins are routed through that)
  • ConditionAccessResolverTrait (involved in block visibility)
  • Block access
  • Layout Builder's section storage
  • A couple of tests

And because I checked this in a dev site I have, I noticed pathauto also directly checks this.

Not sure I'm convinced a behavior change is necessary here.

Eclipse

catch’s picture

Status: Needs review » Needs work

Well I like the patch, but it would be good to remove unnecessary try/catch from core in that case (and also document the change in a change record for contrib. So marking needs work in that case.

eclipsegc’s picture

What's the BC consideration for exception catching? This is the way people have interacted with these methods historically. Changing it would alter the behavior of contrib.

catch’s picture

We're bringing the implementation into line with the interface. As mentioned in #19 it will need a change record, and may need to only happen in a minor. Exceptions are supposed to be used for error conditions, so removing an error condition is not an API change as such.

eclipsegc’s picture

I understand the goal of the issue is to bring the code inline with the interface, but the code has been in use for years at this point and there are many implementations depending on the existing behavior. Furthermore, this assumes buy in from the plugin maintainers on the approach, which I'm not convinced we have. It'd be good to have voices other than my own here.

I don't find the initial report convincing. A plugin can retrieve context objects during config time for PRECISELY the reasons stated in the issue summary already. We recognize that configuration-time context interaction is a thing. This is part of why the Context system uses proxy objects wrapped around values. Under MOST conditions, context values are not necessary during configuration, but if they are, interacting with the Context object directly is simple enough.


$context = $this->getContext('my_context');
$value = NULL;
if ($context->hasContextValue()) {
  $value = $context->getContextValue();
}

If "$context->getContextValue()" is called, and there is NOT a value, or a backup default value, then we are in an actual error state and should not continue. Or at the very least, a try/catch seems warranted for the sake of logging and error handling.

This topic has a lot of nuance in it around the differences between building a plugin type vs building a plugin. I don't want to make this post too long, but if I need to delve into these topics, I'm happy to do so. Let me know.

Eclipse

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.