Since we can create a custom "Condition" by creating Rules plugins, I created one where I don't need any context, it looks like this:
/**
* Provides a "Path is admin route" condition.
*
* @Condition(
* id = "rules_path_is_admin",
* label = @Translation("Path is admin route"),
* category = @Translation("Path"),
* )
*/
class PathIsAdmin extends RulesConditionBase implements ContainerFactoryPluginInterface {
I don't need any context, since I am using dependency injection to grab what I need by using the container, however as soon as I hit "save" button, it shows me the following messages:
Warning: Invalid argument supplied for foreach() in Drupal\rules\Form\Expression\ConditionForm->getContextConfigFromFormValues() (line 111 of modules/contrib/rules/src/Form/Expression/ContextFormTrait.php).
Then I debugged and I found this particular "foreach" implementation within "ContextFormTrait" trait:
foreach ($form_state->getValue('context') as $context_name => $value) {
In order to avoid this annoying message, we should provide a default value to "get" method used by $form_state object, like this:
foreach ($form_state->getValue('context', []) as $context_name => $value) {
Reason why I think, we should update that chunk of code.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2840815-16-fix-plus-test.patch | 8.24 KB | tr |
| #16 | 2840815-16-test-only.patch | 5.96 KB | tr |
| #11 | 2840815-11-context-default-value.patch | 2.28 KB | tr |
| #8 | interdiff-2840815-6-7.txt | 2.28 KB | ritzz |
| #8 | added_default_parameter-2840815-7.patch | 2.33 KB | ritzz |
Comments
Comment #2
keboca commentedComment #3
avpadernoComment #4
fagoSounds good - can you provide PR for that?
Comment #5
fagoComment #6
ritzz commentedAdded Default Value for "getValue('context')".
Comment #8
ritzz commentedAdded a condition to check if empty or not.
Comment #9
ritzz commentedComment #11
tr commentedPatch still applies and tests now run green. But the patch introduced a few coding standards errors, so here's a re-roll to fix those:
Comment #12
tr commented@ritzz: You assigned this issue to yourself - are you still planning to work on this?
Comment #13
tr commentedStill needs some reviews, and wouldn't hurt to add a test case to ensure that Conditions work without a defined context.
Comment #14
tr commentedHere's a test-only patch that proves the error. This should FAIL.
Also, here's a patch that includes both the test and the fix from #11. This should PASS, demonstrating that the patch fixes the error.
Comment #16
tr commentedInteresting - this why we have testing.
Rules already has some testing-only Conditions which are defined without contexts. But they are only used in Kernel and Unit tests - neither of those uses the Rules UI so any UI-related errors, like this issue, won't be detected by the tests. This Kernel and Unit testing does show that actually USING these Conditions that don't have contexts WORKS in code - it's just a UI issue that we're trying to fix here.
The added test in #13 is a Functional test that enables the 'rules_test' module, which contains these no-context testing Conditions. And when those Conditions become available for the first time in Functional tests we see a new error that hasn't been detected before. Specifically, these test-only conditions have incomplete annotation. Not only are they missing the 'context' annotation but they're also missing the 'category' annotation. While the patch fixes the missing 'context' annotation (we WANT to support conditions without context), the missing 'category' annotation will cause other tests to fail. We DON'T want to support missing category, so the fix here is to add the missing category to the test-only Conditions - and to the test-only Actions while we're at it, since they are also missing the category annotation.
New test-only patch and test-plus-fix patch attached, but the only part that changed is the test part.
Comment #18
tr commentedOK, #16 worked exactly as expected - the test-only patch failed, demonstrating the problem, while the test-plus-fix patch PASSED, showing that the fix worked.
Comment #20
tr commentedCommitted #16