Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
When a user can cancel its account, in some cases the user is not redirected to the homepage after confirming its account cancellation.
Steps to reproduce:
- Grant "Cancel own user account" permission to authenticated users.
- Add a new user with no role.
- Login with this new user.
- Cancel your account and click on the confirmation link sent by email.
- When the batch process is done, your user is successfully redirected to the homepage.
Now:
- Create a new page content or article content
- Set it as front page
- Create a new user or unblock the previously created user account.
- Cancel your account and click on the confirmation link sent by email.
- When the batch process is done, your user is redirected to user/ of its user ID leading to a 404.
The batch process doesn't redirect correctly to the homepage.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2855054-25.patch | 2.34 KB | alexpott |
#26 | 24-25-interdiff.txt | 569 bytes | alexpott |
#25 | 2855054-24.patch | 2.69 KB | alexpott |
#21 | User-Cancel-Redirection-2855054-21.patch | 1.47 KB | gawaksh |
#19 | 2855054-user_cancel_redirect-19-tests.patch | 681 bytes | wesleydv |
Comments
Comment #2
ArtusamakHere is a patch trying to fix that.
Comment #3
cilefen CreditAttribution: cilefen commentedI think you forgot the patch in the comment.
Comment #4
ArtusamakHere is the patch, my bad.
Comment #5
dhruveshdtripathi CreditAttribution: dhruveshdtripathi as a volunteer and at DevsAdda for OpenSense Labs commentedPatch applied successfully. It works!
Comment #6
cilefen CreditAttribution: cilefen commentedComment #7
LoMo CreditAttribution: LoMo as a volunteer commented@cliefen, after some research (since I wasn't sure how to test opening the link sent via email for user cancellation, I found there is a test that already covers this process, so I think that existing test just needs a simple assertion that the user is on the front page. I'll experiment and see if I can see it fail without this patch (and pass with it), then update this issue with a patch for the test, etc. (Assigning to myself, in the meantime... )
Comment #8
LoMo CreditAttribution: LoMo as a volunteer commentedThe uploaded patches should fill the need for the test of the redirection to the front page. The "tests-only" patch should fail on its own. The complete patch (original patch, plus the new test assertion) should pass. I've verified the new assertion passes/fails locally depending on whether or not the original patch was applied.
I think this can be set to RTBC now, but I guess someone should take a look at my work, so setting to "Needs review"
Comment #10
LoMo CreditAttribution: LoMo as a volunteer commentedNote that the patch that "failed" should be seen as a success. The test without the fix SHOULD fail. The patch with both the fix and the tests SHOULD pass (and did). :-)
Comment #12
dtv_rb CreditAttribution: dtv_rb commentedThis problem still exists. When will it be officially fixed?
Comment #13
NickDickinsonWildeLooks good. I think this can be targeted to 8.4.x as a bug fix.
Comment #14
xjmThanks everyone, good find!
This is a deceptively tricky little issue, as I was initially confused as to why this would be the fix given that
''
and<front>
should be equivalent. Things I learned while investigating this:batch_process()
still exists.user_cancel()
still exists.user_cancel()
still "needs to be run in a batch", requiring this manual invocation ofbatch_process()
.batch_process()
manually, even outside of low-level things like DB updates and the form API. Locale does too.It looks like this was added when we started allowing users to cancel their own accounts in #8: Let users cancel their accounts. As far as I can see, the reason
batch_process('')
isn't behaving as expected is deep in the bowels of_batch_finished()
, this line:Instead of an
isset()
, that treats an empty value as being the same as the source URL, which is incorrect for the edgecase of''
.The tests prove the fix, but maybe we should actually fix the source of the bug above rather than working around it? We would then keep the test we added with this patch, and possibly add another one for the batch fix (brrr).
Comment #15
xjmHm. Actually, on the other hand, what's suggested in #14 could be seen as a behavior change to undocumented behavior that something might nonetheless rely on (i.e., empty string redirecting you to the batch start page rather than the frontpage could be a part of someone's batch script). Furthermore, given how convoluted the batch system is and how hard to test, developers might not adequately learn of the change. So that's at least a reason to add a change record along with #14 and make such a fix minor-only, and possibly a reason to deprecate the code path with empty string and warn the user.
Given all that, the fix in #8 is probably best in the short term, since it's a totally backportable fix. So, instead of fixing the Batch API here, let's create a followup issue for #14, and just update #8 with an @todo referencing the followup, something like:
@todo Due to https://www.drupal.org/the-node-id, '' is treated as being equivalent to the batch start page rather than the front page. Specify <front> explicitly until this is resolved.
Then we can fix this bug safely, and deal with the deeper Batch API bug in a followup.
Comment #17
blacklabel_tom CreditAttribution: blacklabel_tom at Reason Digital commentedI can confirm the patch in #8 works against 8.5.3 core.
Comment #18
DrupalBoon CreditAttribution: DrupalBoon commentedHey guys,
I'm wondering why this is still not fixed in Drupal Core? It's such a simple thing to replace the
''
with'<front>'
in line 315. Each time we update the core, we have to keep in mind, that this line has to be updated manually again. It would be great if this could be fixed in a further update.Thanks and best regards
Comment #19
wesleydv CreditAttribution: wesleydv at District09 commentedThe patches from #8 no longer apply, these should.
I agree with #14 batch API should be fixed in a separate issue.
Comment #21
gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commentedThis patch would solve the issue.
Comment #22
wesleydv CreditAttribution: wesleydv at District09 commented@gawaksh the second patch in #19 was suppose to fail, as it only contains the test and not the fix. It proves that the first patch fixes the issue if it where not there.
Comment #23
blacklabel_tom CreditAttribution: blacklabel_tom at Reason Digital commentedI can confirm that the patch in #19 works.
Cheers
Tom
Comment #24
alexpottPatch needs a re-roll and has to be applied to 8.6.x first.
Comment #25
alexpottHere's a re-roll and also moves the page related assertions to just after the drupalGet() since this makes it a bit more obvious what is being tested. Also changed to use non-deprecated test method and assert on the status code because currently it is a 404 and this should always be a 200.
Comment #26
alexpottDidn';t mean to remove this line.
Comment #27
alexpottCommitted and pushed a3bb9d878f to 8.6.x and 938a814da0 to 8.5.x. Thanks!
Created #2978922: Improve batch_process() documentation as a followup.