Convert page.tpl.php to Twig

#1: convert-page.tpl_.php-twig-1961870.patch queued for re-testing.

Re-rolling the patch.

OK, let's try this again.

Hi @Cottser, why it failed the SimpleTest?

I am going to work on this task.

As per Cotter's #17 comment, I have created bartik - page.html.twig.

Fixed white spaces issue.

Cottser - thanks! I have combined patch from #15 with bartik page.html.twig

system's page.html.twig
Minor docs cleanup:
1) always?

 * - base_path: The base URL path of the Drupal installation. At the very
 *   least, this will always default to /.

"At the very least" followed by "always"? what does that even mean? How about:

 * - base_path: The base URL path of the Drupal installation. Will usually 
 *    be / unless you have installed drupal in a sub-directory.

2) extra comma

 * - front_page: The URL of the front page. Use this instead of base_path,
 *   when linking to the front page. This includes the language domain or
 *   prefix.

should be

 * - front_page: The URL of the front page. Use this instead of base_path
 *   when linking to the front page. This includes the language domain or
 *   prefix.

3) theme settings

 * - logo: The path to the logo image, as defined in theme configuration.
 * - site_name: The name of the site, empty when display has been disabled
 *   in theme settings.
 * - site_slogan: The slogan of the site, empty when display has been disabled
 *   in theme settings.

twice we refer to 'theme settings' and once 'theme configuration' how about settings everywhere?

 * - logo: The path to the logo image, as defined in theme settings.
 * - site_name: The name of the site, empty when display has been disabled
 *   in theme settings.
 * - site_slogan: The slogan of the site, empty when display has been disabled
 *   in theme settings.

4) all the things we output are HTML!

 * - messages: HTML for status and error messages. Should be displayed
 *   prominently.

how about just

 * - messages: Status and error messages. Should be displayed prominently.

5) entity?!?

 * - node: The node entity, if there is an automatically-loaded node
 *   associated with the page, and the node ID is the second argument
 *   in the page's path (e.g. node/12345 and node/12345/revisions, but not
 *   comment/reply/12345).

how about (note the comma removed after 'page')

 * - node: fully loaded node, if there is an automatically-loaded node
 *   associated with the page and the node ID is the second argument
 *   in the page's path (e.g. node/12345 and node/12345/revisions, but not
 *   comment/reply/12345).

6) we can remove all these

{#
  @todo This is rendered in template_preprocess_page, but we should
    call the final menu Twig template here.
#}

this problem has been solved by renderable everhtings.

Bartik's page.html.twig
7) we need to make the same docs changes as above in this template. It looks like the only significant difference here is the regions?

8) we can remove this twig comment from this template as well

{#
  @todo This is rendered in template_preprocess_page, but we should
  call the final menu Twig template here.
#}

9) two line breaks

  </div></header> <!-- /.section, /#header-->


  {% if messages %}

we probably only need one

  </div></header> <!-- /.section, /#header-->

  {% if messages %}

that's all I can find...

Doesn't the bartik bit go over here #1938840: bartik.theme - Convert PHPTemplate templates to Twig?

#32 - I was asking if we should cut and paste it into the other issue?

nm - just saw #15

Add conversion summary table

This looks good to me! I love the fact the the render arrays for menu's have been moved to theme.inc. This makes our page template so much cleaner.

Anything left here before RTBC?

+ * Custom theme implementation to display a single Drupal page without
+ * sidebars. The sidebars are hidden by regions_hidden for this theme, so
+ * the default page.html.twig will not work without throwing exceptions.

This actually doesn't appear to be true any more. All my tests for the block module are passing locally after I deleted core/modules/block/tests/themes/block_test_theme/page.html.twig

Could we try a patch with that file deleted? see if the d.o. testbots agree with my local test runs.

I can confirm what @thedavidmeister says regarding the block tests passing without core/modules/block/tests/themes/block_test_theme/page.html.twig in place. Here is a re-roll #30 with without this template. We'll see if the testbot has the same results as it does locally.

tagging.

+ * - logo: The path to the logo image, as defined in theme settings.

Path or URL? it's being used in a src attribute so it's probably a URL.

I'm happy other than this.

@thedavidmeister is correct. The logo variable is the fully qualified url of the logo image. For instance: http://yourdrupal.site/core/themes/bartik/logo.png in a default install. Here is a patch that corrects this doc comment.

RTBC?

eh, keep forgetting to tag for review.

A few minor documentation tweaks

+ * Preprocess variables for the page template.

