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:
D7vsD8_page_title

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

  1. Write a patch
  2. Some existing tests might fail (there are some dealing with the page title). If so then alter those accordingly.
  3. Write test for this fix if needed.

User interface changes

TBD

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#41 Screenshot-2.png447.6 KBamit.drupal
#41 Screenshot.png449.22 KBamit.drupal
#41 drupal_8_install_page-2633170-41.patch343 bytesamit.drupal
#40 interdiff.txt1.74 KBjoelpittet
#40 drupal_8_install_page-2633170-40.patch949 bytesjoelpittet
#38 drupal_8_install_page-2633170-38.patch2.66 KBjoelpittet
#35 drupal_8_install_page-2633170-35.patch4.15 KBaneek
#35 interdiff-2633170-30-35.txt2.05 KBaneek
#31 before patch 30.png1.6 MBlluvigne
#31 after patch 30.png1.6 MBlluvigne
#30 drupal_8_install_page-2633170-30.patch3.44 KBnesta_
#30 inderdiff-2633170-26-30.txt1.74 KBnesta_
#26 2633170-26.patch1.7 KBheykarthikwithu
#24 drupal-8-install-page-2633170-24.patch1.37 KBAlviMurtaza
#22 drupal_8_install_page-2633170-22.patch1.32 KBnesta_
#18 interdiff-2633170-17.txt728 bytesimalabya
#18 2633170-17.patch1.32 KBimalabya
#16 drupal_8_install_page-2633170-16.patch68.79 KBnesta_
#16 interdiff.txt1.33 KBnesta_
#14 page_title_on_d8_installation-2633170-14.patch1.33 KBAnishnirmal
#9 page_title_on_d8_installation-2633170-9.patch0 bytesAnishnirmal
#7 page_title_on_d8_installation-2633170-#7.patch1.33 KBAnishnirmal
#7 after patch.jpg161.99 KBAnishnirmal
#7 before patch.jpg115.36 KBAnishnirmal
#2 2633170-02-page-title-fix.patch1.33 KBaneek
D7vsD8_page_title.png264.96 KBaneek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aneek created an issue. See original summary.

aneek’s picture

aneek’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2633170-02-page-title-fix.patch, failed testing.

joelpittet’s picture

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

aneek’s picture

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

Anishnirmal’s picture

Status: Needs work » Needs review
Issue tags: +#ChennaiDrupalGroup
FileSize
115.36 KB
161.99 KB
1.33 KB

Hi,
I have recreated the patch at #2

Before Patch:
before patch

After Patch:
after patch

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 54b038b..f53f032 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -1280,11 +1280,13 @@ function template_preprocess_html(&$variables) {
     // Do an early render if the title is a render array.
     $variables['page']['#title'] = (string) \Drupal::service('renderer')->render($variables['page']['#title']);
   }
+  $site_name = (!empty($site_config->get('name')) ? $site_config->get('name') : 'Drupal');
+
   if (!empty($variables['page']['#title'])) {
     $head_title = array(
       // Marking the title as safe since it has had the tags stripped.
       'title' => Markup::create(trim(strip_tags($variables['page']['#title']))),
-      'name' => $site_config->get('name'),
+      'name' => $site_name,
     );
   }
   // @todo Remove once views is not bypassing the view subscriber anymore.
