Deprecate user_role_grant_permissions(), user_role_revoke_permissions() and user_role_change_permissions()

Awesome clean-up suppose better finish #1872876: Turn role permission assignments into configuration. first
Then split this issue into parts

• user_role_permissions() - 3
• user_roles() - 7
• user_role_names() - 18
• user_permission_get_modules() - 1
• user_role_change_permissions() - 20 (18 + 2)
• user_role_grant_permissions() - 36 (34 +2)
• user_role_revoke_permissions() - 8 (6 + 2)

Is there any suggestions on method names for RoleInterface?
Suppose some work could be done in parallel

Let's use the deprecation process here - more important due to things like #3346953: Role permissions are not sorted when saving via admin/people/permissions

Do we really need changePermissions() not much useful API

Role::changePermissions() is used 3 three time in run-time code in core. I've replaced all the obvious ones in tests in this issue - where the test was only granting but using this method. I think there is likely to be quite a lot of contrib usages - see https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs... - but given they'll need to change at this point I think it could be a good idea. I think what we should do is make this issue only about user_role_grant_permissions() and user_role_revoke_permissions(). I think we should still change any test usage of user_role_change_permissions() that we can because it is harmless.

Tighter issue scope.

I feel that we should tackle #3347842: Deprecate the trusted data concept in configuration first as it will ensure that we don't need to give complex instructions about when to call trustData()

It could use follow-up to replace most of calls to grantPermission() with grantPermissions()

@andypost

It could use follow-up to replace most of calls to grantPermission() with grantPermissions()

Why? I think it is okay to have both. I toyed with making grantPermission accept an array but that looked awkward.

@alexpott because many places doing chained calls which could be converted by rector to new method which accepts arrays

btw Maybe it could just extend existing methods with typed variable arguments
grantPermissions(string ...$permissions): static or grantPermission(string $permission, ?string ...$permissions): static https://3v4l.org/O85Hh

ref https://www.php.net/manual/en/functions.arguments.php#functions.variable...

@alexpot I reverted some changes as in tests roles often has no permissions but grant/revoke without any permission sounds like bad API

interdiff in new MR's commit https://git.drupalcode.org/project/drupal/-/merge_requests/3666/diffs?co...

approach with typed variadic also allows to keep array values as strings https://3v4l.org/49RgT

fixed IS

All occurrences appear to be replaced

Think this will need a deprecation test though.

and needs to extend testing for the methods with more values

Added test, hope it covers both old and new usage

@andypost 'm saddened that you removed the grantPermissions() method. We discussed it on issue and disagree. A method called grantPermission() accepting more that one permission is not great and there is no reason to not add another method to keep the syntax meaningful. Also changing to a variadic is an API change not an addition which is harder for alternate role implementations.

@andypost what does

but grant/revoke without any permission sounds like bad API

mean?
Are you saying this is bad API because you can pass an empty array in? I disagree to be honest. If you pass an empty list into grantPermissions() and it grants no permissions this is exactly how you would expect a method that accepts a list to behave.

Also @andypost

@alexpott because many places doing chained calls which could be converted by rector to new method which accepts arrays

What does this mean?

I think it means that you think there are places doing...

$role
  ->grantPermission('perm 1')
  ->grantPermission('perm 2')
  ->save();

I don't see why that is an argument for a variadic...

We can change this to

$role
  ->grantPermissions(['perm 1', 'perm 2'])
  ->save();

just fine.

@alexpott I mean array argument is bad api because there's no check for array values (empty array is just useless ability). That's why variadics is better approach.

I'm ok to add extra plural method but it looks more confusing and a way to more errors (similar methods on a class). I find a case of overriding roles's entity class is very edge case comparing to API extension which is functionally backward compatible

Main idea is to use less function calls and make code more readable

$role
  ->grantPermission('perm 1')
  ->grantPermission('perm 2')
  ->save();

vs

$role
  ->grantPermission('perm 1',  'perm2')
  ->save();

@andypost I think

$role
  ->grantPermissions(['perm 1',  'perm2'])
  ->save();

