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.
Issue #1961870 by Cottser, foopang, kgoel, shanethehat, tlattimore, c4rl, joelpittet, thedavidmeister: Convert page.tpl.php to Twig.
(as of #76)
Task
Use Twig instead of PHPTemplate
Remaining
- Replace all templates with .html.twig equivalent templates
Theme function name/template path | Conversion status |
---|---|
core/modules/system/templates/page.tpl.php | converted |
core/themes/bartik/templates/page.tpl.php | converted here due to theme() calls in page.tpl.php |
core/themes/seven/templates/page.tpl.php | converted here so that breadcrumb can be converted to a render array |
core/modules/block/tests/themes/block_test_theme/page.tpl.php | converted |
Related
#1885560: [meta] Convert everything in theme.inc to Twig
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment | File | Size | Author |
---|---|---|---|
#69 | interdiff.txt | 3.35 KB | shanethehat |
#69 | twig-page-1961870-69.patch | 47.75 KB | shanethehat |
#65 | 1961870-65-twig-page.patch | 45.05 KB | joelpittet |
#59 | 1961870-59.patch | 45.04 KB | star-szr |
#59 | interdiff.txt | 942 bytes | star-szr |
Comments
Comment #1
foopang CreditAttribution: foopang commentedComment #3
foopang CreditAttribution: foopang commented#1: convert-page.tpl_.php-twig-1961870.patch queued for re-testing.
Comment #4
foopang CreditAttribution: foopang commentedRe-rolling the patch.
Comment #6
star-szrThis patch should probably include this conversion as well if possible:
core/modules/block/tests/themes/block_test_theme/page.tpl.php
Comment #7
foopang CreditAttribution: foopang commentedOK, let's try this again.
Comment #9
star-szrThanks @foopang!
I would look at bringing over any changes made to template_preprocess_page() and template_process_page() in the sandbox to move this along further.
Comment #10
star-szrTagging.
Comment #11
foopang CreditAttribution: foopang commentedHi @Cottser, why it failed the SimpleTest?
Comment #12
star-szrI would run the failing tests locally and look at the verbose results to debug this further.
Please come to #drupal-twig on IRC for more discussion :)
Comment #14
star-szrThis one's next on my list.
Comment #15
star-szrWe'll need to convert Bartik's page.tpl.php in this issue because of the theme() calls in the template.
This patch is mostly for testbot:
I haven't gone through the docs yet, just reformatted the @todos in the Twig templates.
Comment #16
star-szrComment #17
star-szrI think the test failures may just be from the missing Bartik conversion, either way I suggest the next step here is adding a Bartik version of page.html.twig here and uploading for testbot.
Tagging as novice, the process here would be comparing core/modules/system/templates/page.tpl.php with core/themes/bartik/templates/page.tpl.php, and creating core/themes/bartik/templates/page.html.twig to match up with the Bartik overrides (add wrapper divs, etc). Start by copying over core/modules/system/templates/page.html.twig from this patch and modify from there.
If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!
Comment #18
kgoel CreditAttribution: kgoel commentedI am going to work on this task.
Comment #19
kgoel CreditAttribution: kgoel commentedAs per Cotter's #17 comment, I have created bartik - page.html.twig.
Comment #21
kgoel CreditAttribution: kgoel commentedFixed white spaces issue.
Comment #23
star-szr@kgoel - thank you, at a glance that looks like a great start! It looks like your patch only contains the bartik template - can you please upload a patch that has the bartik page.html.twig you created combined with the patch from #15? If you're having trouble the folks in #drupal-twig can probably help out :)
Also a bit more trailing whitespace on this line.
Comment #24
kgoel CreditAttribution: kgoel commentedCottser - thanks! I have combined patch from #15 with bartik page.html.twig
Comment #26
star-szrThanks for pushing this along @kgoel - the patch in #24 doesn't have all the files but not to worry, I was able to apply your patch from #21 and will post a combined and updated patch. I found some syntax errors in the template so I am fixing those and otherwise making sure Stark and Bartik markup match up. I was able to debug the syntax errors by just viewing the homepage, the errors told me where the unexpected syntax was and on what line of the Twig file it was found.
This was the most common thing I noticed:
Inside the
{% if %}
tags you can just put any text, you don't need the double curly braces/brackets unless you are printing out a variable. And you always need an{% endif %}
after an{% if %}
. So it should look more like this (also removes 'is defined' per http://drupal.org/node/1823416#expressions):Comment #27
star-szrThe interdiff (from #15 + #21) is pretty huge, so here's a summary:
Twig coding standards:
Other changes:
Comment #28
star-szrTwo small tweaks to the Bartik template after I went through everything again:
<!-- /#sidebar-first -->
tags to be #content.Comment #29
jenlamptonsystem's page.html.twig
Minor docs cleanup:
1) always?
"At the very least" followed by "always"? what does that even mean? How about:
2) extra comma
should be
3) theme settings
twice we refer to 'theme settings' and once 'theme configuration' how about settings everywhere?
4) all the things we output are HTML!
how about just
5) entity?!?
how about (note the comma removed after 'page')
6) we can remove all these
this problem has been solved by renderable everhtings.
Bartik's page.html.twig
7) we need to make the same docs changes as above in this template. It looks like the only significant difference here is the regions?
8) we can remove this twig comment from this template as well
9) two line breaks
we probably only need one
that's all I can find...
Comment #30
star-szrThanks for that @jenlampton :)
Here's a patch that incorporates all your suggestions. I'm removing the manual testing tag as well. I went through quite a bit of testing to make sure the markup matched up and I also tried placing blocks in different regions etc. and everything worked as expected.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commentedDoesn't the bartik bit go over here #1938840: bartik.theme - Convert PHPTemplate templates to Twig?
Comment #32
star-szrBartik's page.tpl is converted here, yes. Unless you're asking something else :)
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commented#32 - I was asking if we should cut and paste it into the other issue?
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentednm - just saw #15
Comment #34.0
thedavidmeister CreditAttribution: thedavidmeister commentedAdd conversion summary table
Comment #35
tlattimore CreditAttribution: tlattimore commentedThis looks good to me! I love the fact the the render arrays for menu's have been moved to theme.inc. This makes our page template so much cleaner.
Anything left here before RTBC?
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedThis actually doesn't appear to be true any more. All my tests for the block module are passing locally after I deleted core/modules/block/tests/themes/block_test_theme/page.html.twig
Could we try a patch with that file deleted? see if the d.o. testbots agree with my local test runs.
Comment #37
tlattimore CreditAttribution: tlattimore commentedI can confirm what @thedavidmeister says regarding the block tests passing without
core/modules/block/tests/themes/block_test_theme/page.html.twig
in place. Here is a re-roll #30 with without this template. We'll see if the testbot has the same results as it does locally.Comment #38
tlattimore CreditAttribution: tlattimore commentedtagging.
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commented+ * - logo: The path to the logo image, as defined in theme settings.
Path or URL? it's being used in a src attribute so it's probably a URL.
I'm happy other than this.
Comment #40
tlattimore CreditAttribution: tlattimore commented@thedavidmeister is correct. The logo variable is the fully qualified url of the logo image. For instance:
http://yourdrupal.site/core/themes/bartik/logo.png
in a default install. Here is a patch that corrects this doc comment.RTBC?
Comment #41
tlattimore CreditAttribution: tlattimore commentedeh, keep forgetting to tag for review.
Comment #42
shanethehat CreditAttribution: shanethehat commentedA few minor documentation tweaks
Comment #43
thedavidmeister CreditAttribution: thedavidmeister commented+ * Preprocess variables for the page template.
Shouldn't this be "Prepares variables for page templates."?
"displing" is not a word.
Comment #44
star-szrGood catches @thedavidmeister, and thanks for keeping this going @tlattimore and @shanethehat :)
One more thing…
We might as well change this to 'Processes' while we're here per http://drupal.org/node/1354#functions.
Comment #45
shanethehat CreditAttribution: shanethehat commentedFixes #43 and #44
Comment #46
shanethehat CreditAttribution: shanethehat commentedComment #47
star-szrRemoving some unnecessary if statements (and the Novice tag since this needs another review):
Comment #48
c4rl CreditAttribution: c4rl commented#47 looks good overall, attached some nits, mostly:
* Documentation clean-up and consistency between bartik and core
* Removal of theme()
* Some whitespace and HTML comment fixes
Assuming these seem good, this is RTBC in my view.
Comment #49
c4rl CreditAttribution: c4rl commentedAlso, I should mention I elected to remove the note about "directory" in the page.html.twig docblock, as it doesn't seem really that useful. I can't think of a compelling reason to leave it in the docblock, and there are other variables present (db_is_active, user, language) that don't necessitate a mention in the docblock.
Comment #50
star-szrChanges look good to me. Thanks @c4rl!
Comment #52
c4rl CreditAttribution: c4rl commentedHaving breadcrumbs as a render array apparently makes that test fail. We can investigate rewriting the test, but if it seems daunting maybe we leave in theme() and provide a follow-up we can tackle later.
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedAlso, if you get that render array working, it needs a trailing comma on the last line.
Comment #54
joelpittetHmm, passes locally when applied with #1938848: seven.theme - Convert PHPTemplate templates to Twig re-testing. If it fails again, I would say maybe consider rolling those changes into this issue.
Comment #55
joelpittet#48: drupal-1961870-48.patch queued for re-testing.
Comment #57
star-szrThat makes perfect sense @joelpittet, thank you! I didn't realize Drupal\system\Tests\Menu\BreadcrumbTest used the standard profile and the seven theme.
So it doesn't like a render array here:
print $breadcrumb;
Moving seven's page.html.twig over here.
Comment #58
thedavidmeister CreditAttribution: thedavidmeister commentedStill missing trailing comma:
Comment #58.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdate conversion status - Bartik's page.tpl.php has been converted
Comment #59
star-szrAdded trailing comma and removed excessive whitespacery from seven's page.html.twig noticed over in #1938848: seven.theme - Convert PHPTemplate templates to Twig.
Comment #59.0
star-szrUpdating conversion table
Comment #60
thedavidmeister CreditAttribution: thedavidmeister commentedWe're missing documentation for every variable used in the Seven Twig template.
Comment #61
star-szrThanks @thedavidmeister.
Novice task: copy and paste the variable documentation from the default page.html.twig and add to Seven's page.html.twig.
Comment #62
no_angel CreditAttribution: no_angel commentedComment #63
star-szrTagging for profiling.
Comment #64
star-szrThis needs a reroll first.
@no_angel, can you take care of that?
Comment #65
joelpittetStill needs vars @no_angel but here's the re-roll.
Comment #66
star-szrThanks @joelpittet! Setting to needs work for the doc tweaks in #60.
Profiling
Tested with Stark theme on a node page. APC class loader enabled.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51904f8640386&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51904f8640386&...
Comment #67
no_angel CreditAttribution: no_angel commentedsorry all... I will change to unassigned for now.
Comment #68
star-szr@no_angel - No need to apologize :) we're just on a tight timeline with these .tpl.php conversions and this particular one was blocking progress on a couple fronts because the patch no longer applied.
Comment #69
shanethehat CreditAttribution: shanethehat commentedNow with added documentation
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedI'm happy with that then :)
Comment #71
alexpottDo we have to do this on this issue? The increase in cpu time of 1.5ms is quite large compared to other twig conversions... plus there are plans to convert main_menu and secondary_menu to blocks... see #1869476: Convert global menus (primary links, secondary links) into blocks
Comment #72
alexpottDid not mean for the novice tag to come back...
Comment #73
Fabianx CreditAttribution: Fabianx commented#71: Two things: CPU time is not really relevant, wt is.
1.1 ms is still slower.
In general I would agree that we do not need to do this here. However, the benchmark shows that the preprocess overhead is roughly the same, process_page is better (200 ms) and in rough template overhead the overhead of the twig template is minimally higher (239 ms) than the page.tpl.php.
-200 + 239 == 39 ms slower => within measuring range
Therefore I cannot recognize where the slowdown of 1.1 ms could come from.
Perhaps we need to re-profile?
Comment #74
star-szrI'll run profiling on this again.
Comment #75
star-szrI ran these by @Fabianx and he said these numbers and profile runs look reasonable - I think this was just a measuring fluctuation.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5192e6f7bc551&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5192e6f7bc551&...
Ran ab as well.
PHPTemplate:
Twig:
So in XHProf Twig seems slightly slower, but slightly quicker in ab. So I think the performance could definitely be classified as 'comparable' here.
Comment #76
alexpottSeems comparable to me too... especially when you see that the fluctuation in the cpu time in the 8.x...8.x baseline as well.
+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #76.0
alexpottUpdated issue summary.
Comment #77
alexpottCommitted 684aaa5 and pushed to 8.x. Thanks!
Comment #78.0
(not verified) CreditAttribution: commentedUpdated issue summary.