Convert views/templates/views-view-fields.tpl.php to twig

stripped out of meta patch, first draft

Tagging.

Committed and attributed, thanks!

Moving to core queue

This is a straight re-roll of @joelpittet's patch in #1 against 8.x HEAD and deleted the corresponding .tpl.php.

/**
 * Preprocess theme function to print a single record from a row, with fields
 */
function template_preprocess_views_view_fields(&$vars) {
  $view = $vars['view'];

  // Loop through the fields for this view.
  $previous_inline = FALSE;

@tlattimore thanks, it passed!:) Could you clean up the $vars to $variables in the preprocessor? And the preprocessor docs?

Here is an updated patch that includes the requested changes joelpittet mentioned in #8.

Tagging.

Review for #10:

Twig templates need @see template_preprocess(), template_preprocess_views_view_fields() and @ingroup themeable.

{% if field.separator is defined %}

Can we do away with "is defined"?

+ *   - field.handler: The Views field handler object controlling this field. Do not use
+ *     var_export to dump this object, as it can't handle the recursion.

Is var_export something people will be doing inside a Twig template? I think we could drop that from the comment.

The preprocess looks good to me, the only thing it's really changing is $vars to $variables.

I think this is just about ready for manual testing of the markup though, these changes are pretty minor.

Thanks @thedavidmeister! A couple more tiny tweaks:

+++ b/core/modules/views/views.theme.incundefined
@@ -175,24 +175,39 @@ function template_preprocess_views_view(&$vars) {
+ * Prepares variables for Views fields template.
...
+ * Default template: views-view-fields.html.twig

Prepares variables for … templates (plural templates) per #1913208: [policy] Standardize template preprocess function documentation.

The 'Default template' line should end in a period.

If we're only changing $vars to $variables in the preprocess, let's wait until #1963942: Change all instances of $vars to $variables to do that and only update the docblock (roll back the vars -> variables changes). As for the separator, I don't see any need for 'is defined' here.

The tweaks in #12 and here would make a good novice task, tagging.

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!

Here is a patch that makes the changes requested in #13, #12, and also some other minor doc changes.

Tagging for review.

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ * - view: The view in use.
+ * - fields: an array of field objects. Each one contains:

We need to remove mention of complex data structures in templates. How about "A list of fields, each one contains:"

I notice this patch also contains the change from $vars to $variables, which will make the patch in #1963942: Change all instances of $vars to $variables fail. In other views conversions we've been leaving those alone, so for consistency the change should probably be rolled-back here.

Here is a patch that makes the changes requested in #16. Switching $variables back to $vars & the doc change.

tagging

#16, #17 - The reason that linked issue is postponed is so that we don't have to reroll all the conversion patches.

Yeah... I'd rather we postpone this one, since it's much easier to re-roll and requires far less coordination.

We're expecting the patch in #1963942: Change all instances of $vars to $variables to need re-rolling when the Twig conversions land.

+ * - field.raw: The raw data for the field, if it exists. This is NOT output safe.
+ * - field.wrapper_prefix: A complete wrapper containing the inline_html to use.

Comments longer than 80 characters.

+ * - view: The view object.

Need a capital V for View.

+ *       - inline: An array that contains the fields that are to be
+ *       displayed inline.

+ *       - seperator: A string to be placed between inline fields to keep
+ *       them from squishing up next to each other.

I think "them" and "displayed" are supposed to be indented more, aren't they?

+ *       - default_field_elements: If default field wrapper
+ *       - elements are to be provided.

These two lines look like a mistake, is it supposed to be a single dot point?

+ * - row: An array containg information about the current row.

"containg" is not a word.

Here is a patch that makes the doc changes requested in #20.

Quick review :)

Thanks for all your work on this issue @tlattimore!

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,40 @@
+ * - fields: A list of fields, each one contains:
+ *   - field.content: The output of the field.
+ *   - field.raw: The raw data for the field, if it exists.
+ *     This is NOT output safe.
+ *   - field.class: The safe class id to use.
+ *   - field.handler: The Views field handler object controlling this field.
+ *   - field.inline: Whether or not the field should be inline.
+ *   - field.inline_html: either div or span based on the above flag.
+ *   - field.wrapper_prefix: A complete wrapper containing the
+ *     inline_html to use.
+ *   - field.wrapper_suffix: The closing tag for the wrapper.
+ *   - field.separator: an optional separator that may appear before a field.
+ *   - field.label: The wrap label text to use.
+ *   - field.label_html: The full HTML of the label to use including
+ *     configured element type.

Since 'field' is the loop variable used in the template and could be called anything, we shouldn't use 'field' in the docs here.

