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:
- Start the process of cancelling your own account.
- Follow the link the in confirmation email.
- 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.
- 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:
- Immediately after blocking or deleting the user, we log them out by assigning the anonymous user to the global
$user
variable. - 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)
Comments
Comment #1
Rob C CreditAttribution: Rob C commented[edit]
also happens when user is not logged in.
Comment #2
DocuAnt CreditAttribution: DocuAnt commentedConfirming this issue.
Subscribing
Comment #3
DocuAnt CreditAttribution: DocuAnt commentedComment #4
montesq CreditAttribution: montesq commentedReproduced on 7.x-dev but I really don't understand where the issue comes from.
Comment #5
makara CreditAttribution: makara commentedBatch 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.Comment #6
jackread CreditAttribution: jackread commentedI'm on Drupal 7.2 and am getting this error still.
Comment #7
carl.brown CreditAttribution: carl.brown commentedI'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?!
Comment #8
Rob C CreditAttribution: Rob C commented*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)
Comment #9
mefisto75 CreditAttribution: mefisto75 commentedsub
Comment #10
Devin Carlson CreditAttribution: Devin Carlson commentedMarked #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.
Comment #11
BladeduI 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.
Comment #13
schtink CreditAttribution: schtink commentedGetting 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.
Comment #14
jdleonardI'm also seeing this in 7.8
Comment #15
blasthaus CreditAttribution: blasthaus commentedsub, same issue
Comment #16
tscadfx CreditAttribution: tscadfx commentedI've documented one example of this and a workaround regarding the secure pages module here: http://drupal.org/node/1347660
Core 7.9
Comment #17
matthewv789 CreditAttribution: matthewv789 commentedI 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).
Comment #18
Rob C CreditAttribution: Rob C commentedmatthewv789 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?
Comment #19
brunodboComment #20
brunodboJust tested this on a fresh 7.x-dev, happening there as well.
Comment #21
arianek CreditAttribution: arianek commentedI 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.)
Comment #22
Rob C CreditAttribution: Rob C commentedThats a config option arianek
@ settings : admin/config/people/accounts
@ permission : admin/people/permissions
This all works fine, it's just that small notice.
Comment #24
arianek CreditAttribution: arianek commented@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.
Comment #25
Rob C CreditAttribution: Rob C commented@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.
Comment #26
brunodboPatch in #11 removes the error. Not sure if it's ok to remove the session_destroy() though.
Comment #27
brunodboMarking 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.
Comment #28
catchThis 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.
Comment #29
willhowlett CreditAttribution: willhowlett commentedHas 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.
Comment #30
afeijoIt will be fixed for both 8 and 7
I will have a look into the problem and write a simpletest
Comment #31
Steven Jones CreditAttribution: Steven Jones commentedSo 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.
Comment #32
Steven Jones CreditAttribution: Steven Jones commentedHere's a patch for Drupal 7, just to see if this breaks any of the tests, it shouldn't.
Comment #33
Steven Jones CreditAttribution: Steven Jones commentedThis is an attempt a possible test for this.
Comment #35
Steven Jones CreditAttribution: Steven Jones commentedSweet, so the test added in #33 does indeed detect Batch API being interrupted, merged that into the fix in #32 with this patch.
Comment #36
Steven Jones CreditAttribution: Steven Jones commentedCool 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.
Comment #37
Steven Jones CreditAttribution: Steven Jones commentedSorry, removed the backport tag by mistake. Backport is done in #35, but obviously still needs committing into D7.
Comment #38
Rob C CreditAttribution: Rob C commentedTested #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.
Comment #39
Steven Jones CreditAttribution: Steven Jones commentedAh right, sorry I missed that it was actually supposed to doing a drupal set message too.
Comment #40
Steven Jones CreditAttribution: Steven Jones commentedRight, let's not destroy the session completely then, we'll just regenerate it.
Comment #42
Steven Jones CreditAttribution: Steven Jones commentedActually, we don't need to clean up the session that's a batch API bug.
Comment #43
Steven Jones CreditAttribution: Steven Jones commentedAnd here's the patch for Drupal 8.
Comment #45
Steven Jones CreditAttribution: Steven Jones commentedSorry, those tests were clearly broken.
Comment #46
Steven Jones CreditAttribution: Steven Jones commentedAnd here's the update for D8.
Comment #48
Steven Jones CreditAttribution: Steven Jones commentedSorry about all the noise, another go.
Comment #50
Steven Jones CreditAttribution: Steven Jones commentedRight so the message for when the cancel blocks the accounts works, but not the delete account. Working on this...
Comment #51
Steven Jones CreditAttribution: Steven Jones commentedRight, this should do it I think.
Comment #52
Steven Jones CreditAttribution: Steven Jones commentedCool, here's the backport for D7.
Comment #52.0
Steven Jones CreditAttribution: Steven Jones commentedAdded issue summary.
Comment #53
Steven Jones CreditAttribution: Steven Jones commentedUpdated 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
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!
Comment #54
Rob C CreditAttribution: Rob C commentedDrupal 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.)
Comment #55
-Mania- CreditAttribution: -Mania- commentedJust wondering if this will make it to 7.x core once committed or only ever be available as a patch?
Comment #56
Steven Jones CreditAttribution: Steven Jones commented#51: drupal-1034828-drupal8-inc-tests.patch queued for re-testing.
Comment #57
Steven Jones CreditAttribution: Steven Jones commented@-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.
Comment #58
Steven Jones CreditAttribution: Steven Jones commentedHere's a lovely re-roll of the patch from #51 for Drupal 8.
Comment #58.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated issue summary.
Comment #59
clusterpod CreditAttribution: clusterpod commentedStill getting "No Active Batch" after user cancels their account. Drupal 7.16
Comment #60
sandervd CreditAttribution: sandervd commentedThe patch in #52 applied clean in Drupal 7.16, and resolved the issue.
Comment #60.0
sandervd CreditAttribution: sandervd commentedUpdate the location of the patch for Drupal 8.
Comment #61
Rob C CreditAttribution: Rob C commented@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)
Comment #62
brunodboD8 patch in #58 didn't apply, I got multiple errors like this one:
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.
Comment #63
brunodboAlso tested D7 patch in #52: works great as well.
Comment #64
Rob C CreditAttribution: Rob C commentedCan 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).
Comment #64.0
Rob C CreditAttribution: Rob C commented- Added a bit more info about the current state of the issue for D7/D8.
- Updated original username with current name for that user.
Comment #65
brunodbo@Steven Jones: anything else that's needed here before moving to RTBC?
Comment #66
Steven Jones CreditAttribution: Steven Jones commented@brunodbo the issue summary outlines remaining tasks. We might need to contact the security team so we can get a review of the security.
Comment #67
brunodboTagging for security review.
Comment #67.0
brunodboUpdated current patches / info.
Comment #68
Rob C CreditAttribution: Rob C commentedAny update on this?
Comment #69
garyg CreditAttribution: garyg commentedApplied 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
Comment #70
Steven Jones CreditAttribution: Steven Jones commented@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..
Comment #71
garyg CreditAttribution: garyg commentedI 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)
Comment #72
Steven Jones CreditAttribution: Steven Jones commentedHmm...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?
Comment #73
garyg CreditAttribution: garyg commentedI 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).
Comment #74
Rob C CreditAttribution: Rob C commented@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!
Comment #75
garyg CreditAttribution: garyg commentedNetBeans 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.
Comment #76
Steven Jones CreditAttribution: Steven Jones commentedHmm...I'm not sure. Maybe more reviewers are needed to see if they can reproduce this patch not working.
Comment #77
Rob C CreditAttribution: Rob C commentedDo 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)
Comment #78
geerlingguy CreditAttribution: geerlingguy commentedTested 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.
Comment #79
garyg CreditAttribution: garyg commentedI 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.
Comment #80
Steven Jones CreditAttribution: Steven Jones commented#62: drupal-1034828-drupal8-inc-tests_4.patch queued for re-testing.
Comment #81
Steven Jones CreditAttribution: Steven Jones commentedUltimately, this patch needs reviews for Drupal 8, not Drupal 7 first.
Comment #82
checker CreditAttribution: checker commentedI did not test with d8 but with d7.19 still working (#52).
Comment #83
tgeller CreditAttribution: tgeller commentedCouldn't we apply patch #52 to Drupal 7 already? It seems that enough people have reviewed it and found that it works fine.
Comment #84
tgeller CreditAttribution: tgeller commentedI 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?
Comment #85
star-szr#62: drupal-1034828-drupal8-inc-tests_4.patch queued for re-testing.
Comment #87
star-szrTagging for reroll.
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commentedPatching 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
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commentedLost a tag along the way...
Comment #90
Steven Jones CreditAttribution: Steven Jones commentedHere's a re-roll of the patch against Drupal 8.
Comment #91
geerlingguy CreditAttribution: geerlingguy commentedStill 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.
Comment #92
catchPlease 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
Comment #93
Steven Jones CreditAttribution: Steven Jones commentedOkay 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.
Comment #94
catchThanks! Committed/pushed to 8.x.
Moving to 7.x for backport.
Comment #95
Steven Jones CreditAttribution: Steven Jones commentedYeah this needs a re-roll for 7.x
Comment #96
Steven Jones CreditAttribution: Steven Jones commentedHere 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.
Comment #96.0
Steven Jones CreditAttribution: Steven Jones commentedUpdated, didn't made it in time for 7.17.
Comment #97
Rob C CreditAttribution: Rob C commented#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.
Comment #98
geerlingguy CreditAttribution: geerlingguy commentedJust tested here, too, on a D7 site that was having this problem. Problem is resolved, no more weird redirects or 'No active batch' warnings.
Comment #99
alfazaz CreditAttribution: alfazaz commented#96: drupal-1034828-drupal7-inc-tests.patch queued for re-testing.
Comment #100
geerlingguy CreditAttribution: geerlingguy commented#96: drupal-1034828-drupal7-inc-tests.patch queued for re-testing.
Comment #101
sk33lz CreditAttribution: sk33lz commentedI 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.
Comment #102
Steven Jones CreditAttribution: Steven Jones commented#96: drupal-1034828-drupal7-inc-tests.patch queued for re-testing.
Comment #103
Steven Jones CreditAttribution: Steven Jones commentedOkay 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?
Comment #104
sk33lz CreditAttribution: sk33lz commentedI'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).
Comment #105
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #106
geerlingguy CreditAttribution: geerlingguy commented@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.
Comment #107
sk33lz CreditAttribution: sk33lz commentedThanks @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.
Comment #108
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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
Comment #108.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.
Comment #108.1
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch committed in the here http://drupalcode.org/project/drupal.git/commit/7c67fb6 so changed the Remaining tasks to none.
Comment #109.0
(not verified) CreditAttribution: commentedLet's wrap it up, both patches are committed to D7 and D8.