Select lists in action & condition configuration forms

This feature should definitely be a part of Rules asap because the UI is just absolutely confusing without it. I have taken the initial patch and did some further work on it and added the select options to most of the Rules actions and conditions that seemed in dire need of it like:

• Entity Has Field
• System Message
• User Role Add
• User Role Remove
• Entity Is Of Type
• Entity Is Of Bundle

Hi,
I was searching for this patch but it still needs work I see some warning and I hope you can fix them soon. What about if I have multiple context fields lets say I have a drop-down of node_types and next context fields I need to get somehow value which node was selected so I can display node fields of this node...

This is very useful. Good work @Amerie for starting this and @shabana.navas for extending it. Some of those test failures are unrelated, and I will try to work out what is happening with the others. I will also make a Github branch for this.

Good changes, @shabana.navas, I can see you fixed the missing quotes line 83 of ContextFormTrait.php which had caused Exception: Notice: Use of undefined constant list_options - assumed 'list_options', Drupal\rules\Form\Expression\ActionForm->buildContextForm()(). Also good that you cleaned up whitespace and tidied-up some coding standards.

Looking at the test failures in #2 I have made the following fixes:

1. ServiceNotFoundException: You have requested a non-existent service "logger.factory"
   This was caused by leaving old debug in the patch in EntityIsOfBundle:

   +    \Drupal::logger('my_module')->notice(print_r($entity));
   +    \Drupal::logger('my_module')->notice(print_r($type));
   +    \Drupal::logger('my_module')->notice($bundle); 
   

2. Kernel\Engine\AutocompleteTest - 'node.status (Published)' is already fixed by another issue

3. Drupal\Tests\rules\Functional\ConfigureAndExecuteTest::testConfigureAndExecute
   Exception: Strict warning: Non-static method Drupal\rules\Plugin\RulesAction\SystemMessage::repeatListOptions() should not be called statically
   Drupal\rules\Form\Expression\ActionForm->form()() (Line: 98)
   

   This is resolved by defining the functions as public static instead of just public

4. The following change in the patch
   

   +++ b/src/Plugin/RulesAction/EntityCreateDeriver.php
   @@ -89,7 +89,21 @@ class EntityCreateDeriver extends DeriverBase implements ContainerDeriverInterfa
            }
    
            $item_definition = $definition->getItemDefinition();
   -        $type_definition = $item_definition->getPropertyDefinition($item_definition->getMainPropertyName());
   +
   +        // @todo Add support for multi-property fields for entity creation.
   +        // @see https://www.drupal.org/node/2748791.
   +        $main_property_name = $item_definition->getMainPropertyName();
   +
   +        if (is_null($main_property_name)) {
   +          continue;
   +        }
   +
   +        $type_definition = $item_definition->getPropertyDefinition($main_property_name);
   +
   +        // Get around types which don't properly define their main property (or lack of one)
   +        if (is_null($type_definition)) {
   +          continue;
   +        }
   

   I think this is unrelated, that got into the patch by accident? I recognise it from a separate issue, so have removed it from here

There are some more things to do, for coding standards and also for structure in ContextFormTrait, but here is a patch which addresses the items above. I will follow up when the tests are passing, as it is always good to get that target fixed first.

Jonathan

All the tests pass, that's nice. In this patch I have also fixed the coding standards faults (our codebase is completely clean at the moment, and we want to keep it like that). The three reported coding standards fixes are:

1. Added missing function doc comment to userRolesListOptions() in UserHasRole.php
2. Removed unused use statement from EntityIsOfType.php
3. Fixed unused variable $entity_type from fieldListOptions() in EntityHasField.php

Other fixes for readabilty and clarity:

4. In ContextDefinition.php $listOptionsCallback holds the optional function name, so should be a string with default value NULL
5. In ContextDefinitionInterface.php removed unrequired blank line introduced in patch 2. I don't think there is a particular preference for blank lines between Const definitions in the Drupal coding standard. These two are covered by the single comment so it is better to have them together without a blank between.
6. Also in ContextDefinitionInterface.php the parameter for setListOptionsCallback() is the function name so calling it $options is confusing as it sounds like it contains the list options. I have changed it to $name. Also referred to UI not GUI in the comments.
7. In SystemMessage.php fixed indentation in doc block (even though Coder does not report this).
8. Also in SystemMessage.php messageTypeListOptions() put the types in ascending order or severity: info, status, warning, error

The Travis build which matches this patch is https://travis-ci.org/jonathan1055/rules/builds/290551706

While making these changes and adding more selection lists, I realised that the UI for adding conditions and actions is only very minimally covered by our automated testing. Only one condition (rules_data_comparison) and one action (rules_system_message) are used in the existing UI functional tests. Hence I have added new tests to check adding all of the actions and conditions. The work is not finished, but here is a start. This patch is exactly the same as #7 apart from the new tests. It will fail in a few places, which shows that the code changes that passed in patch #7 has faults not being discovered by our existing tests.

As expected the tests fail, showing that the original changes were not covered by tests before. The listOptions functions all need public static. Here's a patch which should pass the new tests.

That's good, we now have test coverage for adding Conditions and Actions via UI.

This patch has several improvements:

