Deprecate redundant (role-/permission related) procedural wrappers from user.module

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

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 📌 Deprecate the trusted data concept in configuration Active 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 grantPermissions(string $first, string ...$permissions): static

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.