Convert theme_item_list() to Twig

dibbs

Adding link to ol/ul issue

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

update required actions

r

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

hahahahaha :) yes.

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

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.

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.

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

This patch:

1. Whitespace
2. Render arrays

should fix most if not all fails.

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

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.

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

#15: drupal-twig-item-list--1939062-15.patch queued for re-testing.

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: theme_item_list()'s $items variable are either strings or render arrays now -- 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.

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)

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.

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.

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

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

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!

r

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

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

rm sand

Related #1828536: Rename 'type' variable of theme_item_list() to 'list_type'

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

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

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)

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.

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

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

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:

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

Changing status to 'needs work' per #44

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.

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!

changing status

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

+ *
+ * @see template_preprocess()
+ * @see template_preprocess_item_list()
+ *

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

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.

Add type/tag issue to summary

Rerolled, removed @todo now that this landed: #1938430: Don't add a default theme hook class in template_preprocess()

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

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

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.

Re-rolled

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

Profiling this...

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

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.

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.

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.

66: theme_item_list-1939062-66.patch queued for re-testing.

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.

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)

82: theme_item_list-1939062-82.patch queued for re-testing.

+++ 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. {%- {{-

@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

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

89: theme_item_list-1939062-89.patch queued for re-testing.

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

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

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

Re-roll of #89

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.

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!

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!

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

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

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

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

Any suggestions on what needs fixing?

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

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),
   );

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

Oh wow, green!

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.

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.

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

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

Committed and pushed to 8.x. Thanks!

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

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