is better than the variadic extension of the grantPermission() method.

@alexpott this approach is more common but it does not check that each array element is string, that's why I find variadic exactly suitable for that specific case

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I'm happy to work on this issue but I think we need clear direction as to what we want. Trying to summarise below:

Variadic or array:
grantPermissions(string ...$permission) or grantPermissions(array $permissions)
As andypost pointed out using an array has no type safety. We could add @param string[] $permissions and rely on phpstan to do type-checking, but not sure it will catch everything.

Single and plural or just plural:
grantPermission and grantPermissions or just grantPermissions
Keeping both is perhaps easier, changing to plural would require deprecating the singular. If we have just the plural I think variadic is better so you don't need to pass an array when granting only one permission.

Updated search link for #23: user_role_change_permissions

I looked at both approaches and I think Alex's approach is much cleaner.

I do understand @andyposts' point, but I think at this point we should just follow Alex, and open a follow up to decide what to do about the singular versions.

I'm going to add the change permissions function too.

I tried applying, but there are dozens of conflicts, this doesn't look to hard to manually reroll though.

I'm about 90% of the way through, I'd like to finish in the morning so assigning to myself.

There is one remaining test failure, I think this is a side effect of something else going on.

core/modules/config_translation/tests/src/Functional/ConfigTranslationRoleUiTest.php
Is failing because the text Authenticated user doesn't exist which was added here: https://www.drupal.org/project/drupal/issues/3582050

I think the case the test is looking at is still an issue, the main issue is here #2910353: Prevent saving config entities when configuration overrides are applied which is still needs work.

I'm not sure if we just remove that part of the test and add a todo on the actual issue fixing it.

You can reproduce the issue with these steps:

1. Install drupal
2. Enable configuration translation
3. Visit admin/people/roles/manage/authenticated/translate
4. Add de as a language
5. Visit admin/people/roles/manage/authenticated/translate/de/add
6. Add the translation Angemeldeter Benutzer
7. Visit de/admin/people/permissions
8. Add change own username permission to Authenticated user role
9. Visit admin/people/roles
10. See the role is German

I addressed the feedback.

The failure is random.
I looked through and think the static call is likely fine in most cases.

Reviewed.

Considering how many calls we to these methods in tests, I could also see us adding some helpers to UserCreationTest, we alread have \Drupal\Tests\user\Traits\UserCreationTrait::grantPermissions for example. but I'm also fine not doing so, it would essentially just save us the save() call.

#3620216: Add loadOverrideFree and loadMultipleOverrideFree to ConfigEntityBase is ready for review!

Rebased after the loadOverrideFree issue landed

Almost ready, just some minor things. And the CR is a placeholder and the IS could use an update too.

Went through it with a word diff, nothing changes around the list of permissions, just how it's called.

Personally, if I'd started this, I probably would have added a trait to UserCreationTrait or so, the vast majority of calls are in tests, and it would make it slightly more convenient, but at least for core, the work is done now and while there are 70ish calls, the replacement isn't that complicated, now that loadOverrideFree() exists.

contrib has 700 calls, but I think this should be easy to automate with rector: https://search.tresbien.tech/search?q=user_role_.*_permissions%20-r%3Adr...

This should be ready for review again!

I reviewed the whole MR and found an incorrect conversion. I left a comment on the MR. Please check it.

@dcam thanks!

I went with @berdir's suggestion since I agreed it makes it easier to review.

That's fine. I also appreciate when changes are minimized in large diffs. It does make those MRs easier to review.

It sounds like this one is ready for committers.

Posted a few comments on the MR, should be fine to self-rtbc after they're fixed.

I applied most of your suggestions.

I updated the install files too.

I created a follow up for the trait: #3621087: Update UserCreationTrait::grantPermissions() to use new RoleInterface::grantPermissions

The failure is random builds/core/modules/system/tests/src/FunctionalJavascript/ThemeSettingsFormTest.php:85

Hopefully the follow up is OK!

Committed and pushed 0f7caab8579 to main and 7065b2372a3 to 11.x. Thanks!