1. In src/Form/Expression/ContextFormTrait.php we need to cater for multiple selections within the options list, for example when checking a condition for user having multiple roles, or adding/removing multiple roles. This was already catered for when using plain text entry by switching to textarea. We need '#multiple' => $context_definition->isMultiple() in the array $form['context'][$context_name]['setting']. The coding can be simplified by re-organising, which also removes the need to check !empty($configuration['list_options']). The interdiff may look complex, but the actual change from the current committed code becomes more simple, and hence easier to understand. This structure also allows for better descriptions beneath the entry field or selection, more appropriate to the type of field.
2. In /Condition/EntityHasField.php, sort the field list alphabetically for easier locating of the field required
3. In /Condition/UserHasEntityFieldAccess.php move 'User' to be the first parameter as this is more logically where it should be. Also get list of fields (same as in EntityHasField) and provide a selection list for the access required (view or edit)
4. In /Condition/UserHasRole.php added a selection list for 'match roles' with allowed values of 'AND' and 'OR'. Improved the description.
5. Expanded the test coverage to include adding 'Show system message' action, as this action is altered in the patch

Thank you #10 works for me

Here are the final changes I intend (apart for fixing any review points). I have added the action or condition id in brackets in the selction list text if it is different from label, as this helps to make it clear what is being selected. For example:

I've created a PR for this patch https://github.com/fago/rules/pull/496
My Travis build is https://travis-ci.org/jonathan1055/rules/builds/297494582

This is great functionality, which is essential for many of the Rule conditions and actions I'm trying to port from D7. I am testing it right now and evaluating the patch. I hope it's OK if I bump this to Major, because this is a D7 feature that just doesn't exist in D8 so it's a serious blocking issue for almost anyone porting conditions and actions.

The big problem I see is that the way this patch is written, the callback functions have to be static functions. That's a problem because I have many options lists which need to be dynamically defined, often through the use of a service injected into the condition or action. You can't really do this properly with a static function.

Indeed, if you look at the patch, many of the conditions and actions you are modifying to use this new feature need services to compute the list of options, and in all cases you're having to get those services through \Drupal::service() rather than injecting them into the condition or action class.

I would like to hear if there is a compelling reason to make these callbacks class methods instead of instance methods. I've made those changes locally and it works just fine as non-static methods.

One smaller point:
ContextDefinition::setListOptionsCallback() should return $this. That's standard practice for setters, so that function calls may be chained. Note that the two other setter functions in that class, ContextDefinition::setAssignmentRestriction() and ContextDefinition::setAllowNull(), both already return $this, it's just this one new function that doesn't. The doc comments for ContextDefinitionInterface::setListOptionsCallback() will also need to be modified with a @return $this.

Hi TR,
This is great feedback, thanks very much. I am new'ish to OO design, so just picked up how the original patch creator did it. I'd be happy to take advice on using dynamic functions instead of static, so would you be able to post your code here? or a patch, or you can contact me via my profile page and we can share code directly. Or if you use github you can fork Rules and show us your alternative versions.

Thanks for the tip on returning $this - I will add that change in the next patch.

Jonathan

@TR I've made the change to remove 'static' from all the list options callback functions, and simply replaced $condition::$list_callback() with $condition->$list_callback(). Also returned return $this to etListOptionsCallback(). Here's the patch and interdiff.

Minor omission in src/Context/ContextDefinition.php.

I took #17 and injected the services into the list_options_callback functions rather than using \Drupal::service(). No other changes.

The only other thing I think I would do is to move the doEvaluate() methods to the end of each controller class - there is always a lot of almost boilerplate stuff at the top (doc comments, instance variables, constructors, static create functions, and now list option callbacks) that it's difficult to find the real functional code for these conditions unless it's always at the bottom of the file.

I didn't want to make that move in this patch because it would only confuse the interdiff.

Also, in your new test, you have a comment

rules_ban_ip fails interactively and in tests with same error: You have requested a non-existent service "ban.ip_manager"

That failure is because you have to enable the core "ban" module in the test (or interactively) in order for the service to be defined. Just stick that in public static $modules. I didn't make that modification.

Also, what's the deal with testing in this queue? It behaves oddly compared to all other issue queues - is the testbot not configured to test patches for 8.x-3.x ?

Do I have to start the test manually? I tried doing that with the trivial patch in #2922828: Spelling. but if failed miserably - "PHP 5.3 & MySQL 5.5, D8.5 Unable to start phantomjs"

Also, the UserHasRoles::doEvaluate() function does not work, as noted in #2816157: Simplify UserHasRole::doEvaluate(). But with the changes in this current issue, we're controlling the input to doEvaluate() by presenting a select widget with the user role selections enumerated as rid => role_name. The $roles array passed to UserHasRole::doEvaluate() will like:
array ( 0 => 'authenticated', 1 => 'administrator', )
Note that this is key => rid

Then doEvaluate() tries to use the rid (e.g. 'authenticated') as an object, and calls $role->id() on it. That's just wrong. This is a legacy from the D7 code. This is why it sounds like we need a full Role object here and that upcasting is the solution, but it isn't

doEvaluate() just has to check if the selected rids are in an the $account->getRoles() array - no Role object needed.

UserHasRole::doEvaluate() does NOT need the Role object, and there is no need to upcast here, and the failure in that other issue ("Call to a member function id() on a non-object") is a logic failure.

Try this very simple fix with no upcasting required, after applying #18. I'm posting this separately because I don't know if this should be fixed here or in #2816157: Simplify UserHasRole::doEvaluate() after #18 (or something like it) is committed.

