There was a talk in Drupal groups about roles failing while cascading creating user + adding user to a specific role, so I created patch for that.

Comments

lauriii’s picture

lauriii’s picture

Status: Patch (to be ported) » Needs review

I think this should work

ssoulless’s picture

Version: 7.x-2.0 » 7.x-2.x-dev
Priority: Normal » Major
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I have tested the patch and it works great, I have tested it in a production site, and so far so good. Please commit this patch as soon as possible.

Regards
Sebastian Velandia Pössinger

fago’s picture

Status: Reviewed & tested by the community » Needs work

I don't see how this patch should fix what issue. The check on $account->uid is on purpose as you cannot add user role to anonymous users objects.

lucas.constantino’s picture

@fago there is a situation where you are assigning roles to a user not yet created. Therefore, the uid property is not set, but it isn't the anonymous user anyway. I'll try and make a patch to consider this scenario.

lucas.constantino’s picture

Here goes the new patch, rolled against 2.x.

roderik’s picture

Confirm #5.
I'm just working on someone else's site; they have this rules action set up at condition "Before saving a user account" and I'm assuming that is a valid use case. $account->uid is not set in that case.

Being pedantic: in above patch, replaced "(isset($account->uid) && $account->uid !== 0)" by "!empty($account->uid)" -- I guess it depends on opinion, what's best.

Except not on the first line - there, you can get rid of the notice by switching the operands from both sides.

tr’s picture

Priority: Major » Minor
Status: Needs work » Needs review
StatusFileSize
new882 bytes

This issue was not set back to "Needs review" when a patch was added in #6, so the testbot didn't automatically test the patch. And the patch author didn't manually trigger a test either. Likewise for #7. Also, the patch in #7 is malformed, which would have been detected by the testbot immediately. (I just manually triggered a test - it has been sitting there malformed an untested for almost 4 years ...)

Issues die when they go into "Needs work" because people see they "need work". You should ALWAYS change the status when you submit a new patch to fix the issue - that way people who are watching this issue will see there is something new to review but also because the testbot is an important part of the workflow here,

Here is a re-roll and fix to the malformed patch in #7. Let's see what the testbot has to say about it now that it can be applied.

tr’s picture

Issue tags: +Needs tests

Now it needs two things:
1) A simple test which creates a Rule that demonstrates the problem and demonstrates that the above patch it fixes the problem.
2) Someone to review the work.

tr’s picture

StatusFileSize
new1.61 KB

Marked #2182369: False Error Message when creating new account from role purchase as a duplicate. The patch there is identical except it also makes changes to the remove role action. While the use case for that is probably not common, it's also not forbidden, so I think it's important to make the same checks on the $account variable for both the add role and the remove role actions.

Patch from #2182369: False Error Message when creating new account from role purchase is attached, attributed to @cricunova_maria