Title not shown during a batch operation

Mike, What title we need to show here, please elaborate little bit more.

The problem seems to be in Drupal core. I was able to reproduce this for Cancel User batch operation. To reproduce:
* Create a test user.
* Go to People listing page, select the checkbox for the test user and in 'With selection' dropdown, select 'Cancel the selected user account(s)'.
* This step may not matter but just in case, select 'Delete the account and its content.' on the next page and 'Cancel accounts'.
* Observe on the next page with the scrollbar that there is no title, even though the code sets it to t('Cancelling account').

Apply the patch and repeat the steps above. The title now appears. This also fixes the original issue with the Migrate Upgrade module.

Tests have been my weakness, but I completely agree - we need one. I will see if I can do it today, otherwise, it will be over the weekend.

Just ran this test--the patch applied cleanly, and looks like a good UX improvement...

This is a very rough stab at adding testing of this, not tried it out yet -- let's see what the testbot says.

- I'm not sure where best to put it. The batch tests seem a little messy. The PageTest class describes itself as testing the batch page, but then its actual test method is about testing the theme.
- I've no idea whether the test can actually 'see' the batch page long enough for us to run an assertion on it.

It's probably going to fail without the fix too... let's just check that.

After a long long analysis, I think I got the test running. I have attached both a test only patch (which should fail) and then the complete patch.

The challenge in writing the test was, as already pointed out, that the test can't capture the progress page where the title is shown. Other methods use batch_test_stack to store state information and that is checked later. I planned on doing the same thing but there was no way to find the title. drupal_get_title is deprecated and the snippet shown instead of drupal_get_title did not work. The only way I could get the title in the test was by using _batch_current_set. If we use that, we are not testing the right thing here.

Instead, I set about fixing why the title resolver didn't work. I made the changes in the patch. There is a call to the title resolver in code (which means the fix in #4 would be unnecessary) but that line would never get called due to the if condition. We should probably fix/remove that in a different issue.

