Task

Use Twig instead of PHPTemplate

Remaining

  • Replace this theme function with .html.twig equivalent template
  • Update the preprocess function for the .html.twig templates
  • Update the hook_theme definitions
  • Clean up the syntax in the template file so that:
    • tags are not printed as variables
    • whitespace is printed as desired

Related

#311011: Replace links.html.twig with item-list--links.html.twig
#1800726: Convert theme_item_list to item-list.twig
#1885560: [meta] Convert everything in theme.inc to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1904582: Refactor theme_item_list to separate functions for ordered lists, unordered lists.
#1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
#1828536: Rename 'type' variable of theme_item_list() to 'list_type'

Files: 
CommentFileSizeAuthor
#120 interdiff-1939062-114-120.txt2.24 KBmdrummond
#120 theme-item-list-1939062-120.patch6.29 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 63,565 pass(es). View
#114 interdiff-1939062-109-114.txt1.89 KBmdrummond
#114 theme-item-list-1939062-114.patch6.46 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 63,252 pass(es), 6 fail(s), and 6 exception(s). View
#109 interdiff-1939062-103-109.txt1.86 KBmdrummond
#109 theme-item-list-1939062-109.patch6.47 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 63,129 pass(es), 140 fail(s), and 54 exception(s). View
#103 theme-item-list-1939062-103.patch6.27 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 63,256 pass(es), 69 fail(s), and 51 exception(s). View
#102 theme-item-list-1939062-102.patch6.27 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 63,214 pass(es), 69 fail(s), and 51 exception(s). View
#99 theme_item_list-1939062-99.patch6.41 KBjerdavis
FAILED: [[SimpleTest]]: [MySQL] 63,077 pass(es), 10 fail(s), and 6 exception(s). View
#89 theme_item_list-1939062-89.patch6.68 KBekl1773
PASSED: [[SimpleTest]]: [MySQL] 59,034 pass(es). View
#89 interdiff-1939062-82-89.txt1.39 KBekl1773
#82 theme_item_list-1939062-82.patch6.74 KBdale42
FAILED: [[SimpleTest]]: [MySQL] 57,928 pass(es), 2 fail(s), and 0 exception(s). View
#66 theme_item_list-1939062-66.patch6.37 KBhussainweb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_item_list-1939062-66.patch. Unable to apply patch. See the log in the details link for more information. View
#64 theme_item_list-1939062-64.patch6.37 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 56,811 pass(es). View
#64 theme_item_list-1939062-interdiff-62-64.txt563 byteshussainweb
#62 twig-item_list-1939062-62.patch6.51 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] 56,736 pass(es), 8 fail(s), and 1,415 exception(s). View
#62 interdiff.txt985 bytesdrupalninja99
#58 twig-item_list-1939062-58.patch6.67 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 56,298 pass(es), 8 fail(s), and 1,377 exception(s). View
#56 theme_item_list-1939062-56.patch7.12 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 58,366 pass(es). View
#52 1939062-52.patch9.17 KBgabesullice
FAILED: [[SimpleTest]]: [MySQL] 57,995 pass(es), 1 fail(s), and 0 exception(s). View
#42 1939062-42.patch9.14 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,351 pass(es). View
#42 interdiff.txt1.23 KBCottser
#38 1939062-38.patch9.28 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,302 pass(es). View
#38 interdiff.txt555 bytesCottser
#37 drupal-1939062-37.patch9.28 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 55,362 pass(es). View
#37 interdiff.txt1.43 KBjenlampton
#35 drupal-1939062-35.patch9.13 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es). View
#35 drupal-1939062-35.patch-interdiff.txt4.41 KBc4rl
#30 1939062-30.patch7.45 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es). View
#28 twig-convert_item_list-1939062-28.patch8.61 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-convert_item_list-1939062-28.patch. Unable to apply patch. See the log in the details link for more information. View
#28 interdiff.txt2.16 KBjenlampton
#25 drupal-twig-item-list--1939062-24.patch7.25 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 54,345 pass(es). View
#25 drupal-twig-item-list--1939062-20-25--interdiff.txt2.3 KBsteveoliver
#20 drupal-twig-item-list--1939062-15-20-interdiff.txt5.23 KBsteveoliver
#20 drupal-twig-item-list--1939062-20-clean.patch6.69 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 53,801 pass(es), 6 fail(s), and 0 exception(s). View
#20 drupal-twig-item-list--1939062-20-temp.patch8.52 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 54,005 pass(es). View
#15 drupal-twig-item-list--1939062-13-15-interdiff.txt770 bytessteveoliver
#15 drupal-twig-item-list--1939062-15.patch8.76 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 53,905 pass(es). View
#13 drupal-twig-theme-item-list--1939062-11-13--interdiff.txt1.92 KBsteveoliver
#13 drupal-twig-theme-item-list--1939062-13.patch8.78 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 54,027 pass(es). View
#11 drupal-twig-theme-item-list--1939062-11.patch8.71 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 53,736 pass(es), 89 fail(s), and 1 exception(s). View
#11 drupal-twig-theme-item-list--1939062-9-11-interdiff.txt1.98 KBsteveoliver
#9 drupal-twig-theme-item-list--1939062-9.patch7.16 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 53,747 pass(es), 141 fail(s), and 1 exception(s). View
#7 interdiff.txt593 bytesjoelpittet
#7 twig-item-list-1939062-7.patch6.48 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,467 pass(es), 94 fail(s), and 454 exception(s). View
#6 twig-item-list-1939062-6.patch6.51 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,459 pass(es), 55 fail(s), and 446 exception(s). View
#6 interdiff.txt3.04 KBjoelpittet
#2 core-convert_item_list_to_twig-1939062-2.patch5.05 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 52,773 pass(es), 217 fail(s), and 2,470 exception(s). View

