Drupal 7 The patch provided in #96 was committed to core!

Drupal 8 The patch provided in #63 was committed to core!

Problem/Motivation

Upon cancelling your own user account the slightly odd error message appears at the end of the process:

No active batch.

Steps to reproduce:

  1. Start the process of cancelling your own account.
  2. Follow the link the in confirmation email.
  3. After the batch that alters content and deletes your user account you will get redirected to the homepage which then has the error message above on it.
  4. Also, there is supposed to be a completion message that appears at the end of the process, which doesn't.

Proposed resolution

This happens because the user's session is destroyed during the last batch operation, but the batch API uses the session ID to lookup the batch. This means that when the batch API operations complete and it looks to run the finished callback, it can't find the batch, because the session has been destroyed. In addition any drupal_set_message calls are wiped out because the session containing those messages is destroyed.

To solve this, we do two things:

  1. Immediately after blocking or deleting the user, we log them out by assigning the anonymous user to the global $user variable.
  2. In the finished callback for the batch, when we no-longer need the session ID to be persisted we regenerate the users session, as per its suggested usage of being called whenever a user is logged in or out.

We also add some tests to make sure that we're not breaking batch API and that we actually see the messages that are set during account deletion.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Original report by Rob C

Hello all, got a brand new D7 install, i enabled the permission for people to cancel accounts.

When i log in to an account that i am going to delete, click on the confirmation email link, cancel the account (finish the process) i get an 'No active batch.' error.

I guess it has a lot to do with http://drupal.org/node/974

(aka i did search, but could only find this one)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Rob C’s picture

Title: "No active batch" error after user cancelling own account and currently logged in » "No active batch" error after user cancelling own account

[edit]
also happens when user is not logged in.

DocuAnt’s picture

Confirming this issue.
Subscribing

DocuAnt’s picture

Priority: Minor » Normal
montesq’s picture

Version: 7.0 » 7.x-dev

Reproduced on 7.x-dev but I really don't understand where the issue comes from.

makara’s picture

Batch API uses session ID (drupal_get_token()) to save and retrieve batch records from DB. After canceling an account, the batch is actually finished but cannot be loaded to display a success message, cause the user session has been destroyed.

jackread’s picture

I'm on Drupal 7.2 and am getting this error still.

carl.brown’s picture

I'm getting the same problem. Interested to know if there's a fix/patch for this as it make absolutely no sense to the user. Hopefully no one will ever cancel their account, but nothing wrong with pessimistic planning, right?!

Rob C’s picture

*realistic planning ;) a communities evolution isn't only growth and even with growth people will still leave. That's just how it goes, nothing negative about that, pure realistic.

The problem is that, as stated above, the session is terminated, maybe you can create a redirect page where people are redirected to after the resignation. So they will never see the message. (not the best fix, but for the user it will work)

edit:
http://drupal.org/node/974 more information (dup)
and http://drupal.org/node/1221972 (other mod)

mefisto75’s picture

sub

Devin Carlson’s picture

Version: 7.x-dev » 8.x-dev

Marked #974: "No active batch" error after cancelling account with who's online block enabled as a duplicate and moving to Drupal 8.

Some modules have had to create work arounds for this problem. One work around was implemented in #1221972: "No active batch" error if import file contains current user.

Bladedu’s picture

Status: Active » Needs review
FileSize
528 bytes

I worked a bit on this issue and seems that removing the session_destroy called inside the batch operation still ensure that the user is not logged in anymore and the batch itself can finish without errors.

Status: Needs review » Needs work

The last submitted patch, no-active-batch-on-user-delete-1034828-11.patch, failed testing.

schtink’s picture

Version: 8.x-dev » 7.8

Getting the same error on Drupal 7.8. I would expect that it would say "Account successfully cancelled". I hope this can be patched in the next update. It looks ugly.

jdleonard’s picture

I'm also seeing this in 7.8

blasthaus’s picture

sub, same issue

tscadfx’s picture

I've documented one example of this and a workaround regarding the secure pages module here: http://drupal.org/node/1347660

Core 7.9

matthewv789’s picture

I also get this in Drupal 7.10. It seems it is impossible to cancel accounts in Drupal - neither the user nor the administrator can do so.

I should also note that the reset password functionality doesn't seem to work either - even using the emailed one-time link, it asks for the old password before allowing the change, which of course the user doesn't have. So only administrators can change users' passwords for them.

