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 #1898458 by widukind, Cottser, c4rl: [READY] theme_test().module - Convert PHPTemplate templates to Twig.
(as of #32)
Task
Convert PHPTemplate templates to Twig templates.
Remaining
Template path | Conversion status |
---|---|
core/modules/system/tests/modules/theme_test/templates/theme_test.template_test.tpl.php | Converting this to Twig is just a matter of renaming the file to from .tpl.php to .html.twig. |
core/modules/system/tests/themes/test_theme/node--1.tpl.php | Doesn't have to be converted, there is already be a Twig version of this file at core/modules/system/tests/themes/test_theme_twig/node--1.html.twig. |
core/modules/system/tests/themes/test_theme/theme_test.template_test.tpl.php | Doesn't have to be converted, there is already be a Twig version of this file at core/modules/system/tests/themes/test_theme_twig/theme_test.template_test.html.twig. |
Related
#1987414: Remove test coverage for theme functions
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff.txt | 756 bytes | widukind |
#29 | 1898458-29.patch | 2.42 KB | widukind |
#28 | 1898458-28.patch | 2.98 KB | widukind |
#23 | interdiff.txt | 1.97 KB | widukind |
#23 | 1898458-23.patch | 5.72 KB | widukind |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
fregan CreditAttribution: fregan commentedComment #3
fregan CreditAttribution: fregan commentedComment #4
carsonblack CreditAttribution: carsonblack commentedComment #5
carsonblack CreditAttribution: carsonblack commentedComment #6
Fabianx CreditAttribution: Fabianx commentedThis will need to stay as .tpl.php as this is testing both engines in core. This will be removed once twig conversion is complete.
Comment #7
star-szrI don't think this needs to be postponed. Part of the conversion is just a file rename (theme_test.template_test.tpl.php -> theme_test.template_test.html.twig), and then there are a couple of tiny theme functions. I'm thinking converting them shouldn't break any of the PHPTemplate tests since modules already work with both PHPTemplate and Twig templates.
Either way, there are more important conversions to be done, that's for sure.
Comment #8
star-szrBack to active in case someone wants to test my theory :)
Comment #9
widukind CreditAttribution: widukind commentedtaking a stab at it...
Comment #10
widukind CreditAttribution: widukind commentedWhat Cottser said in #7.
Please review attached patch.
Thanks!
Comment #12
widukind CreditAttribution: widukind commentedComment #13
star-szrThanks for being brave and taking this on, @widukind! If you can post an interdiff every time you update a patch that would be a huge help as well :)
This change makes sense, nice catch!
Below are some updates that can be done to move this forward, mostly just documentation and coding standards fixes.
Changing this test file is not necessary, twig_extension() returns '.html.twig'.
The second @file is a bit too long per http://drupal.org/node/1354#file, I would recommend removing the "used by tests" part from both.
The list items are indented by one space too many (http://drupal.org/node/1354#lists), and the comments should be complete sentences ending in a period per http://drupal.org/node/1354#drupal. Something like "Dummy text to display." maybe?
The new files added need a blank line at the end of the file per http://drupal.org/coding-standards#indenting.
Slight tweaks here per #1913208: [policy] Standardize template preprocess function documentation:
"Preprocesses" should be "Prepares".
Default template: theme-test.html.twig. (needs to be a complete sentence, add a period at the end).
And the 'dummy text' needs to be a complete sentence, you can use the same text you use in the Twig file docblocks.
These should end in a comma per http://drupal.org/coding-standards#array.
Thanks again!
Comment #14
widukind CreditAttribution: widukind commented@Cottser, thanks for the detailed breakdown.
Fixed the various formatting issue.
On a related note, adding new-lines to the end of the twig templates caused the failure of two unit-tests. I resolved this by trimming the actual output before feeding it into the assertion functions. Feels kludgy, but may be good enough?
Comment #15
star-szrTwo minor tweaks and this is ready to go! Great work :)
In the patch in #14 there's one line deleted from TwigSettingsTest.php, we should not make any changes to this file.
I didn't notice this before, but these are backwards, theme-test-foo.html.twig says it's implementing the theme_test theme hook, and vice versa. Just swap the @file descriptions.
Comment #16
widukind CreditAttribution: widukind commentedFixed the switcheroo in the code-comments and added the missing blank line back in.
Please re-review.
Thanks!
Comment #17
star-szrExcellent, I don't see any issues whatsoever. Great work @widukind!
Comment #18
star-szr@widukind - you could try a whitespace modifier like @joelpittet used in #1898456: common_test.module - Convert theme_ functions to Twig to avoid the trim() change in the test. Give it shot!
Comment #19
c4rl CreditAttribution: c4rl commentedComment #20
widukind CreditAttribution: widukind commentedComment #21
widukind CreditAttribution: widukind commentedUpon revisiting, I noticed that the assertions in
Theme_Test::testRegistryRebuild()
will fail when running in twig debug mode, due to the additional HTML comments that become part of the output.Will fix this while I'm at it - later on today.
Comment #22
star-szrI wouldn't worry about tests failing with twig_debug mode on, I think we have that covered in #1971860: Document that Twig debug mode breaks tests. Might just be a matter of defaulting twig_debug to be off for all tests except when testing twig_debug itself :)
Comment #23
widukind CreditAttribution: widukind commentedThanks @Cottser!
Removed calls to
trim()
and added a white-space modifier in the twig template as suggested.Comment #23.0
widukind CreditAttribution: widukind commentedAdd conversion summary table
Comment #24
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 #1987414: Remove test coverage for theme functions for theme_ function conversion.
Comment #24.0
c4rl CreditAttribution: c4rl commentedUpdate remaining, remove sandbox link
Comment #25
widukind CreditAttribution: widukind commented@c4rl, what work remains to be done here?
Thanks!
Comment #26
c4rl CreditAttribution: c4rl commented#25 @widukind, the patch here should *only* convert the template-based theme callbacks, not the concatenation-based theme callbacks.
So, you'd leave
theme_theme_test_foo
andtheme_theme_test
as-is.Comment #27
star-szrRight, and move those theme_ function conversions to #1987414: Remove test coverage for theme functions.
Comment #28
widukind CreditAttribution: widukind commentedThanks @Cottser and @c4rl.
Here the scaled back version of the previous patch.
Comment #29
widukind CreditAttribution: widukind commentedFurther scale-back in order to completely separate this patch from the one attached to http://drupal.org/node/1987414.
Comment #30
star-szrLooks great and I think it's safe to say this one doesn't need profiling :) thanks @widukind!
Comment #31
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #32
star-szrI think this might truly be a duplicate now, at least based on the latest patch in #1806478: Make twig the default engine once all modules templates are converted from .tpl.php to .html.twig. So for now, I've asked @Fabianx to exclude this patch from the megapatch.
Comment #32.0
star-szrList of templates
Comment #33
alexpottCommitted af28766 and pushed to 8.x. Thanks!
Comment #34.0
(not verified) CreditAttribution: commentedAdded suggested commit message