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.
Problem/Motivation
When running a batch process the page title is not displayed.
The <title> tag is set correctly and the page title is displayed in the browser tab.
Before:
After:
To reproduce, a long batch needs to be run. Using Devel to create nodes is a good way to do this.
Comment | File | Size | Author |
---|---|---|---|
#22 | 9-29-2017 3-28-04 PM.png | 17.64 KB | mahalingam_cs |
#22 | 9-29-2017 3-27-46 PM.png | 16.72 KB | mahalingam_cs |
#20 | batch_missing_title_on-2744663-20.patch | 2.03 KB | jholding |
#20 | batch_missing_title_on-2744663-20-test-only.patch | 1.19 KB | jholding |
#18 | interdiff-2744663-11-18.txt | 914 bytes | jholding |
Comments
Comment #2
John Cook CreditAttribution: John Cook commentedI've found that I can get the title to display if I change
core/modules/system/src/Controller/BatchController.php::batchPage()
to return:But this includes a nested version of the page - complete with <head> tags and javaScript includes.
Comment #6
eelkeblokThis seems to have been the subject of another issue before. #2282035: Title not shown during a batch operation
Comment #7
th_tushar CreditAttribution: th_tushar commentedComment #8
hctomI'll give this a try ;)
Comment #9
th_tushar CreditAttribution: th_tushar commented@hctom,
Just un-assigning the issue. But, you can continue working on the issue though. :)
Thanks!!
Comment #10
hctomOkay, I digged into this issue, and found out, that the
header
region is not set in theBatchController
class. So when this is empty, no header content will be rendered. SeeHtmlRenderer
class (lines 262 and following):As a fix, I now injected a
page_title
render element into theheader
region (if a title is present), which is then rendered at the top of the page. I am not really sure, if this is more like a hacky solution, or if this is the way to go.So please review the patch attached and feel free to provide feedback/customizations.
@th_tushar: Why did you un-assign the issue? Fortunately nobody else was also trying to fix this the same time...
Comment #11
rivimeyI have reviewed the patch and it seems good. I would suggest the attached change to it as a code-quality improvement however, which simplifies title processing and makes it clear that the same value is being used for page title and header.
Comment #13
John Cook CreditAttribution: John Cook commentedThank you @hctom and @rivimey for the patches.
There's a couple of tweaks that are needed.
`$title = isset($output['#title']) ? $output['#title'] : NULL;`
This could introduce a problem as a value in the render array could be set to null. Maybe an empty string would be better.
`// Also inject title as a page header (if available).`
Change to: `// Place the title in the header region.`
Comment #14
jholding CreditAttribution: jholding at manifesto commentedI'm at DrupalCon Vienna, first time sprinter, giving this one a go :)
Comment #15
jholding CreditAttribution: jholding at manifesto commentedActioned as per John's comments #11
Comment #17
John Cook CreditAttribution: John Cook commentedUpdated the summary.
Comment #18
jholding CreditAttribution: jholding at manifesto commentedRe-done patch due diff against wrong branch.
Comment #19
xjmThanks everyone for the work so far here! The screenshots also help explain the issue.
I was just looking into the possibility of adding automated test coverage for this bug. It might be tricky to test during the batch test run, but we do have tests for the start and the end in
core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
. Is the page title gone the whole time? Can we add test assertions that fail in HEAD, but pass when combined with our fix here?It might also be that the title is only missing during the middle of the batch running, but let's check.
Comment #20
jholding CreditAttribution: jholding at manifesto commentedUpdated the page title test to check the text exists in the HTML.
Comment #21
John Cook CreditAttribution: John Cook at Creode commentedAn existing test has been updated by @jholding to test that the title appears on screen as requested by @xjm.
`batch_missing_title_on-2744663-20-test-only.patch` is also an interdiff of `batch_missing_title_on-2744663-18.patch` and `batch_missing_title_on-2744663-20.patch`, and as such is designed to fail on before this issue is committed.
Comment #22
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedApplied patch from #20 and it worked as expected. The titile is appearing for the batch properly. Attached the before and after screenshot.
Comment #24
John Cook CreditAttribution: John Cook at Creode commentedThis implementation, originally by @hctom and improved by @rivimey and @jholding has now been manually tested by @mahalingam_cs.
I asked @andrewmacpherson to look over the implementation from an a11y standpoint and he said that it was OK.
@jholding then altered the title checking test as requested by @xjm. The test shows that the title is missing in 8.4-rc2 and that this patch,
batch_missing_title_on-2744663-20.patch
fixes the issue.With this I've set this issue to RTBC.
I've also changed the issue summary to include the images be @mahalingam_cs are better than the previous ones.
Comment #25
John Cook CreditAttribution: John Cook at Creode commentedComment #26
John Cook CreditAttribution: John Cook at Creode commentedComment #27
eelkeblokAwesome to see this getting fixed. One of those weird little things you're not even sure is an issue but you keep wondering the same thing every time you see it. Awesome work everyone!
Comment #28
gambryMust be mentioned test on #20 is altering an existing
testBatchProgressPageTitle()
test, which doc block reads: "Tests that the batch API progress page shows the title correctly."That bit was introduced on #2282035: Title not shown during a batch operation, but to me it doesn't do what it says. It tests the title can be resolved, but not that the title shows correctly (in fact we have the bug this issue is trying to fix :) ).
Do we understand the reasons why that has been done? Is it still relevant?
If relevant, should we tidy up and maybe split the tests in 2, one checking the title can be resolved and one checking the title shows up correctly?
If not, should we remove the not-relevant bit and keeping only last test?
#20 looks like testing only the initial iteration (
$this->maximumMetaRefreshCount = 0
). I'm not 100% confident with Batch API but is it worth checking title is still there while batch is in progress, i.e.$this->maximumMetaRefreshCount = 1
?Comment #29
John Cook CreditAttribution: John Cook at Creode commented@gambry, the new code does do what the test intends to achieve so I think this is the correct place of it. It may be worth getting some feedback from hussainweb, RavindraSingh, joachim, or alexpott about what the original test was doing but as it done two years ago there might not be any information forthcoming.
Once the batch is running, the title does not get updated so only needs to be run on the initial load. In fact, due to how the batch system works, it might never get to
$this->maximumMetaRefreshCount = 1
depending on the resources available when the tests are run. This might lead to failures as this state can not be guaranteed but the test script expects it.I'm going to set this back to RTBC. It is better to have the issue fixed, with a working test to prevent it happening again, then to postpone it due to uncertainty on the test implementation. We can always create a new issue to fix the test later.
Comment #32
larowlanassertText is deprecated however there are already uses of it in this function so to minimize scope creep, not going to ask for it to be removed.
Committed as 3442528 and pushed to 8.5.x
Cherry-picked as 8db10b6 and pushed to 8.4.x.
Thanks all