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 we install Drupal 8 if we see the page's <title>
tag then there is no value after the Pipe (|). Compared to Drupal 7 this showed "Drupal" keyword. As per the below image:
Proposed resolution
The page title is generated finally in "core/modules/system/templates/html.html.twig" with a Safe Join filter joining strings coming from $variables['head_title']
. If the site "name" is not set then use "Drupal".
Remaining tasks
- Write a patch
- Some existing tests might fail (there are some dealing with the page title). If so then alter those accordingly.
- Write test for this fix if needed.
User interface changes
TBD
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 1.74 KB | joelpittet |
#40 | drupal_8_install_page-2633170-40.patch | 949 bytes | joelpittet |
#38 | drupal_8_install_page-2633170-38.patch | 2.66 KB | joelpittet |
#35 | drupal_8_install_page-2633170-35.patch | 4.15 KB | aneek |
#35 | interdiff-2633170-30-35.txt | 2.05 KB | aneek |
Comments
Comment #2
aneek CreditAttribution: aneek as a volunteer commentedfirst patch.
Comment #3
aneek CreditAttribution: aneek as a volunteer commentedComment #5
joelpittetInstead of hard coding a default couldn't you put a default in config early somehow in the profile? That way if people want empty/FALSE/NULL it will still be that.
Comment #6
aneek CreditAttribution: aneek as a volunteer commented@joelpittet - I believe, the value is coming from
core/modules/system/config/install/system.site.yml
. And the value is only set once the site is installed and the site name is given during the installation phase. Based on your recommendation, where to put the configuration? I think it has to be so generic that even testing profiles can use it. But who will change it to empty/FALSE/NULL if they want to? and how?Please let me know your thoughts.
Comment #7
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and commentedHi,
I have recreated the patch at #2
Before Patch:
After Patch:
Comment #9
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and commentedComment #11
joelpittet@aneek I'm not sure how, it's just strange to default that hardcoded IMO. I was thinking in settings.php or something, but I'm really not sure where is best... The idea would be a distribution install, like commerce would change it to "Drupal Commerce" or "Commerce Kickstart".
Comment #12
aneek CreditAttribution: aneek as a volunteer commented@joelpittet - Okay, I see your point. I haven't looked at the Commerce Kickstart D8 version yet. But I'll look into D7 version of it. That might give me some idea. But lets see what others has to say.
Thanks!
Comment #13
aneek CreditAttribution: aneek as a volunteer commented@Anishnirmal - your patch #9 is blank. Can you please review it once and also post an interdiff.
Comment #14
Anishnirmal CreditAttribution: Anishnirmal as a volunteer and at UniMity Solutions Pvt Limited commentedhi aneek,
I have created the patch again. Please have a look at it and review it.
Before Patch:
After Patch:
Comment #16
nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commentedI recreated the same patch.
Comment #18
imalabya@nesta_ the patch you provided was totally wrong. I guess it removed whole theme.inc.
Have added a patch with the latest pull with a slight change. Let's see what bot has to say.
@joelpittet even I am not sure about hard coding the name, but the
$site_config->get('name')
only once the site is installed fromsystem.site.yml
Checked with other distributions like Lightning and OpenChruch, all suffer the same issue.
Comment #19
imalabyaComment #21
nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commentedYes @malavya, kill me! xD I thought it was well done and went without checking, big mistake!, thank you very much for the comment :)
Comment #22
nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commentedUpload new patch
Comment #24
AlviMurtaza CreditAttribution: AlviMurtaza as a volunteer and at Axelerant commentedAdding a patch with a variable tweak
Comment #26
heykarthikwithuComment #28
emma.mariaWe need to fix the tests for the patch to pass, the patch does not need to be rerolled.
Comment #30
nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commentedFix the Test to the patch.
Credit to @lluvigne please! He help me.
Comment #31
lluvigneI tested manually and looks good. Attached images with preview before and after the patch.
Comment #32
penyaskitoComment #33
aneek CreditAttribution: aneek as a volunteer commentedThanks everyone for this patch!! Lets add this fix to 8.1. :-)
Comment #34
alexpottI think this can be written as
site_config->get('name') ?: 'Drupal'
which is less repetitive.Also it would be good to add a specific assertion for the installer - we can add this to
\Drupal\system\Tests\Installer\InstallerTest
Comment #35
aneek CreditAttribution: aneek as a volunteer commented@alexpott, here is the modified patch.
Comment #36
aneek CreditAttribution: aneek as a volunteer commentedComment #37
dawehnerIMHO we should allow sites to not use any site title, which this doesn't allow yet, in an obvious way. What about simply setting 'Drupal' in
system.site
by default? This also ensures that the behaviour of existing sites isn't touched.Comment #38
joelpittetTo my point in #5 and @dawehner in #37. Here's a patch for that including the tests but reverting the theme.inc changes.
Comment #40
joelpittetWe don't need to change those tests that were failing because they don't load the system.site config.
Comment #41
amit.drupal CreditAttribution: amit.drupal at gai Technologies Pvt Ltd commentedPlease Reviews Patch.
Screenshot-2.png - Before Apply Patch
Screenshot.png - After Apply Patch
Comment #42
lluvigneI think that the latest patch is not ok. We still need the assertion (see #34) on InstallerTest. I tested the patch on #40 and looks good to me.
Comment #44
amit.drupal CreditAttribution: amit.drupal at gai Technologies Pvt Ltd commented@joelpittet :
Please tell me use of function code in your patch.
--- a/core/modules/system/src/Tests/Installer/InstallerTest.php
+++ b/core/modules/system/src/Tests/Installer/InstallerTest.php
@@ -74,4 +74,14 @@ protected function setUpSite() {
parent::setUpSite();
}
+ /**
+ * {@inheritdoc}
+ */
+ protected function visitInstaller() {
+ parent::visitInstaller();
+
+ // Assert the title is correct and has the title suffix.
+ $this->assertTitle('Choose language | Drupal');
+ }
+
}
Comment #45
aneek CreditAttribution: aneek as a volunteer commented#44, this is for testing whether the patch works in the desired way. Thus this
visitInstaller()
is required, I believe.Patch #41 is not valid. Lets go with #40.
Comment #46
aneek CreditAttribution: aneek as a volunteer commentedComment #47
aneek CreditAttribution: aneek as a volunteer commentedCan we make this RTBC based on patch #40
Comment #48
Kumar Kundan CreditAttribution: Kumar Kundan commented# 40 working fine !!!!!.
Comment #49
alexpottSince this is a default config change I've committed to 8.2.x only. I don't think there are any repercussions of committing this to 8.1.x since all sites set this once the install has finished BUT in general I think we should only make default config changes in patch releases for serious bugs. This certainly is not a serious bug.
The site name in SiteConfigureForm does not have a default
so this won't result in any changes in what a user sees here.
Comment #52
amit.drupal CreditAttribution: amit.drupal as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented