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.
Task
Convert PHPTemplate templates to Twig templates.
Remaining
Template path | Conversion status |
---|---|
core/themes/bartik/templates/comment-wrapper.tpl.php | We're deleting this, only the @file differs |
core/themes/bartik/templates/comment.tpl.php | converted |
core/themes/bartik/templates/maintenance-page.tpl.php | converted |
core/themes/bartik/templates/node.tpl.php | converted |
core/themes/bartik/templates/page.tpl.php | Will be converted in #1961870: Convert page.tpl.php to Twig due to theme() calls in page.tpl.php |
To test this code:
@todo
Related
#1938864: [meta] Update all core themes to use Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1939286: The theme that Drupal core runs tests in on d.o. (Bartik) should not be allowed to override core templates
#1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme
Comment | File | Size | Author |
---|---|---|---|
#73 | 1938840-73.patch | 29.94 KB | star-szr |
#73 | interdiff.txt | 11.2 KB | star-szr |
#71 | 1938840-bartik-71.patch | 28.58 KB | tlattimore |
#71 | interdiff.txt | 1.73 KB | tlattimore |
#55 | interdiff.txt | 1.25 KB | shanethehat |
Comments
Comment #0.0
jenlamptontodo the test section
Comment #0.1
jenlamptonupdate blocked issues
Comment #0.2
jenlamptonspace
Comment #1
chrisjlee CreditAttribution: chrisjlee commentedA start
Comment #2
chrisjlee CreditAttribution: chrisjlee commentedThis interdiff between templates might help
Comment #3
star-szrThanks @chrisjlee! Let's run automated testing on this patch to see how that goes.
Comment #4.0
(not verified) CreditAttribution: commentedinfo
Comment #5
star-szrIs this issue still blocking node and comment now that #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion is in?
Comment #6
chrisjlee CreditAttribution: chrisjlee commented@Cottser Probably i can't get the twig template to render.
Comment #7
chrisjlee CreditAttribution: chrisjlee commentedadded engine variable to yml file. maybe it'll make a difference?
Comment #9
star-szr@chrisjlee, thanks for working on this. Slight change of plan, please see #1938864-2: [meta] Update all core themes to use Twig. If you can test out the "override a theme function with a template" theory that would be a huge help :)
Comment #9.0
star-szrAdd core themes conversion meta
Comment #9.1
star-szrAdd conversion summary table, remove blocking issues
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedSooo, I opened this ticket #1939286: The theme that Drupal core runs tests in on d.o. (Bartik) should not be allowed to override core templates. Seems relevant here.
Comment #10.0
thedavidmeister CreditAttribution: thedavidmeister commentedRemove boilerplate from remaining tasks
Comment #11
star-szrWell I've solved why the page template is not getting picked up, it's named page.tpl.twig.html, should be page.html.twig.
Going to test splitting up the templates between #1898054: comment.module - Convert PHPTemplate templates to Twig and this issue and see if that will work. Removing all Bartik templates from comment.module issue, and only adding Bartik's overrides here.
Comment #12
star-szrChanges:
Comment #12.0
star-szradded related issue
Comment #12.1
star-szrUpdate conversion table
Comment #13
star-szrActually, since I'm proposing we just remove comment-wrapper.tpl.php because it only changes the @file docblock, let's try this.
Comment #15
star-szr#13: 1938840-13.patch queued for re-testing.
Comment #16
star-szrLooked like a random test failure. Back to CNW regardless, because conversion is not complete.
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commented@Cottser, looks like progress is good but since you closed the issue I opened in #10 with a link to #1595028: Convert tests using Standard profile to use Testing profile instead that means we possibly need tests here that explicitly enable Bartik and look for the Bartik-specific markup overrides.
What do you think?
Comment #18
star-szr@thedavidmeister - It's definitely an interesting thought, but I don't think we need tests added in this issue.
As far as I know, most tests in core (using the default testing profile) just use Stark and the default templates and theme output. Drupal\color\Tests\ColorTest has a bit of code that would allow for testing the same behaviour with multiple themes. I think if we wanted to do this on a larger scale, it would need a new issue and probably a DRY-er approach than Drupal\color\Tests\ColorTest uses. And it would probably make testbot runs take quite a bit longer :)
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commented#18 - If we can't verify their markup isn't bung, can we really offer extra themes with templates that override core out-of-the-box in core?
Comment #20
star-szr@thedavidmeister - If history is any indication, yes. I'd ask that we please take this discussion to a new issue :) thanks!
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commented#20 - sure thing.
Comment #22
star-szrUnassigning. A good starting point for moving this forward would be updating the comment.html.twig added here to match the markup in Bartik's comment.tpl.php. See the screenshots in #1898054-37: comment.module - Convert PHPTemplate templates to Twig.
Comment #23
star-szrTagging.
Comment #24
duellj CreditAttribution: duellj commentedGiving this a go
Comment #25
duellj CreditAttribution: duellj commentedFirst pass, restores bartik comment styles. Still working on what can be done here and what has to be done in the base template conversions.
Comment #26
micheas CreditAttribution: micheas commentedThe patch looks good to me, but I would want a second set of eyes to look at it.
"Prepares variables for block admin display form templates."
Could probably be rephrased as "Prepares variables for display in block admin form templates."
But other than that I don't see anything.
Comment #27
star-szr@micheas - thanks for taking a look. The standards for template preprocess functions can currently be found here:
#1913208: [policy] Standardize template preprocess function documentation
This is only the start of this conversion, there is still much to do.
Comment #28
star-szrComment #28.0
star-szrUpdate page.tpl.php in table
Comment #29
star-szrWe'll convert Bartik's page.tpl.php in #1961870: Convert page.tpl.php to Twig because of the theme() calls in the template. Replacing only the base page.tpl.php breaks things badly.
Comment #30
star-szrWhen we convert the node template here, we should take #1968322: Remove unused $id and $zebra variables from templates into account.
Comment #31
duellj CreditAttribution: duellj commentedFinished the conversions of node.tpl.php and maintenance-page.tpl.php. Note that I had to move some code out of node.tpl.php into bartik_preprocess_node() that did some links manipulation in order to properly show the links wrapper. If there's a better way to do this (i.e. directly in the twig template) let me know.
Also, I think bartik_menu_tree() and bartik_field__taxonomy_term_reference() need to be converted in their respective issues. All bartik_menu_tree() is doing is adding a class to the wrapper, so if that gets converted to a Attribute then we just need to do a preprocess. I'll update the corresponding tickets.
Comment #31.0
duellj CreditAttribution: duellj commentedUpdate conversion summary table
Comment #31.1
duellj CreditAttribution: duellj commentedMoved theme conversions to respective issues
Comment #32
jenlamptonThis patch won't apply anymore, needs a reroll.
Comment #33
chrisjlee CreditAttribution: chrisjlee commentedComment #34
duellj CreditAttribution: duellj commented#31: 1938840-31-bartik-twig.patch queued for re-testing.
Comment #36
duellj CreditAttribution: duellj commentedDoesn't apply indeed :). Here's a new patch that should apply
Comment #37
star-szrThanks @duellj!
Comment #39
star-szr#36: 1938840-36-bartik-twig.patch queued for re-testing.
Comment #39.0
star-szrAdded converted status to converted theme templates
Comment #40
c4rl CreditAttribution: c4rl commentedI added #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme to the summary as related -- to be addressed post-RTBC.
Comment #40.0
c4rl CreditAttribution: c4rl commentedAdded related issue for Bartik
Comment #41
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 #1987422: bartik.theme - Convert theme_ functions to Twig for theme_ function conversion.
Comment #41.0
c4rl CreditAttribution: c4rl commentedUpdate remaining
Comment #41.1
c4rl CreditAttribution: c4rl commentedRevise summary
Comment #42
star-szrBack to CNR since this only contains template conversions. Just closed #1987422: bartik.theme - Convert theme_ functions to Twig because the bartik theme_ function conversions are handled elsewhere.
Comment #43
chrisjlee CreditAttribution: chrisjlee commentedAttempting changes for #1757550: [Meta] Convert core theme functions to Twig templates: Removing the preprocess hook in the theme; Preprocess function seperated and is in the interdiff.txt.
Comment #44
chrisjlee CreditAttribution: chrisjlee commentedWhoops an extra empty newline somehow snuck in this patch from #36. Removed this extra line too.
Comment #46
star-szr@chrisjlee - Preprocess functions are fine here and in the other .tpl.php conversion issues. We just don't want these issues removing theme_ functions.
Patch to review is #36, letting @steveoliver have a go at this one.
Comment #47
star-szrTiny bit of trailing whitespace in node.html.twig here towards the end of the docblock can be removed per http://drupal.org/coding-standards#indenting.
Comment #48
idflood CreditAttribution: idflood commentedHere is a quick reroll based on patch in #36 with the empty line removed.
Comment #49
star-szrI'm planning on profiling these templates by copying them to the Stark theme.
Comment #50
shanethehat CreditAttribution: shanethehat commentedI'll do the manual testing
Comment #51
shanethehat CreditAttribution: shanethehat commentedManual testing results:
The node/comment comparison seems to be missing some comments:
Maintenance page markup looks almost identical. The only difference is that in the new version the closing section tag is on a new line.
Before:
After:
Comment #52
shanethehat CreditAttribution: shanethehat commentedAh, nevermind the comments issue. In the twig template they are twig comments, which seems fair enough. Just want to fix
Comment #53
shanethehat CreditAttribution: shanethehat commentedThis removes the specific adding of a content class that was causing that extra space. The class is added to $content_attributes in #1898054: comment.module - Convert PHPTemplate templates to Twig.
Comment #54
star-szrHmm, that's the first I've seen Twig comments being used like that. For now can we please make those HTML comments as before? I think changing them would be another issue. Thanks @shanethehat!
Comment #55
shanethehat CreditAttribution: shanethehat commentedComments reverted
Comment #57
star-szr#55: twig-bartik-1938840-55.patch queued for re-testing.
Comment #58
intergalactic overlords CreditAttribution: intergalactic overlords commentedI have created a piece of content with comments on both my local sites (patch-site and clean site) and I have verified that the html-output is the same, except for the afforementioned "content"-class on the comments.
Also verified the maintenance-page output.
Comment #59
intergalactic overlords CreditAttribution: intergalactic overlords commentedHas been manually tested. Still needs profiling I guess.
Comment #60
cosmicdreams CreditAttribution: cosmicdreams commentedLooks great to me, recommend RTBC
Comment #61
cosmicdreams CreditAttribution: cosmicdreams commentedadding back the tags
Comment #62
star-szrUnassigning so this can be profiled.
I mentioned in #49 that I was thinking of profiling these by copying the templates to Stark but that won't really work because then we lose the preprocess functions. So instead I recommend copying node.html.twig to core/themes/bartik/templates (on both branches) for profiling. And make sure you have a node displayed always.
See #1938848-57: seven.theme - Convert PHPTemplate templates to Twig for some extra steps needed for profiling the maintenance-page template.
Comment #63
jerdavisProfiling
Comment #64
jerdavisProfiling this my results varied from wt: 1.7% to 2.5%. I selected the 2.3% run for upload.
This profiles a node page with 100 comments.
Scenario:
* Applied patch in this thread
* Applied patch for page.tpl.php from: #1961870
* Copied node.html.twig into Bartik theme for both 8.x and my twig branch
* Generate a node with 100 comments
* Point find-min-web.sh at a node display with comments shown
* Place node block in sidebar
Maintenance page Scenario
* Place site in maintenance mode
Comment #65
jerdavisComment #66
thedavidmeister CreditAttribution: thedavidmeister commentedComment #67
star-szrPerformance results have been reviewed by @Fabianx, looks good.
The first one is the same as #1898054: comment.module - Convert PHPTemplate templates to Twig and that has been deemed acceptable for now.
The second scenario is showing loading all of Twig which is about 2ms and is fine, we already passed that profiling gate back at BADcamp time :)
Comment #68
alexpottDo we need this function and I can't understand the comment...
Maybe the text should be
Remove the "Add new comment" link on teasers or when the comment form is display on the page.
Comment #69
alexpottComment #70
tlattimore CreditAttribution: tlattimore commentedI will work on this re-roll.
Comment #71
tlattimore CreditAttribution: tlattimore commentedThe reason we have a added a preprocess to this patch is to cleanup the
node.html.twig
file and move some logic that was withinnode.tpl.php
into preprocess, rather than the template. This is good because it makes our template much cleaner - but the patch from #55 is doing some unnecessary effort inbartik_preprocess_node()
. The passing of$links
todrupal_render()
was not necessary and we did not need to create a new variable for this all. We can just referencecontent.links
within our node template.Here is a patch that removes the unnecessary work within bartik_preprocess_node() and adjusts the comment as requested in #68 by @alexpott.
Comment #72
tlattimore CreditAttribution: tlattimore commentedTagging for the bot.
Comment #73
star-szr@tlattimore - thanks, that's much better and should be faster too!
After giving this a review I found that the docs are actually a bit off here - they have been much improved in other issues.
This should remedy the docs by just using the guts of the docblocks from #1898432: node.module - Convert PHPTemplate templates to Twig and #1898054: comment.module - Convert PHPTemplate templates to Twig, and changing one instance of page.tpl.php to page.html.twig.
Also tweaked spacing for the t filter according to http://drupal.org/node/1823416#filters.
Comment #74
Fabianx CreditAttribution: Fabianx commentedRe-reviewed this patch:
Results look good!
Interdiffs are sane => Back to RTBC
Comment #75
alexpottCommitted 8dc1fab and pushed to 8.x. Thanks!
Comment #76.0
(not verified) CreditAttribution: commentedFormatting