Logged in as a sub-admin. After unchecking a role on the user edit form and submitting the form, the role is still assigned to the user.

CommentFileSizeAuthor
#11 administerusersbyrole.role-removal.3343114-11.patch1.01 KBsiemen_hermans
#8 3343114-form-submit-fix.patch920 bytesa.kovrigin
#4 administerusersbyrole.role-removal.3343114-4.patch785 bytesadamps
#3 3343114-after-patch.png24.88 KBarun velusamy
#3 3343114-before-patch.png24.86 KBarun velusamy
#2 3343114-2-administerusersbyrole.role-removal.patch589 bytesadr_p

Comments

adr_p created an issue. See original summary.

adr_p’s picture

adr_p commented
Status: Active » Needs review
StatusFileSize
new 3343114-2-administerusersbyrole.role-removal.patch589 bytes

Attaching a patch.

arun velusamy’s picture

arun velusamy commented
StatusFileSize
new 3343114-before-patch.png24.86 KB
new 3343114-after-patch.png24.88 KB

I have verified the patch #2 and tested it on Drupal version 9.5.x. The patch works fine and I have added the before and after screenshots for reference.

adamps’s picture

adamps commented
Issue tags: +Needs tests
StatusFileSize
new administerusersbyrole.role-removal.3343114-4.patch785 bytes

Thanks for the report. The patch doesn't apply however it showed a good direction. I tested and changed it slightly to work in all cases I could think of.

We've had a series of bugs many of them caused by bad fixes to previous bugs:

It would if someone could write a test for roles, including as many of the above cases as possible.

sagesolutions’s picture

sagesolutions commented
Status: Needs review » Reviewed & tested by the community

I successfully applied patch #4 and can now remove (and add) roles when editing users.

I agree that tests should be added to prevent regression issues

  • AdamPS committed 9535f98b on 8.x-3.x 
    Issue #3343114 by adr_p, AdamPS, Arun Velusamy, sagesolutions: Cannot...
adamps’s picture

adamps commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Thanks. I've committed the fix and raised #3354395: Add tests for roles.

a.kovrigin’s picture

a.kovrigin commented
Status: Fixed » Needs review
StatusFileSize
new 3343114-form-submit-fix.patch920 bytes

It can be a situation, when there is no roles key in $form['account'] array on form submission which causes the fatal error.
The patch fixes the issue.

adamps’s picture

adamps commented
Status: Needs review » Fixed

It can be a situation, when there is no roles key in $form['account'] array on form submission which causes the fatal error.

How would that be? The roles field is always set in AccountForm. If a module wishes to remove the field from display they should use the '#access'. Removing the array key entirely could cause bugs in other modules too.

The patch fixes the issue.

This issue has already been fixed, and I've made a release, so I can't make a new commit here without causing confusion. As far as I can see, the same problem was true before this fix. So please raise a new issue if you wish to continue the discussion.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

siemen_hermans’s picture

siemen_hermans commented
StatusFileSize
new administerusersbyrole.role-removal.3343114-11.patch1.01 KB

The changes introduced by this fix can lead to a TypeError in the array_diff function.
In our specific situation the user can only have 1 role. As a result $form['account']['roles']['#default_value'] returns a string.
This causes a TypeError in the array_diff function.

A simple check of the value returned by $form['account']['roles']['#default_value'] could resolve the issue.