I'm working through the Kernel tests to see why they're broken after #2508884: Make contexts immutable landed.
There were 15 failing tests initially. 10 of those relate to the use of setContextValue() in ContextHandlerTrait. The next 4 seem to be a problem with null handling in RuleState::applyDataSelector(), since Context::getContextValue() returns NULL rather than an instance of TypedDataInterface, causing a crash when it's treated like an object. The last is also related the NULL data case, since Context::getContextValue() as now written treats a NULL'ed context that's supplied as not there for purposes of isRequired().
I have test fixes for the first two issues so far. They run GREEN, but I'm not sure if they're actually correct.
Comment | File | Size | Author |
---|---|---|---|
#3 | workarounds-pass-1.patch | 3.07 KB | Torenware |
Comments
Comment #2
Torenware CreditAttribution: Torenware as a volunteer commentedComment #3
Torenware CreditAttribution: Torenware as a volunteer commentedI have plausible solutions for the first two items, as I've said before.
Getting a context to wrap a NULL value, however, appears to be hard to do. I've tried to create a sort of "typed null" object (an 'any', setting the value to NULL), but Context's implementation of getContextValue() will throw an except:
I don't see a good way around this. Worse: I'm seeing some weird behavior even when it doesn't throw.
I'm going to post patches for my approach to the other two issues, since I don't think it's to the point I can create a PR that's worth posting.
The patch shows changes to ContextHandlerTrait and RulesState that resolve 14 out of the 15 issues in the Kernel tests. Again, not sure that the behavior is what you want, but at least the tests like them.
Comment #4
emclaughlin CreditAttribution: emclaughlin at Genuine commentedI'm running into a problem that may be related to this? I'm working on replacing all the evaluate calls with doEvaluate in conditions, and I'm having an issue with optional context arguments. Specifically: they're coming through as defined as NULL, which means the default doesn't get applied.
I haven't been able to track down why it's doing that, so I'm not 100% sure that it's related to this, but I figured I'd comment just in case.
Comment #5
Torenware CreditAttribution: Torenware as a volunteer commented@emclaughlin: take a look at #2573823: Context in Rules plugins does not respect annotation order. Some of the problems I've seen related to doExecute() getting its arguments in the wrong order, which can happen with default arguments right now.
Is this relevant to your problem?
Comment #6
emclaughlin CreditAttribution: emclaughlin at Genuine commented@Torenware Hmmmm, I'm not sure. It looks like all of the other params are being provided in the expected order. It's just, when you don't specify a value for an optional param, the value ends up being NULL in the doEvaluate instead of what the default value is defined as.
Comment #7
emclaughlin CreditAttribution: emclaughlin at Genuine commented@Torenware Never mind! It's an issue with the
execute()
/evaluate()
functions inRulesActionBase
andRulesConditionBase
respectively. I'm creating another issue. Thanks!Comment #8
Torenware CreditAttribution: Torenware as a volunteer commentedSee #2575977: Major change in \Drupal\Core\Plugin\Context\Context breaks Rules and related issues for details on the fix.