Simplify UserHasRole::doEvaluate()

This is yet another issue for #2800749: Support upcasting entity IDs to full entity contexts. The PR just fixes a symptom, not the root issue. If you want to help, please see #2800749: Support upcasting entity IDs to full entity contexts!

Re-opening for now since this is a major issue for folks using UserHasRole, and this fix seems like at least a temporarily valid workaround until the upcasting issue is properly resolved.

This updates the patch in #5 to always trim the role value in case it comes through with line breaks (which then fails to load the corresponding role object). It also adds an exception if the role still isn't found.

#2824360: Error "Call to a member function id() on string" for condition 'user has role(s)' is a duplicate of this issue, but it seems that neither will be fixed directly, because the full solution will be #2800749: Support upcasting entity IDs to full entity contexts

I have started working on the blocker #2800749: Support upcasting entity IDs to full entity contexts and would appreciate any feedback, to check I am on the right lines. Thanks.

Actually, this doesn't need upcasting. UserHasRole::doEvaluate() does NOT need the Role object, so there is no need to upcast here, and the failure is a logic failure.

The attached very simple fix REQUIRES #2664280: Select lists in action & condition configuration forms to be applied first, as that patch changes the role name input to use a select widget with the selections enumerated as rid => role_name.

With the changes in #2664280: Select lists in action & condition configuration forms, we will be 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 look like:
array ( 0 => 'authenticated', 1 => 'administrator', )
Note that this is key => rid

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

Try this patch with no upcasting required, no Role object required, after applying #18 from #2816157: Simplify UserHasRole::doEvaluate(). I'm posting this is both issues 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.

So I see this fix was proposed as part of #2824360: Error "Call to a member function id() on string" for condition 'user has role(s)'.

The difference is, that issue changes the context annotation to pass a string for the role selection, whereas with the changes #2664280: Select lists in action & condition configuration forms, my patch above in this issue leaves that annotation as entity:user_role.

In #2824360-6: Error "Call to a member function id() on string" for condition 'user has role(s)' @fago says

We should not change the argument from the entity to a dumb string, that will make the condition worse in the end.

So I think this meets that criterion?

Thanks TR, this is very useful. I had also seen that once the selection lists were implemented there would be a simpler way to evaluate the condition. If @fago is OK with this, then it makes sense to me. But I am not the designer or maintainer of Rules, just a willing volunteer happy to contribute my time to getting things fixed. It seems like #2664280: Select lists in action & condition configuration forms is the one to work on first.

Postponing this until #2664280: Select lists in action & condition configuration forms has landed. Then the patch in #10 above is a good solution.

Thanks #10 It's work

I am leaving this open because it is easier to find this issue, but the solution has to come in #2800749: Support upcasting entity IDs to full entity contexts so if this is important to you please contribute to that issue, not here.

@Promo-IL #2800749-36: Support upcasting entity IDs to full entity contexts has a generic patch to solve this. Please test it. Thanks.

We will probably still want patch #10 above committed anyway, once #2664280: Select lists in action & condition configuration forms is done.

EDIT: please disregard this post (I don't know how to delete it). Proper updated patch is in comment #20

I've updated the patch to work with 3.0@alpha

Just to followup... patch on #68 here solved it for me. https://www.drupal.org/project/rules/issues/2800749#comment-13942286

EDIT: I lied, still having issues even with the above patch :(

#20 works for me.
Thanks!

Changed the issue summary now that #2800749: Support upcasting entity IDs to full entity contexts is done.

Needed a re-roll, no material changes from patch #10 / #20

Edit: please disregard this post and my file from comment #24. Apparently there was a mistake in it

#23 wouldn't patch for me. I re-rolled from #20 and fixed a few code linting issues

#23 wouldn't patch for me. I re-rolled from #20 and fixed a few code linting issues

Hi @Steven McCoy,
When making patches it would help us if you could do the following:
(a) please add the comment number within the patch name, as is done in the previous patches here and is standard Drupal good practice. It makes it so much quicker to identify patch files, to make sure a link is to the correct one, and when comparing patches locally
(b) if fixing anything (other than a straight clean re-roll with no actual content changes) then provide an interdiff so that others can more easily identify the changes. You'll get a quicker more helpful review that way.
Thanks
Jonathan

I have just looked at your patches in #24, #25 and #26. None of these apply to the latest 8.x-3.x dev code. I think you are patching against the alpha7 tagged release. Patches have to be applied to -dev. Patch #23 is still the current patch.

However, as this issue is postponed on #2664280: Select lists in action & condition configuration forms there is no automated testing here. And also no point in making any patches for use on sites running alpha7 unless that site also has the patch on 2664280, which is in turn is dependent on the core issue #2329937: Allow definition objects to provide options

However, thank you for noticing the coding standards faults. Here's a new patch #29 which fixes both of those, even though strictly they are faults already committed in -dev and not introduced by the changes in this patch.

At some point this patch lost sync with the changes to Rules. Specifically, all the lines that change $user to $account need to go. For example, this should not be done:

-   * @param \Drupal\user\UserInterface $user
+   * @param \Drupal\user\UserInterface $account

The reason we need to continue to use $user now is because of PHP 8 and the changes made in #3210303: PHP 8 introduced breaking change to call_user_func_array().

And ... Do we have a test for this?

Yes you are right, patch #20 introduced the backwards change. Here's an updated clean patch.

This condition is covered by Unit\Integration\Condition\UserHasRoleTest and this patch 31 will fail now. This is because #2800749: Support upcasting entity IDs to full entity contexts has been committed, so the $roles parameter is now an array of fully upcast roles. We need to change the context of roles from "entity:user_role" to just "string" but that will be done in the next patch. First to demonstarte the failure.

Here is the change to set the roles context to string. This also requires a change to Unit/Integration/Condition/UserHasRoleTest.php where the roles array is now just the role ids not the objects.

All tests pass.
Regarding the three long array lines, in #3178576-11: [meta] Coding Standards in Rules 8.x we talked about increasing the allowed length, so I won't fix those here until the discussion on that issue has been done.

the $roles parameter is now an array of fully upcast roles.

My patch in #10 was to avoid the need to upcast, and thus avoid having to wait for that patch (which is now committed).

But with upcasting, is there any need for this issue anymore? I guess I had assumed that we would just close this when upcasting was added. If we have the full Role object, then that's a lot more useful in general than just the string role machine name.

The changes here are not strictly required any more. But I assumed that because you changed it to 'needs work' you were still interested in making the small efficiency saving in time and processing by not upcasting the role(s) and not having the array_map function to re-extract the ids when they can be made available directly in the $roles parameter. I can't see that the fully upcast role objects would be any use in this condition as we only need the role id here, nothing else. The role objects will still be upcast for the other actions, it is only this condition which I set back to keep them as strings.

Or maybe you are thinking about expanding the condition? Or using the role label in db logging? I'm fine if you want to leave it as is.