user_role_*_permissions doesn't validate the role exists before attempting to operate on it

It works for me.

The fix looks good, thanks. Let's just add a test when using the user_role_revoke_permissions() with non-existing rid.

I came across this issue the other day and found this patch, worked for my situation.

Added the test to check that this patch fixes the issue.

I think the failure is caused because of the patches order (which does matter here). The regular patch needs to be the last one to pass.

Anyway, I think the patch looks good, it has tests and the fix is the same as in user_role_grant_permissions(). Thanks!

Hiding patches so hopefully when it retests it won't auto fail it.

Triggering a 11.x build.

Passed 11.x

And test-only patch of #5 shows the issue.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

This seemed to be a problem in user_role_change_permissions, user_role_grant_permissions and user_role_revoke_permissions.

I've taken a slightly different approach of checking that the role entity can be loaded before doing anything. Added test coverage.

Can IS be cleaned up some please. What steps were being done to trigger this or was this just found in general? Proposed solution needs to be written out.

IS updated, added the default template.

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 necessarily 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.

Fixed merge conflict. Fixed PHPSTAN. Test coverage is in place. Here is test-only output:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3365626/core/phpunit.xml.dist
..E......                                                           9 / 9 (100%)
Time: 00:34.698, Memory: 8.00 MB
There was 1 error:
1) Drupal\Tests\user\Functional\UserPermissionsTest::testUserRoleChangePermissions
Error: Call to a member function revokePermission() on null
/builds/issue/drupal-3365626/core/modules/user/user.module:546
/builds/issue/drupal-3365626/core/modules/user/user.module:504
/builds/issue/drupal-3365626/core/modules/user/tests/src/Functional/UserPermissionsTest.php:218
ERRORS!
Tests: 9, Assertions: 155, Errors: 1.
Exiting with EXIT_CODE=2

This looks good. Could this be close to RTBTC?

Think we should be logging an error if the role doesn't exist.

I retained the existing behaviour of failing silently.

Maybe we should throw a \LogicException or \UnexpectedValueException rather than logging.

That would work too +1

Tests are fixed for this.

Reviewed. I found few minor points, and one probable flaw in the test coverage as written, which is why I felt this needs to be kicked back.

I'm derailing this, but in the slack thread about this, we discussed that we need to find new places for all remaining functions in .module files anyway. It could be a new service with those methods ported 1:1, but I'm not sure it's really worth it. Using the entity API is not that much more complicated (we could add 1-2 helpers such as a a way to add multiple permissions, and then this would be the responsibility of the calling code to decide what to do in that situation.

Also, recipe config actions are kind of a replacement for this as well, think we should re-evaluate whether node_install() and media_install() should exist at all or if it should be the job of recipes and distributions to do this. There is after all a very long comment in node_install() why you shouldn't do that.