#18 failed because I didn't change the tests when I injected the services into the conditions and the actions. The tests don't know about the new constructor parameters yet.

I'll work on fixing these tomorrow.

From #18

move the doEvaluate() methods to the end of each controller class

That makes sense. Do it in a separate step, so that interdiff is clear.

enable the core "ban" module in the test (or interactively)

Thanks for the info and for raising #2922804: BanIP action causes PHP Exception. Maybe we can set that item to be disabled in the condition selection list if the module is not enabled. Then users can see it exists but cannot choose it.

From #19

what's the deal with testing in this queue?

Yeah, I noticed that patches are not automatically queued. This is a maintainer-controlled setting, and probably an oversight. But most testing is first done on Github and Travis, which I find very helpful, as you can test out all core versions and php versions before even submitting a patch here.

From #20
Thanks for this. We can continue the discussion on #2816157: Simplify UserHasRole::doEvaluate()

From #21
OK thanks. I will not make any more patch changes just yet, to allow you to make the test fixes.

OK, took me longer than a day to get back to this ... Made some minor changes to #18 now the tests pass locally. Let's see what the testbot says.

Note, I've made extensive use of this patch over the past week - I've been porting dozens of D7 rules conditions and actions to D8, and almost every single one of them needed a list_options_callback. So for my use case, D8 Rules is unusable without this patch or something like it.

Better, now one more time...

Thanks TR. this is really great. I have made just a couple of small changes to keep the 'use' statements in alphabetical order. I have also tested the 'Ban IP' test alongside your patch in #2922804-5: BanIP action causes PHP Exception and have made the changes to the test file and referenced that issue.

I have updated my github branch to cover all code changes to #25. The PR is still the same https://github.com/fago/rules/pull/496 but I have now squashed the commits into one for ease of compare and rebase. Setting this to RTBC just to see who shouts ;-)

Thanks for your work on this. This work has been blocked a long time due to https://www.drupal.org/node/2329937 being finished. The issue there remains a lot of valuable code though, which we could bring to the typed data module and leverage here. We need to evaluate possible ways forward here before move on with code - suggestions welcome!

OK. Adding #2329937: Allow definition objects to provide options as a related issue.

To follow up the testing question in #19 I have raised #2927534: More automated test runs on drupal.org which is a simple job for the maintainer but very helpful for us all when done ;-)

ok, I see those ways forward here:
a) Move allowed-options code to typed data module and work based upon that. Obviously that misses the metadata of core field definitions and the framework for getting that, but allows us to move on with custom options here at least.
b) Get the core issue done and require latest 8.5.x-dev of core for the feature to work. As we are in alpha state I think that would be do doable

I must say the code looks really good here and I'd love to just move on and commit this, however I think we should make sure this is compatible with the solution that is implemented in #2329937: Allow definition objects to provide options. I'd suggest adding a getOptionsProvider() method that matches the method in the WIP patch there, i.e. we'd need the intermediate wrapper class who could get the action plugin injected? The goal would be to have an $options object which has all dependencies set, such that we can pass it on to future typed data widget plugins which then can rely on the options results.

Hi guys, the patch in #10 works good. You guys rock putting this module to work.

I got a question. I need to assign a Role to the Commerce Order Owner. What should be the selector?

Greetins,

As I am also blocked by add user role issue, I reached here. After some search and some problems, in the end I reached a role solution by "set a data value". I didn't do any patch that mentioned above.

Data selector: node.uid.entity.roles
Value: -role system name- (example: administration)

So it would set the user's role as "administration" and erase all other roles as well. I didn't tried other roads but at least it is possible by set a data value for the role as I see. I just wanted to add for possible blocked guys for roles.

deleted

The patch from #25 needed a lot of re-rolling due to the many changes in Rules since it was posted. This new patch applies to the latest -dev and hopefully the modified tests will run without error on the testbot ...

I missed compensating for rules_ban in its own module and for the Condition: and Action: labels that were removed from the list builder. Let's see if that's all ...

There are still a few places where the ConfigureAndExecuteTest needs to be updated. Also there's some commented code in there which is throwing coding standards violations - that should be looked at an probably uncommented because I think it should work now.

However, these are just problems with the test - the code should work fine, so feel free to use it and post feedback.

@TR would you like me to investigate the failing tests, or are you doing this?

If you have time it would be great if you could work on the tests. I think there are also probably new conditions/actions that were added since this patch was last rolled, and those will need to be examined to see if they also need to add options providers and be included in the tests.

The reason I re-rolled this was so I can start testing it along with the typed data widgets patch to make sure they play well together, then I want to see if I can address @fago's comment in #32 because that impacts both of these. I want to get both of these issues committed before the next alpha because they both are major usability improvements.

There were five tests that failed (four separate cases in testAddConditions and the testMultipleInputContext). The tests now pass OK locally so should run on d.o. You were right that the actual code is OK and it was problems with the tests. The main reason is that before we had the assignment restrictor we were using the free-text entry and using dummy values, but now that some fields only have select lists we need to provide accurate values for the forms. You can see the slight changes to the test data values in the interdiff.