@@ -1292,11 +1294,11 @@ function template_preprocess_html(&$variables) {
   elseif ($is_front_page) {
     $head_title = array(
       'title' => t('Home'),
-      'name' => $site_config->get('name'),
+      'name' => $site_name,
     );
   }
   else {
-    $head_title = ['name' => $site_config->get('name')];
+    $head_title = ['name' => $site_name];
     if ($site_config->get('slogan')) {
       $head_title['slogan'] = strip_tags($site_config->get('slogan'));
     }

Status: Needs review » Needs work

The last submitted patch, 7: page_title_on_d8_installation-2633170-#7.patch, failed testing.

Anishnirmal’s picture

Status: Needs work » Needs review
FileSize
0 bytes

Status: Needs review » Needs work

The last submitted patch, 9: page_title_on_d8_installation-2633170-9.patch, failed testing.

joelpittet’s picture

Issue tags: +CMI

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

aneek’s picture

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

aneek’s picture

@Anishnirmal - your patch #9 is blank. Can you please review it once and also post an interdiff.

Anishnirmal’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

hi aneek,
I have created the patch again. Please have a look at it and review it.

Before Patch:
before patch

After Patch:
after patch

Status: Needs review » Needs work

The last submitted patch, 14: page_title_on_d8_installation-2633170-14.patch, failed testing.

nesta_’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
68.79 KB

I recreated the same patch.

Status: Needs review » Needs work

The last submitted patch, 16: drupal_8_install_page-2633170-16.patch, failed testing.

imalabya’s picture

@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 from system.site.yml
Checked with other distributions like Lightning and OpenChruch, all suffer the same issue.

imalabya’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2633170-17.patch, failed testing.

nesta_’s picture

Yes @malavya, kill me! xD I thought it was well done and went without checking, big mistake!, thank you very much for the comment :)

nesta_’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

Upload new patch

Status: Needs review » Needs work

The last submitted patch, 22: drupal_8_install_page-2633170-22.patch, failed testing.

AlviMurtaza’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

Adding a patch with a variable tweak

Status: Needs review » Needs work

The last submitted patch, 24: drupal-8-install-page-2633170-24.patch, failed testing.

heykarthikwithu’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Status: Needs review » Needs work

The last submitted patch, 26: 2633170-26.patch, failed testing.

emma.maria’s picture

Assigned: aneek » Unassigned

We need to fix the tests for the patch to pass, the patch does not need to be rerolled.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nesta_’s picture

Status: Needs work » Needs review
Issue tags: +DrupalCampES
FileSize
1.74 KB
3.44 KB

Fix the Test to the patch.

Credit to @lluvigne please! He help me.

lluvigne’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.6 MB
1.6 MB

I tested manually and looks good. Attached images with preview before and after the patch.

penyaskito’s picture

aneek’s picture

Thanks everyone for this patch!! Lets add this fix to 8.1. :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -1284,7 +1284,7 @@ function template_preprocess_html(&$variables) {
+      'name' => !empty($site_config->get('name')) ? $site_config->get('name') : 'Drupal',

@@ -1292,11 +1292,11 @@ function template_preprocess_html(&$variables) {
+      'name' => !empty($site_config->get('name')) ? $site_config->get('name') : 'Drupal',
...
+    $head_title = ['name' => !empty($site_config->get('name')) ? $site_config->get('name') : 'Drupal'];

@@ -1449,7 +1449,7 @@ function template_preprocess_maintenance_page(&$variables) {
+  $variables['site_name'] = !empty($site_config->get('name')) ? $site_config->get('name') : 'Drupal';

I 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

aneek’s picture

@alexpott, here is the modified patch.

aneek’s picture

Status: Needs work » Needs review
dawehner’s picture

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

joelpittet’s picture

To my point in #5 and @dawehner in #37. Here's a patch for that including the tests but reverting the theme.inc changes.

Status: Needs review » Needs work

The last submitted patch, 38: drupal_8_install_page-2633170-38.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
949 bytes
1.74 KB

We don't need to change those tests that were failing because they don't load the system.site config.

amit.drupal’s picture

Please Reviews Patch.
Screenshot-2.png - Before Apply Patch
Screenshot.png - After Apply Patch

lluvigne’s picture

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

Status: Needs review » Needs work

The last submitted patch, 41: drupal_8_install_page-2633170-41.patch, failed testing.

amit.drupal’s picture

@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');
+ }
+
}

aneek’s picture

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

aneek’s picture

aneek’s picture

Status: Needs work » Needs review

Can we make this RTBC based on patch #40

Kumar Kundan’s picture

Status: Needs review » Reviewed & tested by the community

# 40 working fine !!!!!.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Since 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

    $form['site_information']['site_name'] = array(
      '#type' => 'textfield',
      '#title' => $this->t('Site name'),
      '#required' => TRUE,
      '#weight' => -20,
    );

so this won't result in any changes in what a user sees here.

  • alexpott committed 0565b28 on 8.2.x
    Issue #2633170 by nesta_, Anishnirmal, aneek, joelpittet, amit.drupal,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

amit.drupal’s picture