Something like:

- fields: A list of fields, each field contains:
  - content: The output…
  - raw: The raw data…

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,40 @@
+  {% if field.separator  %}
+    {{ field.separator }}
+  {% endif %}

Can we remove this if? I don't see why we need it.

+++ b/core/modules/views/views.theme.incundefined
@@ -175,11 +175,26 @@ function template_preprocess_views_view(&$vars) {
+ *   An associative array containing:
+ *     - view: The View object.
+ *     - options: An array of options. Each option contains:
+ *       - inline: An array that contains the fields that are to be
+ *         displayed inline.
+ *       - default_field_elements: If default field wrapper
+ *         elements are to be provided.
+ *       - hide_empty: Whether the field is to be hidden if empty.
+ *       - element_default_classes: If the default classes are to be added.
+ *       - seperator: A string to be placed between inline fields to keep
+ *         them from squishing up next to each other.
+ *     - row: An array containing information about the current row.

The list is indented by two spaces too many, see http://drupal.org/node/1354#lists.

Edited to fix tags.

Thanks for the feedback again, Cottser. Here is an updated patch that makes the changes requested at by Cottser in #22.

gah! tagging.

I've applied the patch & tested the recent comments block display. Text added to the twig template file did not appear (caches flushed from UI).

Manually tested the patch from #23:

1. Enable the Recent comments view at admin/structure/views.
2. Add an article node.
3. Add comments to the article node.
4. Navigate to comments/recent.

Also tested the recent comments block as per #25.

Markup matches other than whitespace.

Before:

<li  class="views-row views-row-1 views-row-odd views-row-first">  
  <div class="views-field views-field-title">    <span class="views-label views-label-title">Reply to: </span>    <span class="field-content"><a href="/node/2">test twig</a></span>  </div>  
  <div class="views-field views-field-timestamp">        <span class="field-content"><em class="placeholder">11 min 33 sec</em> ago</span>  </div>  
  <div class="views-field views-field-subject">        <span class="field-content"><a href="/comment/2#comment-2">test wow</a></span>  </div>  
  <div class="views-field views-field-comment">        <div class="field-content"></div>  </div></li>
          <li  class="views-row views-row-2 views-row-even views-row-last">  
  <div class="views-field views-field-title">    <span class="views-label views-label-title">Reply to: </span>    <span class="field-content"><a href="/node/2">test twig</a></span>  </div>  
  <div class="views-field views-field-timestamp">        <span class="field-content"><em class="placeholder">11 min 41 sec</em> ago</span>  </div>  
  <div class="views-field views-field-subject">        <span class="field-content"><a href="/comment/1#comment-1">wow!</a></span>  </div>  
  <div class="views-field views-field-comment">        <div class="field-content"></div>  </div></li>

After:

<li  class="views-row views-row-1 views-row-odd views-row-first">  

  <div class="views-field views-field-title">
    <span class="views-label views-label-title">Reply to: </span>
    <span class="field-content"><a href="/node/2">test twig</a></span>
  </div>
  

  <div class="views-field views-field-timestamp">
    
    <span class="field-content"><em class="placeholder">14 min 9 sec</em> ago</span>
  </div>
  

  <div class="views-field views-field-subject">
    
    <span class="field-content"><a href="/comment/2#comment-2">test wow</a></span>
  </div>
  

  <div class="views-field views-field-comment">
    
    <div class="field-content"></div>
  </div>
</li>
          <li  class="views-row views-row-2 views-row-even views-row-last">  

  <div class="views-field views-field-title">
    <span class="views-label views-label-title">Reply to: </span>
    <span class="field-content"><a href="/node/2">test twig</a></span>
  </div>
  

  <div class="views-field views-field-timestamp">
    
    <span class="field-content"><em class="placeholder">14 min 17 sec</em> ago</span>
  </div>
  

  <div class="views-field views-field-subject">
    
    <span class="field-content"><a href="/comment/1#comment-1">wow!</a></span>
  </div>
  

  <div class="views-field views-field-comment">
    
    <div class="field-content"></div>
  </div>
</li>

Removing manual testing tag.

I was going to mark this as RTBC but I noticed that there's a few references to "objects" in the Template documentation and we're not using PHP data types in Twig templates.

More documentation nitpicks:

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ * Default view template to all the fields as a row.

to *display* all the fields?

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ *   - class: The safe class id to use.

Capitalize 'ID'.

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ *   - inline_html: either div or span based on the above flag.
...
+ *   - separator: an optional separator that may appear before a field.

These sentences need to start with a capital letter per http://drupal.org/node/1354#drupal.

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ *   - wrapper_prefix: A complete wrapper containing the
+ *     inline_html to use.

Re-wrap please :)

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ *   - label: The wrap label text to use.

