Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sinan Erdem created an issue. See original summary.

rlmumford’s picture

This was a key feature in Drupal 7, I can't seem to find a mention of it in the roadmap?

rlmumford’s picture

Category: Support request » Feature request
Status: Active » Needs review
FileSize
5.56 KB
TR’s picture

You have to start the test manually in this queue ...

rlmumford’s picture

Updated for test fixes.

fago’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This looks like a good start, however I think we should add some test coverage for that!

rlmumford’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
8.37 KB

Noticed another bug in Rules while working through this. It seems like different parts of the code assume different things about the 'context_definitions' and 'provided_context_definitions' properties in component config. In some places "provided_context_definitions" is assumed to be an array of names and "context_definitions" an array of all contexts associated with the component. Other places expect "context_definitions" to contain only those context_definitions required by the component and "provided_context_definitions" to contain the definitions of contexts that are provided by the component.

In the resulting confusion the code ended up such that RulesComponent::createFromConfiguration() and RulesComponent::getConfiguration() were not idempotent. (RulesComponentConfig::getProvidedContextDefinitions() and RulesComponent::createFromConfiguration() clearly expect 'provided_context_definitions' to be an array of definitions - RulesComponent::getConfiguration() would set 'provided_context_definitions' as an array of names)

In this patch, as well as exposing the feature requested in the UI, this bug has been fixed by making sure that "provided_context_definitions" is treated as an array of definitions where ever possible.

The last submitted patch, 7: 2916331-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rlmumford’s picture

Test fixes.

Status: Needs review » Needs work

The last submitted patch, 9: 2916331-9.patch, failed testing. View results

rlmumford’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
4.58 KB

So switching to DI introduced more bugs.

fago’s picture

Status: Needs review » Needs work

thx, looks mostly good. However, I'm not sure the state changes should be done in execute() as usually we do the state setup in Drupal\rules\Engine\RulesComponent

I think this should be done in \Drupal\rules\Engine\RulesComponent::createFromConfiguration()

squibb’s picture

Tag is wrong... in #11 patch

$row['usage'] = [
+        '#title' => $this->t('Usage'),
+        '#title_disply' => 'invisible',
+        '#type' => 'select',
+        '#options' => $usage_options,
+        '#default_value' => $usage,

'#title_disply' has to be '#title_display'

TR’s picture

In regards to #7, those decisions and changes were made in:
See #2664700: Expose Rules components as action plugin
And https://git.drupalcode.org/project/rules/commit/3a0d230
Reading those might shed some light on the issue or indicate other places that need to be fixed.

shubham.prakash’s picture

Status: Needs work » Needs review
FileSize
18.46 KB

Made the changes after rerolling.

TR’s picture

The two test fails are because of bad strings in the test case. The tests are looking for:
"There is no Rules component yet." and "Add rule"

But they should be looking for:
"There are no rules components yet." and "Add component"

Can you fix those, re-roll the patch and see if the testbot likes it?

shubham.prakash’s picture

Made the changes as suggested.

TR’s picture

And that fail is because in the tests all the field names need to start with context_definitions[ instead of context[ because of the changes introduced by #3030295: 'context' deprecated in Rules @Condition annotation and #3092087: Deprecate 'context' in Rules @RulesAction annotation

shubham.prakash’s picture

TR’s picture

Note that the tests stop executing once they hit a failure, so what you are seeing is that fixing one thing will cause more of the test to run, potentially exposing other fails.

But we're near the end of that test now, so this might be the last one. The error is "Button with id|name|title|alt|value "context_data" not found." I think you should just be able to remove that pressButton() call because it was meant to switch to the data selector in the UI but that is not necessary or possible anymore after #2804941: Implement assignment restriction in plugin context was fixed.

shubham.prakash’s picture

Removed pressButton() for context_data

TR’s picture

Nice! Thank you! Tests now pass, so we can move on to evaluating the functionality ... I will try to post a thorough review in the next few days. This is an important patch that would be very nice to get finished and committed to Rules.

rlmumford’s picture

Status: Needs review » Needs work

Just flagging that the patch no longer applies on alpha6

MegaChriz’s picture

Issue tags: -Needs tests +Needs reroll

The patch indeed no longer applies, so it needs a reroll. Tagging with "Needs reroll". I removed the tag "Needs tests" as it seems to have tests already.

TR’s picture

Assigned: Unassigned » TR

There are some major problems and omissions with the above patches.

I have put a lot of work into this and it's almost ready, but I haven't had time to work on it in the past few months. Assigning to myself. I'll try to finish this off soon.

dshields’s picture

It'd be great to get this working

Megha_kundar’s picture

Status: Needs review » Needs work

The last submitted patch, 27: 2916331-27.patch, failed testing. View results

lukus’s picture

Just adding that this would be a really useful piece of functionality.

Without this, the components have limited utility for my main use cases.

If there's anything I can do to help, please let me know.

rodmarasi’s picture

yes lukus. can you reroll a better patch?

rodmarasi’s picture

test

marco5775’s picture

hello,
the Rules plugin is really important to configure my Drupal sites without programming.
Where are we with this "feature request"?
Thank you for everything you do ;-)

kopeboy’s picture

Seconding this. I cannot do anything useful from the UI with Components right now if I cannot pass values from Rules nor calling the component from a View with VBO.
I can't code this but I would be very happy to sponsor your work to have this feature faster 🙏🏻

TR’s picture

This fell off my radar last year. I still have the code, and if I recall correctly the only problem was that the ajax on the parameter add form wasn't working correctly. I'll see if I can reroll my patch and post it here as a first step.

TR’s picture

Issue tags: -Needs reroll +beta blocker
kopeboy’s picture

Up

alimbert’s picture

Is there an update on this?