Comments

Cottser’s picture

Issue summary: View changes

Adding sandbox issue to related issues

jenlampton’s picture

Assigned: Unassigned » jenlampton

dibbs

jenlampton’s picture

Issue summary: View changes

Adding link to ol/ul issue

jenlampton’s picture

Assigned: jenlampton » Unassigned
Status: Active » Needs review
FileSize
5.05 KB
FAILED: [[SimpleTest]]: [MySQL] 52,773 pass(es), 217 fail(s), and 2,470 exception(s). View

Here's my first attempt!
It currently moves the item-list class onto the UL tag (but still leaves the item-list class on the surrounding div tag so CSS in core won't break). Also, whitespace in the rendered HTML is a little funny, but I spent about a half hour fighting with the whitespace controllers and am officially giving up here. Open to feedback :)

jenlampton’s picture

Issue summary: View changes

update required actions

jenlampton’s picture

Issue summary: View changes

r

Status: Needs review » Needs work

The last submitted patch, core-convert_item_list_to_twig-1939062-2.patch, failed testing.

joelpittet’s picture

@jenlampton That's a great start, could you add your twig file to that patch above?

jenlampton’s picture

Assigned: Unassigned » jenlampton

hahahahaha :) yes.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
6.51 KB
FAILED: [[SimpleTest]]: [MySQL] 53,459 pass(es), 55 fail(s), and 446 exception(s). View

Ok well here's a stab at that twig template. @jenlampton maybe you can merge what you have?

The interdiff on the twig is not quite right because there wasn't one and I added it from sandbox or someplace.

May still fail but, better. I think I was having some "attributes" is an invalid render array key issues but didn't see that in the array coming in...

joelpittet’s picture

FileSize
6.48 KB
FAILED: [[SimpleTest]]: [MySQL] 53,467 pass(es), 94 fail(s), and 454 exception(s). View
593 bytes

whoops, remove debug... but that debug is what I was checking to see what's up with attributes... so if you are curious and it fails, use the above patch.

Status: Needs review » Needs work

The last submitted patch, twig-item-list-1939062-7.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
7.16 KB
FAILED: [[SimpleTest]]: [MySQL] 53,747 pass(es), 141 fail(s), and 1 exception(s). View

Attached patch does a few things to get tests passing:

