Files: 
CommentFileSizeAuthor
#36 interdiff.txt1.33 KBCottser
#36 1806478-35.patch29.32 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,832 pass(es).
[ View ]
#32 1806478-32.patch25.11 KBkevinquillen
FAILED: [[SimpleTest]]: [MySQL] 55,803 pass(es), 5 fail(s), and 6 exception(s).
[ View ]
#23 1806478-23.patch30.14 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1806478-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 interdiff.txt585 bytesCottser
#19 1806478-19.patch30.23 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#19 interdiff.txt5.27 KBCottser
#17 interdiff.txt1.11 KBjoelpittet
#17 1806478-17-do-not-test.patch26.54 KBjoelpittet
#14 1806478-14-do-not-test.patch23.56 KBCottser
#14 interdiff.txt13.2 KBCottser
#13 1806478-13-do-not-test.patch25.91 KBCottser
#8 1806478-8-do-not-test.patch14.77 KBCottser
#8 interdiff.txt14.3 KBCottser
#3 1806478-3-do-not-test.patch482 bytesCottser

Comments

Issue tags:+Twig

Adding +Twig

Assigned:Fabianx» Cottser
Status:Postponed» Active

Assigned:Cottser» Unassigned
Status:Active» Needs review
StatusFileSize
new482 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 :)

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

Status:Needs review» Needs work

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

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.

StatusFileSize
new14.3 KB
new14.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.

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

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

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

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.

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

Here is the part before it:

<?php
   
// 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.

StatusFileSize
new25.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.

StatusFileSize
new13.2 KB
new23.56 KB

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

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?

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

StatusFileSize
new26.54 KB
new1.11 KB

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.

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

Assigned:Cottser» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.27 KB
new30.23 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new585 bytes
new30.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1806478-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks @Fabianx :)

Status:Needs review» Needs work

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

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.

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

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

This needs a reroll.

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.

Status:Needs work» Needs review
StatusFileSize
new25.11 KB
FAILED: [[SimpleTest]]: [MySQL] 55,803 pass(es), 5 fail(s), and 6 exception(s).
[ View ]

Updated patch.

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

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

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.

Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll
StatusFileSize
new29.32 KB
PASSED: [[SimpleTest]]: [MySQL] 55,832 pass(es).
[ View ]
new1.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.

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.

Status:Reviewed & tested by the community» Fixed

Looks great. Committed/pushed to 8.x.

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

Issue summary:View changes

Add singular patch issue