As a result, the user management in Drupal 7 seems to be extremely poor (and substantially worse than Drupal 6, where I don't recall running into these issues).

Rob C’s picture

matthewv789 that sounds like another problem, this issue is about a notice you get when deleting an account. (where #13 should be the future i guess.) The actual process still works (user is deleted) and possible work arounds exist.

You might want to start a new issue for your problem: 'It seems it is impossible to cancel accounts in Drupal - neither the user nor the administrator can do so.' But i can tell you i am able to cancel accounts just fine and pass reset also works on D7.10, so you might want to mark this as a support request?

brunodbo’s picture

Version: 7.8 » 7.12
brunodbo’s picture

Version: 7.12 » 7.x-dev

Just tested this on a fresh 7.x-dev, happening there as well.

arianek’s picture

I tested in a pretty plain site on 7.12 and got the same error 'No active batch.'

FWIW - the user 'cancel account' still works fine, in that the user account gets set to blocked after following the confirmation link in the email. Then as an admin, I can opt to actually delete the account. (I do find it odd that to the user it looks like they're deleting their account, but the account is only becoming blocked, but maybe that's a config option, I didn't dig too far.)

Rob C’s picture

Thats a config option arianek
@ settings : admin/config/people/accounts
@ permission : admin/people/permissions
This all works fine, it's just that small notice.

arianek’s picture

@ClusterFCK - thanks i did see that later, but how it *looks* from the end user's perspective is still deceiving. to the person cancelling the account, they are led to think their account is being deleted, when in reality it is not.

Rob C’s picture

@arianek
I believe it's up to the site editors/admins/developers to make it clear to end-users how they configured these settings / how this process works on their site. (about page/some block/email notice/whatever). But let's not dwell to much off-topic. It's not this functionality that 'should' provide this i believe. But it 'Should' state the end-result of ending a user account, may it be deleted or blocked. You can also enable emails to be send, so you can even inform a user on a per user basis when they end an account. (if you really want to) But the process doesnt show that now, it only trow the 'no active batch', but this is i guess how it should work.

brunodbo’s picture

Patch in #11 removes the error. Not sure if it's ok to remove the session_destroy() though.

brunodbo’s picture

Priority: Normal » Critical

Marking as critical, since the user cancel process seems sort of broken now: even though the account is canceled/deleted after clicking the confirmation link, the user is presented with a cryptic error message (instead of a confirmation message indicating successful account deletion), which is confusing.

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Major
Issue tags: +Needs tests, +Needs backport to D7

This needs to go into 8.x first then be backported to 7.x

Since it actually works OK, does not at all feel critical to me, but that's a very strange error to present to end users so I can live with major. I thought we had tests for this already but it looks like they're not catching this.

willhowlett’s picture

Has any progress been made on this? Seems a pretty bad bug.

Also, should it really be tagged with 8? Just worried that it being tagged with 8 is stopping people looking at fixing it, when in reality it's a major bug that is affecting 7 right now.

afeijo’s picture

It will be fixed for both 8 and 7

I will have a look into the problem and write a simpletest

Steven Jones’s picture

So I wonder if all that's need here is to move the code that actually destroys the users session into a finished callback, because looking at the code path, that might do the trick.

Steven Jones’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » Steven Jones
Status: Needs work » Needs review
FileSize
1.33 KB

Here's a patch for Drupal 7, just to see if this breaks any of the tests, it shouldn't.

Steven Jones’s picture

This is an attempt a possible test for this.

Status: Needs review » Needs work

The last submitted patch, drupal-1034828-cancel-possible-test.patch, failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

Sweet, so the test added in #33 does indeed detect Batch API being interrupted, merged that into the fix in #32 with this patch.

Steven Jones’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Steven Jones » Unassigned
Issue tags: -Needs tests, -Needs backport to D7
FileSize
2.13 KB

Cool beans, so here's a patch for D8, that is a forward-port of the patch in #35.

I've added an issue summary also.

Steven Jones’s picture

Issue tags: +Needs backport to D7

Sorry, removed the backport tag by mistake. Backport is done in #35, but obviously still needs committing into D7.

Rob C’s picture

Tested #32 on Drupal 7.14 and the 'No active batch' message is gone.
User is deleted, but, no message at all is displayed now.
Progress yes, but now fix the message.
I'll do some more testing ones i have some time.
(very busy porting a couple modules to D7)

ps. Just to be sure, do i also have to run #35? Looks like that's just for tests.

Steven Jones’s picture

Status: Needs review » Needs work

Ah right, sorry I missed that it was actually supposed to doing a drupal set message too.

Steven Jones’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
4.74 KB

Right, let's not destroy the session completely then, we'll just regenerate it.

Status: Needs review » Needs work

The last submitted patch, drupal-1034828-drupal7-inc-tests.patch, failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

Actually, we don't need to clean up the session that's a batch API bug.

Steven Jones’s picture

Version: 7.x-dev » 8.x-dev
FileSize
4.7 KB

And here's the patch for Drupal 8.

Status: Needs review » Needs work

The last submitted patch, drupal-1034828-drupal8-inc-tests.patch, failed testing.

Steven Jones’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
4.55 KB

Sorry, those tests were clearly broken.

Steven Jones’s picture

Version: 7.x-dev » 8.x-dev
FileSize
4.7 KB

And here's the update for D8.

Status: Needs review » Needs work

The last submitted patch, drupal-1034828-drupal8-inc-tests.patch, failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
4.7 KB

Sorry about all the noise, another go.

Status: Needs review » Needs work

The last submitted patch, drupal-1034828-drupal8-inc-tests.patch, failed testing.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Status: Needs work » Needs review

Right so the message for when the cancel blocks the accounts works, but not the delete account. Working on this...

Steven Jones’s picture

Right, this should do it I think.

Steven Jones’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Steven Jones » Unassigned
FileSize
4.92 KB

Cool, here's the backport for D7.

Steven Jones’s picture

Issue summary: View changes

Added issue summary.

Steven Jones’s picture

Version: 7.x-dev » 8.x-dev

Updated issue status, please see things that need reviewing there, I have provided patches for both Drupal 7 and 8 to make it easy for people to review.

@catch

I thought we had tests for this already but it looks like they're not catching this.

Actually we have tests to 'make sure the user is logged out' by ensuring that their username isn't shown on the final page, when in actual fact, we should see their username on the the final page because it appears in the completion message. At the moment in core we are testing to make sure that this process doesn't fully work!

Rob C’s picture

Drupal 7.14: patch at #52 confirmed working.

The message:
'Username has been deleted.'
Is now displayed correctly after clicking the link in the confirmation e-mail.

(Will do an extensive test including 7.15 (and maybe 8.x) if i can find the time for it, but this looks good.)

-Mania-’s picture

Just wondering if this will make it to 7.x core once committed or only ever be available as a patch?

Steven Jones’s picture

Steven Jones’s picture

@-Mania- If we can get the patch from #51 into Drupal then the patch from #52 can go into Drupal 7, and then it will appear in the next minor release of Drupal 7.

Steven Jones’s picture

Here's a lovely re-roll of the patch from #51 for Drupal 8.

Steven Jones’s picture

Issue summary: View changes

Updated issue summary.

clusterpod’s picture

Still getting "No Active Batch" after user cancels their account. Drupal 7.16

sandervd’s picture

The patch in #52 applied clean in Drupal 7.16, and resolved the issue.

sandervd’s picture

Issue summary: View changes

Update the location of the patch for Drupal 8.

Rob C’s picture

@Steven Jones
D7 is working great, also tested #58 yesterday, but i need another go at it for a report. Anyway... I'll do that today.

@clusterpod
Download the patch from #52 if you want to fix it Right now in Drupal 7. (that will work) We are Really Close! with getting it in Drupal 7, so you can also wait a bit for that. (first have to get the patch from #58 into Drupal 8)

@sandervd Thanks for testing! If you can, please also test #58 on Drupal 8, we need to get that in first, before we can backport this. (before it's accepted)

brunodbo’s picture

D8 patch in #58 didn't apply, I got multiple errors like this one:

Checking patch core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php...
error: while searching for:
    $account = user_load($account->uid, TRUE);
    $this->assertTrue($account->status == 0, t('User has been blocked.'));

    // Confirm user is logged out.
    $this->assertNoText($account->name, t('Logged out.'));
  }

  /**

error: patch failed: core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php:174
error: core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php: patch does not apply

Apparently, the message strings in assertTrue() etc aren't translated anymore - I checked other test files in the user module in D8, this seemed to be the case across tests. Attached is a patch that addresses that change.

Other than that, patch works great.

brunodbo’s picture

Also tested D7 patch in #52: works great as well.

Rob C’s picture

Can we get a RTBC!?! - brunodbo great work with getting it up-to-date for D8!

Tested #62 and it works Great on Current D8 dev (just updated my repo and tested).

We now have working patches for D7 and D8 (again).

Rob C’s picture

Issue summary: View changes

- Added a bit more info about the current state of the issue for D7/D8.
- Updated original username with current name for that user.

brunodbo’s picture

@Steven Jones: anything else that's needed here before moving to RTBC?

Steven Jones’s picture

@brunodbo the issue summary outlines remaining tasks. We might need to contact the security team so we can get a review of the security.

brunodbo’s picture

Issue tags: +Needs security review

Tagging for security review.

brunodbo’s picture

Issue summary: View changes

Updated current patches / info.

Rob C’s picture

Any update on this?

garyg’s picture

Applied patch in #52
Still getting "No Active Batch" when cancellation of account is attempted by either admin or user.
...and user does not get cancelled
Drupal 7.17

Steven Jones’s picture

@garyg What steps did you take to apply the patch etc.? I've just applied the patch to a clean install and tested, and I correctly get the message: User has been deleted..

garyg’s picture

I manually applied the patch to the 2 files indicated and uploaded them to overwrite existing files.
I was very careful to check the work as I proceeded.
Sure would have been easier to just drop in a new file(s)

Steven Jones’s picture

Hmm...applying patches manually is not a good way to test the patch, please see the apply patch guide: http://drupal.org/patch/apply

What settings do you have for account cancellation? Are account deleted/suspended etc?

garyg’s picture

I applied the patch to the original 2 files, this time using NetBeans IDE
Uploaded the new files and the result is the same...no change in the error "No active batch"

I went to Edit | Cancel Account and tried both disable and delete.
I do get a log messages:
Warning: Parameter 1 to page_title_user_cancel() expected to be a reference, value given in module_invoke_all() (line 857 of /home/herblore/public_html/includes/module.inc).

Rob C’s picture

@garyg

Please follow the directions Steven Jones provided in #72. We have multiple confirmations that it's working, try IRC or the Drupal forums if you require additional support. Thanks!

garyg’s picture

NetBeans has a utility that applies patches to files and that is what I used.
I visually inspected the updates it performed and it looks fine.
I don't know what else to do.

Steven Jones’s picture

Hmm...I'm not sure. Maybe more reviewers are needed to see if they can reproduce this patch not working.

Rob C’s picture

  • #52 Appplies clean.
  • The update has been performed.
  • rob has been deleted.

Do a diff of the files patched to confirm changes.

[edit] do a manual patch to make sure it's neatbeans that's the problem. (i'm already quite sure)

geerlingguy’s picture

Tested patch in #52 for Drupal 7, and it works correctly. Can't see any reason this would cause security issues off the top of my head, since the user account and session would be deleted from the system (and thus shouldn't be able to do anything even with a hacked session) but I'm not an expert on sessions either.

For D8, this will probably need some updating if/when #335411: Switch to Symfony2-based session handling lands.

garyg’s picture

Issue tags: -Needs security review

I have applied patch #52 to drupal 7.17 using the patch command from shell
System reports a successful patch

patching file user.test
Hunk #1 succeeded at 661 (offset 491 lines).
Hunk #3 succeeded at 762 (offset 491 lines).
patching file user.module
Hunk #1 succeeded at 2389 (offset 166 lines).
Hunk #2 succeeded at 2273 (offset -1 lines).
Hunk #3 succeeded at 2453 (offset 166 lines).

I cleared all cache and accounts will still not cancel. Message is "No active batch."

The source of this issue I now discover is in the Secure Pages module.

Steven Jones’s picture

Steven Jones’s picture

Ultimately, this patch needs reviews for Drupal 8, not Drupal 7 first.

checker’s picture

I did not test with d8 but with d7.19 still working (#52).

tgeller’s picture

Couldn't we apply patch #52 to Drupal 7 already? It seems that enough people have reviewed it and found that it works fine.

tgeller’s picture

I was about to change this to RTBC, but don't know if that's O.K..

Perhaps this should be split into separate D7 and D8 issues so we can commit the D7 version right away. Could someone in authority comment on this?

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, drupal-1034828-drupal8-inc-tests_4.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

Anonymous’s picture

Issue tags: -Needs reroll

Patching 7.21 with #52 gives offset but works as expected. Thanks!

$ patch -p1 -i ~/drupal-1034828-drupal7-inc-tests_3.patch
patching file modules/user/user.module
Hunk #1 succeeded at 2393 (offset 3 lines).
Hunk #2 succeeded at 2443 (offset 3 lines).
Hunk #3 succeeded at 2456 (offset 3 lines).
patching file modules/user/user.test

Anonymous’s picture

Issue tags: +Needs reroll

Lost a tag along the way...

Steven Jones’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.14 KB

Here's a re-roll of the patch against Drupal 8.

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

Still works for me (just did one manual test of canceling my user account) on D8, and patch in #52 still works great for D7. Would be ridiculously awesome to get this in, and tests cover this functionality much better after patch.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Please post a version of the patch with just the tests.

Also if looking for the username showing up in the message is enough to confirm this works, I think we could remove the assertion for whether the session was destroyed - that's a negative test for this particular bug, just testing it works and fails without the patch is enough

Steven Jones’s picture

Okay okay, so here's the extra patch requested, just tests, and then a re-rolled patch including tests and fix (I think my previous re-roll was dodgy).
Removed the assertion for not breaking batch API.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! Committed/pushed to 8.x.

Moving to 7.x for backport.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones

Yeah this needs a re-roll for 7.x

Steven Jones’s picture

Assigned: Steven Jones » Unassigned
Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D7
FileSize
4.16 KB
2.38 KB

Here we go, I've taken the patch in #52 applied it to 7.x, changed the assertions to the same as those in 8.x now and then provided a only tests and a with tests version.

Steven Jones’s picture

Issue summary: View changes

Updated, didn't made it in time for 7.17.

Rob C’s picture

#96 works like expected, patched my local D7 repo, tested on a clean install, on an existing site, ran a couple tests, changed a couple settings, but everything keeps working like expected, so RTBC for me.

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

Just tested here, too, on a D7 site that was having this problem. Problem is resolved, no more weird redirects or 'No active batch' warnings.

alfazaz’s picture

geerlingguy’s picture

sk33lz’s picture

I just tested this patch, and while the patch applies fine, it does not fix the "No active batch" error. I cannot delete any users on the website.

Steven Jones’s picture

Steven Jones’s picture

Okay that's interesting, because we have tests that are obviously testing to make sure that you can delete users. How are you deleting them? Are you seeing any other log messages or errors?

sk33lz’s picture

I've tried both through the User page and the individual user's page. Both received the same error and do not allow any users to be deleted, even by User 1. There are no errors generated in the Drupal log after applying the patch.

To make matters worse, once I updated to the 7.x-dev version, all Image Styles names disappeared and were throwing many errors. I have since switched back to 7.22. I however now get this error in the Drupal log when trying to disable or delete a user.

Warning: Parameter 1 to page_title_user_cancel() expected to be a reference, value given in module_invoke_all() (line 857 of /home/username/public_html/includes/module.inc).

David_Rothstein’s picture

The image style issue is unrelated; it's because you didn't run update.php.

The Page Title one looks like it could be related, but hard to tell. In general, if you're experiencing this issue and others aren't it is likely caused by a contributed module or custom code running on this particular site. Any way to narrow down what module might be causing it?

Also, does the patch actually break anything that worked correctly before, or does it simply fail to fix the bug? If the latter, it becomes less important to track this down, and I think we could commit the patch (if it looks good otherwise) and deal with the rest in a followup.

geerlingguy’s picture

@sk33lz/#104 - the page title issue is #1282564: page_title_user_cancel() has bad signature, and there's a fix for it in that issue (so it's also unrelated).

This patch is still RTBC.

sk33lz’s picture

Thanks @David_Rothstein and @geerlingguy! The patch in #1 on that linked issue fixed the page_title_user_cancel error, although after applying the patch and the update for the image module update, I still got the No active batch.

Naturally, I did a bit more digging around, and apparently all my issues have been stemming from the Secure Pages module, as I did not ignore the "batch*" path. Not ignoring that path will apparently cause the same error as found in this issue while trying to delete a user, among other things like checking for updates manually.

My apologies, but it seemed to be the same issue initially. Helpful information for others who might run into the same issue that I had though if they end up here and have not ignored batch*. More info here, https://drupal.org/node/1627326.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.23 release notes

Thanks for the additional testing and followup!

This patch looks good to go, so I committed it to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/7c67fb6

David_Rothstein’s picture

Issue summary: View changes

Updated issue summary.

Anonymous’s picture

Issue summary: View changes

The patch committed in the here http://drupalcode.org/project/drupal.git/commit/7c67fb6 so changed the Remaining tasks to none.

Status: Fixed » Closed (fixed)
Issue tags: -7.23 release notes

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Let's wrap it up, both patches are committed to D7 and D8.