Convert theme_views_view_field to twig

When this was tried the first time, the performance was not good enough. Did we changed something here?

Lets see how we fare today. The difference might not be that big anymore ...

But why is this failing tests?

Looking at the list of remaining theme functions this would be a great one to get rid of if we can.

Patch still applies.

=== 8.0.x..8.0.x compared (54dec59a01be2..54dec697a5762):

ct  : 412,752|412,752|0|0.0%
wt  : 1,251,128|1,253,645|2,517|0.2%
mu  : 28,417,184|28,417,184|0|0.0%
pmu : 28,736,984|28,736,984|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54dec59a01be2&...

=== 8.0.x..convertviewfieldsingular compared (54dec59a01be2..54dec6fc087bf):

ct  : 412,752|425,666|12,914|3.1%
wt  : 1,251,128|1,303,376|52,248|4.2%
mu  : 28,417,184|29,586,648|1,169,464|4.1%
pmu : 28,736,984|30,385,248|1,648,264|5.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54dec59a01be2&...

I made a view displaying 100 nodes, 4 fields each. 2 with labels, two without.

My settings were off. This should be a more accurate run.

=== 8.0.x..8.0.x compared (54df8d322535a..54df91f17bc7c):

ct  : 407,275|407,275|0|0.0%
wt  : 1,567,985|1,566,436|-1,549|-0.1%
mu  : 53,512,512|53,512,512|0|0.0%
pmu : 53,602,952|53,602,952|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54df8d322535a&...

=== 8.0.x..convertviewfieldsingular compared (54df8d322535a..54df9298c351d):

ct  : 407,275|416,123|8,848|2.2%
wt  : 1,567,985|1,593,344|25,359|1.6%
mu  : 53,512,512|53,534,512|22,000|0.0%
pmu : 53,602,952|53,625,304|22,352|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=54df8d322535a&...

This one looks good from performance. I did a review in #15

Which is about ~25ms extra for rendering the template 400 times, which is about 62.5microseconds per template render. EDIT sigfig error.

There are some weird things that just cancelled themselves out.

I'm adding manual testing to make sure that whitespace modifier is doing what it's supposed to and just need a confirmation there is no line breaks there. {{ output -}}