1. Whitespace control.
2. Disables the default class added by template_preprocess (from #1938430: Don't add a default theme hook class in template_preprocess())
3. Uses #wrapper_attributes of each item (which are render arrays and must stay #-prefixed so render() doesn't complain about "invalid render element".

If this passes all tests, then so too will #1938430: Don't add a default theme hook class in template_preprocess(), which should be committed asap as it's causing problems in other conversion issues.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-theme-item-list--1939062-9.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
8.71 KB
FAILED: [[SimpleTest]]: [MySQL] 53,736 pass(es), 89 fail(s), and 1 exception(s). View

This should pass, as it includes all the changes that make #1938430: Don't add a default theme hook class in template_preprocess() pass.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-theme-item-list--1939062-11.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,027 pass(es). View
1.92 KB

This patch:

1. Whitespace
2. Render arrays

should fix most if not all fails.

joelpittet’s picture

Nice getting this to pass @steveoliver!

+++ b/core/modules/system/templates/item-list.html.twig
@@ -0,0 +1,44 @@
+    {% for item in items -%}
+      {#
+        We usually don't do this, but here we target a specific render element
+        property--#wrapper_attributes.
+      #}
+      <li{{ item['#wrapper_attributes'] }}>{{ item }}</li>

No way around this? :S

steveoliver’s picture

FileSize
8.76 KB
PASSED: [[SimpleTest]]: [MySQL] 53,905 pass(es). View
770 bytes

Nope, this is what we've got to do, and I think it's a fine enough 'exception' to deal with in order to have child items as render arrays. See #891112-78: Replace theme_item_list()'s 'data' items with render elements.

Attached patch just simplifies the comment in the template.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-item-list--1939062-15.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review

I don't know if that was a fluke -- it appears to be new. Testing again...

steveoliver’s picture

Cottser’s picture

This is looking good! Quick docs review:

+++ b/core/includes/theme.incundefined
@@ -2294,20 +2294,37 @@ function theme_mark($variables) {
+ * Preprocesses variables for item list templates.

s/Preprocesses/Prepares

+++ b/core/includes/theme.incundefined
@@ -2337,66 +2354,28 @@ function template_preprocess_item_list(&$variables) {
+    // @todo remove after http://drupal.org/node/1649780 lands.

Capitalize 'remove'.

+++ b/core/modules/system/templates/item-list.html.twigundefined
@@ -0,0 +1,41 @@
+ * @see http://drupal.org/node/1910996 To deprecate this
+ * @see http://drupal.org/node/1842140 To remove heading tag and div

These don't match the formatting for @see, notably the comments after the URL. @see http://drupal.org/node/1354#see :)

To provide more explanation, just use a regular documentation paragraph.

steveoliver’s picture

Assigned: jenlampton » steveoliver
FileSize
8.52 KB
PASSED: [[SimpleTest]]: [MySQL] 54,005 pass(es). View
6.69 KB
FAILED: [[SimpleTest]]: [MySQL] 53,801 pass(es), 6 fail(s), and 0 exception(s). View
5.23 KB

OK, Updated docs as per Cottser's suggestions in #19, and went through the .html.twig docblock and cleaned that up too. In the process, I added a @see to the change record [#1842756] -- don't know if that's what we want -- it may get outdated at some point. Figured it doesn't hurt for now, though, esp. since theme_item_list has changed so much.

The "-clean" patch should be committed after #1938430: Don't add a default theme hook class in template_preprocess() lands. The "-temp" one includes the changes from that patch to pass tests for the time being.

Assigning to myself for fame and glory.

jenlampton’s picture

Thanks for all this awesome work :) I'm going to create a follow-up issue so we can get rid of the funky syntax:

<li{{ item['#wrapper_attributes'] }}>{{ item }}</li>

There's two things here that are big no-nos for the new theme layer: the array-like syntax and also the hash in the key. It seems like we should be able to do something a little smarter with Twig and its object's __to_strings to take care of attributes on items in loops. I've been thinking about this in a few other similar issues too, but it's a follow-up for sure.

Follow-up: #1963402: Pass variables to Twig in a nicer way (was: Create drillable render arrays)

Cottser’s picture

We could consider Twig's attribute function, but it could get confusing with Drupal's Attribute object in the mix, and of course this wouldn't be needed if it weren't for the hash in the key.

<li{{ attribute(item, '#wrapper_attributes') }}>{{ item }}</li>

Edit: remove quotes from 'item'.

thedavidmeister’s picture

Issue tags: +Needs manual testing

Having a look at #20:

Why do we touch comment.module and block.module in this patch?

+{% if items is not empty %}

Is there a reason for the "is not empty" style conditionals in the template or would {% if foo %} suffice?

Anyway, from a coding standards perspective this patch looks good to me. Nice work everyone :)

This patch is probably ready for somebody to manually test the markup after updating the issue summary to provide manual testing steps.

Cottser’s picture

Status: Needs review » Needs work

Thanks @thedavidmeister!

Now that the #1938430: Don't add a default theme hook class in template_preprocess() is postponed, the -clean.patch should be reworked so that we remove the offending class in the preprocess function, that way the markup matches up and tests pass.

steveoliver’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
2.3 KB
7.25 KB
PASSED: [[SimpleTest]]: [MySQL] 54,345 pass(es). View

Added workaround for default item-list class while #1938430: Don't add a default theme hook class in template_preprocess() is postponed. I don't know what we'd manually test here that isn't covered by testItemList(), so removing the tag.

steveoliver’s picture

Also, the patch in #25 addresses David's point from #23, the is not empty check is not necessary -- it has been removed.

jenlampton’s picture

Assigned: steveoliver » jenlampton
Status: Needs review » Needs work

reviewing, and see some little problems. Will do a quick re-roll here.

jenlampton’s picture

Assigned: jenlampton » Unassigned
Status: Needs work » Needs review
FileSize
2.16 KB
8.61 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-convert_item_list-1939062-28.patch. Unable to apply patch. See the log in the details link for more information. View

changes:
- we decided way back when that HTML tags themselves should not be variables. We can use the variable to choose which HTML tag gets printed, but we should not create the HTML tag by printing that variable in the template. It's messy from a coder perspective, but this kind of output will make a lot more sense to someone looking for a UL tag or an OL tag in their HTML output:

    {% if tag is ul %}
    <ul {{ attributes }}>
    {% else %}
    <ol {{ attributes }}>
    {% endif %}

- the variable named tag. I changed this originally to type, since there are lots of tags being output in the template, but there's now a note in here that type conflicts with '#type' in render arrays. If that's really the case (i haven't had a problem with it but that doesn't mean it's not a real problem) then how about a name like list_type. Since it's not a variable to be printed anymore, but instead a variable to be tested, something that more clearly indicates what is being tested makes more sense here.

    {% if list_type is ul %}
    <ul {{ attributes }}>
    {% else %}
    <ol {{ attributes }}>
    {% endif %}

- changed @see to @todo with a description of what needs to be done. If we decided not TODO this then we can remove this comment later.

- I'm not sure why there's an @see to the change record in the preprocess docblock. Is this a new standard for us? (leaving as-is until I understand more)

I think this is looking great :) Good work team!

jenlampton’s picture

Issue summary: View changes

r

Status: Needs review » Needs work

The last submitted patch, twig-convert_item_list-1939062-28.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
7.45 KB
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es). View

This just combines #25 with the interdiff from #28. Go team indeed :)

jenlampton’s picture

Thanks Cottser! I really want to review this, but it still feels too much like reviewing my own patch. Any takers? :)

thedavidmeister’s picture

#31 - I'm planning to do a bunch of reviews this weekend. It might turn up on my radar then if nobody gets there first..

thedavidmeister’s picture

Issue summary: View changes

rm sand

c4rl’s picture

