seven.theme - Convert theme_ functions to Twig

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

Updated issue summary.

here's the conversion of only the theme function overrides.

#2: twig-convert_seven_theme_funcs-1987424-2.patch queued for re-testing.

I found that the description of the content type and the custom block is missing from the lists. I will add this in an upcoming patch.

Adds description keys to the arrays for custom blocks and content types.

I've set the default theme to Seven. Created a node and promoted it to frontpage.
Run 519fc53e46a65 uploaded successfully for drupal-perf-drupalcon.
Run 519fc817b51ae uploaded successfully for drupal-perf-drupalcon.

=== 8.x..8.x compared (519fc53e46a65..519fc817b51ae):

ct  : 26,146|26,146|0|0.0%
wt  : 89,244|84,689|-4,555|-5.1%
cpu : 88,000|80,000|-8,000|-9.1%
mu  : 12,862,056|12,862,056|0|0.0%
pmu : 12,954,008|12,954,008|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc53e46a65&...

Run 519fc53e46a65 uploaded successfully for drupal-perf-drupalcon.
Run 519fc84e17238 uploaded successfully for drupal-perf-drupalcon.

=== 8.x..twig-convert_seven_theme_funcs-1987424-2.patch compared (519fc53e46a65..519fc84e17238):

ct  : 26,146|26,146|0|0.0%
wt  : 89,244|88,702|-542|-0.6%
cpu : 88,000|84,000|-4,000|-4.5%
mu  : 12,862,056|12,869,864|7,808|0.1%
pmu : 12,954,008|12,964,552|10,544|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fc53e46a65&...

Functional review went great. Steps:

1. Standard install was successful with patch applied
2. Loaded /node/add, saw list as expected
3. Deleted both content types, went to /node/add, saw message as expected
4. Added a content type with malicious description (<script> tag), no problems there

I can't find an example of the 'administrative block' template, but the format is similar to the other twig files that it seems good to go.

I was able to apply the patch, I didn't test install.
I can verify the message was on node/add, as well as block/add. The message was also present for a custom block type.

profiling not run correctly. Needs to be redone

#5: twig-convert_seven_theme_funcs-1987424-5.patch queued for re-testing.

I'll work on profiling this.

Here is some of profiling results.

Scenario:

• Made seven as default theme
• Set node/add as the frontpage
• Gave anonymous users permission to access all node / content related admin pages

Run 51af3d98520b7 uploaded successfully for drupal-perf-drupalcon.
Run 51af430daedb2 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..8.x compared (51af3d98520b7..51af430daedb2):

ct  : 33,916|33,916|0|0.0%
wt  : 301,287|299,935|-1,352|-0.4%
cpu : 288,685|286,864|-1,821|-0.6%
mu  : 42,690,888|42,690,888|0|0.0%
pmu : 42,725,560|42,725,560|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51af3d98520b7&...

Run 51af3d98520b7 uploaded successfully for drupal-perf-drupalcon.
Run 51af435181b2e uploaded successfully for drupal-perf-drupalcon.
=== 8.x..1987424-5 compared (51af3d98520b7..51af435181b2e):

ct  : 33,916|34,004|88|0.3%
wt  : 301,287|300,489|-798|-0.3%
cpu : 288,685|287,731|-954|-0.3%
mu  : 42,690,888|42,759,296|68,408|0.2%
pmu : 42,725,560|42,792,624|67,064|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51af3d98520b7&...

Removing needs profiling tag. We'll see what others think about the profiling results.

Minor nitpick re #5

