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

Fabianx’s picture

Issue tags:+Twig

Adding +Twig

Cottser’s picture

Assigned:Fabianx» Cottser
Status:Postponed» Active
Cottser’s picture

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

Cottser’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.

Cottser’s picture

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.

Cottser’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» Cottser

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

Cottser’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.

Cottser’s picture

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.

Cottser’s picture

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.

Cottser’s picture

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.

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?

Cottser’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

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.

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

Cottser’s picture

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.

Cottser’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.

Cottser’s picture

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.

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.

Cottser’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

Cottser’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
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.

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.

Cottser’s picture

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.

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