The testMultipleInputContext() failed becuase it was using the node_is_of_type condition which has the types field has 'multiple = TRUE'. However, this field is now a selection list (with this patch) so the values cannot be entered free-text. I changed the test to use action rules_send_email instead, because this also has a multiple field, the 'To' addresses, but unlike the node types, this is still a textarea with free text to enter each address.

In addition to fixing the five test failures, I took the oportunity to enhance the test file, making it easier to use/develop/test. The changes are:

1. Login at the end of setUp because every test here needs the login (removed the logins fom the start of each test)
2. Helper function createRule() now does an assertion to check that it worked ok and returns the $rule object
3. Removed confusion over the $label parameter in testAddConditions/testAddActions as this is not the label of the rule but a description
4. The real $label is created as a variable for re-use
5. Extra assertions after the second save to make sure the rule is saved without error
6. Explanation of the structure of the test data arrays
7. Create test data in $data array variable instead of directly returning the full array. This allows for manipulation before the final return. Avoids big chunks of commented out code
8. Fixed the two 'list' conditions tests - they pass ok now
9. Made the test data array keys consistent (and added explanation of this in the header doc block)
10. In the tests for Add Role and Remove Role, deleted 'authenticated' as this is not a strong test. Now derive the actual role name in the test
11. Made the same changes in AddActions as done above for AddConditions

This patch also includes the one-line change to add assignment_restriction = "selector" to src/Plugin/Condition/DataListCountIs.php as mentioned in #3094046-9: Set 'assignment_restriction' in Condition and RulesAction contexts. This is necessary for the tests to pass, but that change will be done in a separate issue, and ultimately will be removed from this patch.

That's irritating - the MultipleInput test passes locally but not on d.o. Reverting back to the long-hand way of creating the rule.

This patch did not code the bundle selection in the "Entity is of bundle" condition properly. Here is a new version of the patch with this fixed, and an interdiff to show the differences.

Note that the "Entity is of bundle" condition does work ... your problem is a result of this patch only. I generally suggest that you get something working first before you test new features with patches like this, that way you will know whether the problem is with the patch or not.

That should have been interdiff-46-48.txt. Here it is again.

The patch needed to be updated for the current HEAD of Rules, but it did still apply but one test needed to be adjusted for changes made in another issue.

Here's a new patch with that update, and an interdiff against #46. You can ignore the patch in #48.

Re-roll after #3159876-7: Set assignment restrictor for two list and data conditions

The updated tests failed due to #3045738-33: Convert entity manager service usage to split up services ( Part 2 ) when Drupal 8.8 becomes the lowest supported version but simple to fix. I also sorted the coding standards message.

And fix the coding standard.

Applied latest patch against dev and works as expected. RTBC +1

FYI: patch fails now due to latest dev updates. You guys are doing well the progress on Rules has been awesome!

Yeah, one of the three commits made in the two hours between #55 where the patch ran green and #57 where @gynster tested it seems to have broken this patch.

Those commits are:
#3101026: Replace deprecated path.alias_manager and AliasManager when Drupal 8.8 becomes the lowest supported version
#3115367: Add provider = "path_alias" to all path conditions and actions when Drupal 8.8 becomes the lowest supported version
#3160067: Refactor and expand ActionsFormTest and ConditionsFormTest

I didn't look at this too deeply, but one thing I noticed is that the 'path_alias' module will need to be enabled in the ConfigureAndExecuteTest in this patch. That shouldn't cause any test failures because it will really only be required for D9, but as I'm working on the deprecated path alias issues right now it's something I noticed.

Thanks @glynster for testing and for the +1 to RTBC. I never mind re-rolling patches and/or fixing things if the reason is that @TR has committed work which improves Rules. I will see what can be done here.

Fixing the tests turned out to be quite easy. It was the new tests added in #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest which failed, due to some random values I specified for the field values and role names. In this issue, because the fields become drop-down selection lists they automatically have validation built in. Also the test failure message gave very good hints as to what the field would accept.

The change involved creating a named role for use in the tests, as apart from 'authenticated' the only other role was a random name, and DrupalCreateUser() does not give any way to specify the role name it creates.

Re-rolled due to #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest

The action plugin SystemEmailToUsersOfRole should have a list_options_callback was defined to select the Roles, but this got missed before. So I have added it - and consequently had to fix the test case. In fact, it was the new tests passing which alerted me to the omission, as the specified role was not an actual existing one.

I also tightened up the test case data for the user selection, which now has 'data selector' not just the free input text.

@jonathan1055 works like a charm. All patches are working together at this time! Keep you the great work as rules is awesome!

That makes sense, because SystemEmailToUsersOfRole wasn't ported to D8 until July 2019, which was long after this patch was started. See #2409059: Port Send mail to all users of a role (mail_to_users_of_role) action to D8.

