All anonymous signups cancelled when any user is deleted

Is there anyway I can get the users signed up back.

From your database backup if you have one. Otherwise no.

If what you say is true, it's a (nasty) bug. However, I thought we already fixed this bug... ;)
#321653: guest "cancel signup" deletes him from all signups.
#233512: Deleted User needs to be removed from the log

Can anyone else easily reproduce this?

I just tried to reproduce this and couldn't.

- Cean D6 test site
- I added a "test1" user
- had that user and 3 others signup for an event
- viewed the signup list and saw 4 users signed up
- deleted the "test1" user
- viewed the signup list and saw 3 users signed up

Can you reproduce this on your test site?
Can you reproduce this on a clean D6 test site with nothing but core and signup.module?
Can you provide detailed information about how to reproduce it, including versions of all installed modules and any signup-related configuration?

Given that I can't reproduce it myself, I don't currently think there's a bug in the code, moving this back to a support request.

I'm not accusing you of lying, nor am I asking for a video. ;) I simply stated the facts from my end: you've reported a problem, I showed you the detailed steps of how I tried to reproduce it (and couldn't). I asked some very specific questions you could answer to make it possible for me to understand the problem you're having and find the bug (if any) that's causing it. Please just answer my questions if you're interested in resolving this. Thanks.

Based on #467042: Anonymous signups cancelled when deleting a user (now marked duplicate of this), perhaps the bug only shows up for anonymous signups. Re-reading this thread, CleaningExpo never specified auth vs. anon signups, and I clearly only tried with auth signups in my testing for comment #2...

Given another independent report, and the likelihood that this is a real bug, and given that it's a data loss bug, I'm bumping this back to active + critical...

@CleaningExpo, if you're still reading this, can you confirm that it was anonymous signups that were canceled when you delete a user?

I've experienced this bug when I have both anonymous and non-anonymous signups to the next event in the event queue. I tried to delete various users, both signed up and not signed up for this particular event, and the result is the same, the anonymous signups are all cancelled, the ones made by registered users are ok.
Might it be related to the message I get when I try to delete a user that all the posts of the deleted user will be assigned to anonymous?... Maybe the uid is set to that of the anonymous user when this happens, and when the signup_user hook is called it deletes signups with the anonymous uid?...

I've just experienced the exact same problem. I went to delete a user who had never signed up for anything and upon confirmation of that user's deletion, I got a long page full of messages saying numerous signups had been deleted.

This deleted all anonymous signups across my entire site (which was over 500 signups). Luckily, I had exported that list somewhat recently so I only lost a few dozen names.

Obviously, this type of loss of data is pretty scary. At this point, I'm afraid to delete any user.

Thanks,
Ben

One more thing: I wanted to point out that all of my authenticated signups seemed to be fine. This issue appears to only effect anonymous signups.

--Ben

Yup, this is critical. Giving a more accurate title... Hopefully shouldn't be too hard to fix. Definitely before rc4 goes out (#363238: Release signup 6.x-1.0-rc4). Anyone feel like trying to debug? (My hands are very full right now). Also, can anyone test this on D5? Thanks!

I put some debug messages in and found that on the user tab, the uid was not correctly being passed to the SELECT * FROM {signup_log} query (newsflash!). This patch corrects the offending line.

this patch is without the debug messages ;).

Looks like an undocumented API change between D5 and D6 core. :/ Alas. Reviewed, tested, and committed to HEAD and DRUPAL-6--1. Thanks!

I am curious what the API change was.

@moshe: See patch #11. The contents of the $edit argument passed into hook_user() are totally different between D5 and D6.

I don't see a new dev version of the signup module that might include this patch given the date of this issue. I'm not equipped to do a patch, so I need to wait for the next version of signup. Do you know when we can expect it? Thanks!

I would like to reopen this, becaus I believe this is not 100% fixed yet. I have a scenario where this bug still persists.
With Drupal core this fix works just fine, but with (some) add-on modules it doesn't work. I've lowered the priority to normal since it now only affects some very specific add-on modules.

First, the fix in the patch:

