Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #1898432 by WebDevDude, steveoliver, vlad.dancer, jenlampton, pixelmord, Fabianx, iztok, EVIIILJ, jwilson3, c4rl, Cottser: Convert node module to Twig.
Task
Convert PHPTemplate templates to Twig
Remaining
- Patch needs review
- Manual testing (see below).
Template path | Conversion status |
---|---|
core/modules/node/templates/node-edit-form.tpl.php | converted |
core/modules/node/templates/node.tpl.php | converted |
To test this code
- Create a node to see if node-edit-form.html.twig is working
- Save your node to see if node.html.twig is working
Depends on
#1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Follow-ups
#54898: Add a description-list.html.twig template (ex. definition list)
#1939136: Combine node-recent-block and node-recent-content templates.
#1939238: Remove $submitted variable and print string directly in template instead
#1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead
Comment | File | Size | Author |
---|---|---|---|
#122 | 1898432-122.patch | 21.45 KB | star-szr |
#122 | interdiff.txt | 1.86 KB | star-szr |
#119 | 8.x-1898432-119.patch | 23.31 KB | steveoliver |
#119 | 8.x-1898432-109-119--interdiff.txt | 1.3 KB | steveoliver |
#109 | interdiff.txt | 885 bytes | Hydra |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
c4rl CreditAttribution: c4rl commentedAssigning
Comment #3
c4rl CreditAttribution: c4rl commentedComment #5
c4rl CreditAttribution: c4rl commentedFixed whitespace modifier
Comment #6
star-szrThanks @c4rl!
Rerolled to address some minor coding style and documentation issues and revert some of the changes in the variables documentation for node.html.twig. In particular, the descriptions for "Node status variables" are IMO much improved in node.html.twig vs. node.tpl.php, and it looks like #5 would actually revert those improvements. Also removed an extra 'created' variable from "Other variables".
I'm curious about the use of "Setup template variables for…" vs. "Preprocess variables for…", is there a distinction? If not, I think we should stick with Preprocess.
Can we also get a list of contributors for the commit message, similar to #1898478: menu.inc - Convert theme_ functions to Twig?
Comment #7
c4rl CreditAttribution: c4rl commentedAgreed.
Aside from myself and Cottser, from what git blame tells me, they would include: WebDevDude steveoliver vlad.dancer jenlampton pixelmord Fabianx iztok EVIIILJ jrwilson3
Comment #8
star-szrOkay, CNW for #7, I'll roll a new patch tonight to update those.
Comment #9
star-szrUpdated the issue summary to include a commit message as well, thanks for that c4rl!
Comment #10
keyhitman CreditAttribution: keyhitman commentedComment #11
c4rl CreditAttribution: c4rl commented@keyhitman Tests have passed, can you provide some insight as to why you are assigning? Thanks
Comment #12
keyhitman CreditAttribution: keyhitman commentedPatch does not apply to latest head needs a re-roll. Do you mind if we have a go at the re-roll?
Comment #13
keyhitman CreditAttribution: keyhitman commentedThis looks too complicated for a first-timer like me :)
Comment #14
star-szr#9: 1898432-9.patch queued for re-testing.
Comment #15
star-szrPatch still applies.
Comment #16
Fabianx CreditAttribution: Fabianx commentedI think we agreed that t('') should be done in templates as filter.
If that is no in our coding standards, lets add it.
According to newest developments, lets use a render array with
#theme => 'table' instead and not use theme() in preprocess().
Please lets add that to the docs. It is vital to render as late as possible.
The same is true for the form, though I do not know if render() will work the same as drupal_render_children or if we need a
#theme => children
Thanks!
Again use:
#theme => table
and maybe use array() instead of NULL.
The same is true here for using a render_array.
And for all future uses of theme().
t() should be in templates.
No need to render() this here. This is just wasting CPU.
Yu, here we would have rendered it needlessly.
Yay! render arrays FTW here.
Totally no:
{% is correct
BECAUSE
{{ == print and hide() does not print something.
Comment #17
steveoliver CreditAttribution: steveoliver commentedExcellent catches, Fabianx.
Comment #18
star-szrI'll take a look at the changes in #16 and creating some documentation around those.
Comment #19
star-szrThanks for the notes @Fabianx, they were really helpful. This should address most of #16, the only part I didn't make changes for was this:
The form in question has an empty child array so I didn't feel confident changing that without being able to test it.
Documentation on "do's and don'ts" to come, but I wanted to get the revised patch up tonight.
I also removed the spaces before and after the t() filter, per the posted Twig coding standards at http://drupal.org/node/1823416.
Comment #19.0
star-szrAdding commit message
Comment #20
Fabianx CreditAttribution: Fabianx commentedExcellent work.
I found why drupal_render_children is used within theme() functions: http://api.drupal.org/comment/45288#comment-45288
We should open up a follow-up as the drupal_render_children should be implicit as well.
Besides that: Very close to RTBC from my side.
Comment #21
star-szrThanks @Fabianx. Created a very basic issue here for drupal_render_children(): #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead.
Comment #22
star-szrThis just fixes the alignment of the docblock in node-search-admin.html.twig.
Comment #24
star-szr#22: 1898432-22.patch queued for re-testing.
Comment #25
Angry Dan CreditAttribution: Angry Dan commentedWe've had a look and there are a couple of questions/comments about this patch:
template_preprocess_node_admin_overview()
andtemplate_preprocess_node_recent_content()
. I thought this was a violation of coding standards?$node
intemplate_preprocess_node_preview()
for$node_teaser
. Was this a conscious decision or a separate bug being fixed because it doesn't seem related to the move to twig?{{ title_attributes }}
, why was this removed?Comment #26
star-szr@Angry Dan - For now I can only comment on your first point, see #1913208: [policy] Standardize template preprocess function documentation which will shorten those significantly. Great catch!
Comment #27
Fabianx CreditAttribution: Fabianx commented#25 Great review catches!
I think both are problems that accidentally slipped into this patch. Probably coming from the sandbox.
Comment #28
star-szrCNW based on #25.
Comment #29
star-szrThis should address all points from #25.
Revised docblocks on preprocess functions per pending standards at #1913208: [policy] Standardize template preprocess function documentation. Feedback welcome, I know the wording on some of them can definitely be improved!
Comment #29.0
star-szrCorrecting one of the names in the commit message.
Comment #29.1
jenlamptonadd link to dt issue
Comment #29.2
jenlamptonblockers
Comment #29.3
jenlamptonblocker
Comment #30
jenlamptonThis patch is actually looking really good. If we can clean up the two blocking issues noted below (and in summary) we should be good to go.
1) Seven theme overrides node_add_list so this patch may not be able to be added until after we figure out how to allow both theme engines to run at the same time in themes. See #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion
2) the infinite loop problem, as indicated by @todos in the comments below. see #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead
That said, a few other small changes:
node-add-list.html.twig
- variables in docblock updated
node-edit-form.html.twig
- missing, converted from .tpl.php
- @todo in here
template_preprocess_node_recent_block
- contained a call to theme(), replaced with a render array.
node-search-admin.html.twig
- not printing out
info
as added into the form (bug in preprocess, var missing from template)- @todo in here
node.module
- bug in node_search_admin(), render array key should be #markup not #value.
node.html.twig
- lots of docblock cleanup
Reroll includes changes noted above.
Also needed:
seven/templates/node-add-list.html.twig
- @todo
Comment #31
star-szrThanks @jenlampton, great work!
Rerolled to tweak some docs and coding standards things and remove a stray debug() and a stray .orig file :)
Left the .orig out of the interdiff.
Comment #33
jenlamptonThis test is failing because there is a space in our class tag. I'm not sure our tests should be that sensitive! Shouldn't they look for a div with A class 'content' not THE div with this *exact* class string?
This new patch allows the test to pass, but I wonder if our testing framework shouldn't be adjusted?
Or, our twig process can be adjusted... see #1939214: Provide a Twig filter for cleaning up whitespace around classes.
While I was in here a few more changes:
- removed stuff that's in $node from preprocess, since we can get there via Twig now. Updated docs to reflect this too.
Also, created a follow-up: #1939238: Remove $submitted variable and print string directly in template instead
Comment #33.0
jenlamptontest
Comment #34
jenlamptonthis one should pass tests. (interdiff replaces last one)
Comment #34.0
jenlamptonmore follow up
Comment #36
star-szrLooks like we won't be able to rename the "name" variable yet (or convert to a render array), at least until #1941286: Remove the process layer (rdf module) is figured out.
So reverting that change. Thanks for keeping this moving, @jenlampton!
Comment #37
star-szrRerolled so that this patch doesn't try to make the same change already committed in #1945066: node_theme() doesn't declare the file for theme_node_admin_overview() . No other changes.
Comment #39
star-szr#37: 1898432-37.patch queued for re-testing.
Comment #40
steveoliver CreditAttribution: steveoliver commented+1 for RTBC if this passes, which it should (just code style and little nitpicks).
Comment #41
star-szrThanks @steveoliver! All these changes look good, except:
This summary line is now too long per http://drupal.org/node/1354#drupal.
Comment #42
star-szrHowever I think that theme_node_search_admin() will get converted in #1938920: Convert node_search_admin theme tables to table #type anyway, so we can probably remove that
and maybe node_recent_blockfrom this conversion.Comment #43
star-szrNeeds work for #41 and #42. Will roll a new patch in the next few days unless someone else wants to :)
Comment #44
chrisjlee CreditAttribution: chrisjlee commentedNot sure if that sentence even makes sense:
Wouldn't it be more clearer to understand if they were two phrases?
Comment #45
star-szr@chrisjlee - Yup, that's the direction I've been going. The second sentence would be on its own line with a blank line underneath the summary, something like this:
But as mentioned in #42 (and swiftly forgotten when posting #43 :)), the theme_node_search_admin() conversion can be removed from this patch since it will be handled in #1938920: Convert node_search_admin theme tables to table #type.
So this is just CNW for removing that conversion from the patch.
Comment #46
star-szrRemoved the theme_node_search_admin() conversion which will be handled in #1938920: Convert node_search_admin theme tables to table #type.
And did another pass at the documentation, shortening summary lines to under 80 characters and improving wording.
Comment #47
joelpittetNice work @Cottser and @steveoliver, it ain't easy being green:)
Is this needed?
Comment #48
star-szrThe test change? See #33. If we can rework the test to be less sensitive that'd be even better.
Comment #48.0
star-szrdepends
Comment #49
star-szrMore documentation tweaks, and left one documentation change off for #1898034: block.module - Convert PHPTemplate templates to Twig to take care of.
Comment #50
star-szrSome node.tpl.php -> node.html.twig replacements. I left two alone:
The node.tpl.php in drupal_pre_render_links() because that has a PHPTemplate example and that will need to be part of a bigger effort to update PHPTemplate documentation in the code to Twig documentation.
The node.tpl.php string in theme_field() is already removed in #1898062: field.module - Convert PHPTemplate templates to Twig.
Comment #51
star-szrI talked to @Fabianx this morning, and he had a great idea.
I'm going to try one more thing for node-edit-form to see if we can get hide() working in the Twig file - basically a cleaned workaround for #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead, currently we are doing this:
Comment #52
star-szrWaiting on results from #1920886-10: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead before posting another patch here. Things are looking good though :)
Comment #52.0
star-szrMove drupal_render_children() issue to follow-ups. I don't think we have to solve it before committing this.
Comment #53
star-szrWill post a patch tonight that puts the logic from #1920886-10: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead into template_preprocess_node_edit_form() only.
Comment #54
star-szrAnd here it is.
Without removing the '#theme' key, WSOD when viewing node add/edit form. Without removing the '#theme_wrappers' key, two
<form>
tags.I think this workaround will do for conversion purposes until we have a better solution.
Comment #54.0
star-szrAdd conversion summary table
Comment #54.1
star-szrUpdate remaining
Comment #54.2
star-szrRemove node-search-admin from testing steps
Comment #55
star-szrThis could use manual testing, steps for testing are up in the summary.
Comment #56
star-szrAssigning to @steveoliver for review.
Comment #57
carsonblack CreditAttribution: carsonblack commentedPatch applies cleanly and I have confirmed that all HTML is output as expected.
In the case of node-add-list.html.twig it is necessary to switch admin theme to Stark inorder to see the output as Seven theme overrides this output in it's seven_node_add_list() function.
Other than that little gotcha in the testing process, this is is all good!
RTBC +1
Comment #58
steveoliver CreditAttribution: steveoliver commentedManually tested this, it works, but maybe not totally as expected (which has nothing to do with conversion, actually. It's just something I noticed in the process). Namely, one issue:
The node-edit-form template is not used by default (as you'll notice when testing node/1/edit in Stark, for example). Only forms that specify 'node_edit_form' as the #theme callback will use this theme callback. Otherwise theme_form() is used. (See node_page_edit and seven.theme).
At the moment, Seven is the only theme that implements the 'node_edit_form' theme callback. Realizing this, I wonder now if it shouldn't be renamed and/or moved to clarify what it is doing. Maybe 'node_edit_form_twocol' or something? Maybe not. I don't really care. In any case I clarified the docs in that template.
The other things I noticed were primarily docs related. Patch and diff show what I found.
Lastly, I changed up the #theme / #theme_wrappers 'hack' to prevent looping over every $form element, just targeting those two keys directly. (Related to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead).
If there are no objections, +1 to RTBC. I'll RTBC it if nobody else wants to.
Comment #59
star-szrOh duh, your hack is much better, thanks @steveoliver :)
I think this should be reformatted so that we say 'advanced' instead of 'form.advanced', but I think we should keep the documentation about hide(). Not many templates use hide() and it probably deserves to be explained in the docs for each one.
Comment #60
thedavidmeister CreditAttribution: thedavidmeister commented+ <div class="content {{ content_attributes.class }}"{{ content_attributes }}>
I'm not a fan of the mixing hardcoded classes and class attributes that's going on in some of the Twig conversion patches. It splits class management across the preprocess and templates which is harder to work with than both approaches are individually and very often leads to trailing whitespace in class attributes.
I noticed you had to hack a test to get this to come back green...
I'm not saying don't RTBC, I'm just saying we should come up with a more elegant way to handle classes like this sometime down the track.
Comment #61
star-szrThis will need a simple reroll to account for #1968322: Remove unused $id and $zebra variables from templates.
Comment #62
steveoliver CreditAttribution: steveoliver commentedUpdate: will be reviewing this Thursday, incase anyone else wants to reroll/RTBC in the meantime.
Comment #63
chrisjlee CreditAttribution: chrisjlee commentedAttempted to reroll.
There was two merge conflicts one was removing the node.tpl.php file and the node.html.twig documentation conflict:
node.html.twig
I'm guessing that with recent commit of #1968322: Remove unused $id and $zebra variables from templates we'll probably want to go with the head. Anyways patch attached.
Comment #64
chrisjlee CreditAttribution: chrisjlee commentedAfter a discussion with @Cottser, looks like with moving to HEAD we nuked and lose some documentation that needs to be restored. I agree with that.
So the following patch contains some docs edits that restore the previous variables missing. Also, there are some small nitpick tweak to the docs.
Comment #65
cosmicdreams CreditAttribution: cosmicdreams commentedI'll test this locally but when I test via Simplytest.me I get multiple ajax errors
Comment #66
cosmicdreams CreditAttribution: cosmicdreams commentedWhen manually testing it appears that the names of the content types are missing. Going to see if I can revert this patch and have that still work.
when creating nodes all side menu stuff is listed below the body field (instead of being listed on the side)
When previewing the node I get some errors saying:
saving works
I don't seem to have the option to add a recent content block.
Comment #67
steveoliver CreditAttribution: steveoliver commentedThanks, guys.
re: #65: this is an unrelated bug; see #1969680: Installation fails with MyISAM - key too long.
re: #63 and #64:
1. I think the node variables should be ID'ed first. This should make the attributes+class and other variables' descriptions more concise and easier to understand.
2. Let's lose "Other variables:"
3. I'd like to see all these node+properties descriptions given a little attention while we're here. Maybe complete sentences? For example, I don't know what value I'll get from 'comment' with a description like "State of comment settings for the node." Just a little love on docs here will go a long way.
typo, looks like a mistake.
Re: #66, I can't reproduce it locally on a clean install. Maybe a local cache issue? Can you reproduce?
Comment #68
steveoliver CreditAttribution: steveoliver commentedHere's my complete pass on the whole patch from #64 (not just the interdiff, as was the last comment):
1. we're introducing template_preprocess_node() and we don't need to. node edit forms don't use any special form or theme callback by default. Seven is the only theme in core that implements this theme callback, and I strongly believe it should implement this theme callback, template and associated preprocess (which is a POS here anyways). Stark and other themes see straight entity forms ala #1499596: Introduce a basic entity form controller, I believe.
2. node.html.twig, however....
2.a
Why are we losing this?
2.b. To follow up on my last comment, let's give these docs some love. They are already really good in some spots, but lacking in organization and clarity. In addition to the ordering suggested above, I'd like to have a more intelligent distinction or consolidation of the content and node variables. I think that'd be a big ++ here.
Comment #69
star-szrI agree about template_preprocess_node_edit_form(), that's my fault for not realizing only Seven uses that template.
Regarding 2.a from above - "Why are we losing this?"
I did not make this change to the preprocess function but I can give you my understanding of it and why I think it's a good change. Now that accessing those properties directly is
{{ node.type }}
rather than<?php print $node->type; ?>
it seems like unnecessary preprocessing to me to create a duplicate 'type' variable available to the Twig template when it can be accessed at 'node.type'.(and after writing that, scrolled up and found #33 which backs this up - "- removed stuff that's in $node from preprocess, since we can get there via Twig now. Updated docs to reflect this too.")
Comment #70
star-szrUnassigning so someone can work on a revised patch, I probably won't get to this over the next few days myself.
Comment #71
shanethehat CreditAttribution: shanethehat commentedI'll have a go
Comment #72
shanethehat CreditAttribution: shanethehat commentedI think I covered all the points in #67 and #68, although I wasn't sure about the order of the documentation in node.html.twig. Moved the preprocess for the edit form into seven.theme until it can be removed completely by #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead.
Comment #73
cosmicdreams CreditAttribution: cosmicdreams commentedsimplytest.me seems to be having some problems with Drupal 8 so I'll test this locally.
Comment #74
cosmicdreams CreditAttribution: cosmicdreams commentedTesting this now and here's the results
To test this code
I'm not sure what the recent content block is supposed to look at but it renders so it's not a show stopper. It just looks bare bones right now.
Comment #75
thedavidmeister CreditAttribution: thedavidmeister commented#74 - when you say "working", did you A/B compare all the markup with the patch applied to a Drupal install without the patch?
Comment #76
shanethehat CreditAttribution: shanethehat commentedThis patch addresses a couple of issues steveoliver raised in IRC yesterday. It does not fix #74.
Comment #77
star-szrThanks @shanethehat and @cosmicdreams! It sounds like this needs more manual testing, re-tagging.
If it's feasible to move the seven code (node-edit-form.html.twig and the preprocess in seven.theme) to #1938848: seven.theme - Convert PHPTemplate templates to Twig can we please do so? If that works out we can have testers use Stark for testing functionality and markup.
Comment #78
shanethehat CreditAttribution: shanethehat commented#77 my last patch moves the edit form template, and the previous one moved the preprocess.I misread. The relocation of the template and preprocess should be really easy to revert. I would recommend doing that rather than picking up an old patch because of the extra changes that have occurred.
Comment #79
shanethehat CreditAttribution: shanethehat commentednode-edit-form back in node module for later removal
Comment #80
shanethehat CreditAttribution: shanethehat commentedI've followed the steps in the issue description and grabbed markup from a clean install and with #79 applied. I then stripped all newlines and compared the output with Kdiff. Other than indentation related whitespace differences, I found the following:
node/add
Identical
admin/structure/types
Identical
node/add/article
Identical
node/add/article - preview
Teaser and Full views have both have the same differences as node, below. Otherwise identical.
node/x
<article id="node-" class="node node-article promoted view-mode-teaser clearfix"
becomes
<article id="node-" class="clearfix node node-article promoted view-mode-teaser"
<div class="content" class="">
becomes
<div class="content">
Recent content block
Identical
I think that the differences in node are either inconsequential (order of classes) or beneficial (no more duplicate class attribute), so I think this must be very close to RTBC.
Comment #81
steveoliver CreditAttribution: steveoliver commentedVery nice, Shane.
A few little nitpicks here, a few that I think will address the 'inconsequential' concerns in #80.
Is this whitespace addition maybe a typo?
Yeah, this 'path' declaration was unnecessary.
Looking forward to this being moved to Seven.
Why can't this theme call be turned into a render array now?
This is an interesting partial render array. Any way we can make more sense of this?
Is an empty array a sane default value for this? Maybe NULL instead?
It'd be nice to replace this with #theme 'definition_list' ala #54898: Add a description-list.html.twig template (ex. definition list)... ;) But not necessary. This is fine for now.
"Do we need to preview a trimmed teaser version of a post..." Yeah, I think we do. Let's lose this comment?
{{ table }} could become {{- table -}} and {{ more_link }} could become {{- more_link -}} to remove the indentation differences in this patch here.
Excellent cleanup on this docblock here. Very nice.
in the node.html.twig file, let's move this content class to preprocess and just print [edit: code wrapped properly:]
<div{{ content_attributes }}>
[/edit]. I think this would resolve the need for that 'typo' looking whitespace addition above.Comment #82
shanethehat CreditAttribution: shanethehat commentedThanks for the review @steveoliver. I've implemented most of this, and have a couple of responses:
Turning this into a render array has a knock-on effect with the creation of $variables['submitted']. Personally I think that $variables['submitted'] shouldn't exist and this string should be constructed in the template, but I feel that would be too severe a change for a conversion task and should probably have a followup issue. Additionally, until #1941286: Remove the process layer (rdf module) lands, this causes an RDF error.
I think that a comment here is valuable to express the intent of the conditional, but it was not well written. I've rephrased it to be a statement of intent, not a question.
I've just move these inside the row condition. If there are no rows there's no point defining the table or the link, and Twig will handle the missing properties.
Comment #83
star-szrExcellent work @shanethehat and @steveoliver!
Comment #84
Fabianx CreditAttribution: Fabianx commented#82: twig-node-1898432-82.patch queued for re-testing.
Comment #86
shanethehat CreditAttribution: shanethehat commentedreroll
Comment #87
shanethehat CreditAttribution: shanethehat commentedwith a patch attached
Comment #88
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987406: node.module - Convert theme_ functions to Twig for theme_ function conversion.
Comment #88.0
c4rl CreditAttribution: c4rl commentedUpdate remaining
Comment #88.1
c4rl CreditAttribution: c4rl commentedRevised list of conversions
Comment #88.2
c4rl CreditAttribution: c4rl commentedTesting steps
Comment #88.3
c4rl CreditAttribution: c4rl commentedCorrect testing steps
Comment #89
cosmicdreams CreditAttribution: cosmicdreams commentedIs that reason enough to demote to needs work?
I've manually tested the patch in 87 and everything works as required in the OP's test case.
Comment #90
thedavidmeister CreditAttribution: thedavidmeister commented#89 - yes, there are theme functions in that patch but we're splitting template and theme conversions into separate tasks. The patch just needs to be rerolled to have the theme->template conversions in one patch and the template->template conversions in another.
"needs work" is just for organisational reasons, not because the code is bad.
Comment #91
shanethehat CreditAttribution: shanethehat commentedComment #92
shanethehat CreditAttribution: shanethehat commentedJust the existing templates converted. No theme_* changes.
Comment #93
shanethehat CreditAttribution: shanethehat commentedThis time without whitespace errors
Comment #94
shanethehat CreditAttribution: shanethehat commentedMissed a few mentions to node.tpl.php
Comment #95
star-szrTagging for profiling.
Comment #96
star-szrThis is just s/themable/themeable/ in node-edit-form.html.twig.
Comment #97
intergalactic overlords CreditAttribution: intergalactic overlords commentednode-page, node-teaser and node-edit-page output the same html with twig and tpl.php.
Comment #98
intergalactic overlords CreditAttribution: intergalactic overlords commentedHas been tested manually
Comment #99
Hydra CreditAttribution: Hydra commentedDid a manual test, and everything is displayed properly.
Comment #100
star-szrThis entire function can be removed now that #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is in :)
Comment #101
Hydra CreditAttribution: Hydra commentedLet's have a look at this
Comment #102
Hydra CreditAttribution: Hydra commentedOkay here is an updated patch. Manual testing worked, don't forget clearing cache :)
Comment #103
Hydra CreditAttribution: Hydra commentedUpdate status to get the bot running :)
Comment #104
tlattimore CreditAttribution: tlattimore commentedI'll take a stab at reviewing this.
Comment #105
tlattimore CreditAttribution: tlattimore commentedThe patch from #102 looks good to me and addressing cotter's request in #100. After desirable results are confirmed with profiling I think this is ready for RTBC.
Comment #106
tlattimore CreditAttribution: tlattimore commentedI can't do profiling so I am going go un-assign.
Comment #107
steveoliver CreditAttribution: steveoliver commentedprofiling...
Comment #108
star-szrWe should not change this hunk in this patch, it is causing an unnecessary conflict, #1898062: field.module - Convert PHPTemplate templates to Twig already updates this and updates both instances of .tpl.php to .html.twig.
Please remove this hunk from the patch :)
Comment #109
Hydra CreditAttribution: Hydra commentedAwesome hint!
Comment #110
geoffreyr CreditAttribution: geoffreyr commentedWill have a stab at profiling Hydra's patch from #110.
Comment #111
geoffreyr CreditAttribution: geoffreyr commentedScenario: Displaying a stack of node teasers in a view, in both the main content area and right sidebar.
Looks like there's a significant speed saving here. Would probably want someone to check and corroborate this, but so far looks good.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d4cd4e396d&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d4cd4e396d&...
Comment #112
thedavidmeister CreditAttribution: thedavidmeister commentedComment #113
mr.baileysThe above set of profiling data seems to be for the node.twig.html only, not the node-edit-form.twig.html template?
I ran the numbers for node-edit-form (using Seven as theme, since Stark does not use the node-edit-form template):
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d5c4cd1983&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d5c4cd1983&...
Comment #114
alexpottWhat's causing the node edit slow down?
Not sure that swapping the clearfix around makes sense. What's in attributes.class is likely to be more significant.
Comment #115
mr.baileysThis could be caused by the fact that I was forced to benchmark using Seven instead of Stark, since the latter does not make use of the node edit template (rather renders the node edit page as a regular form). This means that our trick to load the Twig engine in both the 8.x and twig branch is not working here, and the comparison is "Twig Engine Loading" & "Conversion", instead of just profiling the conversion.
Not sure how to get around that issue and profile just the template conversion itself...
Comment #116
star-szrThis is just the one-time Twig overhead so it's fine, if we want to measure just the node-edit-form template we can copy node.html.twig to seven/templates and re-profile.
Comment #117
cweagansCNW for second point in #114
Comment #118
steveoliver CreditAttribution: steveoliver commentedSince the profile results aren't clear, someone re-profile this, please.
Two scenarios:
1. Node edit form
2. Node view, with template in stark theme.
Then, a few nitpicks. Easy stuff:
3. I agree with Alex's clearfix comment in #114. Let's leave clearfix at the end.
Then...
4. May as well update the path here to include the new /core/ before /modules/...
5. Let's not lose the indentation here. Please re-indent.
Comment #119
steveoliver CreditAttribution: steveoliver commented3., 4., 5.
Comment #120
Fabianx CreditAttribution: Fabianx commentedThanks @steveoliver.
To explain profiling results from #111 - http://drupal.org/node/1898432#comment-7441576:
* We are faster, because we do a _lot_ less preprocessing now.
And from #113 - http://drupal.org/node/1898432#comment-7441780:
* We are at the usual Twig overhead, besides for the fact that Twig is loaded here completely as can be seen by checking the run_1 output and searching for twig_render_template. So just substract the twig loading overhead of 1.3-1.5ms here and the edit form is fine.
Besides that 1.3 ms on an edit form ... is not a big deal. @alexpott just wanted to understand why it is slower than other templates.
The answer is: It isn't - there is just the Twig loading overhead.
I don't think it is worth re-profiling that as results are consistent with other templates, where default loading overhead was substracted.
I will re-review the patch now and provided it passes testbot this is probably RTBC.
Comment #121
Fabianx CreditAttribution: Fabianx commentedThere is a stray field.html.twig in the last patch. Please remove.
Afterwards this is RTBC. I re-reviewed it all.
Comment #122
star-szrStray field.html.twig gone.
Comment #123
Fabianx CreditAttribution: Fabianx commentedRTBC per #121, provided this passes testbot.
Comment #124
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #125
alexpottCommitted 6b4b127 and pushed to 8.x. Thanks!
Comment #126.0
(not verified) CreditAttribution: commentedClarify summary