Which raises the question, are there any other 'new' Conditions/RulesActions which are missed by this patch? (I raised that in #43 as well). I'll take a look to see, but this could use another set of eyes as well.

I have been using #51 on my test site for almost two months now, and I have been using the various previous patches for probably a year, and I think what I've used is working well, but I'm a bit wary because of issues like #47 where it appears the options list for Entity is of Bundle had never been tested by anyone and it had never worked - this patch would have actually broken the Enitity is of Bundle condition had it been committed before that was fixed. That's why I think all the new tests @jonathan1055 has been writing have been very important and useful.

Really the only reason I have held back on committing this is because of #32 - It really would be nice to make this patch consistent with the proposed core change in #2329937: Allow definition objects to provide options. In the past, and hopefully in the future, Rules has been used as a prototype for functionality that gets put into core. The long-term goal should be to build missing core functionality which can be used in places other than Rules. It is hard to maintain code that extends core when core gradually diverges from our implementation - consider for example the core Entity context that is causing a problem with porting Rules to D9. We should be developing this patch with an eye towards the future - a common architecture for options providers is needed in core too, so let's build them the same way as we would for core. That will help get this functionality into core as well as help us offload a lot of our customized code into core which makes it far more maintainable. While this will require a re-working of the above patch, it should also simplify the code because we won't have to duplicate options callbacks for each plugin that uses them. For example, userRolesListOptions() is defined/duplicated three times in this patch. If that were an options provider for user roles, then it would be defined only once and reused. Then, if we ever have to fix a bug in this function, it will only have to be fixed in one place and there will be no chance that three different implementations diverge from each other.

Thanks TR. I am pleased you've been using this and found it basiaclly working. It's good that the new tests are proving worthwhile.

are there any other 'new' Conditions/RulesActions which are missed by this patch?

I have checked all the conditions and actions and there are some which do not have lookups/selection but possibly should

Oeration and operator are easy to add, just a hardcoded list of the values that the context will accept.
Data covert Target type and Variable Add type should be easy, just a choice of string/integer/float/boolean etc..
field_name can re-use the existing functionalty
rounding_behavior and pos are simple hard-coded selection values
Language is a new one, we've not used that before but it could make sense to limit this to just the languages enabeld on the site.
email_type is this a fixed list, or is it dynamic?

It really would be nice to make this patch consistent with the proposed core change

Yes I agree, for all the reasons you give.

we won't have to duplicate options callbacks for each plugin that uses them.

Yes I also noticed that, and was wondering how to avoid it. I will read up on how the proposed solution works in #2329937: Allow definition objects to provide options I see you have been involved in that issue, so any pointers or starters for the re-work would be helpful as I have not read any of that yet.

Regarding the tests in this issue, they are also useful in other issues such as
#2471481: Integrate Typed Data Widgets
#2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields
#2800749: Support upcasting entity IDs to full entity contexts
I think it would be beneficial to get the new and updated tests committed sooner, rather than leave them in this patch. That would mean I could work on any of those issues and have the new tests available instead of having to copy them from branch to branch, or add them in more than one patch. I will prepare a patch here which gets all the relevent test additions together but without any functional code changes so we can see what it looks like.

Before starting the re-work discussed in #32 and #65 I want to get the tests sorted out. I realised after my work in #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest that we have most of that test coverage duplicated in the patches on this issue. So I have worked a lot on ActionsFormTest.php and ConditionsFormTest.php to incorporate the few elements of test coverage which had been added in this issue, so that we can drop completely the two new tests 'testAddActions' and 'testAddConditions' from ConfigureAndExecuteTest.php in the patches on this issue.

Here is a full patch, same as #63 except for changes to three test files. I've included an interdoiff. I can explain more about the changes if required.

There may be one test failure in testMultipleInputContext, as commented in the code, but I want to try it on drupal.org, as this passes locally and on Travis.

OK, so the failure still happens (just wondered if other things might have fixed it).

Drupal\Tests\rules\Functional\ConfigureAndExecuteTest
1) Drupal\Tests\rules\Functional\ConfigureAndExecuteTest::testMultipleInputContext
null does not match expected type "array".
/var/www/html/modules/contrib/rules/tests/src/Functional/ConfigureAndExecuteTest.php:375

The code in question is:

    // Need to get the $rule again, as the existing $rule does not have the
    // changes added above and $rule->get('expression.actions...) is empty.
    // @todo Is there a way to refersh $rule and not have to get it again?
    $config_factory = $this->container->get('config.factory');
    $config_factory->clearStaticCache();
    $rule = $config_factory->get('rules.reaction.test_rule');

    $to = $rule->get('expression.actions.actions.0.context_values.to');
    $this->assertEquals($expected_config_value, $to);   <<< this is line 375

$expected_config_value is a hard-coded array, so the message means that $to = $rule->get('expression.actions.actions.0.context_values.to') has returned NULL not an array. Reverting back to the long-hand way just for that test (the others work fine)

Patch #70 is just the changes to the three test files, so this is being tested again the existing codebase, with no changes. This should pass, and I propose that we get this committed, as the expanded test coverage will be useful in other issues such as:
#2471481: Integrate Typed Data Widgets
#2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields
#2800749: Support upcasting entity IDs to full entity contexts
#3010055: Multiple Event Triggers
All of these involve the actions and conditions forms, and all have had lots of work done. Getting the tests committed (and therefore off this patch) will be a useful step forward.

After studying the work-in-progress in #2329937: Allow definition objects to provide options and in particular @fago's comments starting with #11 and using the final patch #61 I have worked on how to implement this in Rules, and I now have an operatioal version for our conditons and actions, using the OptionsProvider class and the functions in Drupal\Core\TypedData\OptionsProviderInterface. It is all working well with the core patch #61 applied.

I have made the necessary changes to Rules' own ContextDefinition (which is already extending Core ContextDefinition) and which won't be required if/when the core patch is committed.

