Needs review
Project:
Rules
Version:
7.x-2.x-dev
Component:
Module Integrations
Priority:
Minor
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Jun 2013 at 11:14 UTC
Updated:
29 Mar 2020 at 21:14 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
lauriiiComment #2
lauriiiI think this should work
Comment #3
ssoulless commentedI 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
Comment #4
fagoI 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.
Comment #5
lucas.constantino commented@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.
Comment #6
lucas.constantino commentedHere goes the new patch, rolled against 2.x.
Comment #7
roderikConfirm #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.
Comment #8
tr commentedThis 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.
Comment #9
tr commentedNow 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.
Comment #11
tr commentedMarked #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