+++ b/core/modules/system/src/Controller/BatchController.php
@@ -86,6 +86,7 @@ public function batchPage(Request $request) {
+      $page['#title'] = $output['#title'];

+++ b/core/modules/system/system.routing.yml
@@ -392,6 +392,7 @@ system.batch_page.normal:
   defaults:
     _content: '\Drupal\system\Controller\BatchController::batchPage'
+    _title_callback: '\Drupal\system\Controller\BatchController::batchPageTitle'
   requirements:
     _access: 'TRUE'
   options:
@@ -401,6 +402,7 @@ system.batch_page.json:

@@ -401,6 +402,7 @@ system.batch_page.json:
   requirements:

Are you sure you need both of them?

@dawehner: Actually, we don't. I started with the PHP code and then when I started writing the test, I realized that I would have to use _title_callback to get it to work. I left in the PHP code as it wouldn't really do any harm (probably just a slight chance of confusion). Do you think we should remove it?

If it is not needed drop it, there is one less potential source of a bug.
Did you checked whether you can remove the _title_callback from the json route? This route should not have a title as it is never rendered as html.

I have dropped that line as per #17 and also removed _title_callback from json route. The tests only patch is still the same.

I am just reverting one of the changes removed in #18 to see if that is the reason for the test failure.

This is quite strange. I wonder why the _title_callback on JSON path would affect the regular path. I think this needs more debugging, and might be a flag on a bigger bug.

Re #23 I don't understand. In the failing test in #18, both ways of setting the page title were removed, rather than just one:

diff --git a/core/modules/system/src/Controller/BatchController.php b/core/modules/system/src/Controller/BatchController.php
index b17d26c..35f9c3d 100644
--- a/core/modules/system/src/Controller/BatchController.php
+++ b/core/modules/system/src/Controller/BatchController.php
@@ -86,7 +86,6 @@ public function batchPage(Request $request) {
       drupal_set_page_content($output);
       $page = element_info('page');
       $page['#show_messages'] = FALSE;
-      $page['#title'] = $output['#title'];
 
       $page = $this->render($page);
 
diff --git a/core/modules/system/system.routing.yml b/core/modules/system/system.routing.yml
index 8c1641d..62fa09f 100644
--- a/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml
@@ -402,7 +402,6 @@ system.batch_page.json:
   path: '/batch'
   defaults:
     _controller: '\Drupal\system\Controller\BatchController::batchPage'
-    _title_callback: '\Drupal\system\Controller\BatchController::batchPageTitle'
   requirements:
     _access: 'TRUE'
     _format: 'json'
 

In #22 one was added back and the tests pass again. Am I missing something?

Doing a reroll first.

@jhedstrom: See #15. I removed changes from the JSON route and it failed but it worked when I added it back. Like I said in #23, it was rather strange and indicative of some other bug.

Since it is 4 months, I will try removing it again to see if it still fails after the reroll passes.

Fixing the failure.

Trying out by reverting the change I reverted in #22. This removes the _title_callback from the JSON route (which should not be necessary). This made the tests pass earlier but that was 4 months back. Let's see if this was fixed.

It seems that something has changed to fix the bug that we saw in #18. I think the concern in #15 has been addressed.

Adding the beta evaluation. I don't think there is anything else left here except review.

I tried this with testing UI page, which is still not showing the title.

@joelpittet: Can you describe how you tested this? I tested this by canceling few users and checking for title, and the title was there (as seen in attached). This was not present when run without patch.

Can you give a link to testing ui page so that I can check it there too?

The patch does fix that page, so nice work!

I think my issue is an older and different one but there was a title above and below the progress bar that used to update. Right now the below description is showing but the title above doesn't show up anymore.

I was just running any test. Enable testing module (simpletest) and http://d8.dev/admin/config/development/testing
and enable any test to run.

I was hoping this issue would fix that too.

I see it there too. This was run on simplytest.me but I don't think that should make any difference.

Applied the patch #32 still no luck .

Followed these steps to test before and after applying patch:
The problem seems to be in Drupal core. I was able to reproduce this for Cancel User batch operation. To reproduce:
* Create a test user.
* Go to People listing page, select the checkbox for the test user and in 'With selection' dropdown, select 'Cancel the selected user account(s)'.
* This step may not matter but just in case, select 'Delete the account and its content.' on the next page and 'Cancel accounts'.
* Observe on the next page with the scrollbar that there is no title, even though the code sets it to t('Cancelling account').

Apply the patch and repeat the steps above. The title still not appears
Attached is the screenshot

Please try on fresh installation of 8.0.x

Apply the patch and repeat the steps above. The title now appears
Attached is the screenshot

This means it works, right? I don't understand why you marked it as Needs work. Am I missing something?

Applied the patch but its not working as you have mentioned. Maybe I missed something. Lets wait community response if this works perfactly.

@RavindraSingh: I am not sure if I follow. You said and took screenshot that the title does not appear on batch page. After applying the patch, the title appears (like you confirmed in #39). That is what is supposed to happen with the patch. That is the bug we are fixing here.

Uff, updated the line. seems I did some mistake in writing. As attached screenshot on #39 doesn't show the title with counts.

@hussainweb could you attach the test as a separate patch to illustrate the current breakage? Also, if folks aren't seeing the fix in manual testing, I wonder if the new test is testing the proper behavior?

@jhedstrom I did see the fix in manual testing of the user delete batch. I've got a different issue that I should file once I can confirm it wasn't changed on purpose for the batch in the simple test UI.

@jhedstrom: I am attaching the same patch as #32 and the tests only version of the patch. This should be very similar to the tests only patch in #12 as the tests haven't really changed.

#44, I didn't realize you meant that the title still didn't appear. I tested the exact same thing on simplytest.me and it worked as expected. See the screenshots below for two different batch pages (one of which is the delete user page). I too had selected 'Delete the account and it's content' option.

@joelpittet, do you mean to say you still don't see the title on any of the batch pages? I thought the above screenshot was what you wanted.

@hussainweb different title, it used to live in between the breadcrumbs and the progress bar and it was bold and black title for the batch not the title of the page.

Oh, okay. It does sound like a different issue then. However, it might be related to this. What should the text be on the tests ui page, for instance? I can do a quick check.

As expected, the tests only patch failed with just the failure we were expecting.

This should still be Needs review.

I can't find when it was introduced and or if it was reverted/broken though it's been a while...

I did find something to help me not look completely bonkers.

The Seven style guide has what it is/was supposed to look like.

https://groups.drupal.org/node/283223#Progress

Tried a couple alphas and betas but to no avail.

Good find! I will see if I can dig up something about that. It might be a follow up?

This is how it looks on Drupal 7.

Maybe the design shown in style guide is not implemented yet? That makes it a task and not a bug report (which this is). I think this means we need to work this one out in a followup issue.

Couple of comment fixes here and I think this is good to ship. Thanks for working on this @hussainweb

Yeah my thing is unrelated to this issue, sorry for the divergence.

1. +++ b/core/modules/system/src/Tests/Batch/PageTest.php
   @@ -47,4 +47,18 @@ function testBatchProgressPageTheme() {
   +    // Visit an administrative page that runs a test batch, and check that the
   +    // theme that was used during batch execution (which the batch callback
   +    // function saved as a variable) matches the theme used on the
   +    // administrative page.
   +    $this->drupalGet('batch-test/test-title');
   

   I think this comment is a duplicate/incorrect.

2. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
   @@ -167,6 +167,21 @@ function _batch_test_theme_callback() {
   + * Tests the progress page theme by performing a batch callback.
   

   This isn't testing the 'theme' it's testing the 'title'?

Fixing comments as per #56.

its working as expected. moving this to RTBC.

Committed bce75ca and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

This issue seems to have returned (I can't actually remember ever having seen a title when enabling a module). See #2744663: Batch missing title on screen.