function signup_user($type, &$edit, &$user, $category = NULL) {
  switch ($type) {
    case 'delete':
      $uids = array();
      if (is_array($edit['accounts'])) {
        // A multi-user delete from Admin > User management > Users.
        $uids = $edit['accounts'];
      }
      else {
        // A single-user delete from the edit tab on the user's profile.
        $uids[] = $edit['_account']->uid;
      }
      foreach ($uids as $uid) {
        $query = db_query("SELECT * FROM {signup_log} WHERE uid = %d", $uid);
        while ($signup = db_fetch_object($query)) {
          signup_cancel_signup($signup);
        }
      }
      break;
  }

Now the bit that was a fix for this problem, but I think isn't.

      else {
        // A single-user delete from the edit tab on the user's profile.
        $uids[] = $edit['_account']->uid;
      }

The problem is that this fix relies on the $edit array to be correctly set. If add-on modules use the user_delete hook function the edit-array will NOT be properly set. I've checked this with some add-on modules (not many use this hook function) and they all implement it like this:

  user_delete(array(), $uid);

Take a look at the API documentation: http://api.drupal.org/api/function/user_delete/6
The signup_user function is (in this case) called with a blank $edit-array.
I think that the $user object would be a more reliable source for getting the uid, as it is loaded in the user_delete function:

      else {
        // A single-user delete from the edit tab on the user's profile.
        $uids[] = $user->uid;
      }

I've not made a patch because I'm not sure that using $user is more reliable then using $edit.
I currently have Signup running with this small change on all of our sites. If any problems arrise I will report here.

Any more ideas on this? Any other functions/hooks that are connected?

This is the patch containing the changes I made. I've added an extra check on the $uids array to make sure it is an array and that contains some data.

I'm in the process of using this module on a project I'm currently working on, and just stumbled upon this issue.

I didn't dig into the code yet, but a simple question popped up on my head when looking at the code above: since it doesn't make sense to delete the anonymous "user", what about skipping signup cancellation if we find the anonymous user uid? E.g.:

      foreach ($uids as $uid) {
        if ( $uid == 0 ) continue; // Skip anonymous user
        $query = db_query("SELECT * FROM {signup_log} WHERE uid = %d", $uid);
        while ($signup = db_fetch_object($query)) {
          signup_cancel_signup($signup);
        }

Shouldn't this be enough to solve this problem?

No it won't...

Preventing the deletion of the anonymous user is a very good idea, but won't solve the problem I still see.

The problem is that the $uid used in the module might not have been set at all because it is taken from the $edit array. Other modules invoking the user_delete function don't set this $edit array. So the $uid will always be blank (or 0) in that case. Even if a valid user is being deleted! The $uid you can get from the $user object, which is also passed in to the signup_user function, is always correctly set.
The patch I made changes the logic to use the $user object instead of the $edit array.

I have got this patch running on all of our sites and since then the signups are working correctly. I have got 2 modules enabled that use the user_delete hook and all anonymous signups were deleted when this hook was called with a valid non-anonymous user... Offcourse, if you don't use any extra modules that invoke the user_delete function everything will work correctly to. (and very few modules use this hook)

Do you understand the problem now? Or am I not explaining it good enough?

#18 looks pretty good to me. Yeah, using $user is probably better.

However, we should swap the order of the is_array() and !empty() checks, to ensure we don't try to test is_array() on a variable that doesn't exist.

Thanks for the fix!

has this been comitted yet?

I seems like it has not been committed. Need to apply this patch manually.

I have confirmed that this does solve the problem of canceling anonymous user signups on my site.

I've got my patch from #18 running on 11 websites atm and no more problems with the anonymous signups being cancelled.
Anyone else using the patch?

Still needs work for #21. Then this can go in. Sorry for the delays! I've got too many irons in the fire these days... :/

corrected patch:

Great, thanks. If anyone else can test this, it'd be great. I'm at a conference all weekend and won't really have much time for Drupal, but if this can be tested and RTBC by early next week, I can get this into CVS.

Thanks!
-Derek

Using the $account argument rather than taking it from the $edit/form values is the right solution here. Thanks for pointing this out!

Our hook_user() implementation should really call the third argument &$account, rather than &$user per best practices and http://api.drupal.org/api/function/hook_user/6 . I combined this change with yours, tested, and committed: http://drupal.org/cvs?commit=434716

Thanks!