c4rl’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/item-list.html.twigundefined
@@ -0,0 +1,51 @@
+{#
+  The spaceless tag is used and not indented because testItemList() expects
+  item lists to be printed on one line without spaces.
+#}

Seems like we should change the test instead.

c4rl’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
9.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,565 pass(es). View

#wrapper_attributes seems ugly to me, but I'm not sure we have a way around it in the short-term.

I simplified some of the markup for the test.

Interdiff with respect to #30.

jenlampton’s picture

Status: Needs review » Needs work

The tests look much beter, thanks @C4rl!

Unfortunately, the patch in #35 still generates a HTML tag by printing a variable, and that needs to be fixed. (see comment in #28)

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
9.28 KB
PASSED: [[SimpleTest]]: [MySQL] 55,362 pass(es). View

givin' it another go.

Patch contains
- passing tests (but added a newline to get them passing)
- actual UL and OL tags instead of printing a variable
- changed 'type' back to 'list_type' form 'list_tag'

I could go either way on the 3rd one, but I did reroll the patch over at #1828536: Rename 'type' variable of theme_item_list() to 'list_type' to use list_type as well.

Cottser’s picture

FileSize
555 bytes
9.28 KB
PASSED: [[SimpleTest]]: [MySQL] 55,302 pass(es). View

Minor coding standards tweak:

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 65a939d..44815f8 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -2390,7 +2390,7 @@ function template_preprocess_item_list(&$variables) {
   if (!empty($variables['attributes']['class'])) {
     $variables['attributes']['class'] = array_diff($variables['attributes']['class'], array('item-list'));
     if (empty($variables['attributes']['class'])) {
-      unset ($variables['attributes']['class']);
+      unset($variables['attributes']['class']);
     }
   }
jwilson3’s picture

+++ b/core/includes/theme.incundefined
@@ -2344,66 +2365,37 @@ function template_preprocess_item_list(&$variables) {
     }
-  }
-}
-
-/**
- * Returns HTML for a list or nested list of items.
- *
- * @param $variables
- *   An associative array containing:
- *   - items: A list of items to render. Allowed values are strings or
- *     render arrays. Render arrays can specify list item attributes in the
- *     #wrapper_attributes property.
- *   - title: The title of the list.
- *   - type: The type of list to return (e.g. "ul", "ol").
- *   - attributes: The attributes applied to the list element.
- */
-function theme_item_list($variables) {
-  $items = $variables['items'];
-  $title = (string) $variables['title'];
-  // @todo 'type' clashes with '#type'. Rename to 'tag'.
-  $type = $variables['type'];
-  $list_attributes = $variables['attributes'];
-
-  $output = '';
-  if ($items) {
-    $output .= '<' . $type . new Attribute($list_attributes) . '>';
+    else {
+      // Turn the item into an array if it wasn't one already.
+      $item = array('#markup' => $item);
+    }

This patch is extremely difficult to read and review because of seemingly randomly placed pieces of code from the preprocess function being interspersed inside the removed theme_item_list. Weird. Oh, well.

+++ b/core/includes/theme.incundefined
@@ -2301,20 +2301,41 @@ function theme_mark($variables) {
+ *
+ * @see http://drupal.org/node/1842756
  */
 function template_preprocess_item_list(&$variables) {
+  $variables['title'] = (string) $variables['title'];
+  // Change the name since 'type' clashes with #type.
+  // @see http://drupal.org/node/1828536
+  $variables['list_type'] = $variables['type'];

Do we need two @see's here? DOH, they reference different nodes ;) so yes.

+++ b/core/modules/system/templates/item-list.html.twigundefined
@@ -0,0 +1,41 @@
+  {% if title is not empty -%}

I've seen elsewhere where people are only using {% if foo %} but admittedly, don't understand why or when the "is not empty" part is ever necessary.

thedavidmeister’s picture

Status: Needs review » Needs work

#39 - The "is not empty" part should no longer be required so we should remove it. This is something that was needed in some cases before #1975442: drupal_render() should always return a string. and #1975462: Allow and test for NULL and integer 0 values in Twig templates. landed.

Cottser’s picture

#39/@jwilson3 - For reviewing patches like this one that change (parts of) big theme functions, I recommend applying the patch locally and looking at the preprocess function in your editor or diff tool of choice :) I ran into the same thing on a few other patches.

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
9.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,351 pass(es). View

Removed the 'is not empty', removed a #dreammarkup @todo, and tweaked the docs some.

Usually we indent the contents of if blocks - are we making an exception here or do we want to modify the tests a bit more (we're already modifying them and it's just whitespace changes)?

cellear’s picture

Assigned: Unassigned » cellear
jenlampton’s picture

@Cottser can we leave the @todo in there? We don't want D8 to ship without that change so leaving it in will keep reminding us, as long as there's a link to the issue that needs to be resolved I think it's okay (but correct me if you know different).

I'm fine with updating the whitespace and updating the tests, too. :)

I know @cellear is testing, so I'm going to leave at needs review, but @cellear please set to 'needs work' when you are done testing the patch?

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Novice

@jenlampton - From my understanding adding @todos at this point in the release cycle moves us further from release. Committers will be reluctant to commit anything with a @todo in it. In this case we would risk the template shipping with a @todo in it which is an easy risk to avoid :)

Setting to needs work for the if block indent/test update proposed in #42, I think that can proceed along with testing - manual testing I assume?

cellear’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Novice

My first patch review.
- I see the changed documentation, the removed @todo, and the more concise "if not empty" statement in the patches.
- I verified the changes in core/includes/theme.inc and core/modules/system/templates/item-list.html.twig.
- I applied https://drupal.org/files/1939062-38.patch and https://drupal.org/files/1939062-42.patch to virgin downloads of d8 (commit 7a0440d196e754d629ab42ada27f26bf4d7916ee - Wed Jun 19 01:44:06 2013 +0200) and compared the output for admin/modules/list/confirm when adding Views UI (which generates this item list output:

  • You must enable the Views module to install Views UI.

I verified that each of the three patch states (unpatched, 1939062-38, and 1939062-42) generated identical output.

It all seems to work as advertised, so I'm marking this "RTBC." Future patch tests will probably be less wordy :)

cellear’s picture

Assigned: cellear » Unassigned
cellear’s picture

Status: Reviewed & tested by the community » Needs work

Changing status to 'needs work' per #44

Cottser’s picture

Issue tags: +Needs profiling

This needs profiling as well.

Thanks for the review @cellear, I think you were quite thorough :)

Cottser’s picture

Issue tags: +Novice

And Novice tagging for updating the if indent and associated tests as described in #42.

pwieck’s picture

Since this needs rework remove @see template_preprocess()

[policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file

What?... I just stated a policy. :-p

I submitted a patch to correct the exiting ones.

gabesullice’s picture

FileSize
9.17 KB
FAILED: [[SimpleTest]]: [MySQL] 57,995 pass(es), 1 fail(s), and 0 exception(s). View

This is my first patch (thanks to some Drupal ladder mentors at Aten Design). I indented the if blocks as I assume you were needing it done. Wasn't sure what needs to be done for the "associated tests" as mentioned in #50.

We had a small question related to the for loop. Not sure how best to do that, but this is our best attempt.

Feedback much appreciated. Thanks!

pwieck’s picture

Status: Needs work » Needs review

changing status

@gabesullice @see template_preprocess() needs to be removed see policy change in #51.

+ *
+ * @see template_preprocess()
+ * @see template_preprocess_item_list()
+ *
joelpittet’s picture

#52 Thank you @gabesullice. The associated tests are if the tests are checking whitespace they will fail after indent so they would need to be changed to add the space in the tests as well, or better yet, use xpath and ignore whitespace differences.

To get the patch to get tested you need to set the status to needs review. I'm doing that for you here. I usually forget to do that often as well.

Status: Needs review » Needs work

The last submitted patch, 1939062-52.patch, failed testing.

hussainweb’s picture

FileSize
7.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,366 pass(es). View

Trying again... I just added {% spaceless %} in the twig file so that we don't have to account for whitespace in tests. I also removed @see template_preprocess as per #53.

hussainweb’s picture

Status: Needs work » Needs review
hussainweb’s picture

Issue summary: View changes

Add type/tag issue to summary

jenlampton’s picture

FileSize
6.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,298 pass(es), 8 fail(s), and 1,377 exception(s). View

Status: Needs review » Needs work

The last submitted patch, twig-item_list-1939062-58.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
+++ b/core/includes/theme.inc
@@ -2283,65 +2304,25 @@ function template_preprocess_item_list(&$variables) {
+    // @todo Remove after http://drupal.org/node/1649780 lands.

This should be removed and let the issue deal with it.

+++ b/core/includes/theme.inc
@@ -2283,65 +2304,25 @@ function template_preprocess_item_list(&$variables) {
+  // These are the top level list attributes.
+  $variables['attributes'] = new Attribute($variables['attributes']);

These are lazy loaded so not needed here.

Cottser’s picture

Status: Needs review » Needs work

Crosspost with testbot.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
985 bytes
6.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,736 pass(es), 8 fail(s), and 1,415 exception(s). View

Not sure why Jen's patch failed but it did need a re-roll. I re-rolled it and then added Joel's 2 changes from #60. I am re-submitting for review

Status: Needs review » Needs work

The last submitted patch, twig-item_list-1939062-62.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
563 bytes
6.37 KB
PASSED: [[SimpleTest]]: [MySQL] 56,811 pass(es). View

The problem was this block of code:

  // Change the name since 'type' clashes with #type.
  // @see http://drupal.org/node/1828536
  $variables['list_type'] = $variables['type'];

This was kind of a workaround for #1828536: Rename 'type' variable of theme_item_list() to 'list_type'. Since that fix got in, this was not necessary and in fact, it overwrote the correct variable. I have removed this block, as seen in interdiff. I also ran one of the failing tests and it passed. Lets see what the testbot thinks.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs another reroll.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
6.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme_item_list-1939062-66.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled

joelpittet’s picture

Issue tags: -Novice, -Needs reroll

removing tags. Thanks @hussainweb for the re-roll!

joelpittet’s picture

Assigned: Unassigned » joelpittet

Profiling this...

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

Scenario:

  • Stark Theme
  • Generated 50 Nodes with 50 Comments
  • Added a recent comments block to the left sidebar (block uses item_list)
  • Edited the homepage view to show 1 teaser node per page (pager uses item_list)
=== 8.x..8.x compared (5206d663ca0ba..5206d6d973591):

ct  : 88,624|88,624|0|0.0%
wt  : 565,673|565,958|285|0.1%
cpu : 511,086|510,914|-172|-0.0%
mu  : 20,833,880|20,833,880|0|0.0%
pmu : 20,914,296|20,914,296|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5206d663ca0ba&...

=== 8.x..1939062-item_list-twig compared (5206d663ca0ba..5206d72f6f4f8):

ct  : 88,624|89,421|797|0.9%
wt  : 565,673|567,111|1,438|0.3%
cpu : 511,086|513,963|2,877|0.6%
mu  : 20,833,880|20,927,064|93,184|0.4%
pmu : 20,914,296|21,008,920|94,624|0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5206d663ca0ba&...

webchick’s picture

Assigned: joelpittet » catch

Hm. That's quite a bit more of a jump compared to other Twig conversions. Since this is a very user-facing theme bit which is used basically everywhere, tentatively assigning to catch to give final approval.

catch’s picture

Looks from the profiling data like this is the change from three calls rather than one, but yeah it's still quite a big jump. Still thinking about this one a bit sorry for the delayed update.

Cottser’s picture

+0.2% wt doesn't seem that much to me, but it sounds like more data might be useful (with more item_list's)?

catch’s picture

Status: Reviewed & tested by the community » Needs review

0.2% wall time is only not much because the total wt for the request was over 500ms - and 500ms is a depressing figure for a simple-ish page.

What I'm really looking at is the ms difference, which is about 1.5ms, and the function calls which is +797.

Drupal 7 can serve fairly simple pages in around 100ms (obviously depends on a lot of things), but in that context 1.5ms is 1.5% which is not small for three theme() calls.

It would be handy to do a more targeted test I think - marking back to CNR and unassigning for that.

Cottser’s picture

Assigned: catch » Cottser
Issue tags: +Needs profiling

Sure, I will see what I can do.

Cottser’s picture

Assigned: Cottser » Unassigned

Unassigning since I haven't touched this in over 2 weeks - I do have a couple long flights coming up though ;)

Cottser’s picture

Assigned: Unassigned » Cottser

Looking at this today.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs profiling

Here are the results for 11 item lists instead of 3, so this seems a bit high. Perhaps we need to do something similar to #1939102: Convert theme_indentation() to Twig here :/

=== 8.x..8.x compared (5240420daae43..52404261932b3):

ct  : 83,298|83,298|0|0.0%
wt  : 517,874|516,714|-1,160|-0.2%
cpu : 479,718|477,552|-2,166|-0.5%
mu  : 17,934,624|17,934,624|0|0.0%
pmu : 18,061,816|18,061,816|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5240420daae43&...

=== 8.x..item_list-1939062-66 compared (5240420daae43..524042daedba9):

ct  : 83,298|85,645|2,347|2.8%
wt  : 517,874|527,775|9,901|1.9%
cpu : 479,718|489,137|9,419|2.0%
mu  : 17,934,624|17,977,480|42,856|0.2%
pmu : 18,061,816|18,105,632|43,816|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5240420daae43&...

Cottser’s picture

Issue summary: View changes

related

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: theme_item_list-1939062-66.patch, failed testing.

dale42’s picture

Assigned: Unassigned » dale42
Issue summary: View changes
dale42’s picture

Patch failed because of #2120807: Add empty option to item_list.

Added the new 'empty' variable to the twig template. Also fixed docblock inconsistency with list_type parameter name.

dale42’s picture

FileSize
6.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,928 pass(es), 2 fail(s), and 0 exception(s). View

Patch failed because of #2120807: Add empty option to item_list.

Added the new 'empty' variable to the twig template. Also fixed docblock inconsistency with list_type parameter name.

(Resubmitted w/patch)

dale42’s picture

Assigned: dale42 » Unassigned
dale42’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 82: theme_item_list-1939062-82.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 82: theme_item_list-1939062-82.patch, failed testing.

joelpittet’s picture

+++ b/core/modules/system/templates/item-list.html.twig
@@ -0,0 +1,44 @@
+  {% elseif empty %}
+    {{ empty }}
+  {% endif %}

The patch looks right, though the test fails are related to the empty variable. I'd put my money on whitespace errors as some tests are testing whitespace you may need those minus sign whitespace operators to fix this. {%- {{-

ekl1773’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
6.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,034 pass(es). View

@JesseBeach and I had a whack at those testbot errors. Running Drupal\system\Tests\Theme\FunctionsTest in Simpletest on my local install also threw a few more errors from "testLinks()", which are as yet unresolved. I'm uploading what we came up with and an interdiff to keep this moving along.

Mostly we removed {% spaceless %}, added some more whitespace operators, and moved the

tags around a bit.
joelpittet’s picture

@ekl1773 thanks for moving this along! Hoping it'll come back green. There is a twig coding standard to indent the code after the if statement... though personally if the output looks cleaner I'd like to disregard that:)

Also, thanks for removing spaceless! We'd rather the tests no require the whitespace and if possible make the output as easy to read as possible over conforming the output to match the tests when it comes to whitespace which the browser doesn't care. Obviously if the space is required for separating two inline things it's important but otherwise it really doesn't matter.

*crosses fingers for green*

Status: Needs review » Needs work

The last submitted patch, 89: theme_item_list-1939062-89.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Those tests pass locally so I'm calling BS on testbot for now...

Status: Needs review » Needs work

The last submitted patch, 89: theme_item_list-1939062-89.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
joelpittet’s picture

Latest profiling on #89

Scenario:
Bartik theme
4 Aggregator feed blocks
3 recent comment blocks
1 Who's new block
1 Who's online block
1 views pager item list

=== 8.x..8.x compared (52a03729e7d1a..52a03772cdd55):

ct  : 85,024|85,024|0|0.0%
wt  : 368,382|364,639|-3,743|-1.0%
cpu : 362,609|359,138|-3,471|-1.0%
mu  : 20,520,152|20,520,152|0|0.0%
pmu : 20,580,984|20,580,984|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52a03729e7d1a&...

=== 8.x..1939062-item_list-twig compared (52a03729e7d1a..52a037bb4f6e8):

ct  : 85,024|85,773|749|0.9%
wt  : 368,382|367,548|-834|-0.2%
cpu : 362,609|361,746|-863|-0.2%
mu  : 20,520,152|20,586,680|66,528|0.3%
pmu : 20,580,984|20,648,512|67,528|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52a03729e7d1a&...

joelpittet’s picture

+++ b/core/includes/theme.inc
@@ -1867,20 +1867,38 @@ function theme_mark($variables) {
+      if (isset($item['#wrapper_attributes']) && is_array($item['#wrapper_attributes'])) {
+        $attributes = $item['#wrapper_attributes'];

+++ b/core/modules/system/templates/item-list.html.twig
@@ -0,0 +1,43 @@
+        <li{{ item['#wrapper_attributes'] }}>{{ item }}</li>

Can we somehow get away with not having the #wrapper_attributes? Other than that this is good to go.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

jerdavis’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.41 KB
FAILED: [[SimpleTest]]: [MySQL] 63,077 pass(es), 10 fail(s), and 6 exception(s). View

Re-roll of #89

Status: Needs review » Needs work

The last submitted patch, 99: theme_item_list-1939062-99.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 99: theme_item_list-1939062-99.patch, failed testing.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
6.27 KB
FAILED: [[SimpleTest]]: [MySQL] 63,214 pass(es), 69 fail(s), and 51 exception(s). View

Did another reroll. I pulled out the wrapper_attributes on this one, both in the Twig file and in the code where those were set in the preprocess function. I also saw that element_children was deprecated, so I replaced it with Element::children and added the Use statement for the class at the top of theme.inc.

mdrummond’s picture

FileSize
6.27 KB
FAILED: [[SimpleTest]]: [MySQL] 63,256 pass(es), 69 fail(s), and 51 exception(s). View

After further discussion with Cottser on IRC, we need to leave item['#wrapper_attributes'] in the Twig file so that modules can add classes if necessary. I did pull out the first/last/odd/even code in the preprocess function, along with $i and $num_items, which were no longer needed. Hopefully this goes green!

Cottser’s picture

Status: Needs review » Needs work

Thanks!

+++ b/core/includes/theme.inc
@@ -1763,25 +1764,39 @@ function theme_mark($variables) {
+  // Prepare for adding classes to each item.
+  $num_items = count($variables['items']);
+  $i = 0;
...
+    $i++;

I think we can ditch these lines since #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors is in (as you alluded to above @mdrummond), but at the same time I think we may be losing too much of theme_item_list, more specifically I'm not seeing where the li attributes are being set up in preprocess.

Cottser’s picture

And as we talked about, this could be follow-up but it may also make sense to do here, something along these lines:

  • $item['value'] (or whatever other name) could replace $item['#markup']
  • $item['attributes'] could replace $item['#wrapper_attributes']

Then in the Twig template something like this:

<li{{ item.attributes }}>{{ item.value }}</li>

The last submitted patch, 102: theme-item-list-1939062-102.patch, failed testing.

The last submitted patch, 102: theme-item-list-1939062-102.patch, failed testing.

The last submitted patch, 103: theme-item-list-1939062-103.patch, failed testing.

mdrummond’s picture

FileSize
6.47 KB
FAILED: [[SimpleTest]]: [MySQL] 63,129 pass(es), 140 fail(s), and 54 exception(s). View
1.86 KB

Strange. I must have messed something up with the last patch as I didn't intend to pull out the wrapper_attributes code, and I did intend to pull out the num_items.

So.

Here's another revised version that may or may not be correct. I provided an interdiff. We'll see if it works!

mdrummond’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 109: theme-item-list-1939062-109.patch, failed testing.

mdrummond’s picture

I guess I'm at least consistent in increasing the failure rate on my patches.

joelpittet’s picture

@mdrummond try a few of these to see how things will fair.

  1. +++ b/core/includes/theme.inc
    @@ -1806,60 +1821,14 @@ function template_preprocess_item_list(&$variables) {
    +    else {
    +      // Turn the item into an array if it wasn't one already.
    +      $item = array('value' => $item);
         }
    

    This should happen all the time, not only when the item is not an array. Just remove the else block around it.

  2. +++ b/core/includes/theme.inc
    @@ -1806,60 +1821,14 @@ function template_preprocess_item_list(&$variables) {
    -        if (isset($item['#wrapper_attributes'])) {
    -          $attributes = $item['#wrapper_attributes'];
    -        }
    

    This bit is key to have the attributes show up. Add it back in.

  3. +++ b/core/includes/theme.inc
    @@ -1763,25 +1764,39 @@ function theme_mark($variables) {
    +      // List items support attributes via the 'attributes' property.
    +      if (isset($item['attributes']) && is_array($item['attributes'])) {
    +        $attributes = $item['attributes'];
    +      }
    

    This is not needed, it should be the same but for #wrapper_attributes.

  4. +++ b/core/modules/system/templates/item-list.html.twig
    @@ -0,0 +1,43 @@
    +        <li {{ item.attributes }}>{{ item.value }}</li>
    

    Remove the space before the item.attributes, it prints a space before each attribute so it just adds 2 at the start.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
6.46 KB
FAILED: [[SimpleTest]]: [MySQL] 63,252 pass(es), 6 fail(s), and 6 exception(s). View
1.89 KB

Here's a new version of the patch with suggested changes.

Status: Needs review » Needs work

The last submitted patch, 114: theme-item-list-1939062-114.patch, failed testing.

joelpittet’s picture

@mdrummond headway! Only 6 fails/exceptions on one test. You are so close!

There are a couple nitpicks but I'll wait till green as code will change.

mdrummond’s picture

Any suggestions on what needs fixing?

joelpittet’s picture

@mdrummond It's not evident, you may have to find out by running the failed test locally.

The test is \Drupal\views_ui\Tests\UI\FieldUITest

If you run debug($this->content); in the testFieldUI function. You can see the item list on the page in the test results. That way you can possibly see what went wrong.

Before and after the patch markup should give you hints.

The xpath test are looking inside the LI tags for some text like "[age] == Age" or "[id] == ID"
If you aren't familiar with xpath it's like old school DOM query language (like CSS)
//details[@id="edit-options-alter-help"]/div[@class="details-wrapper"]/div[@class="item-list"]/fields/li
is like
details[#id="edit-options-alter-help"] > div.details-wrapper > div.item-list > fields > li

Which now that I explain that bit to you...

+++ b/core/modules/system/templates/item-list.html.twig
@@ -0,0 +1,43 @@
+    {%- if list_type == 'ul' -%}
+      <ul{{ attributes }}>
+    {%- else -%}
+      <ol{{ attributes }}>
+    {%- endif %}
+      {%- for item in items -%}
+        <li{{ item.attributes }}>{{ item.value }}</li>
+      {%- endfor -%}
+    {%- if list_type == 'ul' -%}
+      </ul>
+    {%- else -%}
+      </ol>
+    {%- endif %}

change this to

    <{{ list_type }}{{ attributes }}>
      {%- for item in items -%}
        <li{{ item.attributes }}>{{ item.value }}</li>
      {%- endfor -%}
    </{{ list_type }}>

Because <fields>

joelpittet’s picture

And my nitpickery after that passes.

  1. +++ b/core/includes/theme.inc
    @@ -1763,25 +1764,39 @@ function theme_mark($variables) {
    +      if (isset($item['#wrapper_attributes']) && is_array($item['#wrapper_attributes'])) {
    +        $attributes = $item['#wrapper_attributes'];
    +      }
    

    Don't need the is_array() check here. It wasn't used before, and we already expect it to always be an array.

  2. +++ b/core/includes/theme.inc
    @@ -1806,60 +1821,13 @@ function template_preprocess_item_list(&$variables) {
    +    // Store item attributes for Twig.
    ...
    +    // Store item value for Twig.
    +    $item = array('value' => $item);
    ...
    +    $item['attributes'] =  new Attribute($attributes);
    

    instead of "Store item ..."
    Combine thes to something like:
    "Set the item's value and attributes for the template" or something. Note not 'Twig' is the key here.
    Also instead of mixing array access just define them together.

    array(
      'value' => $item, 
      'attributes' => new Attribute($attributes),
    );
mdrummond’s picture

FileSize
6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 63,565 pass(es). View
2.24 KB

Here's another patch with Joel's suggestion. Go, green, go!

Cottser’s picture

Status: Needs work » Needs review

Woo! Setting to needs review for the bot.

mdrummond’s picture

Oh wow, green!

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs profiling

Hell ya and the the markup is clear now too, no '#wrapper_attributes' or extra nested if statements. Nice work @mdrummond.
I don't think this needs markup review again, mostly because the tests do a good job of finding things on this one. But it does need another round of profiling (should be faster with less whitespace modifiers and conditions), and twig 1.15 is just faster.

@mdrummond you're welcome to give that a crack if you want, just assign yourself for that?
https://drupal.org/contributor-tasks/profiling

What ever profiling is done I'll likely run another one with the same scenario just to compare results.

mdrummond’s picture

I've never doing profiling before, but I can try to learn that this weekend with the link you provided. Should be able to get started tonight.

mdrummond’s picture

With Cottser's help and several hours of work, I was finally able to get profiling to work, and to work well for this patch.

For the scenario, I used Bartik and had nine user login blocks in different areas of the page.

Here are the results:

=== 8.x..8.x compared (52f2e60305388..52f2e62091f2c):

ct  : 78,816|78,816|0|0.0%
wt  : 341,748|343,149|1,401|0.4%
cpu : 319,991|322,749|2,758|0.9%
mu  : 20,258,992|20,259,200|208|0.0%
pmu : 20,454,104|20,454,824|720|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f2e60305388&...

Run 52f2e60305388 uploaded successfully for drupal-perf-drupalcon.
Run 52f2e62cef49a uploaded successfully for drupal-perf-drupalcon.

=== 8.x..theme-item-list-120-profiling compared (52f2e60305388..52f2e62cef49a):

ct  : 78,816|79,557|741|0.9%
wt  : 341,748|339,463|-2,285|-0.7%
cpu : 319,991|317,938|-2,053|-0.6%
mu  : 20,258,992|20,318,088|59,096|0.3%
pmu : 20,454,104|20,512,560|58,456|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f2e60305388&...

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs profiling

@mdrummond awesome work, thanks for profiling! it does take a lot of time!

I did one to compare and it's different but also not bad. Let's mark this RTBC:)

=== 8.x..8.x compared (52f30aadf2c65..52f30b3f0ac5a):

ct  : 85,080|85,080|0|0.0%
wt  : 597,010|597,338|328|0.1%
cpu : 590,557|590,720|163|0.0%
mu  : 41,416,904|41,416,904|0|0.0%
pmu : 41,510,496|41,510,496|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f30aadf2c65&...

=== 8.x..1939062-item_list-twig compared (52f30aadf2c65..52f30b8747627):

ct  : 85,080|85,988|908|1.1%
wt  : 597,010|601,208|4,198|0.7%
cpu : 590,557|594,637|4,080|0.7%
mu  : 41,416,904|41,490,696|73,792|0.2%
pmu : 41,510,496|41,582,976|72,480|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52f30aadf2c65&...

Cottser’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Cottser’s picture

I'm going to open a follow-up issue to fix the empty text logic and add test coverage, the logic here is not quite right.

joelpittet’s picture

I'll follow that follow-up! Yeah just realized it is a bit off too.

jhodgdon’s picture

We may want to consider reverting this instead, as it isn't really right? And where's the follow-up?

Cottser’s picture

Sorry, forgot to comment here. It's a really small follow-up, just needs testbot approval and someone to OK it.

#2191323: item-list.html.twig always shows empty text

Status: Fixed » Closed (fixed)

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