Task

Use Twig instead of theme function

Remaining

  • Remove the theme_ function
  • Update all hook_theme definitions
  • Review patch
  • Test patch
  • Revisit performance

There is already a twig file for this porpuse. views-view-field.html.twig.

There is a preprocess function for the template already as well: template_preprocess_views_view_field.

Manual testing steps

  1. Install Drupal 8.
  2. Create a node of any content type.
  3. Visit the content administration page at admin/content
  4. Ensure that the table rows (not the header row) render correctly
Files: 
CommentFileSizeAuthor
#68 views_view_field-after.png28.03 KBakalata
#68 views_view_field-before.png27.64 KBakalata
#62 convert-2348729-62.patch2.07 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,647 pass(es). View
#59 convert_2348729_59.patch2.73 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,719 pass(es), 4 fail(s), and 0 exception(s). View
#46 interdiff2348729-42-46.txt1.42 KBsubhojit777
#46 convert-2348729-46.patch5.47 KBsubhojit777
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch convert-2348729-46.patch. Unable to apply patch. See the log in the details link for more information. View
#42 interdiff.txt2.45 KBjoelpittet
#42 convert-2348729-42.patch4.52 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,215 pass(es), 3 fail(s), and 0 exception(s). View
#31 convert-2348729-31.patch2.07 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,891 pass(es), 4 fail(s), and 1 exception(s). View
#25 NodePage.png68.63 KBjdcosta
#25 CustomView.png65 KBjdcosta
#25 ContentView.png77.09 KBjdcosta
#25 Patchapplication.png108.08 KBjdcosta
#21 2348729-22.patch2.07 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es). View
#16 XHProf__Hierarchical_Profiler_Report.png327.67 KBjoelpittet

Comments

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
1.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,610 pass(es), 80 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: 2348729-convert-theme_views_view_field-twig-1.patch, failed testing.

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,965 pass(es), 80 fail(s), and 0 exception(s). View

Previous patch wasn't applying anymore.
Now that #2226207: Make 'template' the default output option for hook_theme() got in, we no longer need to set the 'template' in hook_theme.
Removed some comments in the template since it'd be now used instead of the theme function.

Status: Needs review » Needs work

The last submitted patch, 4: 2348729-convert-theme_views_view_field-twig-4.patch, failed testing.

dawehner’s picture

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

Fabianx’s picture

Assigned: Unassigned » Fabianx
Issue tags: +Needs profiling

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

But why is this failing tests?

Fabianx’s picture

Assigned: Fabianx » Unassigned
Manuel Garcia’s picture

OK, I've managed to get a promise to help on profiling, if I get the patch to show up green =)

So, I've been trying to fix the tests, and it seems that the errors derive from the twig file (or something before), which adds an empty space after the output. Typical error reads:

Value 'eCvgVR6C' is equal to value 'eCvgVR6C '.

I can't figure out why this is happening though... theme_views_view_field($variables) that was being used before the patch doesn't do any processing at all, and the twig file is simple enough. Any pointers¿?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,064 pass(es). View

OK, so @Cottser in IRC pointed out to me that it is the new line at EOF that is causing this. This is however a coding standard, so until we figure out what we'll do about this (see #2245901: Trim trailing EOF whitespace from templates), here is an updated patch which modifies the twig file to spit out {{ output -}} to get rid of the new line.

I've ran the offending tests locally and they all passed, at least we'll be able to profile this patch now.

Cottser’s picture

Issue tags: +sprint

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

davidhernandez’s picture

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

davidhernandez’s picture

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

davidhernandez’s picture

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

akalata’s picture

Issue tags: -Needs profiling
joelpittet’s picture

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

Fabianx’s picture

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

joelpittet’s picture

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.

dawehner’s picture

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.

akalata’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es). View
dawehner’s picture

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.

akalata’s picture

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

lauriii queued 21: 2348729-22.patch for re-testing.

jdcosta’s picture

Status: Needs review » Needs work
FileSize
108.08 KB
77.09 KB
65 KB
68.63 KB

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

Manuel Garcia’s picture

Status: Needs work » Needs review

@jdcosta thanks for the review, can you try again applying the patch against the 8.0.x branch? (you can see git instructions here https://www.drupal.org/project/drupal/git-instructions ).

Also, make sure to run a cache rebuild after applying the patch. (drush cr if you are using drush, or clear the cache in the performance page of your drupal installation).

Cottser’s picture

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.

daffie queued 21: 2348729-22.patch for re-testing.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

Cottser’s picture

Issue tags: +Needs profiling
FileSize
2.07 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,891 pass(es), 4 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 31: convert-2348729-31.patch, failed testing.

Cottser’s picture

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

evilfurryone’s picture

Assigned: Unassigned » evilfurryone
evilfurryone’s picture

Assigned: evilfurryone » Unassigned
subhojit777’s picture

Assigned: Unassigned » subhojit777

Looking into it

subhojit777’s picture

Assigned: subhojit777 » Unassigned
joelpittet’s picture

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

subhojit777’s picture

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

joelpittet’s picture

Category: Task » Bug report
Issue tags: -Novice

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.

joelpittet’s picture

Issue tags: +Needs tests
+++ 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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,215 pass(es), 3 fail(s), and 0 exception(s). View
2.45 KB

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.

Status: Needs review » Needs work

The last submitted patch, 42: convert-2348729-42.patch, failed testing.

subhojit777’s picture

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

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
FileSize
5.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch convert-2348729-46.patch. Unable to apply patch. See the log in the details link for more information. View
1.42 KB

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

subhojit777’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: convert-2348729-46.patch, failed testing.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned

Will look into it tomorrow.

joelpittet’s picture

Category: Bug report » Task
Status: Needs work » Postponed
Issue tags: -Needs tests

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

joelpittet’s picture

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

The last submitted patch, 42: convert-2348729-42.patch, failed testing.

Cottser’s picture

Issue tags: +Twig conversion

iMiksu queued 46: convert-2348729-46.patch for re-testing.

Status: Postponed » Needs work

The last submitted patch, 46: convert-2348729-46.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,719 pass(es), 4 fail(s), and 0 exception(s). View

Quick reroll. Still expecting tests to fail.

Status: Needs review » Needs work

The last submitted patch, 59: convert_2348729_59.patch, failed testing.

The last submitted patch, 59: convert_2348729_59.patch, failed testing.

joelpittet’s picture

FileSize
2.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,647 pass(es). View

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

akalata’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 62: convert-2348729-62.patch, failed testing.

The last submitted patch, 62: convert-2348729-62.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
akalata’s picture

+++ 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 suggestions is passed to #theme will be less easy to test. Oh well.

joelpittet’s picture

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.

joelpittet’s picture

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

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 62: convert-2348729-62.patch, failed testing.

davidhernandez’s picture

aws fails.

mdrummond’s picture

Status: Needs work » Reviewed & tested by the community

Patch is green again.

Cottser’s picture

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.

dawehner’s picture

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.

webchick’s picture

Assigned: Unassigned » catch

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.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

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.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • catch committed 8d196a7 on 8.0.x
    Issue #2348729 by Manuel Garcia, joelpittet, akalata, subhojit777,...

Status: Fixed » Closed (fixed)

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