Can't delete user accounts

Dunno, really dunno. What happens with orphaned nodes ?

I don't think I can reproduce this. I just managed to delete user #1 (without this patch).

+1 for committing m3avrck's patch - see http://drupal.org/node/41996

Tried it on my test bed to get to user 39082398797324, and worked well for me. I'd say ready to go.

Does that new user_load() add sql queries to each page request?

It would be nice if that query wasn't necessary. The overhead of that query depends on the number of users in the database. BTW, a single query that takes 1ms costs drupal.org ~20 hour of processing time a year. :)

Why do we lower integers? "LOWER(u.status) = LOWER('1')". Also, why do we check whether u.status < 3 when status has to be LOWER('1')? :P

Also, don't introduce special cases and remove:

+  // Don't allow anyone to edit uid = 1 unless logged in as uid = 1
+  if (!($account->uid == 1 && $user->uid != 1)) {
+    $form['submit'] = array('#type' => 'submit', '#value' => t('Submit'), '#weight' => 30);
+  }
+  // Don't allow the deletion of uid = 1
+  if (user_access('administer users') && $account->uid != 1) {

A few offsets when patching, but not errors, and patched perfectly fine. The module also works as predicted. attemted to delete some users, and got the expected results with this patch. i'd say, RTC.

I disagree about not special-casing uid 1. That account should not be deletable. The recovery of that error is painful, and someone asleep at the wheel can do it.

If the system completely removed the dependency on uid 1 I'd be more agreeable, but since uid 1 is already special-cased, I don't see why we can arbitrarily say not to special-case it here.

Tested patch and I can confirm that it does what it says on the tin. No offsets when I patched against HEAD.

However, we have lost the potential for adding a "destinations" to the delete or edit operation. I could never see how this would be useful myself, but presumably it must have been thought useful at some point.

So, using the same method that node.module uses to do this (unsetting the $_REQUEST['destination']) I've taken the liberty of adding a couple of lines to this patch. I hope nobody minds.

Simon

I don't like the change to user_view(). Passing the $account and doing a check is not very clean. I'd rather have duplicate queries and caching in user_load(). Plus, the extra query should only happen in case the URL is of the format "user/num/*". The original patch did that query for all page views, which does not appear to be necessary.

Couple of issues still.

One of them is my fault, because the patch I submitted last night had a silly mistake in it.

The patch I'm submitting does the following.

1. Users can now be deleted using the delete button from admin/users.
2. Users are now returned to admin/users if they delete a user and to user/number if they have just edited the profile.
3. A destination is not set when entering the user editing/deleting form from admin/users but if one was set from another source, it wouldn't break the system.
4. The tabs no longer appear if the user doesn't exist and the url e.g. user/1245 is accessed.

Which we all seem agreed on.

With this done, I was running into problems with content which was authored by deleted users. Nodes were appearing as teasers and just "N/A" as body.

My opinion is that the content should be set to 'uid = 0' when the user is deleted, at least there should be an option.

So this patch also:

5. Sets uid=0 in the node table for all the nodes where uid=deleted_uid.

Couple of issues still.

One of them is my fault, because the patch I submitted last night had a silly mistake in it.

The patch I'm submitting does the following.

1. Users can now be deleted using the delete button from admin/users.
2. Users are now returned to admin/users if they delete a user and to user/number if they have just edited the profile.
3. A destination is not set when entering the user editing/deleting form from admin/users but if one was set from another source, it wouldn't break the system.
4. The tabs no longer appear if the user doesn't exist and the url e.g. user/1245 is accessed.

Which we all seem agreed on.

With this done, I was running into problems with content which was authored by deleted users. Nodes were appearing as teasers and just "N/A" as body.

My opinion is that the content should be set to 'uid = 0' when the user is deleted, at least there should be an option.

So this patch also:

5. Sets uid=0 in the node table for all the nodes where uid=deleted_uid.

+1,

just installed against HEAD, works as advertised...
I like user_load returning false instead of empty object.
slightly concerned about its implications else where,

blog.module, comment.module, contact.module, node.module,
profile.module, statistics.module, and tracker.module..

A cursory check say things will keep working...
most required changes will probably be minor..

ala blog.mode: 140
- if ($account->uid)
+ if ($account !== false)

If this patch gets commited I'll pick up the cleanup work.

hrm #26 applies to #23

I just tested this patch against a fresh install of drupal HEAD cvs. It worked perfectly. At first before applying it, I couldn't delete my test user account. Then, after applying the patch, it properly prompted me to delete the user and the user deleted subsequently after confirming it.

I also checked the mysql database and the user was deleted from the database.

My above post refers to user.module_24.patch (4.41 KB) by the way.

Don't you find this problem:

With this done, I was running into problems with content which was authored by deleted users. Nodes were appearing as teasers and just "N/A" as body.

with that patch?

Ok, fair call.

This patch fixes all the issues with deleting users but does not do anything with their content (despite the warning message!).

Please review and RTC.

Haven't applied the patch but can't you put the new user_load after the 'if (arg(0) == 'user' && is_numeric(arg(1))) {'?

Yes, you can. New patch attached.

Committed the patch. Thanks.