Calling user_save from within hook_cron removed role_expire items. [#1464124]

Found this edge case bug when developing a custom feature for a client.

When cron is run I loads all users who are within a day of their expiry date and adds a field to the users $data array and then saves. This then removes the users entry from the role_expire table.

It runs through line 389 for each role and removes them all. Has anyone seen this before?

Comment	File	Size	Author
#5	role_expire-tempfixtostopnuking-1464124-5.patch	502 bytes	mikebell_
#4	role_expire-tempfixtostopnuking-1464124-4.patch	3.46 KB	mikebell_

Comments

Comment #1

stewsnooze

he / him

commented 3 March 2012 at 08:45

I definitely have not seen this before. Were you running cron as an authenticated user or through wget/drush ? user_save does different things depending on who is logged in which I hate but it would be interesting to know?

Comment #2

mikebell_ commented 3 March 2012 at 19:58

I was running it through drush as anon. I'll test as an authed user and report back.

Comment #3

mikebell_ commented 3 March 2012 at 20:43

Ok even if I run it as uid 1 it's still being removed.

The role is initially applied using role_expire_write_record() on a node_save if that helps.

From looking through the code this is happening because $role is blank (see line 374). The array_key_exists() check is failing. I have no idea why this would be happening.

Comment #4

mikebell_ commented 9 March 2012 at 14:18

Status	File	Size
new	role_expire-tempfixtostopnuking-1464124-4.patch	3.46 KB

I keep going round in circles trying to find an issue with this but can't, the following patch does some minor code formatting changes and comments out the offending call to role_expire_delete_record(). It's most likely wrong and will break things for others but my testing finds no issues in this use case.

Comment #5

mikebell_ commented 28 March 2012 at 10:25

Status	File	Size
new	role_expire-tempfixtostopnuking-1464124-5.patch	502 bytes

New patch with compat for http://drupal.org/node/1504216

Comment #6

stewsnooze

he / him

commented 28 March 2012 at 10:38

Status:

Active

» Needs work

This last patch doesn't seem right. It just comments out a call to a function which in essence makes the whole else clause not needed. If this is your intended functionality then the whole else clause should be taken out.