Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Issue tags: +Twig

Adding +Twig

star-szr’s picture

Assigned: Fabianx » star-szr
Status: Postponed » Active
star-szr’s picture

Assigned: star-szr » Unassigned
Status: Active » Needs review
FileSize
482 bytes

Testbot 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 :)

star-szr’s picture

Seven and Bartik don't have the .info.yml changes so I just closed the Stark issue.

Fabianx’s picture

Status: Needs review » Needs work

Uhm, this should at least also remove the double engine code in here ...

Fabianx’s picture

Yes, please.

However do _not_ mark any of the "revisit-before-release" issues as duplicate until the mega patch is committed.

"active" is fine though.

star-szr’s picture

FileSize
14.3 KB
14.77 KB

I 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.

star-szr’s picture

Title: Make twig the default engine once all modules templates are converted from .tpl.php to .twig » Make twig the default engine once all modules templates are converted from .tpl.php to .html.twig
Assigned: Unassigned » star-szr

@joelpittet rerolled both of those patches (thanks!) so I'll be having another look at this locally.

Fabianx’s picture

+++ b/core/includes/theme.incundefined
@@ -1120,9 +1086,10 @@ function theme($hook, $variables = array()) {
+    // Include Twig, the default theme engine.
+    include_once DRUPAL_ROOT . '/core/themes/engines/twig/twig.engine';

Is that include necessary?

+++ b/core/includes/theme.incundefined
@@ -1120,9 +1086,10 @@ function theme($hook, $variables = array()) {
+    $extension = twig_extension();

This will add 1 function call to theme() every time.

I think hard-coding for the default theme engine is fine.

+++ b/core/includes/theme.incundefined
@@ -1136,13 +1103,6 @@ function theme($hook, $variables = array()) {
-      // @todo Change this 'if' back to 'elseif' once Twig conversion is
-      // complete. Normally only modules have this key set.
-      if (isset($info['engine'])) {
-        if (function_exists($info['engine'] . '_render_template')) {
-          $render_function = $info['engine'] . '_render_template';

I don't think this should be removed completely, but rather set to elseif instead, so that themes can still override the default engine.

star-szr’s picture

Thanks 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.

star-szr’s picture

I think we were just missing context on the last diff in #10.

Here is the part before it:

    // The theme engine may use a different extension and a different renderer.
    global $theme_engine;
    if (isset($theme_engine)) {
      if ($info['type'] != 'module') {
        if (function_exists($theme_engine . '_render_template')) {
          $render_function = $theme_engine . '_render_template';
        }
        $extension_function = $theme_engine . '_extension';
        if (function_exists($extension_function)) {
          $extension = $extension_function();
        }
      }
      // (Removed code is here - which allows modules to use multiple theme engines.)
    }

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.

star-szr’s picture

FileSize
25.91 KB

More progress, patch got bigger:

  • Moved include_once of twig.engine to the same spot as before (_drupal_theme_initialize) - I think we always need to load twig.engine so that contrib PHPTemplate themes can exist.
  • Made twig.engine look for theme functions as well as template files in twig_theme().
  • Hardcoded .html.twig extension in theme() to save function calls.
  • Shuffled around the test themes: test_theme_twig is now test_theme, test_theme is now test_theme_phptemplate.

Still todo:

  • Add actual tests for test_theme_phptemplate!

Currently all tests in the 'Theme' group are passing locally.

star-szr’s picture

Rolled the patch with git diff -C -M1 instead and here is the missing interdiff from #8 as well.

jwilson3’s picture

Shuffled around the test themes: test_theme_twig is now test_theme, test_theme is now test_theme_phptemplate.

If 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?

star-szr’s picture

@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.

joelpittet’s picture

Same 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.

star-szr’s picture

Thanks @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).

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
5.27 KB
30.23 KB

New patch:

  • Actually makes PHPTemplate work by moving theme_render_template() from theme.inc to phptemplate.engine and renaming to phptemplate_render_template().
  • Removes duplicate test from ThemeTestTwig that is exactly duplicated already in ThemeTest. ThemeTestTwig is now for Twig-specific tests like making sure PHP data types print correctly. General theme tests are in ThemeTest.
  • Used PHP to print the message from the PHPTemplate test template, to make sure we are using PHPTemplate and it's not just Twig handling it.

Status: Needs review » Needs work

The last submitted patch, 1806478-19.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

Forgot -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.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.incundefined
@@ -267,11 +267,9 @@ function _drupal_theme_initialize($theme, $base_theme = array(), $registry_callb
+  // Always include Twig so that PHPTemplate themes do not have to convert all
+  // core and contrib templates in order to use the PHPTemplate engine.
   include_once DRUPAL_ROOT . '/core/themes/engines/twig/twig.engine';

This 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.

star-szr’s picture

Status: Needs work » Needs review
FileSize
585 bytes
30.14 KB

Thanks @Fabianx :)

Status: Needs review » Needs work

The last submitted patch, 1806478-23.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Just 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

Issue tags: +Twig

The last submitted patch, 1806478-23.patch, failed testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Twig

The last submitted patch, 1806478-23.patch, failed testing.

star-szr’s picture

Status: Needs work » Reviewed & tested by the community

RTBC 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

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Needs reroll

This needs a reroll.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Twig, -Needs reroll

#23: 1806478-23.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Twig, +Needs reroll

The last submitted patch, 1806478-23.patch, failed testing.

kevinquillen’s picture

Status: Needs work » Needs review
FileSize
25.11 KB

Updated patch.

Status: Needs review » Needs work
Issue tags: -Novice, -Twig, -Needs reroll

The last submitted patch, 1806478-32.patch, failed testing.

hass’s picture

Status: Needs work » Needs review

#32: 1806478-32.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Twig, +Needs reroll

The last submitted patch, 1806478-32.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
29.32 KB
1.33 KB

Thank 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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Re-reviewed patch, reviewed interdiff, => Changes look good to me, pass tests => Back to RTBC

Lets get that first critical closed.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Add singular patch issue