Support upcasting entity IDs to full entity contexts

I'll take this up if you can give me a little more to go one. For example, where are we looking to make these changes? What type of tests need to be created?

This is still happening, when adding a "add user role" action, adding a "entity has field" condition...and there could be many more. Any progress with this issue or any pointers on how to get this rolling.

Regarding #2 I have written basic 'Add Condition' and 'Add Action' UI functional tests in #2664280: Select lists in action & condition configuration forms, so these could be expanded to cover what is needed here.

... write a test that takes some entity IDs as a context value and verifies that the plugin gets entity objects passed

But isn't that the current problem? The context value is passed as a string or array of strings. Or are you saying you'd like a new test which asserts the behavior we want and that test will currently fail?

Corrected the <li> tags in isue summary.

Here is a first attempt at generic upcasting. It is not complete, and a couple of existing tests fail, but I want to share what I have started so that others can test it and give feedback on my approach. The general idea is that the text string id remains stored in the contex config as-is now (I did start off trying to store the object in config and when that was done got errors about invalid config types and that seemed like the wrong path to follow). So, the principle is that for each action or condition which might need an id to be upcast into an entity, you can define a callback function which takes the id and returns the object. This is flexible and generic because each action or condition can define their own. The name of this (optional) callback is given in the context definition using upcast_callback = "function-name" and this function needs to be defined within the action or condition class.

During runtime execution of the action or condition, in execute() instead of doing $args[$name] = $this->getContextValue($name) for each context definition, the code now determine if an upcast callback function is defined and if the value (or array of values) is string type then the upcast function is used to get the entity. If the context value is already object then this upcasting is not done.

