Problem/Motivation

#1532512: Ensure empty item-list title h3's are never generated introduced this bug. Typecasting this prevents themes that wish to pass renderable arrays as the title.

Proposed resolution

Remove typecasting and let Twig handle the checking in item-list.html.twig:
{% if title is not empty %}

Remaining tasks

None

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#22 interdiff.txt1.74 KBjoelpittet
#22 2260059-fix-itemlist-title-22-pass.patch2.43 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,774 pass(es). View
#22 2260059-fix-itemlist-title-22-fail.patch1.46 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,643 pass(es), 2 fail(s), and 1 exception(s). View
#21 2260059-21-titletypecasted.patch1.97 KBLinL
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,234 pass(es). View
#19 2260059-18-titletypecasted.patch1.99 KBSebCorbin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es). View
#7 drupal8-title-typecasted-2260059-7.patch964 byteser.pushpinderrana
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,611 pass(es). View

Comments

Cottser’s picture

Title: Title in item_list is typecasted as string » Title in template_preprocess_item_list() is typecasted as string
Issue summary: View changes
Issue tags: +Novice

Thanks for creating the issue. Tagging as Novice and tweaking some wording.

er.pushpinderrana’s picture

Status: Active » Needs review
FileSize
485 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,579 pass(es). View

Removed string typecasting from title in attached patch.

m1r1k’s picture

Status: Needs review » Needs work

I think we need something like $variables += array('title' => ''); but not useless redefinition.

er.pushpinderrana’s picture

Miroshnik, I completely agree with you and I would remove that extra typecasting code from next patch. Could you please elaborate this issue for me in detail. I am digging into code but not finding the exact scenario to reproduce this issue.

m1r1k’s picture

We're working Bootstrap theme porting to D8. It has enchainment for item_list theme that allow to override list title output and provide renderable array instead of pure string. E.g. we won't to use

instead of recently hardcoded

. (string) $variables['title'] in template_preprocess prevent it. As this typecasting doesn't add anything, doesn't prevent anything from error or notice but breaks original preprocess flow it has to be removed.

Cottser’s picture

Thanks for working on this @er.pushpinderrana!

The line from preprocess can be removed, and the default item-list.html.twig should have 'if title' changed to 'if title is not empty'.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
964 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,611 pass(es). View

Thankyou Miroshnik and Cottser for explaining this issue in detail.

I have made the required changes in files and also tested its impact by calling item_list in my custom module. I am sharing some code with you to confirm my approach towards fixing this issue.

theme.inc: Removed this unused code $variables['title'] = (string) $variables['title'];

item-list.html.twig: Replaced {%- if title -%} with {%- if title is not empty -%}.

I have tested this function by creating module and using '#theme' => 'item_list', for rendering output.

$items = array('Test One', 'Test B', 'Test C', 'Test D');
    return array(
      '#theme' => 'item_list',
      '#items' => $items,
      '#title' => '' // Test Title Empty Issue
    );

I have noticed one thing when I was testing this issue found {%- if title -%} also works fine and take care of empty value. I mean if title is empty then <h3></h3> don't render on page. But when I gone through twig template engine documentation I found {%- if title is not empty -%} is there to validate not empty condition. So in this patch I am using this code but still I am unable to reproduce any case in which {%- if title -%} code fail. If you have any scenario for this please share with me.

m1r1k’s picture

Status: Needs review » Reviewed & tested by the community

Well, it should work with such values:

  • '#title' => 'text', // output

    text

  • '#title' => '', // do not output anything
  • '#title' => NULL, // do not output anything

and allow handle #title in custom hook_preprocess_item_list() as I want.
In our particular case we're passing '#title' => array('title' => 'TEXT', 'attributes' => ...),

However patch looks fine.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Hmm, if we can avoid adding the 'is not empty' we probably should. This should also have some automated test coverage as well, that should help decide what solution to go with here.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
505 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,510 pass(es). View

Cottser I have checked both code for number of cases and found both work fine except following case.

'#title' => 0 if we use {%- if title is not empty -%} it render title value <h3>0</h3>but {%- if title -%} print nothing.

'#title' => '0' if we use {%- if title is not empty -%} it print title value <h3>0</h3> but {%- if title -%} print nothing.

In this case we pass '0' value in title then {%- if title -%} don't render title because it treat this as FALSE condition but <code>{%- if title is not empty -%} treat this as TRUE condition. I am sure no one like to make title '0' of any page so we can go ahead with {%- if title -%}. In this patch I am using {%- if title -%} instead of {%- if title is not empty -%}.

PS: I also tried both code by passing FALSE in title and both works fine. Don't render <h3>FALSE</h3>.

markcarver’s picture

Status: Needs review » Needs work

Please use {%- if title is not empty -%}. The only reason this issue exists is because #1532512: Ensure empty item-list title h3's are never generated was trying to fix the use case when a title value was 0.

Still needs tests to ensure this works properly.

markcarver’s picture

FWIW, this perhaps should be a general policy when dealing with Twig templates in general. Maybe a new issue should be created to find all cases of {% if ... %} through-out core and replace them with {% if ... is not empty %}.

Cottser’s picture

I forgot about the pesky '0' case. So for this one, 'is not empty' for sure.

I'm not sure about a blanket 'is not empty' but I agree it would make sense to evaluate conditional logic in templates. Not really anything new though, this issue would still exist if we were still on PHPTemplate.

er.pushpinderrana’s picture

In #7 patch, I have use 'is not empty' and its working fine. I have tested this patch for most of cases, so we can go ahead with this patch.

drupal8-title-typecasted-2260059-7.patch

Cottser’s picture

Yep #7 looks good. This needs automated test coverage:

https://drupal.org/contributor-tasks/write-tests

markcarver’s picture

er.pushpinderrana’s picture

Cottser, I need your support on this. As I'm digging into drupal 8 test cases, found testItemList() is already there. When I ran this test case, it executed successfully and produces following message.

Empty theme_item_list() generates no output. Other FunctionsTest.php 42 Drupal\system\Tests\Theme\FunctionsTest->testItemList() Pass

Do I need to write another test case here or need to edit this test case itself. Please suggest.

Cottser’s picture

We would want to add a test case/test code with a title of "0" and make sure the 0 is output.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es). View
gnuget’s picture

Issue summary: View changes
Issue tags: -Novice, -Needs tests

I'm just updating the summary of this issue.

LinL’s picture

FileSize
1.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,234 pass(es). View

Rerolled for PSR-4. No other changes.

joelpittet’s picture

FileSize
1.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,643 pass(es), 2 fail(s), and 1 exception(s). View
2.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,774 pass(es). View
1.74 KB

I've added one more test for the original issue summary bug:

Typecasting this prevents themes that wish to pass renderable arrays as the title.

Also think this one is RTBC thanks for finding the bug and fixing this.

The last submitted patch, 22: 2260059-fix-itemlist-title-22-fail.patch, failed testing.

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Additional tests address concerns identified in the issue summary and in comments. Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ad64edd and pushed to 8.0.x. Thanks!

  • alexpott committed ad64edd on 8.0.x
    Issue #2260059 by er.pushpinderrana, joelpittet, LinL, SebCorbin | Mark...

Status: Fixed » Closed (fixed)

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