seven.theme - Convert PHPTemplate templates to Twig

A few of us made a bit of progress on this today.

http://drupal.org/sandbox/mrf/1950628

In order to override the overrides in the template.php we'll need #1898038: custom_block.module - Convert theme_ functions to Twig #1898432: node.module - Convert PHPTemplate templates to Twig in before this will apply cleanly.

Also overriding #1939090: Convert theme_tablesort_indicator() to Twig

Our first pass at converting the seven theme to the twig engine.

I'd like to find out if and how this effort can help implement the proposed new styles for Seven (http://drupal.org/sandbox/ry5n/1932040)

@yoroy Folks more familiar with the Twig work should correct me on the following. AFAIK, the template conversions involve first porting the existing templates as-is to Twig, then cleaning them up. Until the first step is done, we can work in parallel. Once we hit the second step, we need to coordinate much more closely. I’m hoping to have all our prototyping work done by the time the Twig conversions are ready. Then we can do the Seven UI integration along with markup and CSS cleanup.

@ry5n, that sounds exactly right to me. Thanks!

#4: drupal-convert_seven_theme_to_twig-1938848-4.patch queued for re-testing.

@mfr, @hefox
The first test failed because twig wasn't enabled during the install phase. http://drupal.org/node/1875992#comment-7234864 was committed fixing the problem.

That was the wrong link. This is the patch that was committed Fri Mar 29 11:02:49 2013: http://drupal.org/node/1942490#comment-7235030. It cleared the way for the patch in #4.

info

Thanks @Jaesin.

As far as I know, this is no longer a blocker for #1898038: custom_block.module - Convert theme_ functions to Twig, so I'm removing that issue from the summary.

Add core themes conversion meta

Should just be an update for changes to seven in core and update for #5 patch in #1939090: Convert theme_tablesort_indicator() to Twig

Interdiffs would be great here, thanks for working on this @hefox :)

Edit: Sorry, nevermind, this is just a reroll I think.

Some manual testing here would be great.

Installed patch on new copy of 8.x before install. Immediately noticed menu appearance changes. All the menus got wider and 'structure' and 'reports' seem to have lost some styling. Attached some images. You can't see it in image but 'after' image had horizontal scroll bar.

Thanks @dcrocks, that's great!

Please see #1938864-3: [meta] Update all core themes to use Twig, I think I'll try to put together a simplytest.me link for testing this issue along with its dependencies.

Removing blocking issue from summary

seven_admin_block_content() needs converting.

This will also need a reroll now that #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" is in.

I added seven_preprocess_admin_block_content.
Updated to patch against seven.theme.

Patches against Commit: 809138a7c6951528e626987bc4e524ba1e85759a
Date:Thu Apr 11 21:58:27 2013 +0100

@Jaesin, thank you. It looks like the patch didn't come across quite right. The patch does not contain any of the changes in #15 and it removes two files:

core/themes/seven/templates/maintenance-page.tpl.php
core/themes/seven/templates/page.tpl.php

Please find more information here on contributing patches:
http://drupal.org/node/707484

#22: drupal-convert_seven_theme_to_twig-1938848-22.patch queued for re-testing.

I guess I should look over the patch before uploading it.

Patch applies to Commit: ee1c53e517fdf466a770d27fddb67bbcbaeb209a
Date: Thu Apr 11 22:42:45 2013 +0100

custom-block-add-list.html.twig needs work.

See: http://drupal.org/node/1898038#comment-7280932

Also, this isn't right:

<dl class="node-type-list">

Since they're not node types.

@TODO: compare custom-block-add-list.html.twig in both
http://drupal.org/files/drupal-convert_seven_theme_to_twig-1938848-26.patch
and
http://drupal.org/files/1898038-22.patch
and make the best template file.

Here's a patch that does what I said in #27.

I'll go remove custom-block-add-list.html.twig and any changes to seven_preprocess_custom_block_add_list() from #1898038: custom_block.module - Convert theme_ functions to Twig

Huh. Noticed some whitespace issues in the docblock for seven_preprocess_custom_block_add_list().

Add conversion summary table

Just tweaked the tablesort indicator override to work alongside #1939090: Convert theme_tablesort_indicator() to Twig and added steps for testing that to the summary. Adding testing steps for the other overrides would be a great help, thanks to everyone who's worked on this issue so far :)

Fixed some invalid HTML. Namely closing tags from </dl> to </ul> on admin-block-content.html.twig and node-add-list.html.twig.