Will this template still be used when views field handling uses entity displays? (#1867518: Leverage entityDisplay to provide fast rendering for fields)

As of that patch so far it is still used I think... but thanks for mentioning it, I'm curious how that will pan out.

Will this template still be used when views field handling uses entity displays? (#1867518: Leverage entityDisplay to provide fast rendering for fields)

Yes it will.

I'm curious what exactly gain we by replacing a trivial theme function with a template by default ... you are able to override the template, how you want it to be.
Not rendering via the twig template is a performance improvement I think which is worth to keep.

@dawehner, that might be a better discussion to have in #2348381: [META-0 theme functions left] Convert/refactor core theme functions. Overall, I think we're trying to keep things consistent by removing all theme_ functions.

I downloaded the patch and applied on my local. I did not see expected results. It needs some work I believe.

I tried it on Drupal 8 beta 10 version on Ubuntu 14.04 with Nginx as running as server.

Attached are some screen shots

The pending performance improvements around SafeMarkup (#2295823: Ensure that we don't store excessive lists of safe strings) give me some hope that we could get this performing well enough. That may be false hope but there you go.

Looks good to me.

I don't think that the rtbc in #29 has addressed the performance concerns raised by @dawehner in #22.

The patch still applied with offsets but there was a mode change to core/modules/views/views.theme.inc
that snuck in somewhere along the way, this patch reverts the mode change.

And this could use another round of profiling.

No interdiff because the only thing that changed was the mode of the previously mentioned file.

I looked into the failure in Drupal\views_ui\Tests\CustomBooleanTest, it's because custom boolean values like <p>Yay</p> are being escaped.

Looking into it

Two pickups and drop. Is it really hard reroll? Any hints may help the next person.

@joelpittet I was trying to fix it. I checked that the taxonomy links are escaping. Dont have much context about it, so I left it.
Any idea regarding how to fix it OR where to look in code may help the novices.

[OFFTOPIC] I see that for fixing even novice issues you need some context about Drupal 8 - about the archtecture and all (partially at least).

Thank you @subhojit777 that is helpful feedback that helps sort out why links are escaping.

Regarding off topic, if it becomes not Novice we usually remove the tag. We expected just a simple re-roll. Also for all Novice issues, look up at the parent Meta/Plan issue to see how others were resolved, as they likely have a pattern.

The reason this is happening is because we have auto-escape turned on. So markup that are passed to that output variable that aren't marked safe will be escaped.

Off hand I don't know the right solution ATM but what I plan to do put a breakpoint in the/a preprpocess before this is printed and see where the variable 'output' is being built up and why it may be marked unsafe.

@subhojit777 what would really help is let us know your or a "Steps to reproduce" that we can take to easily see the problem so we don't commit this without a test.

Moving this to Bug report because this needs to work for themes to use with autoescaping and templates as that is a regression since auto-escape was turned on.

+++ b/core/modules/views/templates/views-view-field.html.twig
@@ -24,4 +20,4 @@
-{{ output }}
+{{ output -}}

This whitespace modifier should do absolutely nothing. But we should actually add a test to ensure this.

This likely needs to be moved to a follow-up for the bug (if it doesn't exist already). But This should fix at least the taxonomy term list links.

Thanks @joelpittet I will keep that in mind for future reference. I will work on the last patch and try to fix the issues.

I was able to fix one test. I got some work to do, will look into it after I get free.

Tests are failing in CustomBooleanTest.php. HTML tags are getting escaped in line 103:
$output = \Drupal::service('renderer')->renderRoot($output);

$output is getting escaped even for <p> tags.

Will look into it tomorrow.

@subhojit777 ok I'm moving these fixes to their own issue and we will postpone this one on those fixes because they will get in faster and they are major/critical.

1. +++ b/core/modules/user/src/Tests/Views/HandlerFieldRoleTest.php
   @@ -42,8 +42,6 @@ public function testRole() {
   -    debug(db_query('SELECT * FROM {user__roles}')->fetchAll());
   -
   

   lol was this the fix? Whoops that shouldn't be there:) Nice find!

2. +++ b/core/modules/views/src/Plugin/views/field/PrerenderList.php
   @@ -95,7 +95,7 @@ public function renderItems($items) {
   -      return $this->renderer->render($build);
   +      return \Drupal::service('renderer')->renderRoot($build);
   

   Why renderRoot()? This is hardly a root call. Maybe renderPlain() but still how is this fixing things?

@subhojit777 could you upload your next patch over in #2560553: Views render pipeline is escaping CustomBooleanTest?

I'm setting up a test to show it's failing in head.

Quick reroll. Still expecting tests to fail.

Just removing the excess from our existing tests, and going to resolve just the boolean test in the other issue.

#62 passes once #2560553-54: Views render pipeline is escaping CustomBooleanTest is applied.

+++ b/core/modules/views/templates/views-view-field.html.twig
@@ -24,4 +20,4 @@
-{{ output }}
+{{ output -}}

@joelpittet I know you weren't sure if this was affecting anything - should it be kept in the patch?

Manually tested the following:

• Searched for any implementations of theme_views_view_field and did not find any other than the ones the patch is changing.
• Tested in latest head and with patch applied following the manual testing steps documented in the issue summary. Screenshots attached.

Also noting that once this patch gets in, #2118743: Twig debug output does not display all suggestions when an array of theme hooks is passed to #theme will be less easy to test. Oh well.

Oh I tested the white space modifier and it looks to indeed help with the trailing new line removal for inline fields. I did my test outside Drupal but it should be the same.

Here's my latest profiling. About 3.8%.

=== 8.0.x..8.0.x compared (5608311397db4..5608318a89619):

ct  : 494,667|494,667|0|0.0%
wt  : 951,165|955,922|4,757|0.5%
mu  : 30,339,856|30,338,608|-1,248|-0.0%
pmu : 30,934,128|30,931,512|-2,616|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5608311397db4&...

=== 8.0.x..convert-2348729-62 compared (5608311397db4..560831fb3d4d9):

ct  : 494,667|506,634|11,967|2.4%
wt  : 951,165|991,828|40,663|4.3%
mu  : 30,339,856|30,360,408|20,552|0.1%
pmu : 30,934,128|30,955,568|21,440|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5608311397db4&...

aws fails.

Patch is green again.

Hmm I see contextual_preprocess() is getting called every time, that's unfortunate. I think @joelpittet and I talked briefly about it. It's because template_preprocess() is not called for theme functions. I wonder if there's something we can do to optimize these types of cases, at least for contextual.

Either way I think it's just worth pointing out that contextual_preprocess() accounts for almost 1.4ms here. Not sure if it's worth reprofiling over.

Regarding the contextual_preprocess() problem. I'm curious whether we could introduce a flag on render elements which will be require to run contextual_preprocess() on those particular elements in the first place? Given that we have max 19 usages of #contextual_links we could bypass most of the calls in the first place I think.
Could we require callers to maybe also add contextual_preprocess() manually? I guess this could be possible.

Hm. I think this could use sign-off from catch as performance topic coordinator. It's worth pointing out that some views, such as issue queue views on d.o, have 50x10 instances of this template, so 3.8% performance slowdown is significant.

I'm afraid that views-view-field-html.twig markup is being escaped :(

EDIT: Might be only views markup fields. I'm testing this manually and RTBC'ing back soon if I can make sure that is correct.

Bug exists only for the custom text views field. Back to RTBC.

We've committed to using templates everywhere, so places like this (and actual field templates) we need to take the performance hit to gain the security/consistency.

I did open #2580263: Find a way to not run contextual_preprocess() on every template to look at contextual_preprocess().

Committed/pushed to 8.0.x, thanks!