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
-
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 2677162-13.patch | 2.93 KB | quietone |
| #13 | 2677162-13-fail.patch | 2.24 KB | quietone |
| #13 | interdiff-2-13.txt | 2.09 KB | quietone |
| #2 | d8_context_exception.patch | 706 bytes | fago |
Comments
Comment #2
fagoHere is the API docs of ContextInterface::getContextValue:
Let's make it follow that.
Comment #3
klausiMakes sense, this should come with a test case. We can probably extend existing context test cases somewhere.
Comment #9
catchThis came up again. Bumping to major.
Comment #13
quietone commentedHere 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.
Comment #15
jibranTests coverage looks good to me.
Comment #17
catchAre there cases where we're catching this exception but shouldn't be now that could be cleaned up?
Comment #18
eclipsegc commentedYes, there are a number of locations in which we try/catch this very behavior both in core and contrib.
Core uses this in the:
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
Comment #19
catchWell 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.
Comment #20
eclipsegc commentedWhat'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.
Comment #21
catchWe'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.
Comment #22
eclipsegc commentedI 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.
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