Shouldn't this be "Prepares variables for page templates."?

+ * - site_name: The name of the site, empty when displing the site name has been
+ *   disabled in theme settings.

"displing" is not a word.

Fixes #43 and #44

#47 looks good overall, attached some nits, mostly:

* Documentation clean-up and consistency between bartik and core
* Removal of theme()
* Some whitespace and HTML comment fixes

Assuming these seem good, this is RTBC in my view.

Also, I should mention I elected to remove the note about "directory" in the page.html.twig docblock, as it doesn't seem really that useful. I can't think of a compelling reason to leave it in the docblock, and there are other variables present (db_is_active, user, language) that don't necessitate a mention in the docblock.

Having breadcrumbs as a render array apparently makes that test fail. We can investigate rewriting the test, but if it seems daunting maybe we leave in theme() and provide a follow-up we can tackle later.

+    $variables['breadcrumb'] = array(
+      '#theme' => 'breadcrumb',
+      '#breadcrumb' => drupal_get_breadcrumb()
+    );

Also, if you get that render array working, it needs a trailing comma on the last line.

Hmm, passes locally when applied with #1938848: seven.theme - Convert PHPTemplate templates to Twig re-testing. If it fails again, I would say maybe consider rolling those changes into this issue.

#48: drupal-1961870-48.patch queued for re-testing.

Still missing trailing comma:

+    $variables['breadcrumb'] = array(
+      '#theme' => 'breadcrumb',
+      '#breadcrumb' => drupal_get_breadcrumb()
+    );

Update conversion status - Bartik's page.tpl.php has been converted

We're missing documentation for every variable used in the Seven Twig template.

Still needs vars @no_angel but here's the re-roll.

sorry all... I will change to unassigned for now.

Now with added documentation

I'm happy with that then :)

+++ b/core/includes/theme.incundefined
@@ -2894,6 +2895,36 @@ function template_preprocess_page(&$variables) {
+  // Pass the main menu and secondary menu to the template as render arrays.
+  if (!empty($variables['main_menu'])) {
+    $variables['main_menu'] = array(
+      '#theme' =>'links__system_main_menu',
+      '#links' => $variables['main_menu'],
+      '#attributes' => array(
+        'id' => 'main-menu',
+        'class' => array('links', 'inline', 'clearfix'),
+      ),
+      '#heading' => array(
+        'text' => t('Main menu'),
+        'class' => array('element-invisible'),
+      )
+    );
+  }
+  if (!empty($variables['secondary_menu'])) {
+    $variables['secondary_menu'] = array(
+      '#theme' =>'links__system_secondary_menu',
+      '#links' => $variables['secondary_menu'],
+      '#attributes' => array(
+        'id' => 'secondary-menu',
+        'class' => array('links', 'inline', 'clearfix'),
+      ),
+      '#heading' => array(
+        'text' => t('Secondary menu'),
+        'class' => array('element-invisible'),
+      )
+    );

@@ -2919,7 +2949,10 @@ function template_process_page(&$variables) {
-    $variables['breadcrumb'] = theme('breadcrumb', array('breadcrumb' => drupal_get_breadcrumb()));
+    $variables['breadcrumb'] = array(
+      '#theme' => 'breadcrumb',
+      '#breadcrumb' => drupal_get_breadcrumb(),
+    );

@@ -2928,7 +2961,7 @@ function template_process_page(&$variables) {
-    $variables['messages'] = $variables['show_messages'] ? theme('status_messages') : '';
+    $variables['messages'] = $variables['show_messages'] ? array('#theme' => 'status_messages') : '';

Do we have to do this on this issue? The increase in cpu time of 1.5ms is quite large compared to other twig conversions... plus there are plans to convert main_menu and secondary_menu to blocks... see #1869476: Convert global menus (primary links, secondary links) into blocks

Did not mean for the novice tag to come back...

#71: Two things: CPU time is not really relevant, wt is.

1.1 ms is still slower.

In general I would agree that we do not need to do this here. However, the benchmark shows that the preprocess overhead is roughly the same, process_page is better (200 ms) and in rough template overhead the overhead of the twig template is minimally higher (239 ms) than the page.tpl.php.

-200 + 239 == 39 ms slower => within measuring range

Therefore I cannot recognize where the slowdown of 1.1 ms could come from.

Perhaps we need to re-profile?

Seems comparable to me too... especially when you see that the fluctuation in the cpu time in the 8.x...8.x baseline as well.

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

Updated issue summary.

Committed 684aaa5 and pushed to 8.x. Thanks!