Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#81 | 1843746-views-view-field-81.patch | 3.24 KB | tlattimore |
#81 | interdiff.txt | 564 bytes | tlattimore |
#73 | 1843746-73-views-view-field.patch | 3.27 KB | joelpittet |
#73 | interdiff.txt | 1.58 KB | joelpittet |
#69 | interdiff.txt | 2.94 KB | shanethehat |
Comments
Comment #1
joelpittetseparated from meta patch, first draft
Comment #2
rovoI've run this patch against the latest Front-end branch.
It created:
/drupal_8_twig_sandbox/core/themes/stark/templates/views-view.html.twig
I compared the output before and after the patch.
There were no errors, and appeared the same.Attaching DOM capture of #content before and after the patch.
Is there more I should look for?
Needed to switch the Admin to Stark in order for Views to pick up the views-view.html.twig template.
Now the "H2 A" elements are not printing out in the Summaries on the generated View Page.
<h2><a href="/node/32" rel="bookmark"></a></h2>
Comment #3
mbrett5062 CreditAttribution: mbrett5062 commentedTagging.
Comment #4
adamdicarlo CreditAttribution: adamdicarlo commentedWorking OK for me except for image fields - however I suspect that might be due to a separate issue.
Views' PHP template:
Twig template:
Comment #5
dawehnerThe problem mentioned in #4 seems to be unrelated to this issue though for sure worth to report! Thank you.
Comment #6
FluxSauce CreditAttribution: FluxSauce commentedCommitted and correctly attributed, thanks!
Comment #8
joelpittetMoving to core queue
Comment #9
tlattimore CreditAttribution: tlattimore commentedThis is a straight re-roll of @joelpittet's patch in #1 against 8.x HEAD and deleted the corresponding .tpl.php.
Comment #10
joelpittetThanks @tlattimore
I am moving this back to needs work for some cleanup docs and the preprocessor.
There is a theme function for this that needs to be moved into the preprocessor.
Also, would like to get the variable
$vars
converted to$variables
Comment #11
trrroy CreditAttribution: trrroy commentedComment cleanup, theme function removed and 'vars' changed to 'variables'.
Comment #12
steveoliver CreditAttribution: steveoliver commentedA few more nitpicks:
1. Change "Variables available" to "Available variables" -- it's what we've been doing in all the other templates.
2. Add punctuation to the ends of the sentences.
Let's add an '\n@ingroup themeable' to the bottom of this docblock.
Since the theme function is gone, the entirety of this docblock text should be:
"Prepares variables for views field templates.\n\n Default template: views-view-field.html.twig."
Comment #13
trrroy CreditAttribution: trrroy commentedChanges are applied.
Comment #15
damiankloip CreditAttribution: damiankloip commentedLooks good, I just think the definition in views_theme needs changing too, and this is good to go.
This only part I'm not sure about is this part: "It is not actually used in default Views ..." huh? :)
Comment #17
joelpittetComment #18
jastraat CreditAttribution: jastraat commentedRemoved the line from http://drupal.org/node/1843746#comment-7252450 since without the theme function it no longer applies.
Added @see to documentation in line with http://drupal.org/node/1823416
Add $variables param to documentation in line with http://drupal.org/node/1913208
Comment #20
jastraat CreditAttribution: jastraat commented@ damiankloip
Quick question about #15: could you explain a bit more about the change from pattern to template in views_theme()? I think views_view_grouping is a similar case, and I want to understand a little better.
Comment #21
dawehnerI'm wondering whether we should have performance testing for this one ... as fields are rendered a lot. (could be more then hundred on a table view). Previously there is the theme_ vs. template trick, should we at least test this?
Comment #22
joelpittet@jastraat I am almost positive that is not necessary for this conversion. Remove that change and give that a go again. Nice work so far:)
Comment #23
joelpittetI retract #22, that is the default pattern, so shouldn't be necessary and probably does need template because it had a theme_ function and template file...
This one needs another pair of eyes, please.
Comment #24
dawehnerSo the idea of this small piece of code was the following: Use the theme function by default, as it's certainly faster then using a template. As written before, if you have like 100 or more calls to this theme function on a page, this can be problematic.
As there will be no theme functions in D8, just twig, there is no need to special case the pattern/template bit.
Comment #25
jastraat CreditAttribution: jastraat commented@dawehner - Does that mean that the entire section for views_view_field in views_theme() can be removed? Since the theme function will no longer exist?
Comment #26
star-szrI can answer #25, the hook_theme() definition is still needed even though the output will be through a template file.
Back to needs work, the test failures need to be addressed. Thanks everyone!
Comment #27
star-szrTagging.
Comment #28
joelpittetWould love to see profiling as mentioned in #21.
This should fix the errors as as Views advanced_render() returns null from time to time.
Comment #29
dawehnerWell, but what is the problem with maybe put out a NULL variable, is this a specific twig problem?
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commented#29 - Looks like it's a problem with the way that Twig is implemented in Drupal - It will throw an exception whenever a NULL variable is passed to a Twig template. Same problem we're seeing in #1961872: Convert html.tpl.php to Twig.
I've opened a couple of tickets to see what can be done about it, if anything #1975442: drupal_render() should always return a string., #1975462: Allow and test for NULL and integer 0 values in Twig templates..
Regardless of whether this is resolved within Twig itself, can we not put the workaround inside the template? I really think this will be confusing to front end developers, as they'll have no idea when something coming in could possibly be NULL - if we break the seal we'll see {% if foo is defined %} appearing *everywhere* in contrib/custom templates.
Just change
+ $variables['output'] = $variables['field']->advanced_render($variables['row']);
to something that enforces a string data type like:Comment #31
bdone CreditAttribution: bdone commentedAside from performance, manual testing of #28 looks good:
Before:
After:
Comment #32
star-szrDoing some profiling here and likely posting a revised patch to lose the if statement.
Comment #33
dawehnerYeah we do have a preprocess function, which allows us totally to fix this NULL problem. I guess "" is a proper value for that.
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commented#33 - As in #1961872: Convert html.tpl.php to Twig, #1975442: drupal_render() should always return a string., if there's a function that's supposed to be "rendering" values - in this case Views advanced_render() - we probably want to make sure that it returns a string reliably rather than NULL. This would hopefully be a better fix than working around the Twig engine in the preprocess or the template.
Incidentally,
advanced_render() sometimes calls drupal_render(), which sometimes returned NULL until yesterday. Is there any other way for Views advanced_render() to return NULL? If not, this problem is already fixed.
Comment #35
dawehnerMaybe this could fix this 100%, though i'm not sure.
Comment #36
dawehnerMaybe this could fix this 100%, though i'm not sure.
Comment #37
thedavidmeister CreditAttribution: thedavidmeister commentedtestbot
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commented@dawehner - how about
return (string) $value;
? then yeah, from the looks of it, it should be impossible for Views to return anything non-stringy from advanced_render().Comment #41
dawehnerYeah, this could work. I guess we have to manually adapt one of the tests?
Comment #42
thedavidmeister CreditAttribution: thedavidmeister commented#41 - or drupal_render($value);? anyway, we're totally hijacking this thread now. We should make a new issue for the Views render.
We should just re-roll #28 without the "is not null" and see what happens.
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commented#18: drupal_twig_views_field_template-1843746-18.patch queued for re-testing.
Comment #44
dawehnerI agree.
Comment #45
star-szrI'm not sure we need the 'if' at all, will post a patch tonight and some preliminary profiling numbers.
Comment #46
star-szrIt does make sense to keep the if block, at least with the current state of things.
Profiled with @Fabianx's xhprof-kit, and thanks to his help and patience I put together and profiled a scenario that renders 200 fields in a table view.
Comparisons:
Theme function vs. PHPTemplate implementation:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51774c26bf054&...
Theme function vs. Twig template implementation:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51774c26bf054&...
After looking at the profiling results and discussing with @Fabianx, we already have at least a few ideas for optimization:
Total - somewhere around 11ms, which should at least make the Twig implementation quicker than the PHPTemplate one. Longer term maybe we can get rid of template_preprocess() altogether or slim it down some more.
I'm also wondering (completely untested theory) if we could lose the whitespace operators here if that would mean better performance.
Scenario profiled:
The site homepage was used for profiling, with one of the 50 nodes set as the default homepage and the table view added as a block to the homepage.
Comment #47
star-szrI couldn't detect any difference in performance at all after removing the whitespace modifiers. Good to know.
Comment #48
star-szrOops, that'll teach me to reuse a tab :)
Comment #49
joelpittetI am quite sure we don't need the if statement if we get this guy in #1975462: Allow and test for NULL and integer 0 values in Twig templates. If I remember right it's why I put that statement in the first place. Also, that should help a bit with the performance a bit as well. Cottser maybe you can roll the two for a test?
Comment #50
star-szr@joelpittet/#49 - after doing some tests it looks like #1975462: Allow and test for NULL and integer 0 values in Twig templates. will either have no impact on performance or a small negative impact for this particular scenario. The numbers are quite close though and it's tough to pin them down. After adding #1975462: Allow and test for NULL and integer 0 values in Twig templates. and removing the if block from views-view-field.html.twig there are 75 more function calls in the scenario described in #46.
Comment #51
dawehnerThanks for this great benchmark. So was far as I understand this performance regression is too much at the moment, so we should better wait until the points you listed went it. Hopefully this will lower then the costs.
Comment #52
star-szrWell, currently #1938430: Don't add a default theme hook class in template_preprocess() is postponed on conversion. Maybe we need to rethink that now if this can't be committed without it.
Edit: Or we do all the other optimizations we can and keep #1938430: Don't add a default theme hook class in template_preprocess() postponed?
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commented#1975462: Allow and test for NULL and integer 0 values in Twig templates. just landed. Does this mean we can remove the "if output" and just leave {{ output }} to improve performance?
Comment #54
star-szrYes, but doing so didn't seem to have a measurable effect on performance from what I remember. Which is both good and bad :)
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commented#54 - sure. well... remove it anyway for readability?
Comment #56
star-szrYes, we should still remove it because it's unnecessary.
Comment #57
joelpittetRemoved twig if;)
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commentedIs that failing because of whitespace?
Comment #60
joelpittetYeah @thedavidmeister that seemed to be the issue.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedHow does "field handler object" and "raw SQL result" fit in with our rules against discussing PHP data types in template comments?
See http://drupal.org/node/1757550#comment-7358798
Comment #62
shanethehat CreditAttribution: shanethehat commentedComment #63
star-szrWe should also split off the theme_ function removal into its own issue along the same lines as #1843752: Convert views/templates/views-view-grouping.tpl.php to twig and #1912606: Remove theme_views_view_grouping function. Thanks!
Edit: In the end we will probably either keep the theme_ function (still allowing template overrides as we do now), or we may convert it to use a markup utility function - see #1986116: Improve performance by replacing very small and simple templates and theme function with "Markup Utility Functions" for discussion. So for now, maybe we don't need the other issue created, since it's a trivial patch to create.
Comment #64
shanethehat CreditAttribution: shanethehat commentedNew patch, only replaces the template and modifies the documentation. Leaves theme function alone.
I've removed the paragraph about the preprocess not usually running because it is run when the theme function is used.
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedtrailing spaces here.
+ * - field: The field handler object for the current field..
Double period at the end of this sentence.
+ * When fetching output from the row, this construct should be used:
Unnecessary comma.
These comments can be "reflowed" to use up the full 80 characters available.
Comment #66
Kevin Morse CreditAttribution: Kevin Morse commentedThis should address those issues. I also fixed a few other of the worst offenders in views.theme.inc with regards to comments going past 80 characters.
Comment #67
thedavidmeister CreditAttribution: thedavidmeister commentedI appreciate the good intentions but we really have to keep these views conversion patches from creeping in scope. There's a separate issue for every one of those preprocess functions that you modified the docs for at #1757550: [Meta] Convert core theme functions to Twig templates.
Could we get a re-roll with just the docs updates for views-view-field?
Comment #68
star-szrTagging for profiling.
Comment #69
shanethehat CreditAttribution: shanethehat commentedFixes the docs issues from #65, and also puts the template in the templates directory!
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedComment #71
joelpittetgrabbing for profiling
Comment #72
joelpittetScenario
View table with 50 rows and 5 fields = 250 fields rendered. A views-view-field.tpl.php in 8.x and views-view-field.html.twig both in Stark on their respective branches.
View block full content node in the content region.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519bd405e5fe3&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519bd405e5fe3&...
Comment #73
joelpittetAdded the warning back in for that the template is not being used and is for reference as the theme_ function is used for performance.
No changes from performance teston #72
Also, reverted $variables to $vars because that is being done in. #1963942: Change all instances of $vars to $variables
Comment #74
star-szrStill RTBC, ready for review. Thanks @joelpittet!
Comment #76
Fabianx CreditAttribution: Fabianx commented#73: 1843746-73-views-view-field.patch queued for re-testing.
Comment #77
star-szrComment #78
joelpittetComment #79
alexpottWhy are we using the spaceless functionality here? If we need it then we to doc why...
Comment #80
joelpittetOk I replied but I guess the internet was too slow or something...
@alexpott I put that in because the twig template didn't remove the last newline character and failed whitespace related tests.
I wanted to get the markup as close as possible. So the real purpose in this case is to remove the ending \n character.
I'm sure there could be another way to do this, either leave the newline and change the whitespace tests or possibly something better but spaceless does the trick.
Comment #81
tlattimore CreditAttribution: tlattimore commentedAfter running some tests locally I am not sure we really need spacelacess here at all to get it to pass the tests. Here is a patch that removes
spaceless
stuff from this instance - we'll see if the testbot approves. Figures crossed!Comment #82
tlattimore CreditAttribution: tlattimore commentedOops. Tagging for review.
Comment #84
tlattimore CreditAttribution: tlattimore commented#81: 1843746-views-view-field-81.patch queued for re-testing.
Comment #86
tlattimore CreditAttribution: tlattimore commented#81: 1843746-views-view-field-81.patch queued for re-testing.
Comment #87
tlattimore CreditAttribution: tlattimore commentedThats nice! Passed tests without
spaceless
. Anything else here you're concerned about, @Alexpott? RTBC?Comment #88
star-szrGreat, thanks @tlattimore and @joelpittet! Back to @alexpott.
Comment #89
joelpittetNice @tlattimore. It seems that the twig templates are behaving like the docs indicated and remove the last newline as does the PHP template.
http://twig.sensiolabs.org/doc/templates.html#whitespace-control
Comment #90
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #91
alexpottCommitted c68896f and pushed to 8.x. Thanks!
Comment #92.0
(not verified) CreditAttribution: commentedUpdated issue summary.