Convert views/templates/views-view-summary-unformatted.tpl.php to twig

Need to deal with this in a nicer way:
{{ row_classes[id] }}

Can someone point me in the right direction?

Tagging.

I'm not 100% sure, but i guess the patch in #1773832: Replace usage of drupal_attributes, refactor to use Attribute better would make it easier.

Moving to core queue

Taking the original conversion, removing the tpl file and cleaning up $vars to $variables.

Needs Doc Cleanup to standards.

• #1913208: [policy] Standardize template preprocess function documentation
• #1823416 Twig coding standards

Thanks for your hard work on that!

+++ b/core/modules/views/views.theme.incundefined
@@ -393,13 +393,12 @@ function template_preprocess_views_view_summary(&$vars) {
+ * Template preprocess theme function to print summary basically unformatted.

The standard seems to be "Processes variables for blub.tpl.php".

+++ b/core/modules/views/views.theme.incundefined
@@ -393,13 +393,12 @@ function template_preprocess_views_view_summary(&$vars) {
+  $view     = $variables['view'];

If we change the line, please also drop the empty spaces.

Needs some clean up see #6 and #7

Cleanup from #6 and #7

Tagging.

Review for #9:

+ * Default theme implementation for unformatted summary links.
+ * 

Trailing space on the second line.

The documentation for variables in the template could be better:

- No mention of what options is
- No mention of row_classes
- row.url is used but not described
- row.link is used but not described
- row.count is used but not described

'row.count' thi this option is set.

Grammatical error.

Show's the 'row.count'

Grammatical error.

Wraps items in a span if set to inline, or a div if not.

Unnecessary comma.

Template docs are missing @see template_preprocess() and @see template_preprocess_views_view_summary_unformatted()

Thanks for reviewing @thedavidmeister. I'm tagging the documentation tweaks in #12 as a novice task.

If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!

Changes from #13

typo and finger slip on the trigger there... there is a tab.. It's removed here.

+ * - count: An option to show/hide the 'count' to show the row's 'count'.

Still reads a bit weird. What about "A flag that indicates whether the row's 'count' should be displayed."

+ * - row_classes: A list of classes that correlate to each row by id.

ID

+ * - inline: Wraps each item in a span tag if set or a div if not set.

Well it's just a boolean, it doesn't "do" anything - we should be trying to separate the documentation of the variables from describing how the template implements markup using the variables. How about "A flag that indicates whether the item should be wrapped in an inline or block level HTML element"?

+    url(current_path(), array('alias' => TRUE)), // Force system path.
+    url(current_path()), // Could be an alias.

Comments should be on their own line, not "inlined" like this.

+ * - options: @todo.

I'm sure we can come up with *something* reasonable for this now.

Tagging for profiling.

This should help out #16

+ * - options: Flags to change how each row will be displayed.

What about "Flags indicating how each row should be displayed." - just because they're still not "doing" anything. You can't say the flags "will change" anything, they're only flags.

Looking pretty good though.

Does anyone have steps to manually test this? I'm not sure exactly what it is :P

#19 that may be a bit too nitpicky but I did it. "to change" is the ability to do something without necessarily doing anything. Like you said "will change" is the act of actually doing the possible.

#20 - thanks for that :) we just need some manual testing now.

Couple tiny tweaks here, correcting the @ingroup and rewrapping a comment that fits within 80 chars. Hope you don't mind @joelpittet :)

aaaah i found the views summary!

1. Add some content
2. Add contextual filter
3. Under "WHEN THE FILTER VALUE IS NOT IN THE URL" select "display a summary" and select "unformatted"

doing some testing now

Updated issue summary.

HEAD:

<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/asdfasdf">asdfasdf</a>
(1)
</div>
<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/laksjdf%3Baljsf">laksjdf;aljsf</a>
(1)
</div>
<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/laskdjf%3Balsf">laskdjf;alsf</a>
(1)
</div>

#22:

<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/asdfasdf" class="">asdfasdf</a>
(1)
</div>
<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/laksjdf%3Baljsf" class="">laksjdf;aljsf</a>
(1)
</div>
<div class="views-summary views-summary-unformatted">
<a href="/d8html/node/laskdjf%3Balsf" class="">laskdjf;alsf</a>
(1)
</div>

Diff:

--- /Users/thedavidmeister/tmp/diff/original-mod 
+++ /Users/thedavidmeister/tmp/diff/new-mod 
@@ -1,12 +1,12 @@
 <div class="views-summary views-summary-unformatted">
-<a href="/d8html/node/asdfasdf">asdfasdf</a>
+<a href="/d8html/node/asdfasdf" class="">asdfasdf</a>
 (1)
 </div>
 <div class="views-summary views-summary-unformatted">
-<a href="/d8html/node/laksjdf%3Baljsf">laksjdf;aljsf</a>
+<a href="/d8html/node/laksjdf%3Baljsf" class="">laksjdf;aljsf</a>
 (1)
 </div>
 <div class="views-summary views-summary-unformatted">
-<a href="/d8html/node/laskdjf%3Balsf">laskdjf;alsf</a>
+<a href="/d8html/node/laskdjf%3Balsf" class="">laskdjf;alsf</a>
 (1)
 </div>

Unfortunately this patch adds a new, empty class attribute to the links that doesn't exist in HEAD.

row_classes is a misnomer because it's actually an Attribute object which can contain classes in the 'class' attribute.

PHPTemplate:
<a href="<?php print $row->url; ?>"<?php print $row_classes[$id]; ?>>

Twig:
<a href="{{ row.url }}" class="{{ row_classes[id] }}">

So the Twig version should probably look more like this:
<a href="{{ row.url }}"{{ row_classes[id] }}>

…and see the related issue #1968398: Convert Views $row_classes to $row['attributes'] for making this better :)

Doc changes and fixes for #row_classes, reverted $variables back to $vars, leaving it to: #1963942: Change all instances of $vars to $variables

+ * - row_classes: A list of classes that correlate to each row by ID.

This is not what this is. It can either be "active" or nothing. I tried to add row classes to my view for testing #27 through the normal style option settings for "row class" and it didn't work.

First we do this:

  $active_urls = drupal_map_assoc(array(
    // Force system path.
    url(current_path(), array('alias' => TRUE)),
    // Could be an alias.
    url(current_path()),
  ));

Later we do this:

    $vars['row_classes'][$id] = array();
    if (isset($active_urls[$vars['rows'][$id]->url]) || TRUE) {
      $vars['row_classes'][$id]['class'][] = 'active';
    }
    $vars['row_classes'][$id] = new Attribute($vars['row_classes'][$id]);

There's no "list of classes" and they don't correlate to rows by ID, they correlate to whether the url is active or not. I don't know why we aren't just using l() here but hey...

+ // Collect all arguments foreach row, to be able to alter them for example

"foreach" is "for each" in english :P

according to the documentation, its definition is:

 $attributes = new Attribute(array('id' => 'socks'));
 $attributes['class'] = array('black-cat', 'white-cat');

now

  $attributes = new Attribute();
  $attributes['class'] = $vars['row_classes'][$id]['class'];
  $vars['row_classes'][$id] = $attributes;

But you haven't changed this bit:

    $vars['row_classes'][$id] = array();
    if (isset($active_urls[$vars['rows'][$id]->url]) || TRUE) {
      $vars['row_classes'][$id]['class'][] = 'active';
    }

Which means that $vars['row_classes'][$id] can still never be anything other than an empty array() or array('active'). This is how HEAD behaves so we shouldn't change that here, but like I said in #28 we shouldn't document the variable in the Twig template as being a "list of classes" because it isn't. It's either an active class or nothing.

It's important that it's still an Attribute object though, not just a string. So I think it should be documented as something like "HTML attributes for the row, either contains an 'active' class or no attributes."

#33 - Yup, just need to clean up the docs. Based on what's there at the moment I assumed that if I added "row classes" through the Views interface they would appear in my summary view but they do not.

If we could rename 'row_classes' to 'row_attributes' or something maybe that would make more sense?

Yeah, #1968398: Convert Views $row_classes to $row['attributes'] for that :)

Right, so how's this? I think it's important to continue to mention that the attributes (or attribute!) is keyed by the row ID, so this merges the existing text with #33.

Ah right, it's an array of Attribute objects, not an Attribute object.

Change looks good to me, thanks @shanethehat!

+++ b/core/modules/views/views.theme.incundefined
@@ -424,7 +436,9 @@ function template_preprocess_views_view_summary_unformatted(&$vars) {
-    $vars['row_classes'][$id] = new Attribute($vars['row_classes'][$id]);
+    $attributes = new Attribute();
+    $attributes['class'] = $vars['row_classes'][$id]['class'];
+    $vars['row_classes'][$id] = $attributes;

This change can be rolled back, both do the same thing.

+++ b/core/modules/views/views.theme.incundefined
@@ -377,11 +377,20 @@ function template_preprocess_views_view_summary(&$vars) {
@@ -393,12 +402,15 @@ function template_preprocess_views_view_summary_unformatted(&$vars) {

@@ -393,12 +402,15 @@ function template_preprocess_views_view_summary_unformatted(&$vars) {
 
   $count = 0;
   $active_urls = drupal_map_assoc(array(
-    url(current_path(), array('alias' => TRUE)), // force system path
-    url(current_path()), // could be an alias
+    // Force system path.
+    url(current_path(), array('alias' => TRUE)),
+    // Could be an alias.
+    url(current_path()),
   ));
 
-  // Collect all arguments foreach row, to be able to alter them for example by the validator.
-  // This is not done per single argument value, because this could cause performance problems.
+  // Collect all arguments for each row, to be able to alter them for example
+  // by the validator. This is not done per single argument value, because
+  // this could cause performance problems.
   $row_args = array();
   foreach ($vars['rows'] as $id => $row) {
     $row_args[$id] = $argument->summary_argument($row);
@@ -406,7 +418,7 @@ function template_preprocess_views_view_summary_unformatted(&$vars) {

@@ -406,7 +418,7 @@ function template_preprocess_views_view_summary_unformatted(&$vars) {
   $argument->process_summary_arguments($row_args);
 
   foreach ($vars['rows'] as $id => $row) {
-    // only false on first time:
+    // Only false on first time.
     if ($count++) {
       $vars['rows'][$id]->separator = filter_xss_admin($vars['options']['separator']);
     }

I'm wondering about all this docs cleanup within the preprocess function - can we leave that for another issue and focus on the Twig conversion here? I don't think we'd be patching these hunks otherwise. It's not Twig conversion's job to clean this up, we have enough to do already! :)

#38 - but undoing them once they're already done is also more work..

Backed out code, per #38. The rest I'll leave alone, per #39.

unassigning myself so someone can review.

I have tested the html output. Is correct.

Profiling.

=== 8.x..8.x compared (519c0a514701f..519c0a965645d):

ct  : 45,528|45,528|0|0.0%
wt  : 350,136|351,642|1,506|0.4%
cpu : 309,283|310,236|953|0.3%
mu  : 8,206,024|8,206,024|0|0.0%
pmu : 8,269,928|8,269,928|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0a514701f&...

=== 8.x..1843764 compared (519c0a514701f..519c0ad3d3949):

ct  : 45,528|46,218|690|1.5%
wt  : 350,136|351,982|1,846|0.5%
cpu : 309,283|312,782|3,499|1.1%
mu  : 8,206,024|8,471,116|265,092|3.2%
pmu : 8,269,928|8,535,084|265,156|3.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0a514701f&...

Can we get some understanding as to why the 1% performance regression....

There are more function calls yes, wt only goes up 0.1% though. Can someone else run this please so we can get some more numbers?

@cottser: I will work on re-profiling this.

The profiling results from #47 show that views-view-summary-unformatted was in fact not being used, @Fabianx and I just confirmed this. So this really needs re-profiling and needs to definitely use this template :) If you are not sure turn on twig_debug but ONLY for testing, turn off for profiling.

Thanks!

Here is my results from profiling this again. Very different from what cottser reported in #47, not sure why, but I ran it several times and even got a new baseline and ran it again.

Test Scenario

• Created 50 article test nodes
• Added a block display with an unformatted summary to the frontpage View
• Made the View's block with the unformatted summary display on the frontpage.

I then confirmed that the views-view-summary-unformatted.html.twig in the branch created from the work in #41 would be called in this scenario with twig_debug enabled (of course, I disabled twig_debug this prior to profiling). See attached screenshot for the scenario I profiled against.

Run 519d9cf28420e uploaded successfully for drupal-perf-drupalcon.
Run 519d9d52c2102 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..8.x compared (519d9cf28420e..519d9d52c2102):

ct  : 101,892|101,892|0|0.0%
wt  : 677,023|677,244|221|0.0%
cpu : 642,715|645,086|2,371|0.4%
mu  : 55,321,664|55,321,664|0|0.0%
pmu : 55,480,936|55,480,936|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d9cf28420e&...

=== 8.x..1843764-views-view-summary-unformatted compared (519d9cf28420e..519d9d94433aa):

ct  : 101,892|102,562|670|0.7%
wt  : 677,023|679,539|2,516|0.4%
cpu : 642,715|643,660|945|0.1%
mu  : 55,321,664|55,391,112|69,448|0.1%
pmu : 55,480,936|55,552,936|72,000|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d9cf28420e&...
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self

Back to needs review to see what @cottser and @alexpott think about these results.

Looks good to me. Is within measuring range and I cannot see any apparent slowdown.

=> Back to RTBC

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Committed e7d739a and pushed to 8.x. Thanks!