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.

#1987414: Remove test coverage for theme functions
#1757550: [Meta] Convert core theme functions to Twig templates

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

fregan’s picture

Assigned: Unassigned » fregan
fregan’s picture

Assigned: fregan » Unassigned
carsonblack’s picture

Assigned: Unassigned » carsonblack
carsonblack’s picture

Assigned: carsonblack » Unassigned
Fabianx’s picture

Status: Active » Postponed

This will need to stay as .tpl.php as this is testing both engines in core. This will be removed once twig conversion is complete.

star-szr’s picture

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

star-szr’s picture

Status: Postponed » Active

Back to active in case someone wants to test my theory :)

widukind’s picture

Assigned: Unassigned » widukind

taking a stab at it...

widukind’s picture

Status: Active » Needs review
FileSize
5.39 KB

What Cottser said in #7.

Please review attached patch.

Thanks!

Status: Needs review » Needs work

The last submitted patch, drupal-1898458-10.patch, failed testing.

widukind’s picture

Status: Needs work » Needs review
FileSize
6.72 KB
star-szr’s picture

Status: Needs review » Needs work

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

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.phpundefined
@@ -63,8 +63,8 @@ function testFindThemeTemplates() {
-    $this->assertEqual($cache['theme_test_template_test']['template_file'], 'core/modules/system/tests/modules/theme_test/templates/theme_test.template_test.tpl.php', 'theme_test is using theme_test.template_test.tpl.php as template file');
-    $this->assertEqual($cache['theme_test_template_test']['engine'], 'phptemplate', 'theme_test is using phptemplate as engine.');
+    $this->assertEqual($cache['theme_test_template_test']['template_file'], 'core/modules/system/tests/modules/theme_test/templates/theme_test.template_test.html.twig', 'theme_test is using theme_test.template_test.html.twig as template file');
+    $this->assertEqual($cache['theme_test_template_test']['engine'], 'twig', 'theme_test is using twig as engine.');

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.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.phpundefined
    @@ -76,7 +76,7 @@ function testTwigDebugOverride() {
    -    $extension = twig_extension();
    +    $extension = '.html.twig';
    
    @@ -108,5 +108,4 @@ function testTwigCacheOverride() {
    -
    

    Changing this test file is not necessary, twig_extension() returns '.html.twig'.

  2. +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test-foo.html.twigundefined
    @@ -0,0 +1,14 @@
    + * @file
    + * Default theme implementation for the 'theme_test' theme hook used by tests.
    
    +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test.html.twigundefined
    @@ -0,0 +1,15 @@
    + * @file
    + * Default theme implementation for the 'theme_test_foo' theme hook used by tests.
    

    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.

  3. +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test-foo.html.twigundefined
    @@ -0,0 +1,14 @@
    + * Available variables:
    + *  - foo: dummy text
    
    +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test.html.twigundefined
    @@ -0,0 +1,15 @@
    + * Available variables:
    + *  - foo: dummy text
    

    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?

  4. +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test-foo.html.twigundefined
    @@ -0,0 +1,14 @@
    \ No newline at end of file
    
    +++ b/core/modules/system/tests/modules/theme_test/templates/theme-test.html.twigundefined
    @@ -0,0 +1,15 @@
    \ No newline at end of file
    

    The new files added need a blank line at the end of the file per http://drupal.org/coding-standards#indenting.

  5. +++ b/core/modules/system/tests/modules/theme_test/theme_test.incundefined
    @@ -1,14 +1,13 @@
    + * Preprocesses variables for theme test templates.
    + *
    + * Default template: theme-test.html.twig
    + *
    + * @param array $variables
    + *   An associative array containing:
    + *   - foo: dummy text
    

    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.

  6. +++ b/core/modules/system/tests/modules/theme_test/theme_test.moduleundefined
    @@ -7,6 +7,7 @@ function theme_test_theme($existing, $type, $theme, $path) {
    +    'template' => 'theme-test'
    
    @@ -16,6 +17,7 @@ function theme_test_theme($existing, $type, $theme, $path) {
    +    'template' => 'theme-test-foo'
    

    These should end in a comma per http://drupal.org/coding-standards#array.

Thanks again!

widukind’s picture

Status: Needs work » Needs review
FileSize
5.49 KB
7.7 KB

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

star-szr’s picture

Status: Needs review » Needs work

Two minor tweaks and this is ready to go! Great work :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigSettingsTest.phpundefined
@@ -108,5 +108,4 @@ function testTwigCacheOverride() {
-

In the patch in #14 there's one line deleted from TwigSettingsTest.php, we should not make any changes to this file.

+++ b/core/modules/system/tests/modules/theme_test/templates/theme-test-foo.html.twigundefined
@@ -0,0 +1,14 @@
+ * Default theme implementation for the 'theme_test' theme hook.

+++ b/core/modules/system/tests/modules/theme_test/templates/theme-test.html.twigundefined
@@ -0,0 +1,15 @@
+ * Default theme implementation for the 'theme_test_foo' theme hook.

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.

widukind’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
7.08 KB

Fixed the switcheroo in the code-comments and added the missing blank line back in.
Please re-review.
Thanks!

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, I don't see any issues whatsoever. Great work @widukind!

star-szr’s picture

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

c4rl’s picture

Assigned: widukind » Unassigned
widukind’s picture

Assigned: Unassigned » widukind
widukind’s picture

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

star-szr’s picture

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

widukind’s picture

FileSize
5.72 KB
1.97 KB

Thanks @Cottser!
Removed calls to trim() and added a white-space modifier in the twig template as suggested.

widukind’s picture

Issue summary: View changes

Add conversion summary table

c4rl’s picture

Title: Convert theme_test module to Twig » theme_test.module - Convert PHPTemplate templates to Twig
Status: Reviewed & tested by the community » Needs work

Per #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.

c4rl’s picture

Issue summary: View changes

Update remaining, remove sandbox link

widukind’s picture

@c4rl, what work remains to be done here?
Thanks!

c4rl’s picture

#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 and theme_theme_test as-is.

star-szr’s picture

Right, and move those theme_ function conversions to #1987414: Remove test coverage for theme functions.

widukind’s picture

Status: Needs work » Needs review
FileSize
2.98 KB

Thanks @Cottser and @c4rl.
Here the scaled back version of the previous patch.

widukind’s picture

FileSize
2.42 KB
756 bytes

Further scale-back in order to completely separate this patch from the one attached to http://drupal.org/node/1987414.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks great and I think it's safe to say this one doesn't need profiling :) thanks @widukind!

alexpott’s picture

Title: theme_test.module - Convert PHPTemplate templates to Twig » [READY] theme_test.module - Convert PHPTemplate templates to Twig
Status: Reviewed & tested by the community » Closed (duplicate)
star-szr’s picture

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

star-szr’s picture

Issue summary: View changes

List of templates

alexpott’s picture

Title: [READY] theme_test.module - Convert PHPTemplate templates to Twig » theme_test.module - Convert PHPTemplate templates to Twig
Status: Closed (duplicate) » Fixed

Committed af28766 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Added suggested commit message