I have implemented this for one condition 'user has role' and one action 'user add role' and these now work for me in interactive testing, and for the existing condition and action tests. I have not yet written new tests (as requested by fago in #2) but that will be next, provided that I am on the right track with this solution.

The patch contains commented-out debug, so there will be lots of coding standards errors. There may be places where the code is verbose and can be made efficient, but as this is a kind of proof-of-concept I have left that for later. Also the test Unit\Integration\Action\EntityPathAliasCreateTest fails with Call to undefined method Drupal\Core\Plugin\Context\ContextDefinition::getUpcastCallback() and I have not yet worked out why.

So, lets see what happens on d.o. testing, and I would appreciate any feedback. If my approach is totally wrong, let me know and we can work out a better way to do it.

Jonathan

Ah, the patch in #8 was made on top of other changes. Here's a clean one.

Just a thought ... Drupal\Core\ParamConverter\EntityConverter does this for route parameters. If you look at the body of that code, you'll see it deals with things like entity translations when upconverting. Is there a need to create a whole new mechanism, and require actions/conditions to define their own upcasting functions for their own entity types? I'm pretty sure most will miss the translation bit ... Entity upcasting is not really something that needs to be customized, is it? If so, the paramconverter service will let you register your own converter for a specific type.

Perhaps we can delegate the conversion to EntityConverter? Wrap it in a Rules-specific class to hide the route-specific stuff.

And if you don't think we can/should make use of EntityConverter directly, then certainly the design of that mechanism applies here and we can do something similar.

Is this something that can go in the Typed Data module. A DataUpcaster service?

Added related issue.

Since 3.0.0-alpha5 is published patch #9 doesn't work. This is the new version of it.

Thanks for the re-roll, but as @fago said in #12 the current patch is not the way we want to be doing this.

From #6:

Or are you saying you'd like a new test which asserts the behavior we want and that test will currently fail?

Yes, exactly, I think that would be really useful here. Other test will be needed and will depend on the actual implementation we settle on, but the minimum is that it works to properly fix the issues we know about, namely with Roles. So having that 1 test case which demonstrates the current problem will help.

I'll take a crack at writing that test. I also have a preliminary implementation for upconverting based on my thought from #10 of using ParamConverters - I'll post that in the next few days. It is a bit of a hack to re-purpose ParamConverters for our needs, but there's really quite a lot of code there and quite a lot of flexibility in how those are implemented, with almost no extra code needed in Rules. My implementation to upcast any entity using ParamConverters is only 15 lines of code(!) so I don't feel THAT bad about this - the core EntityConverter and the plugin definitions and annotations and services involved are easily 100 times that amount of code, so that saves us a LOT and I think it gives us more functionality that we would have otherwise (it deals with translations and revisions, for example ...).

Here is the stand-alone example script I mentioned above. I am still investigating where we can put this code, but I haven't had time to work on it for the past week or so. This script demonstrates how to use the paramconverter service to upcast any entity, given the entity id and datatype.

This does not rely on additional context parameters or require additional functions in each condition/action that wants to do upcasting. I am looking to make this a data processor or in the context processor or something along those lines where the upcasting can be done in a
transparent and extensible manner.

Looking for feedback about this approach and for suggestions about where to put this. And of course if you'd like to write a patch using this that would be great too.

This looks very promising. Thanks for getting it moving with this demo script. I will also try to work out where we should fit it in.

Patch in #14 fails against latest dev:

10.upcast.patch
patching file src/Context/ContextDefinition.php
patching file src/Context/ContextDefinitionInterface.php
patching file src/Core/RulesActionBase.php
Hunk #1 succeeded at 134 (offset 13 lines).
patching file src/Core/RulesConditionBase.php
patching file src/Plugin/Condition/UserHasRole.php
Hunk #2 FAILED at 21.
Hunk #3 succeeded at 71 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file src/Plugin/Condition/UserHasRole.php.rej
patching file src/Plugin/RulesAction/UserRoleAdd.php
Hunk #2 FAILED at 21.
Hunk #3 succeeded at 78 (offset 2 lines).
1 out of 3 hunks FAILED -- saving rejects to file src/Plugin/RulesAction/UserRoleAdd.php.rej

Hi Nchase
Please see TR's comment in #17

Thanks for the re-roll, but as @fago said in #12 the current patch is not the way we want to be doing this.

There is no patch that you can apply directly yet. The solution will involve implementing the paramconverter work started by TR in #19

Hello,

I made sample working patch for Rules using upcastEntityId() function from #19.

Patch in #23 seems to be working for me.

No, #23 is not acceptable as it just hacks the one SystemMailToUsersOfRole action by explicitly upcasting and does not attempt to fix the problem in a general way that applies to ALL actions and conditions by passing them the object they need without altering those actions and conditions.

Fair enough. Just wanted to report that it appeared effective in at least getting past the error.

Sorry all, just devouring hours of threads around this issue and want to confirm that Rules is not an option--as of right now--if I want to:

• reaction=send an email when
• condition=user with a specific role
• event=saves a new item of content

Is there a way to use Rules for this usecase, or no?

I get to here:

USER
@user.current_user_context:current_user.roles

ROLES
(nothing makes sense in the dataselector, and no role machine name values are accepted via direct input)

I have patch #10 from https://www.drupal.org/project/rules/issues/2816157

Not sure if I'm in the middle of this most unfortunate stumbling block, or if I just don't know how to use the module.

This is my use case and I have it working with the following patches:
rules-2664280-46.select-lists
rules-user-hasroles-2816157-10

Those being 'interim' solutions while we don't have this upcasting improvement.

You you can link directly to comments to find the patches more easily
#2816157-10: Simplify UserHasRole::doEvaluate()
#2664280-46: Select lists in action & condition configuration forms

The user value is @user.current_user_context:current_user. To get this, swicth to Data Selection and start typing.

the patch in #23 didn't work for me. Basically nothing changed in the UI.

@Nchase I'm not surprised, read TR's comment in #25.

We need to take the idea from #19 and make it work in a generic way for Rules. That's not been done yet.

Here is a fully functional upcasting attempt, using TR's excellent research on using \Drupal::service('paramconverter_manager'). I have not written any new tests for this yet, but can do. Also there is a bit duplication in the new loop in the execute function of RulesActionBase and RulesConditionBase, so this could be made into a separate process if necessary.
Another thing to note is that I just checked for the $type to start with the string 'entity' and this works so far, but may not be the best way to do it. Also if the value passed is already an object it just gets returned. This could be changed.

So, this works intereactively with condition "User has role" and action "User remove role", and some of the phpunit tests run ok, but I've not tested the full suite locally.

Updated issue summary

tested the patch in #37
while removing a user role works, adding doesn't work:

TypeError: Argument 1 passed to Drupal\rules\Plugin\RulesAction\UserRoleAdd::doExecute() must implement interface Drupal\user\UserInterface, null given in Drupal\rules\Plugin\RulesAction\UserRoleAdd->doExecute() (line 51 of modules/contrib/rules/src/Plugin/RulesAction/UserRoleAdd.php).

Tested the patch in #37

Neither adding a role or removing a role works for me. Very simple rule:
After saving a new user of type User
Action:
Data Selector: user
Role: Name of Role
No error message and when creating the new user role is not set.

After updating a user of type User:
Data Selector: user
Role: Name of Role
No error message and the user role is not removed.

Hi @Nchase and @avostar,
Thanks for your feedback. I have tested both UserRoleAdd and UserRoleDelete and they are working fine for me, in both D8.8 and 8.9.

Note that when specifying the roles you need to enter the internal machine name/id, not the label, and if entering more than one then they should be on separate lines in the textare (there is another issue dealing with converting this input into checkboxes for ease of use)

Please can you list the precise steps to repoduce the error, starting with a clean install of Drupal.

Thanks @jonathan1055 for your feedback. I can narrow it down to the input method of the action.
If the input is on "direct input" then it throws an error. If the input is set to "data selection" everything is fine.

Thanks for the info. After adding #2804941: Implement assignment restriction in plugin context we set the assignment restrictors on #3094046: Set 'assignment_restriction' in Condition and RulesAction contexts but I can see that UserRoleAdd and UserRoleRemove did got get any restriction. Maybe they should be restricted to 'data selection' only? Are there any scenarios where you would not want to use data selection for a user entity?

"direct input" is always welcome if you know what you want to use. Like for example: I created first the "remove user role" and then the "add user role" rule, and because their is now "duplicate rule"-function I went for the direct input and just put "user" into it as I assumed that there will be no difference between "direct input" and "data selection"...

AND the real problem is that when someone adds the "add user role" they will get the "direct input" first and then they can switch to the "data selection".

From an UX perspective we could remove the "direct input" and present the user with the "data selection" right away.

From an UX perspective we could remove the "direct input" and present the user with the "data selection" right away.

That is exactly what the assignment restrictor does. There is no button, and input has to be by selection.

tested patch in #36 with clean 8.9.1 install and rules 8.x-3.x-dev

works here both adding and removing user role.
but only once, after user had his new role added and removed its not possible to add another time, you have to clear cache first, then it works once again.

Hi idot,
Thanks for testing. Pleased to hear that it works in principle.

I checked and cannot reproduce the behavior you are seeing. I have a rule which adds a role when saving content with a given title, and another rule that removes the role when the title is something else. This works continually, I can repeat the add/remove process many times. Maybe in my case a specific cache is being cleared when I save. What conditions are you using to trigger the actions? Is the role being added to the current logged-in user?

Hi @jonathan1055,

First thank you so much for your hard work on this! Comment #41 did the trick for me. Once I selected data selection and set it to user in the action on the rule to add a user role it worked like a charm!

Thanks avostar.

I think we should restrict to 'selector' for both of these actions. Patch #48 is simply patch #36 with the two assignment restrictions added.

After implementing the latest patch, I was able to get my "send email to all users of a role" action working.

However, I still could not get "send email" to the author of a node working. I used the data selector node_unchanged.uid.entity and got this error upon trying to save the action:
"Expected a entity:user data type for context User but got a entity_reference data type instead."

I had to continue using the 'add variable' workaround as per the last example of this issue: https://www.drupal.org/project/rules/issues/2978800

Hi TLHenriksen,
Thanks for testing, and good to hear that "Send email to all user of role" is working.

For the general Send email action, for 'send to' you need to pick node(or node_unchanged).uid.entity.mail.value as that will be the actual e-mail address of the author. However currently this does not work either - see #2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields

Work is progressing on these, but it is slow going. So we appreciate your help in testing.

Jonathan

Ah, I have just realised you were using "User - Send account email" which requires a user not an e-mail address. Yes, I get that error too. I've been meaning to report it as a task to fix.

Or maybe we can tackle that here, along with upcasting? It is kind-of a similar task.

@TLHenriksen: There seems to be an existing problem with entity references, as has been reported in several issues:
https://www.drupal.org/project/issues/rules?text=for+context+but+got+a+e...

I haven't had the time to look through all these issues and consolidate them into something we can work on. The resolving of entity references into actual entities is something that (according to comments in the typed_data code and elsewhere) should currently be working, so the first step is to figure out from these issues what it is that is broken and how to simply reproduce the problem. A test case which uses entity references and demonstrates a failure of something we expect to work is also necessary here, that way when we develop a fix we can be sure it works and stays working.

I don't think that casting from entity reference to entity should be part of this issue. But if you used, for example, node_unchanged.uid.entity.uid instead of node_unchanged.uid.entity then this issue should be be able to upcast the uid to the User entity.

I tried as TR suggested node_unchanged.uid.entity.uid but this fails with
"Expected a entity:user data type for context User but got a list data type instead". I also tried node_unchanged.uid.entity.uid and which gives "Expected ... but got a integer data type instead", as does node.uid.target_id

So I think we need to look at these cases and probably use one of the issues TR linked to, for the work and testing:
#2965726: Entity context fields fails on validation on update for entity reference
#3059402: Set data action in referenced entity.
#3088265: How to access a comment's parent-entity bundle-specific fields?
#3121906: Node reference entity cannot acquired from "Entity is of type" or "Entity is of Bundle"

For this issue, I think I will work on some tests to show us which actions and conditions can now use upcasting, so we get a proper idea of what this patch can (and can't) achieve.

Thanks for the updates. FWIW, the workaround still didn't work - my custom variable always comes up empty.

Here's what I have set up:

• Event: Saving a node of type MyType
• Conditions:
  • Node is unpublished: node_unchanged
  • Node is published: node
• Actions:
  • Add variable: type entity:user, value node.uid.entity, alias author
  • Send email: to author.mail
  • Add role: my_role to author

The User inputs expect an entity:user... node.uid and node.uid.entity.uid are rejected because they are 'list' data types; node.uid.entity is rejected as an 'entity reference' data type. The mail expects a list... node.uid.entity.mail.value is rejected as a 'mail' data type. The above setup is the only way it will actually save for me, but in action, author turns up empty and both the 'Send Email' and 'Add Role' actions fail.

One exception: I can save node.uid.entity.mail as the recipient for Send Email action, but then I get this fun error upon triggering the action:

TypeError: Argument 1 passed to Drupal\rules\Plugin\RulesAction\SystemSendEmail::doExecute() must be of the type array, object given in Drupal\rules\Plugin\RulesAction\SystemSendEmail->doExecute() (line 111 of modules/contrib/rules/src/Plugin/RulesAction/SystemSendEmail.php).

Sorry I can't be of help to contrib, I can only share the issues I'm getting. :(

Sorry I can't be of help to contrib, I can only share the issues I'm getting

No, you are being a great help by testing patches and providing real case steps-to-reproduce, so thank you. I'll re-create your rule and check out what is happening.

Just requeued patch 48 for testing, as I think it will fail now due to #3160067: Refactor and expand ActionsFormTest and ConditionsFormTest. The correction is easy, which I will post next.

As expected, the tests now fail, which is good. Now that more is being done in those expanded tests, the data selection fails validation.

New patch and interdif.

I have replicated the rule in #54 and confirm I get exactly the same errors and validation problems. However, none of this rule is actually using the new upcast functionality in the patch being tested. These problems all happen before that gets a chance. As TR said in #52 these are not part of this issue and could be followed up on one of the issues I listed in #53.

This patch does solve the 'user has role' condition and the 'user add/remove role' actions. We need to find all examples where this form of upcasting needs to be used, and then I'll write a test to cover them.

@jonathan1055 the latest patch will not apply if you have https://www.drupal.org/files/issues/2020-08-05/2664280-74.select-lists.p.... It will continue to fail. At the moment you can only use one or the other!

I used to run all these and it worked:

"Allow single-valued data selector": "https://www.drupal.org/files/issues/2020-07-21/2723259-20.single-value-selector-to-array.patch",
"Select lists": "https://www.drupal.org/files/issues/2020-08-05/2664280-74.select-lists.patch",
"Helps with a dropdown for roles": "https://www.drupal.org/files/issues/2020-08-08/2800749-58.upcast.patch"

Hi glynster,

Yes these three issues are eventually bound to clash. It was only the change to ActionsFormTest.php in patch #58 which clashed with the select-list patch, so here is patch #61 without that change. This is not for testing on drupal.org, because the test will fail, but you can use this patch instead of patch #58 and you will be able to have all three applied again.

Note due to another commit I have had to re-roll the select-list patch, it is now
/files/issues/2020-09-16/2664280-78.select-lists.patch

It is good to hear that these are all working together in real sites, that gives me confidence that the changes I've been working on are OK in principle. Let me know how you get on.

Jonathan

In order to prevent continual clashing of test updates, here is a consolidated patch of changes to three test files, but only the tests, not any actual code changes. This should pass, and if so, it would be good to have this committed now. Then we'll have an easier time on these issues without clashing patches.

Hi @jonathan1055 you are on it as usual! That said I can only get these patches to play nice without the tests. But then we get lovely white screens of death when editing rules.

Fails:

https://www.drupal.org/files/issues/2020-09-16/2800749-62.form-tests-only.patch (Support upcasting entity IDs to full entity contexts tests)
    https://www.drupal.org/files/issues/2020-09-16/2800749-61.upcast-without-test-fix.patch (Support upcasting entity IDs to full entity contexts)
    https://www.drupal.org/files/issues/2020-09-16/2664280-78.select-lists.patch (Select Lists)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-09-16/2664280-78.select-lists.patch

Success:

https://www.drupal.org/files/issues/2020-09-16/2800749-61.upcast-without-test-fix.patch (Support upcasting entity IDs to full entity contexts)
    https://www.drupal.org/files/issues/2020-09-16/2664280-78.select-lists.patch (Select Lists)

I truly think a commit is in order as these patches are solving problems for sure.

Maybe I wasn't clear, sorry. Patch #62 was never meant for your situation, that is the one I am proposing to get committed first. Patch #61 was designed for you, so that you can use it + select-lists #78 and single-value-selector #20 all together

If the rules were created before these patches were applied you should be able to make them work in principle. Maybe a condition or action has a saved value which is now impossible to chose with the select list? Remember to clear your cache after patching before trying to edit any rule. Also you might find it worthwhile exporting the individual rules, so that you have a text file which can be imported to re-create the rules, if things get trashed. If you get WSOD you should also be able to display the errors somehow. Do you have access to your webserver log? You can also display exceptions on screen using the Devel module (apologies if you know all this already).

Patch #58 worked for my use case. Check if user has specific role (i used the machine name) and redirect after saving the user account. Drupal 8.9.10.

Thanks Volker23 that is good to hear.
I have written two new tests to check upcasting in a condition and in an action. Patch #66 only adds the new tests so this will fail with the usual "Error: Call to a member function id() on string".

As expected the new tests in #66 fail. Now patch #68 adds in the upcasting code from #58. This should pass.

Patch #68 works for me.
After saving a new content item - User has role(s) - Send email to all users of a role

Thank you!

patch #68 also work for me. +1 for RTBC

patch #68 also work for me also. +1 for RTBC

Patch #68 has an edit for the ActionsFormTest file located inside tests/src/Functional. But Alpha 6 does not have this line; ['user' => '@user', 'roles' => 'Editor'],. I guess it only works for the Dev version.

+++ b/tests/src/Functional/ActionsFormTest.php
@@ -307,11 +307,17 @@ class ActionsFormTest extends RulesBrowserTestBase {
       ],
       'User role add' => [
         'rules_user_role_add',
-        ['user' => '@user', 'roles' => 'Editor'],
+        [
+          'user' => '@user.current_user_context:current_user',
+          'roles' => 'Editor',
+        ],
       ],
       'User role remove' => [
         'rules_user_role_remove',
-        ['user' => '@user', 'roles' => 'Editor'],
+        [
+          'user' => '@user.current_user_context:current_user',
+          'roles' => 'Editor',
+        ],
       ],
       'Unblock user' => [
         'rules_user_unblock',

FYI, patch #68 may be breaking other features. For instance, create a new content item is not working for me.

Hi @eit2103
Yes the patches here are always for the latest -dev, because that is how drupal.org tests them. If you are applying the patches manually, you can simply delete all the changes for the test files, as you only need them if you are running the phpunit tests.

patch #68 may be breaking other features. For instance, create a new content item is not working for me.

I assume you mean the rules action 'Create new content', not the general 'Create new content' functionality? Can you explain in more detail please. What errors do you see? Can you list the other conditions and actions in your rule, or better still export your rule, save in a .txt file and upload it here so we can try to reproduce the problems exactly.

Jonathan

@jonathan1055 - I just found out that another module was causing that error, big apologies!

With the following configuration, I'm still getting the following error even after using the patch. I enter @user.current_user_context:current_user for the user field and university_student (machine name of the user role) for the roles field. But I'm terrible with D8, I may be doing something wrong here.

Error: Call to a member function id() on null in Drupal\rules\Plugin\Condition\UserHasRole->Drupal\rules\Plugin\Condition\{closure}() (line 58 of /code/modules/rules/src/Plugin/Condition/UserHasRole.php)

Hi @eit2103, good that you have resolved the 'create new content' problem and that it is not caused by Rules.

Regarding the error you are still getting, here are some things to check

1. Have you cleared the cache before trying the rule?
2. Make sure you have typed the machine name of role correctly. Invalid role ids will produce the same error as you have
3. Check that you do not have leading or trailing spaces either side of the text, or any blank lines. This will also cause the same error

The precise entering of the text will all be resolved by #2664280: Select lists in action & condition configuration forms so there is no need to make any amendments to this patch to trim or validate, etc. So for now, we just have to be careful to enter the precise and correct text.

Worked! Turns out, I had an extra _ in the role name. Thank you so much Jonathan, I appreciate your help very much!

Hmm, Integrate Typed Data Widgets (#141) seemed to solve #2824360: Error "Call to a member function id() on string" for condition 'user has role(s)' for me.

Note: Patch #68 was incompatible.

Hi @Chris Happy, thanks for letting us know, that's great that your problem is solved by this.

Yes #2471481-141: Integrate Typed Data Widgets could fix this particular problem. However, the upcasting in this issue is still required for the generic solution, and for potential other cases in new conditions and actions not written yet.

Also yes these patches will conflict, it's been said on other issues, and that's unfortunate, but it can be resolved quite easily as soon as one of them is committed. I recall somewhere that I created a consolidated patch for all three of these major pieces of work:
#2471481: Integrate Typed Data Widgets
#2664280: Select lists in action & condition configuration forms
#2800749: Support upcasting entity IDs to full entity contexts
But I stopped trying to maintain that, as its just not worth the trouble. As soon as one of the patches gets committed I will re-roll the other two and we can make some progress.

@jonathan1055 & @drupalfan79 how hopeful is it that we might get this patch committed?

Thanks for asking @glynster. That's a question for @TR as he is the only active maintainer of Rules. I know he manages several other contrib projects and is busy with lots of Drupal things, but he knows we are here to help.

Patch #68 is working for several users (we had a question from eit2103 but that turned out to be a distraction which was not to do with the new functionality, see #78). I think this issue is ready, and I have written test coverage for the new functionality.

I think it would be great if @jonathan1055 has interest in becoming as maintainer of rules module and @TR is open to collaboration and having @jonathan1055 assist as a maintainer of this project.

Agreed @drupalfan79. @jonathan1055 has been instrumental in various patches for Drupal 8 rules. For me rules still serves a very important role in Drupal.

Thank you so much for the Patch #68, it works for me.

How could I support Rules to go further..

Below error is thrown every time when user tries to update any field after latest Drupal core update ie: version 8.9.14

Error: Call to a member function id() on null in Drupal\rules\Plugin\Condition\UserHasRole->Drupal\rules\Plugin\Condition\{closure}() (line 58 of /var/www/html/modules/contrib/rules/src/Plugin/Condition/UserHasRole.php)

@jas1988 Are you using the patch in this issue?
See #2824360: Error "Call to a member function id() on string" for condition 'user has role(s)'

This patch doesn't apply cleanly if #2471481: Integrate Typed Data Widgets is applied, but only because it patches the same area in tests/src/Functional/ActionsFormTest.php (and the changes appear to do the same thing).

So if you also have that patch in your patches.json you can use this one instead.

Thank you @djdevin, I too am already using the patch for #2471481, and your patch to the patch is working for me.

Thanks @djdevin and @richard.walker.ardc, that is helpful to know that the patches are working for you, and good to have a version here that can be used with the Typed Data patch. I've compared, and confirm that #88 is identical to #68 apart from the change to tests/src/Functional/ActionsFormTest.php has been deleted completely. I have added an update in the issue summary.

Yes I mentioned in #80 above these patches conflict with each other but I have stopped trying to make them all work simultaneouly. The test changes are necessary to prove that each patch runs green. I left the tests in, so that when @TR comes to commit these, each patch will be complete in its own right. As soon as the first is committed we can re-roll the others.

hi i get this issue on my website and no idea how to fix it
The website encountered an unexpected error. Please try again later.
Error: Call to a member function id() on null in Drupal\rules\Plugin\Condition\UserHasRole->Drupal\rules\Plugin\Condition{closure}() (line 58 of modules/contrib/rules/src/Plugin/Condition/UserHasRole.php).

Rerolled patch from #88 for latest dev.

Hi,

EDIT: this was fixed by removing another module (style switcher)

I had Rules dev-3.x 615221d and patch #68 applied.
With those, a rule which removed or added a user role worked.

Now after upgrading rules (dev-3.x 615221d => dev-3.x 7092780)
the role removing/adding stopped working.
(there is no error messages , the user just not get new role or
current roles are not removed. )

• Then I applied the patch #93 = role add/remove does not work
• Then I applied patch #88 and 2471481-146.integrate-typed-data-widgets.patch = role add/remove does not work
• And also tried #88 alone but no help

So how can I revert back to dev-3.x 615221d what is the composer command for that?
This does not help: composer require drupal/rules:dev-3.615221d

Having two "when user logged in" rules featuring a condition "has user role" breaks both rules.

Even if I delete one of them after... the other still doesn't work. I have to delete them both entirely and recreate a single rule to get it to work.

Tried every combination of patch and related patch... still an issue. Nothing in the log to troubleshoot from. Stumped.

On Drupal 9.2.7 if that matters.

@AnnaE1990 in #92 the patches in this issue will fix your problem.

@cornifex in #93 Thanks for the re-roll. However you droped one vital change in Functional/ConfigureAndExecuteTest.php +use Drupal\user\Entity\User; which caused the tests to fail. There were also unrelated changes to indentation and an extra blank line, which caused coding standards faults. Patch 93 no longer applies to the latest -dev, see the new re-roll patch 97.

@Anaconda777 in #95 says it is fixed now.

@ChuckSimply in #96 That's unrelated to this specific issue. Please let us concentrate here on the main task. You are welcome to start a new issue if you want to.

New patches

File 2800749-97.upcast.patch is a re-roll and contains all the required changes to the functionality and to the test files. This is the patch that ultimately should be committed.

File 2800749-97.upcast-no-test.patch has the necessary functionality changes to allow sites to use the upcasting but does not have any of the changes to the test files, in an attempt to reduce the conflict with other Rules patches. If you want to apply several Rules patches to your site (and do not intend to run the Rules phpunit tests) you should therefore use 2800749-97.upcast-no-test.patch

"Needs Review" to get the automated testing

Thanks for keeping up with this @jonathan1055, especially for all the re-rolls you had to do while other things got committed.

Thanks for keeping up with this @jonathan1055, especially for all the re-rolls you had to do while other things got committed.

It's a pleasure @TR to see this committed, makes all the work fully worthwhile. Thank you to you too, back in comment #19 for finding the paramconverter_manager service which led to this solution.

Thank you to everyone who helped with testing this issue. I will check the other issues that conflicted with this, and re-roll where necessary.

In doing testing on #2664280-95: Select lists in action & condition configuration forms I have discovered that action UserRoleRemove has assignment_restriction = "selector" in the plugin annotation for entering the user, but UserRoleAdd does not have the restriction. This seemed odd to me that they are different.

I then looked back over this issues and my patch #48 added the restriction to both actions, but in patch #68 the change for UserRoleAdd has been accidentally missed. It must have been my error, I usually compare patches as well as creating interdif files.

Here is a patch to add back the missing selector that got dropped.

Looking at this in more detail, the three other actions involving user are UserBlock, UserUnblock and SendAccountEmail - none of these are restricted.

There are three conditions involve selecting a user: UserHasEntityFieldAccess is restricted to selector, but UserHasRole and UserIsBlocked are not restricted.

In my comments from #42 to #48 above we decided to add the restrictor. If that is the right decision then I suggest we should also do it for these other actions and conditions which still allow free text input. Or if it was the wrong decision to add the restrictor then we should remove it from UserRoleRemove and UserHasEntityFieldAccess

Maybe this should this become a new issue? and leave this one as fixed for the upcasting feature.

I have discovered that action UserRoleRemove has assignment_restriction = "selector" in the plugin annotation for entering the user, but UserRoleAdd does not have the restriction.

Yes, I see. I found the same thing just a little while ago - I think this causes the test fail in #3254675: Improve ActionsFormTest and ConditionFormTest, although I think that test fail can be corrected by changing the test because the role-remove test expects a button to switch to the data selector, which is not present because of the assignment restriction.

I think we discussed this in some issue at some point, that perhaps we should be open to the possibility that we should allow someone to enter a uid instead of a User object and upcast it if necessary. I think that's why we didn't add an assignment restriction to UserRoleAdd. But I could go either way so perhaps let's go with the assignment restriction for now then if we want to allow a uid in the future we can remove it. There's no concrete use case for that yet.

So yes, please open a new issue with your patch from #102, but be aware that adding that to UserRoleAdd will probably cause a second failure in #3254675: Improve ActionsFormTest and ConditionFormTest which will need to be addressed.

Thanks for the feedback. I have raised the follow-up #3254747: Make selection of 'user' value consistent hence putting this back to 'fixed'.

I've used the patch suggested at the top ( https://www.drupal.org/files/issues/2021-12-08/2800749-97.upcast.patch ) and I am still getting the same error:

Error: Call to a member function id() on string in Drupal\rules\Plugin\RulesAction\SystemEmailToUsersOfRole->Drupal\rules\Plugin\RulesAction\{closure}() (line 125 of /var/www/app/npd_davenport.edu_test_build15/web/modules/contrib/rules/src/Plugin/RulesAction/SystemEmailToUsersOfRole.php)

Am I applying the wrong patch, or is there something still going wrong here?

Hi rreedy,
I have just checked this, applied the patch to a clean Rules 3.x alpha7 and it works fine. The e-mails are sent with no error. Please check that you are using the patched files, and that you have cleared your cache.
Jonathan