Display the page title, even if "0"

Test please. :) Let's make sure we don't have this embarrassing bug anymore. ;)

This is an excellent issue for someone new to rolling patches and writing tests to get their hands dirty with. Anyone game to try it?

re-rolled

While reproducing this bug i've found another issue. I can't "provide a menu link" with a "Menu link title" value == 0. Should I fill another issue for that?

This patch brings back check_plain() on the title, but I had to change page.tpl.php in themes ( doing if(isset($title)) instead of if($title)).

#7: 578400-7-empty-page-title.patch queued for re-testing.

1. Tested the patch.
2. Checked out the latest version of Drupal HEAD as of a few minutes ago.
3. Reproduced the Garland issue with an article node.
4. Reproduced the issue in Seven by editing the menu title for "admin/content", and verifying it displays 0 in the toolbar and disappears in Seven.
5. Applied the patch.
6. Verified that the patch fixed the issue with the node (in Garland) and the admin page displayed in Seven.

7. Also did a code review and the code looks good.

I think this is all good to go.

Had plenty of time to create a simple test for this. Let's prove it fails without the patch. This one creates a node titled '0' and fails in Garland.

Ok, so now that we proved we have a test which shows it failing, let's include that test with the patch and show it works. Only change included in test is fix for some comment copy-paste. Let's see now.