+++ b/core/themes/seven/seven.theme
@@ -39,93 +39,62 @@ function seven_preprocess_page(&$vars) {
 /**
- * Overrides theme_admin_block_content().
+ * Implements hook_preprocess_HOOK() for theme_admin_block_content().

Should be for admin-block-content.html.twig. like the rest.

dibs

dibS!

dibs

dibs!

dibs!!

dibs!

dibs

I tried to reroll the patch, but I got an error
Warning: Missing argument 3 for template_preprocess(), called in /drupal/core/includes/theme.inc on line 1130 and defined in template_preprocess() (line 2474 of core/includes/theme.inc).

Reroll of #5. This needs work so that the arrows show up on table headers.

Carolyn could you do another re-roll it looks like all the $vars to $variables got reverted in in #29

+++ b/core/themes/seven/templates/admin-block-content.html.twig
@@ -0,0 +1,26 @@
+ * @see template_preprocess()

+++ b/core/themes/seven/templates/custom-block-add-list.html.twig
@@ -0,0 +1,30 @@
+ * @see template_preprocess()

+++ b/core/themes/seven/templates/node-add-list.html.twig
@@ -0,0 +1,26 @@
+ * @see template_preprocess()

These can be removed too.

-Rerolled patch
-Changed $vars to $variables
-Removed references to @see template_preprocess()

Hold on, patch is wrong, let me take another crack at it.

-Fixed the re-roll.

removed tag.

EDIT: blergh, issue queue fail.

#35: seven-1987424-35.patch queued for re-testing.

The latest patch is also trying to add ie.css back in the theme. It also removes the CSS from the install page.

trying again.

If test bot turns green, then I think this can be considered RTBC. After applying the patch, I made seven the default theme. I played around a little and everything looked fine.

+++ b/core/themes/seven/seven.themeundefined
@@ -145,9 +102,6 @@ function seven_tablesort_indicator($variables) {
 function seven_preprocess_install_page(&$variables) {
   drupal_add_js(drupal_get_path('theme', 'seven') . '/js/mobile.install.js');
-  drupal_add_css(drupal_get_path('theme', 'seven') . '/install-page.css', array('group' => CSS_AGGREGATE_THEME));
-  $variables['styles'] = new RenderWrapper('drupal_get_css');
-  $variables['scripts'] = new RenderWrapper('drupal_get_js');
 }
 

This patch still removes css from preprocess_install_page, apart from that we're good to go.

Revise summary

Rerolled!

Um.... lemme just fix those function description comments.

Spotted a couple additional documentation comments to modify, I hope this isn't off the mark.

Oops. Here's the actual patch.

Sorry, white space.

Code seems ok and patch applies well. Let go for RTBC?

+++ b/core/themes/seven/seven.theme
@@ -62,104 +62,62 @@ function seven_preprocess_page(&$variables) {
-    $output .= '</ul>';
   }
-  else {
-    $output = '<p>' . t('You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.', array('@create-content' => url('admin/structure/types/add'))) . '</p>';
-  }
-  return $output;

This looks like someone may have forgot to convert the 'You have not created any content types yet. '... bit from node_add_list.

Can someone manually test this?

@joelpittet: The message is still there. It seems to be in the template that makes sense and it's better ;)

@rteijeiro so the steps to reproduce:
Remove content types
Go to /node/add

Screenshot of what it should say after the content types are removed. This needs test with the patch:)

Yes, you are right! Just confused the page.

Re-created the patch after discussing with you ;)

Added core/themes/seven/templates/node-add-list.html.twig template missed from #2 patch.

Forget Cottser comments in #63.

This is the good patch and I hope final one :P

It looks like the twig templates got lost in #42 So we'll need to make sure those get added back in from #35

Thanks @rteijeiro for adding in the node-add-list.html.twig

These two need to be added as well.
+++ b/core/themes/seven/templates/admin-block-content.html.twig
+++ b/core/themes/seven/templates/custom-block-add-list.html.twig

Ok, I will try to fix this issue today ;)

Sorry for the delay. Here is the patch with the two templates from comment #35.

Thanks for the patch and adding those twig files back in @rteijeiro. One of the exceptions/fails is related to no {{ compact }} variable passed by preprocess, check #35 again in the preprocess and compare to see where those got lost.

Let's see if now passes the test. It seems that seven_preprocess_admin_block_content function was not created :(

#74: 1987424-74.patch queued for re-testing.

+++ b/core/themes/seven/templates/node-add-list.html.twig
@@ -0,0 +1,25 @@
++{% if content %}

Doubl ++ on this node-add-list.html.twig

Most likely why it's failing.

Also I think this is missing an tablesort-indicator.html.twig. It may not be needed but the width/height of the image may need changing from the system version? Just need a markup check on that one.

Whoops. Needs work, see #78

Re-rolled!

It seems that for seven theme the test is using:

$this->assertLinkByHref(url('block/add/foo', $options));

But for bartik and startk themes is using:

$this->clickLink('foo');

I will try to fix it tomorrow. Today my simpletest module is not working and I don't know why :(

New re-roll.

Re-rolled

Patch for the failing test. It seems to be something weird in $options of $this->assertLinkByHref(url('block/add/foo', $options));

Sorry for the crappy patch in #87. It seems I uploaded the interdiff :(

It seems that there is no theme parameter for block/add so that's because I removed $options in $this->assertLinkByHref(url('block/add/foo', $options));

Hope now patch it's right.

Some nitpicks and regression stuff after another review.
@rteijeiro thanks for the new patch.

1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
   @@ -171,7 +171,7 @@ public function testsCustomBlockAddTypes() {
   -          $this->assertLinkByHref(url('block/add/foo', $options));
   +          $this->assertLinkByHref(url('block/add/foo'));
   

   I think we need to keep that... but I could be wrong. It looks like it makes sure the block is added to the right theme.

2. +++ b/core/themes/seven/seven.theme
   @@ -43,104 +43,62 @@ function seven_preprocess_page(&$variables) {
   +      $variables['types'][$type->type]['label'] = check_plain($type->name);
   +      $variables['types'][$type->type]['description'] = filter_xss_admin($type->description);
   ...
   +      $variables['types'][$id]['label'] = check_plain($type->label());
   +      $variables['types'][$id]['description'] = filter_xss_admin($type->description);
   ...
   +      $variables['content'][$key]['label'] = filter_xss_admin($item['title']);
   ...
   +      $variables['content'][$key]['description'] = filter_xss_admin($item['description']);
   

   check_plain and filter_xss_admin have been replaced with String::checkPlain() and Xss::filterAdmin

3. +++ b/core/themes/seven/seven.theme
   @@ -43,104 +43,62 @@ function seven_preprocess_page(&$variables) {
   -      $output .= \Drupal::l($content, 'custom_block.add_form', array('custom_block_type' => $id), $options);
   ...
   +      $variables['types'][$id]['path'] = url('block/add/' . $type->id);
   

   This this needs be the \Drupal::l() above for the route to stay in tact.

4. +++ b/core/themes/seven/templates/node-add-list.html.twig
   @@ -0,0 +1,25 @@
   +  <p>{{ 'You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.'|t({'@create-content': url('admin/structure/types/add')}) }}</p>
   

   This would be nicer as a trans block.

It seems that tests fail now in a different place. Trying to figure out how to fix it.

Sorry @rteijeiro that custom_block_add_list is confusing because we changed it from using $variables['types'] to using ['content'] which is what types is originally built from. This had all sorts of things done in it. So I moved the query params up to seven's url.

The trans block was a bit trickier but the purpose is you don't need a giant string any longer, the trick is that only simple variables can be used inside so I had to set the url above to use it inside.

Update conversion table

I got part-way through comparing before & after....

For function node_add_list, no output markup change.

Before:

After:

For function custom_block_add_list, no output markup change.

Before:

After:

For admin_block_content, only change was an added space in the ul tag:
<ul class="admin-list ">

Before:

After:

Add steps to test

93: 1987424-93-twig-seven-theme.patch queued for re-testing.

Rerolled.

@longwave nice work on the re-roll, any chance you want to grab the items from #95?

Followed the suggestion in #95, removed a blank comment line, and fixed a function name reference.

template_preprocess_admin_block_content() and template_preprocess_node_add_list() don't actually exist yet, though they will after #1987410: [meta] system.module - Convert theme_ functions to Twig and #1987406: node.module - Convert theme_ functions to Twig get in.

diff --git a/core/modules/system/templates/image.html.twig b/core/modules/system/templates/image.html.twig
new file mode 100644
index 0000000..b6b238a
--- /dev/null
+++ b/core/modules/system/templates/image.html.twig
@@ -0,0 +1,14 @@
+{#
+/**
+ * @file
+ * Default theme implementation of an image.
+ *
+ * Available variables:
+ * - attributes: HTML attributes for the img tag.
+ *
+ * @see template_preprocess_image()
+ *
+ * @ingroup themeable
+ */
+#}
+<img{{ attributes }} />

I think this image template got into this patch by accident. It doesn't look to be part of the seven theme.

Rerolled without image.html.twig.

+++ b/core/themes/seven/seven.theme
@@ -57,104 +59,69 @@ function seven_preprocess_page(&$variables) {
+function seven_preprocess_tablesort_indicator(&$variables) {
+  $variables['uri'] = drupal_get_path('theme', 'seven') . '/images/arrow-' . ($variables['style'] == 'asc' ? 'asc' : 'desc') . '.png';
+  $variables['attributes']['src'] = file_create_url($variables['uri']);
 }

I found that the preprocess_tablesort_indicator is not working:( when I was doing manual testing. The image doesn't change to the seven image. All 3 other templates work perfectly and output matches.

Ok here is a fix to #103 and #102. Removed the image.html.twig that got in there by accident and fixed the tablesort indicator output.

Here's 4 theme's markup comparisons:

This of course won't work... but it should. There should be some globally accessible variables in the twig context and maybe a few more functions like 'drupal_get_path' or something of it's kin.

FYI, re #108 even though It passed... theme_path is not a real variable... yet.

Here is the reroll. I'm not sure about preprocess I've added, maybe someone may suggest better solution.

@InternetDevels what did you add, could you provide an interdiff for the changes you made from #108?

It's a bit difficult to create interdiff between an applicable and a non-applicable patches, but I hope that attached file may help you to see what changes I've added.

@interdevels sorry, wasn't clear there. Don't worry about interdiffs on rerolls. Just for your changes. It helps see clearly what you changed from the previous patch. I know it's tricky if your changes are right after a reroll though if you are using branches in it helps to commit and post the reroll first then commit and post the patch + interdiff. I could have miss read your post when you said your not sure about stuff you added. So if that was part of the conflict resolution I apologize but maybe you can point out that section of code?

rerolled.

+++ b/core/themes/seven/seven.theme
@@ -73,6 +74,7 @@ function seven_preprocess_page(&$variables) {
+<<<<<<< HEAD

@@ -150,103 +152,70 @@ function seven_menu_local_task($variables) {
+=======
...
+>>>>>>> applying old patch

Some merge markers got into the re-roll. Could you give it another crack?

Patch #119 rerolled.

Thanks for the re-roll here are some notes we should take care of to avoid any confusion or pushback.

1. +++ b/core/themes/seven/seven.theme
   @@ -112,104 +113,68 @@ function seven_menu_local_task($variables) {
   +      $variables['types'][$type->type]['path'] = url('node/add/' . $type->type);
   ...
   +      $variables['types'][$type->id()]['path'] = \Drupal::url('custom_block.add_form', array('custom_block_type' => $type->id()), $options);
   ...
   +      $variables['content'][$key]['path'] = url($item['link_path']);
   

   'url' or 'href' may be a better key to not confuse people who have a conceptual concept of 'path'

2. +++ b/core/themes/seven/templates/node-add-list.html.twig
   @@ -0,0 +1,28 @@
   +  {% set add_content_type_url = url('admin/structure/types/add') %}
   

   I'd rather do this in the preprocess and as a route because we'll very likely change these paths all to routes and depreciate/change the url() function in twig.

3. +++ b/core/themes/seven/templates/tablesort-indicator.html.twig
   @@ -0,0 +1,17 @@
   +  <img src="{{ url(theme_path ~ '/images/arrow-asc.png') }}" width="9" height="5" alt="{{ 'Sort ascending'|t }}" title="{{ 'Sort ascending'|t }}" />
   ...
   +  <img src="{{ url(theme_path ~ '/images/arrow-desc.png') }}" width="9" height="5" alt="{{ 'Sort descending'|t }}" title="{{ 'Sort descending'|t }}" />
   

   I know I put this url/theme_path stuff in here but can we move that back to preprocess because again we may be changing url() to routes and hopefully coming up with a nicer way to get a global theme_path variable or function into twig. @see [2168231]

Made changes as suggested by joelpittet.

Patch attached.

Couple more things, hope this goes green.

+++ b/core/themes/seven/seven.theme
@@ -120,8 +120,10 @@
+    $variables['add_content_type_url'] = url('admin/structure/types/add');

So we don't get in trouble by the routing police, the url() call can become:
\Drupal::url('node.type_add');

+++ b/core/themes/seven/seven.theme
@@ -112,104 +113,71 @@ function seven_menu_local_task($variables) {
+  $variables['arrow_asc'] = url(drupal_get_path('theme','seven') . '/images/arrow-asc.png');
+  $variables['arrow_desc'] = url(drupal_get_path('theme','seven') . '/images/arrow-desc.png');

Have been told that url() will likely break things for multilingual by prefixing the file path with /fr/ or something. So we should be using file_create_url() here.

I made the routing police happy again.

Thanks for the routing policing fix:)

+++ b/core/themes/seven/seven.theme
@@ -112,104 +113,71 @@ function seven_menu_local_task($variables) {
+  $variables['arrow_asc'] = file_create_url('/images/arrow-asc.png');
+  $variables['arrow_desc'] = file_create_url('/images/arrow-desc.png');

Any chance you can confirm this actually works? I'd assume you need to still use the drupal_get_path('theme', 'seven') prefix in there, but I could be wrong.

The drupal_get_path() call is still needed; without it, the tablesort indicators do not display properly.

lol,

Failed to run tests: @reason.

128: 1987424-twig-seven-theme-128.patch queued for re-testing.

Re-rolled, but this fails at the Drupal installation test. Need to do a trace to determine the issue here.

Thanks for the re-roll @mark.labrecque, here are a couple notes though in general the re-roll looks great. I think it's the duplicate use statement.

1. +++ b/core/themes/seven/seven.theme
   @@ -8,6 +8,7 @@
    use Drupal\Component\Utility\Xss;
   ...
   +use Drupal\Component\Utility\Xss;
   

   This is now a duplicate, likely due to the patch getting in that did the remainder of these. Remove one 'use' statement.

2. +++ b/core/themes/seven/seven.theme
   @@ -113,104 +114,72 @@ function seven_menu_local_task($variables) {
   +  } else {
   

   Just noticed this else is not to code standards. The word else should start on the next line as it did before.

Thanks for spotting that @joelpittet. I will try that.

Let's try this...

Awesome could you post the interdiff as well? Docs here https://drupal.org/documentation/git/interdiff
Or the bottom of this little reference. http://pittet.ca/drupal/sprint/patch

+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+  } ¶

Somehow a an extra space got added to the end of this line. You can make your editor strip whitespace from the end of lines.
If you are using sublime add these two lines to your settings:

"trim_trailing_white_space_on_save": true,
"ensure_newline_at_eof_on_save": true,

Made changes as suggested by joelpittet.

Patch attached.

Marking as needs review.

And this is the right interdiff.

+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+      $variables['types'][$type->type]['url'] = url('node/add/' . $type->type);

Sorry to bring up the routing police again, but shouldn't this be changed to the route name?

\Drupal::url('node.add', array('node_type' => $type->type));

Otherwise, the code looks good. I'm also attaching a before/after diff on the themed output, which looked fine to me.

I went ahead and made the change so we can try and get this through at DrupalCon.

I ran a before/after diff on the output again after applying the last patch. I'm attaching the results.

What you'll see is that the only difference (other than the twig debug output) is that the tablesort indicator is now missing

 typeof="foaf:Image"

@joelpittet suggests that we note it and ask @scor if that is an issue.

Also, we noted that there is a difference between how system.module and seven are rendering the tablesort indicator.

The implementation of the twig template at core/modules/system/templates/tablesort-indicator.html.twig might benefit from the approach taken here in seven.

Don't worry about typeof="foaf:Image", it's printed as a side effect of the way we render images. It's perfectly fine if it is no longer there, since those images are just from the presentation layer (not the actual data being displayed).

+++ b/core/themes/seven/seven.theme
@@ -113,104 +113,72 @@ function seven_menu_local_task($variables) {
+ * Implements hook_preprocess_HOOK() for custom block add list templates.
+ *
+ * Add variables for the label and the path separately.

Any reason why this one is not at the third person? ("Adds ...")