Start on testing steps

Great catch @Jaesin!

After giving this another look I updated the issue summary to indicate that everything is converted. Woohoo!

Here are some points that can be touched up, all minor and related to coding standards and documentation:

1. +++ b/core/themes/seven/templates/node-add-list.html.twigundefined
   @@ -0,0 +1,25 @@
   +{% endif %}
   \ No newline at end of file
   

   This file needs a newline at the end of the file per http://drupal.org/coding-standards#indenting.

2. +++ b/core/themes/seven/templates/maintenance-page.html.twigundefined
   @@ -0,0 +1,53 @@
   +{#
   + # @file
   + # Seven's theme implementation to display a single Drupal page while offline.
   + #
   + # All of the available variables are mirrored in page.tpl.php.
   + #
   + # @see template_preprocess()
   + # @see template_preprocess_maintenance_page()
   + # @see seven_preprocess_maintenance_page()
   + #
   + # @ingroup themeable
   +#}
   +
   +<!DOCTYPE html>
   

   This docblock needs some tweaking to match up with http://drupal.org/node/1823416#docblock (We're missing the PHP-style /**) and we should remove the blank line between the docblock and the doctype.

   And page.tpl.php should be page.html.twig here.

3. +++ b/core/themes/seven/templates/page.html.twigundefined
   @@ -0,0 +1,58 @@
   + * they can be found in the html.tpl.php template normally located in the
   

   html.html.twig.

4. +++ b/core/themes/seven/templates/page.html.twigundefined
   @@ -0,0 +1,58 @@
   + * @see template_process()
   

   template_process() itself is gone in Drupal 8 so let's remove this line. #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess()

Fixed issues from comment #32, also removed line 2 whitespace from page.html.twig.

tyvm Long Beach Drupal contrib meetup!

There needed to be a line at the end of node-add-list.html.twig

Update conversion table

patch applies cleany and everything working when switch to seven theme.

However, seems to be a styling problem related to seven/templates/page.html.twig & #main-content ... but that seems outside of scope of this issue.

If we can get full manual testing steps in the summary that would be great for testing this patch! Here's my review of the latest patch:

1. +++ b/core/themes/seven/seven.themeundefined
   @@ -39,93 +39,57 @@ function seven_preprocess_page(&$vars) {
    /**
   - * Overrides theme_custom_block_add_list().
   + * Overrides variables for custom-block-add-list.html.twig
   + * Add variables for the label and the path separately.
   + *
   + * Displays the list of available custom block types for creation, adding
   + * separate variables for the label and the path.
     *
   - * Displays the list of available custom block types for creation.
     */
   ...
    /**
     * Overrides theme_admin_block_content().
     *
   + * Prepare variables for administrative content block templates.
     * Uses an unordered list markup in both compact and extended mode.
     */
   ...
    
    /**
   - * Overrides theme_tablesort_indicator().
   - *
   - * Uses Seven's image versions, so the arrows show up as black and not gray on
   - * gray.
   + * Implements hook_preprocess_HOOK() for tablesort-indicator.html.twig.
     */
   

   For these, 'Implements hook_preprocess_HOOK' makes more sense, but let's keep the comments explaining these overrides.

2. +++ b/core/themes/seven/templates/admin-block-content.html.twigundefined
   @@ -0,0 +1,25 @@
   + * - compact: class indicator for compact mode (empty if not compact)
   

   Capitalize 'class' and end this comment in a period per http://drupal.org/node/1354#drupal.

3. +++ b/core/themes/seven/templates/node-add-list.html.twigundefined
   @@ -0,0 +1,25 @@
   + * - types: List of content types.
   + *   - type.add_link: Link to create a piece of content of this type.
   + *   - type.description: Description of this type of content.
   

   Since 'type' is just the loop variable and can be called anything, we should say something like:

   - types: List of content types. Each content type contains:
     - add_link: Link to create…
     - description: Description…
   

   +++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
   @@ -0,0 +1,28 @@
   + * - types: A collection of all the available custom block types.
   + *   - type: The custom block type, containing all the items below.
   + *   - type.link: A link to add a block of this type.
   + *   - type.description: A description of this custom block type.
   + *   - type.label: The title of the custom block type.
   + *   - type.path: A path for the link to add a block of this type.
   

   Same thing here.

   +++ b/core/themes/seven/templates/admin-block-content.html.twigundefined
   @@ -0,0 +1,25 @@
   + * - content: List of administrative menu items.
   + *   - item.path: Path to the admin section.
   + *   - item.label: Short name of the section
   + *   - item.description: Description of the administrative menu item.
   

   And here.

4. +++ b/core/themes/seven/templates/maintenance-page.html.twigundefined
   @@ -0,0 +1,54 @@
   +      {% if sidebar_first %}
   +        {{ sidebar_first }}
   +      {% endif %}
   

   I think since we're not printing any markup around sidebar_first, let's just remove the if block.

   +++ b/core/themes/seven/templates/maintenance-page.html.twigundefined
   @@ -0,0 +1,54 @@
   +    </main>
   +
   +  </div>
   +  </body>
   +</html>
   
   +++ /dev/nullundefined
   @@ -1,62 +0,0 @@
   -    </main>
   -
   -  </div>
   -
   -  <footer role="contentinfo">
   -    <?php print $page_bottom; ?>
   -  </footer>
   -
   -  </body>
   -</html>
   

   maintenance-page.html.twig is missing the <footer>

5. +++ b/core/themes/seven/templates/node-add-list.html.twigundefined
   @@ -0,0 +1,25 @@
   + * Default theme implementation to list the available types for content creation.
   

   This is one character too long per http://drupal.org/node/1354#file. The summary line has to fit within 80 characters.

6. +++ b/core/themes/seven/templates/page.html.twigundefined
   @@ -0,0 +1,56 @@
   +      <div role="tab"> {{ primary_local_tasks }} </div>
   
   +++ /dev/nullundefined
   @@ -1,54 +0,0 @@
   -      <?php print '<div role="tab">' . render($primary_local_tasks) . '</div>'; ?>
   

   Not sure about the extra whitespace here in the Twig version, caught my eye. I think the spaces around printing the primary_local_tasks variable can be removed.

Lets hope this gets it.

Thanks @frob! I'm just moving page.html.twig from this issue to #1961870: Convert page.tpl.php to Twig because the automated tests use the seven theme for breadcrumb testing.

After that, I will post a quick review of the changes in #39.

Update remaining

Thanks again for moving this forward @frob! Here's another review:

1. +++ b/core/themes/seven/seven.themeundefined
   @@ -39,93 +39,60 @@ function seven_preprocess_page(&$vars) {
    /**
   - * Overrides theme_custom_block_add_list().
   + * Implements hook_preprocess_HOOK() for custom-block-add-list.html.twig
   + * Add variables for the label and the path separately.
   + *
   + * Displays the list of available custom block types for creation, adding
   + * separate variables for the label and the path.
     *
   - * Displays the list of available custom block types for creation.
     */
   

   The summary line here needs to end in a period and have a blank line below it per http://drupal.org/node/1354#drupal and http://drupal.org/node/1354#functions.

2. +++ b/core/themes/seven/seven.themeundefined
   @@ -39,93 +39,60 @@ function seven_preprocess_page(&$vars) {
    /**
   - * Overrides theme_admin_block_content().
   + * Implements hook_preprocess_HOOK() for theme_admin_block_content().
     *
   + * Prepare variables for administrative content block templates.
     * Uses an unordered list markup in both compact and extended mode.
     */
   

   In this case the 'Prepare variables' comment is unnecessary, let's please remove it.

3. +++ b/core/themes/seven/templates/admin-block-content.html.twigundefined
   @@ -0,0 +1,25 @@
   + * Available variables:
   + * - content: List of administrative menu items.
   + *   - path: Path to the admin section.
   + *   - label: Short name of the section.
   + *   - description: Description of the administrative menu item.
   + * - compact: Class indicator for compact mode (empty if not compact).
   
   +++ b/core/themes/seven/templates/custom-block-add-list.html.twigundefined
   @@ -0,0 +1,28 @@
   + * Available variables:
   + * - types: A collection of all the available custom block types.
   + *   - type: The custom block type, containing all the items below.
   + *   - link: A link to add a block of this type.
   + *   - description: A description of this custom block type.
   + *   - label: The title of the custom block type.
   + *   - path: A path for the link to add a block of this type.
   
   +++ b/core/themes/seven/templates/node-add-list.html.twigundefined
   @@ -0,0 +1,26 @@
   + * Available variables:
   + * - types: List of content types.
   + *   - add_link: Link to create a piece of content of this type.
   + *   - description: Description of this type of content.
   

   I should have referenced this before but when starting a new list indentation we need to end the previous sentence in a colon (http://drupal.org/node/1354#lists), so for these it would make sense to say something like:

   - content: List of administrative menu items. <strong>Each menu item contains:</strong>
     - path: …

4. +++ b/core/themes/seven/templates/maintenance-page.html.twigundefined
   @@ -0,0 +1,57 @@
   +  <footer id="footer" role="contentinfo">
   +    {{ page_bottom }}
   +  </footer>
   
   +++ /dev/nullundefined
   @@ -1,62 +0,0 @@
   -  <footer role="contentinfo">
   -    <?php print $page_bottom; ?>
   -  </footer>
   

   We shouldn't be adding an id="footer" here, it's not in maintenance-page.tpl.php.

5. +++ b/core/themes/seven/templates/node-add-list.html.twigundefined
   @@ -0,0 +1,26 @@
   +{#
   +/**
   + * @file
   + * Default theme implementation to list the available types for content
   + * creation.
   

   I didn't think of this before but this isn't actually the default theme implementation. This is Seven's theme implementation (see maintenance-page.html.twig):

   +++ b/core/themes/seven/templates/maintenance-page.html.twigundefined
   @@ -0,0 +1,57 @@
   + * @file
   + * Seven's theme implementation to display a single Drupal page while offline.
   

   We also can't wrap summary lines, they have to fit within 80 characters per http://drupal.org/node/1354#file.

   So for this one "Seven's theme implementation to list node types available for adding content." would just barely fit within 80 characters :)

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987424: seven.theme - Convert theme_ functions to Twig for theme_ function conversion.

Update conversion table

Doc tweaks from #42

Haven't split out the theme_functions yet from #43

Though this one is a bit strange because page.tpl.php is over at #1961870: Convert page.tpl.php to Twig, thoughts?

@Cottser have another crack at reviewing this, I added a few more bits missing, hopefully in the right direction. I bet after another little cleanup it would be good to go for the split.

Thank you @joelpittet! page.tpl.php was moved over so breadcrumb tests would pass over at #1961870: Convert page.tpl.php to Twig.

This should pass tests, I wasn't thinking it through leaving page.tpl.php here and also leaving engine: twig in seven.info.yml. We're only allowing Twig templates in PHPTemplate themes temporarily for conversion, not the other way around.

I'm thinking this issue should live on as converting seven's maintenance-page.tpl.php (I'd probably say move it to #1987426: Convert maintenance-page.tpl.php to Twig but then it'd make sense to move Bartik's there too) and we should split off the theme_ function conversions to #1987424: seven.theme - Convert theme_ functions to Twig as described in #43.

This is the last patch that needs to be split up to only contain .tpl.php conversions and related preprocess functions.

Found some tweaks and will do manual testing.

Two changes:

1. page_top was missing from the template.
2. Used {{ attributes }} instead of {{ attributes.class }} for body tag.

The markup matches up, checked via markup diff and DaisyDiff. If someone can give the docs and code a once-over I think this one is RTBC.

Had a scan through the code and docs and looks great, thanks @Cottser!

I'm planning on profiling this template by copying it to the Stark theme.

Profiling.

Profiling results:

Added this to the end of seven_preprocess_maintenance_page():

  $node = node_load(1);
  $vars['content'] .= theme('node', node_view($node));

I copied node.html.twig from modules/node/templates to seven/templates.

=== seven-maintenance-8.x..seven-maintenance-8.x compared (5198b38f0f1d5..5198b3b171080):

ct  : 16,931|16,931|0|0.0%
wt  : 107,587|107,545|-42|-0.0%
cpu : 92,995|93,049|54|0.1%
mu  : 8,896,408|8,896,408|0|0.0%
pmu : 8,969,600|8,969,600|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...

=== seven-maintenance-8.x..seven-maintenance-1938848-53 compared (5198b38f0f1d5..5198b3cd5c9c1):

ct  : 16,931|17,051|120|0.7%
wt  : 107,587|108,046|459|0.4%
cpu : 92,995|93,272|277|0.3%
mu  : 8,896,408|8,938,544|42,136|0.5%
pmu : 8,969,600|9,004,104|34,504|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5198b38f0f1d5&...

Revise summary

Loading the extra node is for the purpose of isolating profiling to this conversion only, not loading all of the Twig classes etc.

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Add profiling results

Committed 3d24449 and pushed to 8.x. Thanks!

Awsome! Thanks to everyone who worked on this.