+++ includes/theme.inc	25 Jun 2010 13:23:21 -0000
@@ -2176,8 +2176,9 @@ function template_preprocess_html(&$vari
+  if (isset($title)) {
...
+++ includes/theme.inc	25 Jun 2010 13:23:21 -0000
@@ -2380,8 +2381,9 @@ function template_preprocess_maintenance
+  if (isset($title)) {
...
+++ themes/garland/page.tpl.php	25 Jun 2010 13:23:21 -0000
@@ -43,7 +43,7 @@
+          <?php if (isset($title)): ?>
...
+++ themes/seven/page.tpl.php	25 Jun 2010 13:23:21 -0000
@@ -5,7 +5,7 @@
+    <?php if (isset($title)): ?>

I think I prefer Damien's #1 patch that used strlen(), so that an empty string is considered the same as NULL, especially since as I read drupal_get_title(), it never returns NULL, unless in bootstrap. Unless we want to consider empty string as a valid title, in which case we need to change check_plain(NULL) to return NULL or not have drupal_get_title() call check_plain() if menu_get_active_title() returns NULL. In any case, let's add whatever we decide with respect to empty string to our test, and make sure our test also includes a situation where menu_get_active_title() returns NULL.

I guess this makes the issue no longer a "Novice" one.

[EDIT: if we decide to leave empty string title as equivalent to a NULL title, let's put the strlen() check into template_preprocess_page(), so that page.tpl.php can be left using isset().]

Powered by Dreditor.

Disclaimer: we found this issue for a live session demonstrating patching and test writing as well as issue collaboration. It proved to be a perfect use case for it. So let's make this even better and do not refrain from taking the patches apart. No offense :)

Let's check first if the issue affects D8. Update: it does

The patch is missing the title tag. Added that part.

Second try

Let's send it to the testbot.

Still needs testing

I'm at the Austin sprint and working on a test for this.

Updated the patch in #23 per line changes, and added tests for edge case where node title is set to 0.

Thanks @aaronschachter! Test case looks good at least from reading it, one nitpick:

+++ b/core/modules/node/src/Tests/NodeTitleTest.php
@@ -65,5 +65,18 @@ function testNodeTitle() {
+    $xpath = '//title';
+    $this->assertEqual(current($this->xpath($xpath)), 0 . ' | Drupal', 'Page title is equal to 0.', 'Node');

You can use assertTitle() here to make this a little shorter :)

--

If you can please post a test-only version followed by a complete patch that would be great, then we can see the fail/pass because the test should fail without the fix. As pictured on https://drupal.org/contributor-tasks/write-tests :)

Sure @cottser I can re-post... Buuut... I was following the same pattern that already exists in the NodeTitleTest, in line 53-54:

xpath = '//title';
$this->assertEqual(current($this->xpath($xpath)), $node->label() .' | Drupal', 'Page title is equal to node title.', 'Node'); 

What's considered best practice here? It would make sense to update that original assertEqual statement I copied from as well... but that's not really within the scope of this issue. Is it best practice to create a separate issue for it and leave the original as is?

Hmm, so it seems like the patch in #23 which I modified to update line numbers in #31 isn't even necessary. I'm using the latest 8.x branch, and the node title correctly displays as 0 without patching anything.

This was my first attempt at submitting a patch to core, and I am realizing today, I did not actually test whether or not the 0 page title worked from the start :|

Hard to say whether or not it was broken at the time of #23, but anyhoo here are some tests to hopefully close this issue.

I did a git bisect and confirmed that this was fixed in #2216437: Entity labels are not in-place editable on "full entity page" (prime example: node title). So let's add this test and then go to 7.x which probably needs a bugfix + tests.

Is it best practice to create a separate issue for it and leave the original as is?

Sounds good to me.

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

Patch for D7.
Please check.

Thanks @joshi.rohit100. If we can get a test-only version to see that the test fails without the fix that would be very helpful. An example of this can be seen on https://www.drupal.org/contributor-tasks/write-tests (below point 13).

1. +++ b/includes/theme.inc
   @@ -2504,9 +2504,10 @@ function template_preprocess_html(&$variables) {
   -  if (drupal_get_title()) {
   +  $title = drupal_get_title();
   +  if (isset($title)) {
   

   Won't $title always be set here?

2. +++ b/modules/node/node.test
   @@ -1873,6 +1873,16 @@ class NodeTitleTestCase extends DrupalWebTestCase {
   +    ¶
   

   Looks like a stray tab character here, please remove per https://drupal.org/coding-standards#indenting.

Please see test-only version of the patch which also removes stray tab character there.

Thank you @amitgoyal!

Nevermind my #39.1, didn't realize that title would be null (check out drupal_set_title()). So now that we've seen the test fail we need a complete patch without the stray tab character.

Updated the patch. Please review now.

+++ b/modules/node/node.test
@@ -1873,6 +1873,16 @@ class NodeTitleTestCase extends DrupalWebTestCase {
+    //Test edge case where node title is set to 0.

Ah, @amitgoyal's patch fixed this but we need to add a space here after the // and before "Test edge case…" per https://www.drupal.org/node/1354#inline.

Otherwise looks good :)

Updated patch as @Cottser suggested in #44.

Looks good, thanks!

Hmm, actually I went through manually and tested this and I think this patch is supposed to fix not just the title tag but also the <h1> when viewing a node with a title of '0', and the D7 patch only fixes the first case. This also exposes a weakness in the test (we may need to revisit the 8.x test to make it more solid) because the test-only patch in #40 should have 2 fails.

I'm new to fixing issues but I thought I'd give it a try. I fixed it within the bartik theme.

@gvorbeck, thanks!

Once you add any patch also change Status from Need Work to Need Review.

Re-rolled patch #48.

Re-rolled #51 but it seems same

I'm new to patching/testing, but have manually tested the patch and can confirm it works. Is there a possibility that the test itself is faulty? Is it possible to get a copy of that for review?

Also, I looked for the reference in modules/node/node.test mentioned by @Cottser on comment #44 (https://www.drupal.org/node/578400#comment-8946193) but I can't find that comment, so I'm wondering if I have the same files.

Sorry if my comment isn't helpful, trying to get up to speed here to make some useful contributions.

The D7 Testbot will occasionally crash during a test for some reason. If you're not sure why a test failed, check the results. If the failure was in an unrelated system and it says "The test failed to complete for some reason," then it's a good bet that it was one of these random failures. In that case, just re-test the patch.

Doing changes in .tpl.php will be restricted to theme specific. Adding code on includes/theme.inc will resolve this specific problem of "Display the page title, even if 0".

Thanks @makbul_khan8. Here is some feedback.

+++ b/includes/theme.inc
@@ -2676,6 +2676,11 @@ function template_process_page(&$variables) {
+  // Display the page title, even if "0" https://www.drupal.org/node/578400
...
+    $variables['title'] = $variables['title']. ' ';
+  }

Shall we check for $variables['title'] === "0" instead?

These need some updates to conform to the coding standargs https://www.drupal.org/coding-standards

* Add a space after if
* Add a space before the string concatenation operator
* Comment should end with a full stop (period)

Tackling this for #devdays

Re-rolled, fixed the whitespace issue, and used === '0' as suggested in #68, since lots of things can == 0

(Hopefully the previous FAILED was just a "server burp.")

Thank you @mw4ll4c3!

1. +++ b/includes/theme.inc
   @@ -2680,6 +2680,11 @@ function template_process_page(&$variables) {
   +  // Display the page title, even if "0" https://www.drupal.org/node/578400
   

   We don't usually include links to d.o issues in code comments unless they are @todos.

2. +++ b/includes/theme.inc
   @@ -2680,6 +2680,11 @@ function template_process_page(&$variables) {
   +  if ($variables['title'] === '0') {
   +    $variables['title'] = $variables['title'] . ' ';
   +  }
   

   This doesn't seem like a proper fix to me, it's just adding whitespace.

I agree with Cottser that adding a space to the title isn't a proper solution. I don't see any other way to display a title of '0' except changing the conditions in the template files from if ($title) to something like if (is_string($title)). This could mean that quite a few templates or themes need to change if they want to handle this edge case.

#45 and others used isset($title) which I think should work in all cases but is_string($title) does a little better to indicate what we're testing for.

#40 appears to be the most recent test only patch so I started there. It had an issue where the second test passed when it shouldn't. I believe I've fixed that. Here's a test only patch and and interdiff with 40.

Trigger testbot

A new full patch and interdiff with full patch #45.

Forgot status for testbot's attention again.

Edit: looks like I've got a mistake still.

$xpath = '//h1[@id="page-title"]';
$this->assertEqual(current($this->xpath($xpath)),
  '0',
  'Node title is displayed as 0.',
  'Node');
debug(current($this->xpath($xpath)));

Debug outputs:

SimpleXMLElement::__set_state(array(
   '@attributes' => 
  array (
    'class' => 'title',
    'id' => 'page-title',
  ),
))

Manually testing shows that the page title is omitted before the patch and present after, but I gather from the test's debug() that it IS outputting an H1 with id of "page-title" but the element is empty.
I'm not sure if that's a problem in my test or a problem in the template outputting the title.

Should have it fixed this time. Local testing has the test only patch failing both tests with both passing after the full patch.

I reviewed #82 on a 7.38 install and confirmed that the page title "0" does appear. This patch was successful for me.

Last patch works properly, attaching images

Putting is_string() throughout all tpl.php files like that really doesn't look like the right solution. Also, are we really sure you can never pass an integer to drupal_get_title() and have it work?

Maybe isset() rather than is_string() would work, though if the solution involves changing the tpl.php files then it still means this won't work with lots of non-core themes unless they update too. It's such an edge case anyway, maybe we shouldn't fix it at all if we can't fix it consistently? Or fix the $head_title part only, since at least that's simpler?

Hi,
I have worked in article content type to display the title as "0", its working fine.

Tested the patch.

It works for me.

After applying the patch.

I would like to work on this issue.

This patch works for me.

I always refer to http://php.net/manual/en/types.comparisons.php

if ($title) will never work for 0 or "0".
Also, changing to use either isset/is_string will cause an empty string to print, which would be an even more problematic regression.

Adding the inter diff and the new patch.

I am not able to understand this error.

Can someone help me explain the defect of this patch?

The failing test is looking for the title output on the page (as opposed to in the head). I think that your problem is here:

+++ b/themes/bartik/templates/page.tpl.php
@@ -99,7 +99,7 @@
+          <?php if (!empty($title) || $title === 0): ?>

We'd need to compare to the string 0, not integer 0.

Fixed the string comparison and re-uploaded patch.

Fixed review comments and uploading patch.

Why don't we use strlen to check for the title length.
It works for all the possible cases while skipping empty strings.

Updated the patch. Uploading the interdiff too.
This worked for me.
Adding the screenshot along with the patch.

Another try, let's see.
Using strlen(trim($title)) as the check

All tests are passing. Good work Mehul

@therealssj, Nice work. The patch was tested successfully.

Removing 'Needs tests' (since there is a test now)

I know this issue is really old and I know the approach was verified, but I really don't want to spend 6 function calls for such an edge case on every page.

I would suggest:

  $title = drupal_get_title();
  if (isset($title) && $title !== '') {
     $head_title = 
  }
  else {
     $title = NULL;
  }

Then use isset($title) in page.tpl.php, which also matches the original twig solution with "title is defined".

Explanation:

isset() costs almost zero performance as it is a build-in, but strlen(trim()) costs 2 function calls and two string traversals.

@Fabianx the reason we didn't go with isset in the first place was bcoz @tim.plunkett pointed out that it would cause regression with allowing the printing of empty strings ( though I am not sure how would one be able to save an empty string )

Other than that using isset($title) in the twig files seems to be correct and using strlen(trim()) there is useless.
I have changed this in the current patch.

Attached wrong patch file before.

#124:

Thank you for your patch!

Unfortunately you missed the else { } part and timplunkett is right, but my else {} part in #122 ensures that if the title is empty it is unset and isset() works then.

@Fabianx I don't think we need that $title = NULL in the else part.
This is how the following lines look

$title = drupal_get_title();
  if (strlen(trim($title))) {
    $head_title = array(
      'title' => strip_tags($title),
      'name' => check_plain(variable_get('site_name', 'Drupal')),
    );
  }
  else {
    $head_title = array('name' => check_plain(variable_get('site_name', 'Drupal')));
    if (variable_get('site_slogan', '')) {
      $head_title['slogan'] = filter_xss_admin(variable_get('site_slogan', ''));
    }
  }
  $variables['head_title_array'] = $head_title;
  $variables['head_title'] = implode(' | ', $head_title);

I am not sure how $head_title is processed further but from the above lines it does look like that we pass$title in the $head_title array and in the else we are not passing it so by default I guess it becomes null.

As for why I didn't go with isset($title) && $title !== '' is that it wouldn't work for a string like ' ', I maybe over thinking so I would like to know what you think.

#126: The problem is:

Lets assume drupal_get_title() returns ''.

$title = drupal_get_title(); // $title === ''

Now do:

if ($title) {
  echo "I have a title."
}

works as it won't display the title.

if (isset($title)) {
  echo "I have a title."
}

won't work as it would show that is has a title, but it is empty actually.

Therefore the solution is to do above:

if (strlen(trim($title))) {
}
else {
  // Reset the title to NULL, so it can be checked with isset().
  $title = NULL;
}

Afterwards:

if ($title) {
  echo "I have a title."
}

works as it won't display the title, as NULL fails this check (BC for existing templates).

if (isset($title)) {
  echo "I have a title."
}

works too as the title is actually NULL, which isset() returns as FALSE.

Does that make the plan more clear why we need a $title = NULL?

Thank you for your work on this!

@Fabianx, as @therealssj pointed out, there might not be any way in the UI to ever save an empty string in the required Title field, so coding for $title = NULL seems unnecessary?

#128 There is many possibilities for title to get to be '', not just the UI, so the code will be needed to work like this for BC reasons.

I'd like to chime in and proceed on this issue.

If I understand it correctly, the situation is as following, right?

as is:

• node title = '0' ... not displayed (the bug)
• node title = '' (empty string) ... not displayed
• node title = ' ' (string only with spaces) ... displayed (this is also a bug to be fixed in this issue?)
• node title = NULL ... not displayed

to be:

• node title = '0' ... displayed
• node title = '' (empty string) ... not displayed
• node title = ' ' ... (string only with spaces) not displayed
• node title = NULL ... not displayed

I believe this patch will work If my recognition above is correct.

As Fabianx explained, we need to prevent empty titles to be displayed. Since $title in the page.tpl.php template is prepared not in temlate_preprocess_html() but in template_process_page() and I added the following lines into the patch instead of adding else section into temlate_preprocess_html(). I believe this is a right approach.

@@ -2697,7 +2698,8 @@ function template_process_page(&$variables) {
     $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
   }
   if (!isset($variables['title'])) {
-    $variables['title'] = drupal_get_title();
+    $title = drupal_get_title();
+    $variables['title'] = strlen(trim($title)) ? $title : NULL;
   }

In addition, I think it's better to add tests for the cases 2 and 3 above (we have a test for the case 1 in the previous patch). So I added such tests, too.

I'm sorry to interrupt. I'd like someone to review this. Thank you.

Thanks for your work on this!

Nice work on the ?: I almost missed that you implemented my solution :).

I wonder why tests fail though ...

I'm sorry to have tried the test again and again. There seems to be errors randomly but the patch passed the tests after a few trial!

@Fabianx thank you for your reply! Theme builders may use if ($title) { ... } pattern in page.tpl.php in their custom themes and we should keep the existence of $title variable with value NULL in page.tpl.php when strlen(trim($title)) becomes 0 exactly as you told.

The status of this issue became "Needs work" because of the random failure and I'd like to move this to NR again.

This is even RTBC - indeed just random failures! Great work!

I would like a "change record" or an documentation update, where we state that isset() is preferred for checking title variables to ensure that "0" and other evaluation to FALSE can be properly displayed.

Not really sure where.

Will discuss with Stefan and David at some time.

Besides this, this looks really good! Thanks so much!

Committed and pushed to 7.x, thanks!

This still needs a change record.

Adding for release notes, tag

In addition to the missing change record, I think there's another problem here that needs more discussion. Therefore I rolled this patch back for now.

Currently in Drupal, you can do something like $variables['title'] = '' in a preprocess function and that will suppress the title - that is pretty reasonable code.

But with this patch, the title is only modified to NULL in some cases not others, so won't the above code stop working and result in the title HTML being printed to the page (and potentially breaking the layout)?

I think we need a solution that allows the above code to still work.

Also, the isset() in templates is a bit ugly... seems like it would be cleaner to define a new (boolean) $has_title variable and check that?

I do agree with the reasoning for the revert.

I think we can instead solve this with an invisible UTF-8 character to prevent PHP from evaluating "0" to FALSE:

e.g. http://www.fileformat.info/info/unicode/char/200B/index.htm