However, when I try it on plain Core 8.9 (without the patch) it obviously fails, because there are necesary changes to Drupal\Core\TypedData\TypedDataManager. I am trying to work out the best way to resolve this, as Rules does not have its own TypedDataManager extension of the core version.

I will post a patch here, with work done so far, but until I resolve the TypedDataManager changes it wont work on d.o. testing.

Here's a patch with the new OptionsProvider work, including a drupalci.yml file to attempt to patch core with patch #61 so that the tests run OK.

I have made some decisions, just to get this started, but there are things that we'll need to discuss

1. The new Options Provider files are in src/Plugin/OptionsProvider but this may not be the right place
2. The class is \Drupal\rules\Plugin\OptionsProvider\{name}Options but this can be discussed
3. I've named re-useable options to indicate what they give, eg AndOrOptions instead of previously named for where they are used such as RepeatListOptions

There will be plenty more to discuss. But let's see how the patching of core does.

Forgot the interdiff

Actually patch #72 includes more than is necessary, as it had some of my WIP to make this run without the core patch. Here is the reduced version.

Hi jonathan. And thanks for dedicating so much of your time on this. But it's moving so fast that I can't follow.

I have applied patch #74 from here and the core patch #61 in #2329937: Allow definition objects to provide options

I am now trying to do a rule to give a specific role to the current user. I can see the list fine in the rules administration. But when my rules is triggered, I got an error:
Call to a member function id() on string in Drupal\rules\Plugin\RulesAction\UserRoleAdd->doExecute() (line 55 of modules\contrib\rules\src\Plugin\RulesAction\UserRoleAdd.php)

What am I missing?

Hi vdsh,
Thanks for testing, and good to hear that the selection lists are working for you.

Call to a member function id() on string is another well-known issue. Please see
#2725525: Fatal error: Call to a member function id() on a non-object in UserRoleAdd.php on line 49 and
#2824360: Error "Call to a member function id() on string" for condition 'user has role(s)'

Both of these will be solved by the patch on #2800749-48: Support upcasting entity IDs to full entity contexts which you can try.

Thanks jonathan, indeed with the upcasting patch, it is all working as expected. Thanks for your help

Straight re-roll of #74 due to #3168040: Query string in AddressEquals (D9.1 test failure)

Today if apply #78 to current rules dev, i cannot edit any condition or action in rules.

In Recent log:
Message Error: Call to undefined method Drupal\rules\Context\ContextDefinition::getOptionsProvider() in Drupal\rules\Form\Expression\ConditionForm->form() (line 115 of /var/www/html/web/modules/contrib/rules/src/Form/Expression/ConditionForm.php)

Hi VasyOK,
The function getOptionsProvider() is new and comes from the core issue #2329937-61: Allow definition objects to provide options patch #61. This was mentioned in the comments above but I agree that unless the reader has been following this issue closely that would not have been obvious. So I have updated the issue summary here to explain what is needed.

When you have applied that core patch (if you can) let me know if the conditions and actions selection lists work for you. Thanks.

I had already worked on some extra optionsProviders and your post above prompted me to add the patch here. There are nine additional options providers, covering all the text and numeric data comparisions we cater for, the data conversion types, rounding option, list position, system email type and system language options. This now covers every action and condition that could have selection lists, apart the 'type' choice in VariableAdd rules_variable_add because this probably should be derived from the current system/environment (and that is a bigger task, suitable for follow-up work).

This patch still needs this core patch #61, which applies OK in core 8.8, 8.9, 9.0 and 9.1

Patch updated for latest dev

Error #82 patch with last dev.

Thanks ConradFlashback. Here's a re-rolled the patch following the commit on #2694553: Description wrong: "content is of type" should be "Entity is of bundle"

Tested the last patch on SystemMail action, noticed and fixed the following issues/errors.
- Fixed error on rounding behavior during data conversion.
- Added support for data conversion of lists
- Fixed error "Warning: preg_match_all() expects parameter 2 to be string" during token replacement

Fixing a typo in the last patch

Added missing files added in previous patch

patch #89 returns this error when trying to add an action to set a data value. Using D9.2.6
Error: Call to undefined method Drupal\rules\Context\ContextDefinition::getOptionsProvider() in Drupal\rules\Form\Expression\ActionForm->form() (line 104 of /home/projects/public_html/web/modules/contrib/rules/src/Form/Expression/ActionForm.php)

The patch currently under consideration is #84.

What is #85 supposed to be? There is no interdiff, so the only way to see the changes between this and #84 is to compare them line-by-line. But if the text of the comment is any indication, this new patch adds a number of out-of-scope changes to #84 without explaining them.

Likewise, the latest iteration #89 doesn't even apply.

Please provide an interdiff against #84, along with an explanation of why the changes were made. Also, please fix the patch so it applies and passes the tests.

Sorry, I have been away for few days and will add the interdiff very soon, I tried to explain the changes in #85 but I will try and elaborate it.

Tested the last patch on SystemMail action, noticed and fixed the following issues/errors.
- Fixed error on rounding behavior during data conversion.
- Added support for data conversion of lists
- Fixed error "Warning: preg_match_all() expects parameter 2 to be string" during token replacement

I attempted to apply patch in #84 to the latest version of the 8.x-3.x-dev (commit 615221dd) and it failed. I'm not sure exactly why yet - that's a really long patch file!

nevermind, my bad, I had another patch applied and #84 conflicted with the changes from the other patch. It does apply cleanly on 615221dd.

