User cancel link doesn't redirect to the homepage

I think you forgot the patch in the comment.

Patch applied successfully. It works!

@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... )

The 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"

Note 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). :-)

This problem still exists. When will it be officially fixed?

Looks good. I think this can be targeted to 8.4.x as a bug fix.

Thanks 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 of batch_process().
• This is not the only place in HEAD that still calls 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:

      $redirect = $_batch['batch_redirect'] ?: $_batch['source_url'];

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).

Hm. 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.

I can confirm the patch in #8 works against 8.5.3 core.

Hey 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

The patches from #8 no longer apply, these should.

I agree with #14 batch API should be fixed in a separate issue.

This patch would solve the issue.

@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.

I can confirm that the patch in #19 works.

Cheers

Tom

Patch needs a re-roll and has to be applied to 8.6.x first.

Here'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.

+++ b/core/modules/user/tests/src/Functional/UserCancelTest.php
@@ -73,7 +73,6 @@ public function testUserCancelChangePermission() {
     $this->config('user.settings')->set('cancel_method', 'user_cancel_reassign')->save();
-
     // Create a regular user.

Didn';t mean to remove this line.

Committed and pushed a3bb9d878f to 8.6.x and 938a814da0 to 8.5.x. Thanks!

Created #2978922: Improve batch_process() documentation as a followup.