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.
This is an followup of #1696786: Integrate Twig into core: Implementation issue.
Will be part of #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff.txt | 1.33 KB | star-szr |
#36 | 1806478-35.patch | 29.32 KB | star-szr |
#32 | 1806478-32.patch | 25.11 KB | kevinquillen |
#23 | 1806478-23.patch | 30.14 KB | star-szr |
#23 | interdiff.txt | 585 bytes | star-szr |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedAdding +Twig
Comment #2
star-szrRolling a patch to be part of #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch.
Comment #3
star-szrTestbot won't like it but I think this is all we need. Should be fine as part of #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch, and we can remove the .info.yml changes from #1938848: seven.theme - Convert PHPTemplate templates to Twig and #1938840: bartik.theme - Convert PHPTemplate templates to Twig and close #1938854: Convert Stark theme to Twig.
There are some inline docs that could be updated too of course :)
Comment #4
star-szrSeven and Bartik don't have the .info.yml changes so I just closed the Stark issue.
Comment #5
Fabianx CreditAttribution: Fabianx commentedUhm, this should at least also remove the double engine code in here ...
Comment #6
star-szrI can combine these here if you like:
#1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion
#1805592: Remove static double theme engine code once all module provided .tpl.php files are converted to twig and twig is default
Anything else needed?
Comment #7
Fabianx CreditAttribution: Fabianx commentedYes, please.
However do _not_ mark any of the "revisit-before-release" issues as duplicate until the mega patch is committed.
"active" is fine though.
Comment #8
star-szrI went a bit more in-depth to update docs and tests, I can't test this very well locally at this time because we have a couple patches that need rerolls (#1961870: Convert page.tpl.php to Twig and #1898034: block.module - Convert PHPTemplate templates to Twig) but this should be a step in the right direction. Leaving at CNW for now.
Comment #9
star-szr@joelpittet rerolled both of those patches (thanks!) so I'll be having another look at this locally.
Comment #10
Fabianx CreditAttribution: Fabianx commentedIs that include necessary?
This will add 1 function call to theme() every time.
I think hard-coding for the default theme engine is fine.
I don't think this should be removed completely, but rather set to elseif instead, so that themes can still override the default engine.
Comment #11
star-szrThanks for taking a look @Fabianx!
I think the include is necessary unless we move twig_render_template() to theme.inc. I agree we can hardcode the extension :)
As for the last one, that was my @todo comment added there and the whole hunk was added by #1805592: Remove static double theme engine code once all module provided .tpl.php files are converted to twig and twig is default so I assumed it could go. Will double check that tonight.
Comment #12
star-szrI think we were just missing context on the last diff in #10.
Here is the part before it:
However I will look into adding a PHPTemplate test theme and adding some tests around that. I'm seeing a few test failures in the theme group so I will work those out locally and/or post a partial megapatch here.
Comment #13
star-szrMore progress, patch got bigger:
Still todo:
Currently all tests in the 'Theme' group are passing locally.
Comment #14
star-szrRolled the patch with
git diff -C -M1
instead and here is the missing interdiff from #8 as well.Comment #15
jwilson3If the plan is to leave test_theme_phptemplate in the codebase, then could we not leave test_theme_twig as is (instead of renaming it to test_theme) for consistency between the two?
Comment #16
star-szr@jwilson3 - I'd be fine with calling it test_theme_twig, but more work would be needed to update the existing theme tests to use that theme and make them all pass.
Comment #17
joelpittetSame as #1987510-19: [meta] Convert all core *.tpl.php templates to Twig as singular patch with the ThemeTest.php using twig instead of php template.
Comment #18
star-szrThanks @joelpittet. Did you want to write the ThemeTestPhpTemplate tests as discussed yesterday? These will just test templates and theme suggestions from the test_theme_phptemplate theme and can probably be based heavily on the pre-patch ThemeTestTwig tests (minus the data type tests).
Comment #19
star-szrNew patch:
Comment #21
star-szrForgot -do-not-test.patch :)
This needs to be tested as part of #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch.
Comment #22
Fabianx CreditAttribution: Fabianx commentedThis is confusing and probably should read something like:
"Always include Twig as default theme engine."
I reviewed this again and besides the very small doc change above this is ready to fly and RTBC.
CNW then can be RTBC'ed.
Comment #23
star-szrThanks @Fabianx :)
Comment #25
Fabianx CreditAttribution: Fabianx commentedJust a docs change, so RTBC per #22.
This will pass tests as part of the Twig-Megapatch when all templates are converted.
Patch has extensive test coverage, tests that phptemplate can still be used in contrib themes, fixes a bug that theme_ functions could not be specified in twig templates.
=> RTBC
Comment #28
star-szrRTBC per #26 as part of the megapatch, this passes testing along with the conversions: #1987510-34: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #29
star-szrThis needs a reroll.
Comment #30
alexpott#23: 1806478-23.patch queued for re-testing.
Comment #32
kevinquillen CreditAttribution: kevinquillen commentedUpdated patch.
Comment #34
hass CreditAttribution: hass commented#32: 1806478-32.patch queued for re-testing.
Comment #36
star-szrThank you @kevinquillen! The tests for PHPTemplate and test_theme_phptemplate were missing from the patch, I highly recommend
git apply --index
for applying patches so that newly added files are not lost. The instructions at http://drupal.org/patch/reroll are quite good IMO :)Here is another reroll, and I have slightly tweaked the PHPTemplate test to be a bit more intentional - see interdiff.
Comment #37
Fabianx CreditAttribution: Fabianx commentedRe-reviewed patch, reviewed interdiff, => Changes look good to me, pass tests => Back to RTBC
Lets get that first critical closed.
Comment #38
catchLooks great. Committed/pushed to 8.x.
Comment #39.0
(not verified) CreditAttribution: commentedAdd singular patch issue