This could use some better wording.

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,38 @@
+ * @ingroup views_templates

@ingroup themeable instead of @ingroup views_templates please.

+++ b/core/modules/views/views.theme.incundefined
@@ -175,11 +175,26 @@ function template_preprocess_views_view(&$vars) {
+ * Prepares variables for Views fields templates.
...
+*    - view: The View object.

I think this should be lowercase 'views' and 'view'.

@Cottser - this is for you #1992358: [Policy] Decide on capitalisation of the "v" in "view(s)" when talking about views objects/module in documentation :)

nitpick:

+*      - seperator: A string to be placed between inline fields to keep
+*        them from squishing up next to each other.

I'm not sure that the phrase "squishing up next to each other" is friendly for users that don't speak English as a first language. It wasn't in the original documentation, could we just say "to keep them visually distinct." or "keep them separated."?

Other than that, this looks RTBC to me :)

Tagging for profiling.

dealing with nitpickery...

last patch from comment #31 does not apply anymore, rerolling.

nevermind comment #36. the patch does apply. back to tweaking the comments as requested in #33.

fixed typo while at it.

Great, and we have done manual tests for this already.

More docs/coding standard tweaks, then back to RTBC:

+++ b/core/modules/views/templates/views-view-fields.html.twigundefined
@@ -0,0 +1,36 @@
+ * @ingroup themable

s/themable/themeable/

+++ b/core/modules/views/views.theme.incundefined
@@ -175,11 +175,26 @@ function template_preprocess_views_view(&$vars) {
+ * @param array $vars
+ *   An associative array containing:
+*    - view: The view object.
+*    - options: An array of options. Each option contains:
+*      - inline: An array that contains the fields that are to be
+*        displayed inline.
+*      - default_field_elements: If default field wrapper
+*        elements are to be provided.
+*      - hide_empty: Whether the field is to be hidden if empty.
+*      - element_default_classes: If the default classes are to be added.
+*      - separator: A string to be placed between inline fields to keep them
+*        visually distinct.
+*    - row: An array containing information about the current row.
  */

These asterisks in the docblock should all line up, the lines starting at '- view' and ending at '- row' need another space before the asterisk.

Improvements from #40

Much better, thanks @c4rl!

Profiling.

Forgot to assign this to myself. I just ran a profile on this one, results not great.

Scenario

* Convert front page view to fields from content
* Front page view shows title and body
* 50 nodes generated with devel generate
* Full node view block displaying one node placed on front page

=== 8.x..8.x compared (519c0e87b9da9..519c0eb2eb2a9):

ct  : 46,565|46,565|0|0.0%
wt  : 220,513|220,051|-462|-0.2%
cpu : 210,591|210,040|-551|-0.3%
mu  : 15,028,040|15,028,040|0|0.0%
pmu : 15,223,752|15,223,752|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0e87b9da9&run2=519c0eb2eb2a9&source=drupal-perf-drupalcon&extra=8.x..8.x

Run 519c0e87b9da9 uploaded successfully for drupal-perf-drupalcon.
Run 519c0eeeccc37 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..views-view-fields-twig compared (519c0e87b9da9..519c0eeeccc37):

ct  : 46,565|47,705|1,140|2.4%
wt  : 220,513|225,391|4,878|2.2%
cpu : 210,591|215,241|4,650|2.2%
mu  : 15,028,040|15,069,192|41,152|0.3%
pmu : 15,223,752|15,264,976|41,224|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0e87b9da9&run2=519c0eeeccc37&source=drupal-perf-drupalcon&extra=8.x..views-view-fields-twig

My results weren't particularly good either.

Scenario:

• View on homepage
• Displaying all nodes of Article type, numbering about 50
• Displaying fields: Post date (Post date), Content: Title (Title), Content: Body (Body), Content: Comment count (Comment count), Content: Has new content (Has new content), Content: Image (Image), Content: Link to content (Link to content), Content: Nid (Nid), Content: Path (Path), Content: Post date (Post date), Content: Sticky status (Sticky status), Content: Tags (Tags), Content: Title (Title), Content: Type (Type)

=== 8.x..8.x compared (519c116ac5313..519c13f61d210):

ct  : 149,545|149,545|0|0.0%
wt  : 963,123|962,213|-910|-0.1%
cpu : 905,670|906,381|711|0.1%
mu  : 9,972,008|9,972,008|0|0.0%
pmu : 10,372,976|10,372,976|0|0.0%

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

=== 8.x..1843748 compared (519c116ac5313..519c146573182):

ct  : 149,545|163,751|14,206|9.5%
wt  : 963,123|1,019,342|56,219|5.8%
cpu : 905,670|968,498|62,828|6.9%
mu  : 9,972,008|10,232,408|260,400|2.6%
pmu : 10,372,976|10,639,096|266,120|2.6%

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

Removing "Needs profiling" tag.

Should this one be bumped to needs work or some other status given the results of profiling?

Please re-profile this one so we can get some more data.

Re-profiled with the same scenario as #45:

=== 8.x..8.x compared (519de1c349c58..519de2415c35a):

ct  : 157,223|157,223|0|0.0%
wt  : 628,439|627,684|-755|-0.1%
cpu : 591,206|591,522|316|0.1%
mu  : 18,686,112|18,686,112|0|0.0%
pmu : 19,033,944|19,033,944|0|0.0%

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

=== 8.x..views-view-fields-1843748-41 compared (519de1c349c58..519de26535836):

ct  : 157,223|171,423|14,200|9.0%
wt  : 628,439|661,766|33,327|5.3%
cpu : 591,206|625,758|34,552|5.8%
mu  : 18,686,112|18,731,528|45,416|0.2%
pmu : 19,033,944|19,085,056|51,112|0.3%

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

The 33ms overhead can be seen here:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=__TwigTempla...

getAttribute with 20 ms here is more overhead than usual, because it is dealing with objects.

twig_render_var is slightly slower, but within normal what we had seen.

getContextReference is not slow, but called a lot.

Two ways to go:

* a) Create a special "mode" within twig to directly generate syntax to drill down into objects and never use any references. (that would remove almost all of the overhead, but one would need to use this "high-performance" mode).