Patch #95 is a re-roll of #84 due to recent commits. No functional changes. This uses core patch 65 from #2329937: Allow definition objects to provide options which is applicable to core 9.1 - the default for Rules testing on drupal.org.

[To test here at 9.2 the core patch would be 69, and to test at core 9.3 the core patch would be 72]

Investigating these six new test failures

1x Drupal\Tests\rules\Functional\ConfigureAndExecuteTest::testAssignmentRestriction
2x Drupal\Tests\rules\Functional\ConditionsFormTest::testConditionsFormWidgets
3x Drupal\Tests\rules\Functional\ActionsFormTest::testActionsFormWidgets

The test output on drupal.org is only indirectly helpful, the actual cause of the faults is a repsonse code 500, and testing locally I see the exception

The object must either implement FieldStorageDefinitionInterface of FieldDefinitionInterace
in Drupal\Core\TypedData\ListDataDefinition->getRelatedFieldStorageDefinition()
(line 115 of core/lib/Drupal/Core/Field/TypedData/FieldDefinitionOptionsProviderTrait.php).

This is a new exception, created in the new Drupal\Core\Field\TypedData\FieldDefinitionOptionsProviderTrait.

In the ConditionsFormWidgets test, all 20 conditions are tested and only 2 fail. In ActionsFormWidgets all 36 actions are tested but only 3 fail.

The problem is nothing to do with recent Rules commits, it is the change in the core patch from 61 to 65 on #2329937: Allow definition objects to provide options that has caused it. That change, shown in interdiff_61-65.txt removed FieldDefinitionOptionsProviderTrait from Drupal\Core\Field\BaseFieldDefinition.php and put it into Drupal\Core\TypedData\ListDataDefinition.php. This fixed one of the core test failures

Declaration of Drupal\Core\TypedData\DataDefinition::getOptionsProvider($data = NULL) must be compatible with Drupal\Core\Field\FieldDefinitionInterface::getOptionsProvider($property_name = NULL, ?Drupal\Core\Entity\FieldableEntityInterface $entity = NULL, $delta = 0)
in /var/www/html/core/lib/Drupal/Core/Field/FieldDefinition.php on line 261

so it may be the correct change.

The Rules problems occur only with the actions rules_email_to_users_of_role, rules_user_role_add and rules_user_role_remove, and the conditions rules_node_is_of_type and rules_user_has_role. None of the other actions and conditions have this problem. The difference with these is they have multiple=TRUE. Removing this parameter removes the error and the problematic conditions and actions can be edited.

That obviously is not a solution but at least we now know what the problem is.

@jonathan1055: What do you think about committing this in stages? I think the OptionsProvider plugins (1 commit) and the changes to the Condition/Action annotation (1 commit) are ready to go in right away without affecting any other code - the new annotation and the new plugins will be ignored without the code that uses them. That's the bulk of the patch anyway, aside from the tests. We can use a new issue for each of those commits in order to separate out the description of what the commits do so we can track these changes in the future. It will also make a good reference for documenting this feature so that contrib can start to use it.

Once those are committed, I think it will be easier to get this finished. For me, at least.

I notice there is some code in the patch that injects new services into some conditions (EntityHasField, EntityIsOfBundle, EntityIsOfType, NodeIsOfType) - I believe that is no longer needed now that the options are in a plugin rather than as methods on those conditions.

What do you think about committing this in stages?

I think that is a very good idea. It will reduce the patch, save re-rolls and make it easier to work on.

Here's a patch which removes the injected services from those four actions. I also have some improvements to the tests, for better analysis following the failures in #95 but I will add those to a subsequent patch, not to confuse the work with removing the injected services.

OK we have the same 6 test failures as expected. Patch #102 makes two changes to the tests:

1. ActionsFormTest and ConditionsFormTest now have a sequential number added into the test data key. This has two benefits (a) it makes it easier to identify which test case has failed and shows how many others passed, and (b) makes it easier to filter and run just one or two of the full set of test cases locally without editing the file. For example phpunit modules/rules --filter='ActionsForm.*(32|33)' will run just test cases 32 and 33 from the ActionsForm test.
2. ConfigureAndExecuteTest::testAssignmentRestriction() was unnecessarily using two conditions and two actions when it can have the same test coverage with one of each. This removes the use of the currently failing rules_node_is_of_type condition. The failure is nothing to do with assignment testing, and this condition is tested to the same extent in ConditionsFormTest, therefore no loss of test coverage results from simplifying this test.

There should be just 5 failures now, only in ActionsFormTest and ConditionsFormTest

As expected we have five test failures, and these are for the options providers which are used with multiple=true, as explained earlier. The failures are now response code 500, which shows that some kind of exception prevented the page from loading.

The ConditionsForm failures are:

11. Node is of type
18. User has role

and the ActionsForm failures are

25. Email to users of role
32. User role add
33. User role remove

Here's a patch which removes the injected services from those four actions.

The create() methods and the ContainerInterface and ContainerFactoryPluginInterface need to go, too.

ActionsFormTest and ConditionsFormTest now have a sequential number added into the test data key.

We can commit that part right away, if you put it in a separate issue.

Here are the artifacts for the test fails in #102:
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/90786/artifa...
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/90786/artifa...

ActionsFormTest and ConditionsFormTest now have a sequential number added into the test data key.

