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 #1961868 by 2ndmile, foopang, Cottser: [READY] Convert region.tpl.php to Twig.
(as of #25)
Task
Use Twig instead of PHPTemplate
Remaining
Replace all templates with .html.twig equivalent templates
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 |
---|---|---|---|
#20 | 1961868-20.patch | 2.43 KB | star-szr |
#20 | interdiff.txt | 1.01 KB | star-szr |
#17 | convert-regiontplphp-to-twig-1961868-17.patch | 2.43 KB | 2ndmile |
#17 | interdiff.txt | 396 bytes | 2ndmile |
#13 | convert-regiontplphp-to-twig-1961868-13.patch | 2.46 KB | 2ndmile |
Comments
Comment #1
foopang CreditAttribution: foopang commentedLet me try working on this.
Comment #2
foopang CreditAttribution: foopang commentedPlease review.
Comment #3
star-szrTagging.
Comment #4
star-szrThis is looking good, tests are passing and I did a manual test and everything works as expected.
My only concern is related to documentation. The Twig template has all the same variables available to it as the PHPTemplate one, but the Twig template file only talks about content, attributes, and attributes.class. The PHPTemplate file has better documentation, in that it also describes what classes are there by default, and it also describes the 'region' variable.
So can we bring over (and reformat) the following documentation items from the PHPTemplate file?
Here's an example of what the attributes documentation could look like (taken from #1898432: node.module - Convert PHPTemplate templates to Twig):
I think we can probably leave out is_admin, is_front, and logged_in since those are available to every template, see _template_preprocess_default_variables().
Comment #5
star-szr@foopang - thanks for your work on this! Would you be able to take care of the documentation updates noted in #4? If not, please unassign and someone else can work on this.
Comment #6
foopang CreditAttribution: foopang commentedSorry @Cottser, I have been busy I couldn't work on the documentation right now, so let unassign it for me. I will look back to this issue if no one else works on it.
Comment #7
star-szrNo need to apologize, thanks for your work on this :)
Tagging as Novice to take care of the documentation tweaks in #4 and post a new patch and interdiff.
Comment #8
2ndmile CreditAttribution: 2ndmile commentedNewb alert: Review and let me know if there is anything else that needs done or anything I did incorrectly.
Comment #9
2ndmile CreditAttribution: 2ndmile commentedwoah.. screwed that up... please hold
Comment #10
2ndmile CreditAttribution: 2ndmile commentedOK, this looks better (embarrassed). Please review.
Comment #11
2ndmile CreditAttribution: 2ndmile commentedIndentation and line wrapping for coding standards. Please review.
Comment #12
star-szrThat looks much better, thanks @2ndmile! I found a couple more small tweaks related to documentation and Twig coding standards:
Let's also remove "String of" and just say "Classes that can be…" per Twig coding standards (we don't describe data types in Twig template files).
We should indent the contents of the {% if %} tag by two spaces per http://drupal.org/node/1823416#expressions.
Comment #13
2ndmile CreditAttribution: 2ndmile commentedChanges from #12 implemented. Please review.
Comment #14
star-szrThanks!
Let's rewrap this now to be as long as possible within the 80 character limit per http://drupal.org/coding-standards#indenting.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commented+ * @see template_process()
Why do we have this? The other Twig template patches don't have this line.
Other than that and the review in #13 this looks good.
This is probably ready for manual testing, which should not be hard. If the site disappears this is broken ;)
Comment #16
star-szrAgreed on #15, let's remove the @see template_process().
Removing manual testing tag, I've already gone through the markup locally, we're just working on docs.
Thanks everyone!
Comment #17
2ndmile CreditAttribution: 2ndmile commentedImplemented #15. Please review.
Comment #18
chrisjlee CreditAttribution: chrisjlee commentedComment #19
joelpittetThis looks great! Couple of nitpiks:)
This should probably just be - class: as the indentation denotes it's related to the above. Or don't indent (to be like the one on our Twig Coding Standards http://drupal.org/node/1823416 ])
Could you make that just {{ attributes }} please? We will be doing this in Bartik I believe but not in core.
Comment #20
star-szrDocs tweaks and changes including #19 and more. Thanks @joelpittet!
Comment #21
joelpittetI approve this message.
Comment #21.0
star-szrRemove sandbox, update remaining
Comment #22
star-szrTagging for profiling.
Comment #23
star-szr…and assigning.
Comment #24
star-szrIt took me quite a while to get a solid baseline but now the numbers are coming back consistently and are looking very good.
Tested with Stark theme on a node page, with blocks placed in all 7 regions of the Stark theme. APC class loader enabled.
The first result is HEAD vs. itself - to show the the baseline run used for comparing runs is consistent.
The second result is HEAD vs. #20.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518eceb4aac68&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518eceb4aac68&...
Comment #25
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #25.0
alexpottUpdated issue summary.
Comment #26
alexpottCommitted 5b2afa0 and pushed to 8.x. Thanks!
Comment #27.0
(not verified) CreditAttribution: commentedAdd proposed git commit message