* b) Or we accept that syntactical sugar costs 5% in this case, but to make core not any slower, we add a theme_ function for views-view-fields.html.twig and have the theme function be used by default like views-view-field.html.twig and field.html.twig.

Most users would not need to override this anyway, so a theme function is probably the best solution short term and would also help overall Drupal performance.

Just removing an unneeded change from the preprocess function, which was the removal of a blank line.

Re: #51, I am partial to 'A', a high performance mode is interesting idea and I would even say that would be default. Sure show()/hide() have their use-cases but I would venture to guess they are not common and most themers don't take advantage of those regularly. Would love to hear others opinion though.

+1 for b in #51. I'm personally not a fan of using templates to achieve a loop of simple tags or are themselves basically a simple tag that usually gets called in a loop - in these cases the template isn't making anything "clearer" or "easier" for themers, it's there "just in case".

wait... I like both options. The options don't seem to be mutually exclusive in any way.

@rcaracaus and I rerolled, moving the stuff from the template to a theme function. Maybe we need to add some more documentation on this.

I just added a doc block for the theme_ function to look more like similar functions in core.

Re-rolled changes from 52-57 into one patch. Disregard this one, left some cruft in there from older patches.

Updated patches from 58.

Profiling results are in, and of course the theme function is faster than the .tpl.php file.

Attached patch updates docs and uses $variables for the theme function param.

=== 8.x..8.x compared (519f012418c24..519f02001a77a):

ct  : 157,223|157,223|0|0.0%
wt  : 630,992|631,315|323|0.1%
cpu : 595,393|593,939|-1,454|-0.2%
mu  : 18,686,112|18,686,112|0|0.0%
pmu : 19,033,944|19,033,944|0|0.0%

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

=== 8.x..views-view-fields-1843748-60 compared (519f012418c24..519f02278cd90):

ct  : 157,223|156,803|-420|-0.3%
wt  : 630,992|628,538|-2,454|-0.4%
cpu : 595,393|592,181|-3,212|-0.5%
mu  : 18,686,112|18,679,408|-6,704|-0.0%
pmu : 19,033,944|19,014,816|-19,128|-0.1%

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

Looked through and that code looks solid, nice work guys!

Okay, I changed the scope of #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code to include typing objects for faster performance, so the followup for this is created as well.

+1 for RTBC

Needs a reroll

Needs a reroll

Let's a go, Mario!

Oops, hadn't refreshed my page, didn't mean to steal that! go on with your work!

Reroll, no interdiff, because basiclly nothing changed

I will review this.

I can confirm that the patch in 68 applies cleanly and works as described. RTBC if it the testbot goes green.

Hmmm. This was 2 hours ago that the test request was sent and I am not sure why the test hasn't run yet? And since it hasn't run yet there isn't the link to re-queue it for re-testing...

I suspect testbot is just overloaded. There's a huge number of people here at DrupalCon.

Committed dc98af8 and pushed to 8.x. Thanks!