We can commit that part right away, if you put it in a separate issue.

I extracted this part out of #102 and put it into a child issue #3254675: Improve ActionsFormTest and ConditionFormTest

The patch in the core issue #2329937: Allow definition objects to provide options makes a lot of changes to core Fields that aren't needed in order for Rules to use options providers.

I *think* the only part of that patch that might be needed for our purposes are the changes to the typed data manager?

If that's the case, can't we just extract those changes to the typed data manager and put them into the Typed Data API module for use by Rules? That way we can get this issue working without requiring a core patch. The way to do this would be to override the typed data manager service with our own typed data manager class that subclasses core in includes the changes from the core patch.

Between #3254620: Add new OptionsProvider files #3254624: Add options_provider annotations to conditons and actions and #3254675: Improve ActionsFormTest and ConditionFormTest, it looks like about 2000 lines of this patch has now been committed, leaving us with less than 500 lines. That's going to be a lot easier to manage!

Yes, and I have just rasied #3255074: Improve ConfigureAndExecute test which will reduce the changes here even further, and give us better test coverage. When that is done it will leave just four files in this issue:

 src/Context/ContextDefinition.php     |  1 +
 src/Context/Form/ContextFormTrait.php | 38 ++++++++++++++++++++++++++++----------
 src/Form/Expression/ActionForm.php    |  3 +++
 src/Form/Expression/ConditionForm.php |  3 +++
4 files changed, 35 insertions(+), 10 deletions(-)

New patch #110 now only changes four Rules files (plus the drupalci.yml file to apply the core patch). There are no changes from patch 102 for these files. The entire patch is 148 lines compared to 2,470 in patch #102.

This patch will still fail for the actions and conditions which have an options_provider specified and also have multiple=true

The next step would be to try out TR's ideas in #107 and create an issue and patch for Typed Data API which we could use here instead of the core patch.

In additon to the known failures, we have a new one:

rules\Unit\Integration\Engine\LoopTest::testPropertyPathList
Prophecy\Exception\Call\UnexpectedCallException: Unexpected method call on Double\Drupal\Core\Entity\TypedData\EntityDataDefinition\P12:
  - setOptionsProviderDefinition(
        null
    )

This is not caused by the change to test at Core 9.2 compared to 9.1 in the previous patch, as it fails in Core 8.9. So why the extra failure?

The same test failure occurs on the main 8.x-3.x Rules branch following the commits on #3254620: Add new OptionsProvider files and #3254620: Add new OptionsProvider files when the core branch also has the appropriate patch from #2329937: Allow definition objects to provide options. Hence the failure can be seen even without the patches on this issue.

The next step would be to try out TR's ideas in #107 and create an issue and patch for Typed Data API which we could use here instead of the core patch.

I already did this work - one minor problem to fix first before I post it, but I won't be able to get to that today. I'll be posting an issue in the typed_data issue queue and linking to that here.

So why the extra failure?

Sounds like it's related to this, from the LoopTest:

    // We cannot use EntityDataDefinitionInterface here because the context
    // system in core violates the interface and relies on the actual class.
    // @see https://www.drupal.org/node/2660216

I found the cause of the extra test failure. A change to getContextDefinitionFor() in
tests/src/Unit/Integration/RulesEntityIntegrationTestBase.php which was in patch 102 had got missed when I created patch 110. I must have lost it, due to all the other test file changes, action and conditions, and providers being done in other issues. I have now double-checked and nothing else has got dropped by mistake.

The next step would be to try out TR's ideas in #107 and create an issue and patch for Typed Data API which we could use here instead of the core patch.

I already did this work - one minor problem to fix first before I post it, but I won't be able to get to that today.

The "minor problem" turned into a major problem, to the extent that I don't think this strategy is feasible. The typed data code is just too interdependent on the context code and the whole data definition hierarchy, so while decorating the typed data manager works there are too many places where context and data definitions are obtained outside of the typed data manager that can't be controlled without hacking core.

So we're back to needing to get the core issue finished.

Note that the test failures also show up interactively. For example, try the "Node is of type" condition and try to choose the type, and you'll see:
Uncaught PHP Exception LogicException: "The "Content types" object (class \\Drupal\\Core\\TypedData\\Plugin\\DataType\\ItemList) must either implement FieldDefinitionInterface or FieldStorageDefinitionInterface." at /var/www/example.com/public_html/core/lib/Drupal/Core/Field/TypedData/FieldDefinitionOptionsProviderTrait.php line 115

I'm actually getting that logic exception for almost every single condition/action that has an options provider.

I'm using 2329937-75.options-provider.patch with core Drupal 9.3.x

#115 no longer applied to the newest dev, so here is a re-roll. No interdiff needed since nothing was changed.

I'm not getting any Logic Exceptions with the latest core patch, but I'm getting alot of InvalidArgumentExceptions when choosing the wrong options.

Just wanted to note this patch conflicts with Integrate Typed Data Widgets

patch 118 crashes with 3.x-dev 9595fca

Call to undefined method Drupal\rules\Context\ContextDefinition::getOptionsProvider()

NVM, I see I need a core patch from:
#2329937-91: Allow definition objects to provide options
, thanks for the work on this! :)

The latest patch is excellent, thank you!

As I'm not getting any select list in rules module version 4.0.x-dev , I have applied changes in patch #118.
Please review.