Updated: Comment #78

Problem/Motivation

There are currently two .tpl.php files in core and they are only used for automated tests that ensure PHPTemplate still works. Everything else has been converted to .html.twig.

The Twig conversion patches committed as part of #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch got rid of a lot of references to .tpl.php but not everything.

Additionally, some preprocess functions should be updated to comply with Template preprocess functions documentation directives.

Proposed resolution

  1. Create a patch that updates all relevant occurrences (in documentation) of .tpl.php to .html.twig. There is a separate issue for the non-documentation Views changes: #2049209: Remove 'Theming information' feature from Views in favor of twig_debug
  2. Convert specific tests like HtmlTplPHPAttributesTest to more generic tests, for example HtmlAttributesTest (see #36 and #38).

Example of what we need to update, from default.settings.php:

/**
 * A custom theme can be set for the offline page. This applies when the site
 * is explicitly set to maintenance mode through the administration page or when
 * the database is inactive due to an error. It can be set through the
 * 'maintenance_theme' key. The template file should also be copied into the
 * theme. It is located inside 'core/modules/system/maintenance-page.tpl.php'.
 * Note: This setting does not apply to installation and update pages.
 */
# $conf['maintenance_theme'] = 'bartik';

Remaining tasks

Patch needs to be reviewed and tested to make sure everything was covered (see #78).

User interface changes

n/a

API changes

n/a

#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
#1757550: [Meta] Convert core theme functions to Twig templates
Simultaneous effort to fix things in smaller patches by chris_hall_hu_cheng:
#2098399: Seven commenting has a number of references to .tpl.php files
#2095109: Comment for html.html.twig in Bartik theme still refers to html.tpl.php

CommentFileSizeAuthor
#114 2049207-114.patch696 bytesLinL
#104 drupal8.documentation.2049207-104.patch1.56 KBdlu
#97 drupal8.documentation.2049207-96.patch33.29 KBdlu
#89 drupal8.documentation.2049207-89.interdiff.txt514 bytesdlu
#89 drupal8.documentation.2049207-89.patch33.28 KBdlu
#86 drupal8.documentation.2049207-86.interdiff.txt898 bytesdlu
#86 drupal8.documentation.2049207-86.patch33.27 KBdlu
#82 drupal8.documentation.2049207-82.interdiff.txt1.34 KBdlu
#82 drupal8.documentation.2049207-82.patch33.41 KBdlu
#81 drupal8.documentation.2049207-81.patch33.27 KBdlu
#79 drupal8.documentation.2049207-79.interdiff.txt740 bytesdlu
#79 drupal8.documentation.2049207-79.patch34.23 KBdlu
#78 drupal8.documentation.2049207-78.patch32.66 KBModerate
#76 drupal8.documentation.2049207-76.patch33.5 KBramlev
#71 drupal8.documentation.2049207-71.patch34.48 KBdlu
#71 drupal8.documentation.2049207-71.interdiff.txt1.13 KBdlu
#68 drupal8.documentation.2049207-68.interdiff.txt1.46 KBdlu
#68 drupal8.documentation.2049207-68.patch33.65 KBdlu
#67 drupal8.documentation.2049207-67.patch32.78 KBdlu
#65 drupal8.documentation.2049207-65.interdiff.txt836 bytesdlu
#65 drupal8.documentation.2049207-65.patch33.58 KBdlu
#63 drupal8.documentation.2049207-63.interdiff.txt766 bytesdlu
#63 drupal8.documentation.2049207-63.patch33.93 KBdlu
#61 drupal8.documentation.2049207-61.interdiff.txt1.16 KBdlu
#61 drupal8.documentation.2049207-61.patch33.93 KBdlu
#57 drupal8.documentation.2049207-57.interdiff.txt1.93 KBdlu
#57 drupal8.documentation.2049207-57.patch686.02 KBdlu
#49 drupal8.documentation.2049207-49.interdiff.txt771 bytesdlu
#49 drupal8.documentation.2049207-49.patch33.21 KBdlu
#47 drupal8.documentation.2049207-47.patch34.22 KBdlu
#47 drupal8.documentation.2049207-47.interdiff.txt2.07 KBdlu
#44 drupal8.documentation.2049207-43.patch34.38 KBdlu
#43 drupal8.documentation.2049207-43.interdiff.txt1.62 KBdlu
#43 drupal8.documentation.2049207-43.patch0 bytesdlu
#42 drupal8.documentation.2049207-42.patch34.44 KBdlu
#33 2049207-33-replace-tpl-with-twig-in-documentation.patch33.05 KBjan.stoeckler
#33 interdiff-32-33.txt1.62 KBjan.stoeckler
#32 2049207-32-replace-tpl-with-twig-in-documentation.patch32.09 KBjan.stoeckler
#32 interdiff-30-32.txt1.35 KBjan.stoeckler
#30 2049207-30-replace-tpl-with-twig-in-documentation.patch32.12 KBjan.stoeckler
#30 interdiff-25-30.txt597 bytesjan.stoeckler
#25 2049207-25-tpl-php-documentation-cleanup.patch33.06 KBjan.stoeckler
#25 interdiff-23-25.txt955 bytesjan.stoeckler
#23 2049207-23-tpl.php_.twig_.patch33.44 KBjan.stoeckler
#23 interdiff-21-23.txt2.63 KBjan.stoeckler
#21 2049207_tpl_php_documentation_cleanup_8.patch34.37 KBblakehall
#21 interdiff-2049207-7-8.txt1.49 KBblakehall
#19 2049207_tpl_php_documentation_cleanup_7.patch34.15 KBblakehall
#19 interdiff-2049207-6-7.txt7.41 KBblakehall
#15 2049207_tpl_php_documentation_cleanup_6.patch34.65 KBblakehall
#15 interdiff-2049207-5-6.txt18.77 KBblakehall
#13 2049207_tpl_php_documentation_cleanup_5.patch24.01 KBblakehall
#13 interdiff-2049207-4-5.txt922 bytesblakehall
#9 2049207_tpl_php_documentation_cleanup_4.patch23.91 KBblakehall
#9 interdiff-3-4.txt773 bytesblakehall
#7 2049207_tpl_php_documentation_cleanup_3.patch23.72 KBblakehall
#7 interdiff-2049207--2-3.txt1.99 KBblakehall
#5 2049207_tpl_php_documentation_cleanup_2.patch22.68 KBblakehall
#3 2049207_tpl_php_documentation_cleanup_1.patch25.85 KBblakehall
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

I did a quick grep through 8.x and there were 64 hits. A few of them are in HtmlTplPhpAttributesTest.php and a couple are in binary files. I'm not sure about HtmlTplPhpAttributesTest -- is that one of those cases where it is actually testing a tpl.php? If not, probably the name of that test should be changed too, and if it is actually still testing a tpl.php, then this should be left alone.

All the rest probably need attention... Anyway it looks like around 55-60 lines need changing, so let's try doing them all in one patch.

star-szr’s picture

Thanks @jhodgdon :) I think HtmlTplPhpAttributesTest can safely be renamed to something like HtmlAttributesTest, it's actually testing html.html.twig.

blakehall’s picture

Status: Active » Needs review
FileSize
25.85 KB

Here's a first stab at the documentation cleanup. Hopefully I've caught everything.

star-szr’s picture

Status: Needs review » Needs work

Thanks @blakehall! I reviewed the patch via Dreditor and git diff --color-words, looks like a great start. An observation and a couple things that need work below:

+++ b/core/modules/user/user.moduleundefined
@@ -1421,7 +1421,7 @@ function user_view_page($account) {
- * To theme user profiles, copy modules/user/user.tpl.php
+ * To theme user profiles, copy core/modules/user/templates/user.html.twig

Nice! :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -1886,7 +1886,7 @@ public function buildOptionsForm(&$form, &$form_state) {
-          $output .= '<pre>' . check_plain(file_get_contents('./' . $this->definition['theme_path'] . '/' . strtr($this->definition['theme'], '_', '-') . '.tpl.php')) . '</pre>';
+          $output .= '<pre>' . check_plain(file_get_contents('./' . $this->definition['theme_path'] . '/' . strtr($this->definition['theme'], '_', '-') . '.html.twig')) . '</pre>';
         }
 
         $form['analysis'] = array(
@@ -1906,7 +1906,7 @@ public function buildOptionsForm(&$form, &$form_state) {

@@ -1906,7 +1906,7 @@ public function buildOptionsForm(&$form, &$form_state) {
         }
         else {
           $output .= '<p>' . t('This is the default theme template used for this style.') . '</p>';
-          $output .= '<pre>' . check_plain(file_get_contents('./' . $plugin->definition['theme_path'] . '/' . strtr($plugin->definition['theme'], '_', '-') . '.tpl.php')) . '</pre>';
+          $output .= '<pre>' . check_plain(file_get_contents('./' . $plugin->definition['theme_path'] . '/' . strtr($plugin->definition['theme'], '_', '-') . '.html.twig')) . '</pre>';
         }
 
         $form['analysis'] = array(
@@ -1926,7 +1926,7 @@ public function buildOptionsForm(&$form, &$form_state) {

@@ -1926,7 +1926,7 @@ public function buildOptionsForm(&$form, &$form_state) {
         }
         else {
           $output .= '<p>' . t('This is the default theme template used for this row style.') . '</p>';
-          $output .= '<pre>' . check_plain(file_get_contents('./' . $plugin->definition['theme_path'] . '/' . strtr($plugin->definition['theme'], '_', '-') . '.tpl.php')) . '</pre>';
+          $output .= '<pre>' . check_plain(file_get_contents('./' . $plugin->definition['theme_path'] . '/' . strtr($plugin->definition['theme'], '_', '-') . '.html.twig')) . '</pre>';
         }
 
         $form['analysis'] = array(
@@ -1943,7 +1943,7 @@ public function buildOptionsForm(&$form, &$form_state) {

@@ -1943,7 +1943,7 @@ public function buildOptionsForm(&$form, &$form_state) {
 
         // Field templates aren't registered the normal way...and they're always
         // this one, anyhow.
-        $output .= '<pre>' . check_plain(file_get_contents(drupal_get_path('module', 'views') . '/templates/views-view-field.tpl.php')) . '</pre>';
+        $output .= '<pre>' . check_plain(file_get_contents(drupal_get_path('module', 'views') . '/templates/views-view-field.html.twig')) . '</pre>';

Please leave these changes for #2049209: Remove 'Theming information' feature from Views in favor of twig_debug, they are functional changes and need a bit more thought.

+++ b/core/modules/views/views.moduleundefined
@@ -475,7 +475,7 @@ function views_preprocess_html(&$variables) {
-  // page.tpl.php, so we can only find it using JavaScript. We therefore remove
+  // page.html.twig, so we can only find it using JavaScript. We therefore remove

+++ b/core/themes/seven/seven.themeundefined
@@ -11,7 +11,7 @@ use Drupal\Core\Template\RenderWrapper;
-  // While markup for normal pages is split into page.tpl.php and html.tpl.php,
+  // While markup for normal pages is split into page.html.twig and html.html.twig,

These comment lines are now too long, please wrap to as long as possible within 80 characters per http://drupal.org/node/1354#drupal.

blakehall’s picture

Status: Needs work » Needs review
FileSize
22.68 KB

Here's another go...

star-szr’s picture

Status: Needs review » Needs work

Thanks @blakehall! Looking better, I manually diffed the two patches but interdiffs would be helpful here too :)

We lost the change to default.settings.php - that should be brought back. I'd also like us to rename the HtmlTplPhpAttributesTest class in this issue. When updating the @file docblock please use the newer 'Contains \Drupal…' style, see https://drupal.org/node/1354#file.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.phpundefined
@@ -10,7 +10,7 @@
 class HtmlTplPhpAttributesTest extends WebTestBase {
blakehall’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
23.72 KB

Thank you @Cottser for your patience :)

Here's a new patch changing HtmlTplPhpAttributesTest to HtmlTwigAttributesTest and also includes the default.settings.php changes as well as an interdiff.

star-szr’s picture

Status: Needs review » Needs work

Two more small things:

--- a/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.php

Rename the HtmlTplPhpAttributesTest.php file to match the class name (per PSR-0).

+ * Contains Drupal\system\Tests\Theme\HtmlTwigAttributesTest.

Contains \Drupal (need to add the leading backslash before 'Drupal')

Other than that I think this is ready to go!

blakehall’s picture

Status: Needs work » Needs review
FileSize
773 bytes
23.91 KB

Incorporating #8.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Functions like function template_preprocess_region(&$variables) are not even supposed to have .html.twig in their one-line descriptions. See https://drupal.org/node/1354#themepreprocess -- template_preprocess_foo() functions should say "Prepares variables for foo templates" and the hook_preprocess_HOOK functions for hook "foo" should say "Implements hook_preprocess_HOOK for the foo template".

Also, just as a note, after applying this patch I grepped for tpl.php and aside from Views (being handled elsewhere), I think this patch caught everything.

And I looked at the test results page and verified that the newly renamed HtmlTwigAttributesTest.php file is being run, so that is good doo.

star-szr’s picture

@jhodgdon - great catch, I wasn't thinking about that! So it sounds like the preprocess functions being updated should use the new standards.

blakehall’s picture

Status: Needs work » Needs review
FileSize
922 bytes
24.01 KB

Looking through theme.inc it looked to me like the only documentation that was inconsistent was template_preprocess_region.

If I missed any others let me know and I'll reroll.

jhodgdon’s picture

Status: Needs review » Needs work

These types of lines also need an update:

+++ b/core/modules/edit/edit.module
@@ -159,7 +159,7 @@ function edit_field_formatter_info_alter(&$info) {
 }
 
 /**
- * Implements hook_preprocess_HOOK() for field.tpl.php.
+ * Implements hook_preprocess_HOOK() for field.html.twig.
  */
 function edit_preprocess_field(&$variables) {

Should say "Implements hook_preprocess_HOOK() for field templates."

Also, by convention we do not need to document $variables for either template_preprocess() or hook_preprocess_HOOK() functions.

blakehall’s picture

Status: Needs work » Needs review
FileSize
18.77 KB
34.65 KB

Thanks for the guidance @jhodgdon.

Here's another go.

star-szr’s picture

Status: Needs review » Needs work

Coming along, thanks @blakehall! Feedback on the latest patch:

+++ b/core/includes/theme.incundefined
@@ -3001,13 +3001,17 @@ function template_preprocess_install_page(&$variables) {
+ * @param array $variables
+ *  An associative array containing:
+ *    - elements: An associative array containing properties of the region.

Indent is off by one space here - the 'An associative array' line and the following line need to be indented by one more space.

EDIT: The associative array line needs to be indented by one and the elements line needs to be outdented by one, per http://drupal.org/node/1354#lists

+++ b/core/modules/node/node.api.phpundefined
@@ -824,7 +824,7 @@ function hook_node_view(\Drupal\Core\Entity\EntityInterface $node, \Drupal\entit
- * node.html.twig. See drupal_render() and theme() documentation respectively
+ * node templates. See drupal_render() and theme() documentation respectively

+++ b/core/modules/taxonomy/taxonomy.api.phpundefined
@@ -282,7 +282,7 @@ function hook_taxonomy_term_view(\Drupal\taxonomy\Plugin\Core\Entity\Term $term,
- * hook_preprocess_HOOK() for taxonomy-term.tpl.php. See drupal_render() and
+ * hook_preprocess_HOOK() for taxonomy term templates. See drupal_render() and

+++ b/core/modules/user/user.api.phpundefined
@@ -347,7 +347,7 @@ function hook_user_view(\Drupal\user\UserInterface $account, \Drupal\entity\Plug
- * user.tpl.php. See drupal_render() and theme() documentation
+ * user templates. See drupal_render() and theme() documentation

I would leave these comments as referring to the specific .html.twig file, they will end up as links to the template files on api.d.o which will be more useful.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -340,7 +340,7 @@ function overlay_system_info_alter(&$info, $file, $type) {
+ * Implements hook_preprocess_HOOK() for the html template.

@@ -355,7 +355,7 @@ function overlay_preprocess_html(&$variables) {
+ * Implements hook_preprocess_HOOK() for the maintenance page template.

+++ b/core/modules/rdf/rdf.moduleundefined
@@ -226,7 +226,7 @@ function rdf_theme() {
+ * Implements hook_preprocess_HOOK() for the html template.

+++ b/core/modules/system/tests/modules/theme_test/theme_test.moduleundefined
@@ -150,7 +150,7 @@ function _theme_test_suggestion() {
+ * Implements hook_preprocess_HOOK() for html template.

+++ b/core/modules/views/tests/modules/views_test_data/views_test_data.moduleundefined
@@ -56,7 +56,7 @@ function views_test_data_handler_test_access_callback_argument($argument = FALSE
+ * Implements hook_preprocess_HOOK() for views-view-table templates.

+++ b/core/modules/views_ui/views_ui.moduleundefined
@@ -212,7 +212,7 @@ function views_ui_library_info() {
+ * Implements hook_preprocess_HOOK() for views-view templates.

+++ b/core/themes/bartik/bartik.themeundefined
@@ -6,7 +6,7 @@
+ * Implements hook_preprocess_HOOK() for html templates.

@@ -73,7 +73,7 @@ function bartik_preprocess_page(&$variables) {
+ * Implements hook_preprocess_HOOK() for maintenance-page templates.

+++ b/core/themes/seven/seven.themeundefined
@@ -8,11 +8,11 @@
+ * Implements hook_preprocess_HOOK() for maintenance-page templates.

@@ -20,14 +20,14 @@ function seven_preprocess_maintenance_page(&$variables) {
+ * Implements hook_preprocess_HOOK() for html templates.

These ones (maybe missed a couple) are a bit off - always "templates" at the end (plural), and I would refer to the base preprocess to see how the template are referred to. e.g. template_preprocess_html() refers to "HTML document templates". So "maintenance-page" should not be used for example.

jhodgdon’s picture

Um... Just to clarify, on lists, it should be:

 * @param $whatever
 *   An associative array containing:
 *   - First thing, which contains:
 *     - Another thing.

The - goes right under the start of the line above. The description for @param is indented by two spaces from the @.

Other comments from Cottser: spot on. :)

star-szr’s picture

Yeah, I corrected myself after looking at my comment, thanks @jhodgdon :)

blakehall’s picture

Status: Needs work » Needs review
FileSize
7.41 KB
34.15 KB

Another go...

star-szr’s picture

Status: Needs review » Needs work

Nice, looking much better @blakehall! A couple things I spotted with the latest patch:

+++ b/core/includes/theme.incundefined
@@ -2994,13 +2994,17 @@ function template_preprocess_install_page(&$variables) {
+ * @param array $variables
+ *  An associative array containing:
+ *  - elements: An associative array containing properties of the region.

This is closer but the two lines under the @param need to be indented by another space, see @jhodgdon's example for what it should look like.

+++ b/core/modules/system/tests/modules/theme_test/theme_test.moduleundefined
@@ -177,7 +177,7 @@ function theme_theme_test_function_template_override($variables) {
 /**
- * Process variables for theme-test-render-element.tpl.php.
+ * Process variables for theme-test-render-element.html.twig.
  */
 function template_preprocess_theme_test_render_element(&$variables) {

I know this is only in a test module but this docblock should be formatted like https://drupal.org/node/1354#themepreprocess with the "Prepares" line, default template, and @param.

blakehall’s picture

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

Status: Needs review » Needs work

Those changes look great! I missed these:

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -365,12 +365,12 @@ function overlay_preprocess_maintenance_page(&$variables) {
 /**
- * Implements template_preprocess_HOOK() for overlay.tpl.php
+ * Implements template_preprocess_HOOK() for overlay templates.
  *
  * If the current page request is inside the overlay, add appropriate classes
  * to the <body> element, and simplify the page title.
  *
- * @see overlay.tpl.php
+ * @see overlay.html.twig
  */
 function template_preprocess_overlay(&$variables) {

This preprocess function docblock should be given the full http://drupal.org/node/1354#themepreprocess treatment like the one in the theme_test module.

+++ b/core/modules/block/block.api.phpundefined
@@ -20,7 +20,7 @@
- * block.html.twig. See drupal_render() and theme() documentation respectively
+ * block templates. See drupal_render() and theme() documentation respectively

+++ b/core/modules/comment/comment.api.phpundefined
@@ -113,7 +113,7 @@ function hook_comment_view(\Drupal\comment\Plugin\Core\Entity\Comment $comment,
- * comment.html.twig. See drupal_render() documentation for details.
+ * comment templates. See drupal_render() documentation for details.

These lines should be left as is per the second point in #16.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
33.44 KB

I tried to incorporate the changes proposed by Cottser in #22.

To document the preprocess variables, i looked at the return value documentation of the relevant functions.

This is my first time uploading a patch, so I'm sorry if I did anything incorrectly.

star-szr’s picture

Status: Needs review » Needs work

Thanks @janstoeckler! Great work on your first patch I might add :)

The rolled back lines look good. The only thing that I can see that needs work now is the @param documentation for template_preprocess_overlay(). All those variables are actually built in template_preprocess_overlay(), not passed in. The param passed in to the preprocess function is a render element of 'page', this is defined in overlay_theme().

So for the @param I think we can reuse the documentation from template_preprocess_html():

 * @param array $variables
 *   An associative array containing:
 *   - page: A render element representing the page.
jan.stoeckler’s picture

Thanks for your response, is that better?

jan.stoeckler’s picture

Status: Needs work » Needs review

sorry, forgot to change status.

yanniboi’s picture

I've had a quick look at #25. Installed 8.x and patched it. Applied cleanly with no whitespace and no errors, and clean log messages.

star-szr’s picture

@janstoeckler - that looks good to me! I will give this another review in the evening.

star-szr’s picture

Status: Needs review » Needs work

So close! After looking at template_preprocess_theme_test_render_element() the docblock there is not quite right.

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -177,7 +177,14 @@ function theme_theme_test_function_template_override($variables) {
+ * Prepares variables for test render element templates.
+ *
+ * Default template: theme-test-render-element.html.twig.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - attributes: An associative array containing the properties of the
+ *     element.
  */
 function template_preprocess_theme_test_render_element(&$variables) {

The @param is a bit off - this preprocess function adds to the attributes array but is passed an 'elements' render array - see theme_test_theme():

  $items['theme_test_render_element'] = array(
    'render element' => 'elements',
    'template' => 'theme-test-render-element',
  );

I think something like this should be fine for the docblock (grabbed from template_preprocess_block()):

- elements: An associative array containing the properties of the element.

Edit: So just change 'attributes' to 'elements' and the whole thing should be able to fit on one line as well.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
597 bytes
32.12 KB

I'm hoping I understood your instruction correctly.

star-szr’s picture

Status: Needs review » Needs work

@janstoeckler - Right idea but wrong function. Interdiffs are great here so thanks for adding them :). The overlay preprocess documentation was great in the #25 patch but is a bit off now (we should change it back). We need to update the documentation for the preprocess function in theme_test.module.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
32.09 KB

Maybe this time :-)

jan.stoeckler’s picture

Let's try that again, I forgot to git rm core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.php in the previous patch.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Went through the whole patch again, looks great to me. Thanks for all your hard work on this @blakehall and @janstoeckler :)

webchick’s picture

Assigned: Unassigned » jhodgdon

Since there's quite a bit here, this could probably benefit from jhodgdon's ever-watchful eye.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

I've been lurking and watching Cottser doing an excellent job of reviewing/mentoring in this patch, so I've mostly stayed out of the discussion. :)

So, I think the current patch is mostly fine.

I'm a bit concerned about this one change though:

+++ b/core/modules/system/lib/Drupal/system/Tests/FileTransfer/FileTransferTest.php
@@ -37,7 +37,7 @@ function _getFakeModuleFiles() {
       'fake.module',
       'fake.info.yml',
       'theme' => array(
-        'fake.tpl.php'
+        'fake.html.twig'

What does this test/method do, and should this change have actually been made?

Also, I thought in changing the HtmlTplPhpAttributesTest class, we were going to take out all references to either tpl.php or html.twig, and make it be something like HtmlThemeAttributesTest? That would be more in line with our current philosophy of saying "Processes variables for foo templates" rather than "processes variables for foo.html.twig templates" I think? So maybe that class's docs should be redone? Really it is not specific to using PHP or Twig templates -- it is just testing whether the preprocess function results in the attributes getting into the HTML for the page, right?

I'm willing to be overruled on either of these points though, so I've set the status back to "needs review" rather than "needs work".

star-szr’s picture

Assigned: jhodgdon » Unassigned
Status: Needs review » Needs work

I think the update to the file test is justifiable. The array of files is used to build out a directory and test copying it. The _getFakeModuleFiles() method was updated previously with the change to .info.yml files, so I think it's fair to update to .html.twig since that is the default. It's just an example in a test class and the tests pass, the example files created only contain their filename and aren't actually used beyond testing where the directory can be copied. See the testJail() method.

  function _buildFakeModule() {
    $location = 'temporary://fake';
    if (is_dir($location)) {
      $ret = 0;
      $output = array();
      exec('rm -Rf ' . escapeshellarg($location), $output, $ret);
      if ($ret != 0) {
        throw new Exception('Error removing fake module directory.');
      }
    }

    $files = $this->_getFakeModuleFiles();
    $this->_writeDirectory($location, $files);
    return $location;
  }

  function _writeDirectory($base, $files = array()) {
    mkdir($base);
    foreach ($files as $key => $file) {
      if (is_array($file)) {
        $this->_writeDirectory($base . DIRECTORY_SEPARATOR . $key, $file);
      }
      else {
        //just write the filename into the file
        file_put_contents($base . DIRECTORY_SEPARATOR . $file, $file);
      }
    }
  }

However, looking again at HtmlTplPhpAttributesTest I do agree, we can make it generic. Just need to update the documentation and assertions to remove mention of .tpl.php OR .html.twig and rename the class.

So needs work to make HtmlTplPhpAttributesTest (in the most recent patch HtmlTwigAttributesTest) generic.

tstoeckler’s picture

Making some concrete suggestions since this is still tagged Novice:

+++ b/core/modules/system/lib/Drupal/system/Tests/FileTransfer/FileTransferTest.php
index d06b165..fb26da6 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.php

--- a/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTplPhpAttributesTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -2,7 +2,7 @@
- * Definition of Drupal\system\Tests\Theme\HtmlTplPhpAttributesTest.
+ * Contains \Drupal\system\Tests\Theme\HtmlTwigAttributesTest.

@@ -10,9 +10,9 @@
-class HtmlTplPhpAttributesTest extends WebTestBase {
+class HtmlTwigAttributesTest extends WebTestBase {

So the file should be renamed to HtmlAttributesTest.php and the class to HtmlAttributesTest

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -10,9 +10,9 @@
- * Functional test for attributes of html.tpl.php.
+ * Functional test for attributes of html.html.twig.

Should be:

Functional test for <html> and <body> attributes.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -23,20 +23,20 @@ class HtmlTplPhpAttributesTest extends WebTestBase {
-      'name' => 'html.tpl.php html and body attributes',
-      'description' => 'Tests attributes inserted in the html and body elements of html.tpl.php.',
+      'name' => 'html.html.twig html and body attributes',
+      'description' => 'Tests attributes inserted in the html and body elements of html.html.twig.',

Should be:

'name' => '<html> and <body> attributes',
'description' => 'Tests attributes inserted in the <html> and <body> elements of the page.',

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -23,20 +23,20 @@ class HtmlTplPhpAttributesTest extends WebTestBase {
-   * Tests that modules and themes can alter variables in html.tpl.php.
+   * Tests that modules and themes can alter variables in html.html.twig.

Should be:

Tests that the variables in the <html> and <body> tags can be altered.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -23,20 +23,20 @@ class HtmlTplPhpAttributesTest extends WebTestBase {
-  function testThemeHtmlTplPhpAttributes() {
+  function testThemeHtmlTwigAttributes() {

The function should be named testThemeHtmlAttributes()

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlTwigAttributesTest.php
@@ -23,20 +23,20 @@ class HtmlTplPhpAttributesTest extends WebTestBase {
-    $this->assertTrue(count($attributes) == 1, 'Attribute set in the html element via hook_preprocess_HOOK() for html.tpl.php found.');
+    $this->assertTrue(count($attributes) == 1, 'Attribute set in the html element via hook_preprocess_HOOK() for html.html.twig found.');
...
-    $this->assertTrue(count($attributes) == 1, 'Attribute set in the body element via hook_preprocess_HOOK() for html.tpl.php found.');
+    $this->assertTrue(count($attributes) == 1, 'Attribute set in the body element via hook_preprocess_HOOK() for html.html.twig found.');

Simply omit the "for html.html.twig"

EDIT: Fixed HTML formatting per below.

star-szr’s picture

@tstoeckler - thanks, can you edit your post to wrap your HTML tags in <code> tags please?

tstoeckler’s picture

Thanks @Cottser, I always forget that...

jenlampton’s picture

Issue tags: +theme system cleanup

tagging so this stays on my radar

dlu’s picture

Assigned: Unassigned » dlu
Status: Needs work » Needs review
FileSize
34.44 KB

Reroll of #33 before starting to work on the comments that follow it.

dlu’s picture

Made changes suggested by tstoeckler in #38.

dlu’s picture

Hmm, try that patch again. Interdiff in #43.

tstoeckler’s picture

The interdiff is incorrect, but the patch looks good.

star-szr’s picture

Status: Needs review » Needs work

Thanks @dlu! I'll mention that it's worth configuring git to handle file renames better, that would make this patch a bit easier to review. You can run `git config --global diff.renames copies` or edit your .gitconfig directly. More details and goodness here: https://drupal.org/documentation/git/configure

That would make it so that the renamed test file in this patch is not shown as being completely deleted and added, it would only show the differences.

As for the patch itself this is looking very good to me, only found a few very minor things to tweak:

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlAttributesTest.php
    @@ -0,0 +1,42 @@
    + * Functional tests for <html> and <body> for attributes.
    

    I think we can remove the second 'for'.

  2. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -481,6 +481,7 @@ Drupal.viewsUi.RearrangeFilterHandler = function ($table, $operator) {
    +
    
    @@ -488,13 +489,14 @@ $.extend(Drupal.viewsUi.RearrangeFilterHandler.prototype, {
    +
    

    Please remove these added blank lines.

  3. +++ b/core/modules/views_ui/js/views-admin.js
    @@ -488,13 +489,14 @@ $.extend(Drupal.viewsUi.RearrangeFilterHandler.prototype, {
    -    // match the action links styling used in a typical page.tpl.php. Note that
    -    // Drupal does not provide a theme function for this markup, so this is the
    -    // best we can do.
    +    // match the action links styling used in a typical page.html.twig.
    +    // Note that Drupal does not provide a theme function for this markup, so
    +    // this is the best we can do.
    

    Definitely nitpicking but 'Note' can fit on the first line here :)

dlu’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
34.22 KB

Incorporated suggests from #46 along with an edit of the comment in item 3 to make it read a bit more smoothly.

star-szr’s picture

Status: Needs review » Needs work

Thanks @dlu! The changes look great. Rewording that comment is a bit out of scope for this issue but I'll let @jhodgdon be the judge of that, it seems reasonable.

This looks RTBC from my perspective other than:

+++ b/core/modules/views_ui/js/views-admin.js
@@ -488,13 +488,14 @@ $.extend(Drupal.viewsUi.RearrangeFilterHandler.prototype, {
+

@@ -756,7 +757,6 @@ $.extend(Drupal.viewsUi.RearrangeFilterHandler.prototype, {
-

A couple stray line insertions/deletions remain in views-admin.js.

dlu’s picture

Status: Needs work » Needs review
FileSize
33.21 KB
771 bytes

Correct patch to eliminate some stray additions/deletions.

star-szr’s picture

Assigned: dlu » jhodgdon
Status: Needs review » Reviewed & tested by the community

That looks good to me, thanks again @dlu!

Assigning to @jhodgdon for another review.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs work

This all looks great to me...

I have only one concern, and that is that html and body tags have been placed un-escaped into the /** */ documentation and/or test getInfo where they weren't before:

 /**
- * Functional test for attributes of html.tpl.php.
+ * Functional tests for <html> and <body> attributes.
  */
-class HtmlTplPhpAttributesTest extends WebTestBase {
+class HtmlAttributesTest extends WebTestBase {
 
   /**
    * Modules to enable.
@@ -23,20 +23,20 @@ class HtmlTplPhpAttributesTest extends WebTestBase {
 
   public static function getInfo() {
     return array(
-      'name' => 'html.tpl.php html and body attributes',
-      'description' => 'Tests attributes inserted in the html and body elements of html.tpl.php.',
+      'name' => '<html> and <body> attributes',
+      'description' => 'Tests attributes inserted in the <html> and <body> elements of the page.',
       'group' => 'Theme',
     );
   }
 
   /**
-   * Tests that modules and themes can alter variables in html.tpl.php.
+   * Tests that the variables in the <html> and <body> tags can be altered.
    */
-  function testThemeHtmlTplPhpAttributes() {
+  function testThemeHtmlAttributes() {

I am not sure, but I think these are all a bad idea. In /** */ docs, HTML meant to be illustrative should be enclosed in @code/@endcode. In getInfo... I'm just not sure what it will do, does it work? Why did this need to be changed from what it was before anyway?

Everything else is good!

star-szr’s picture

I was suspicious about that but unsure, thanks @jhodgdon!

I think it should be fine to do something like this for all these cases:

Tests that the variables in the 'html' and 'body' tags can be altered.

jhodgdon’s picture

I agree with #52. Should be a quick fix and then we can get this patch in. Thanks!

dlu’s picture

Assigned: Unassigned » dlu

Should the change happen in both the comments and the stings in code?

That is, should I also make changes like this:

'name' => '\'html\' and \'body\' attributes',

star-szr’s picture

Yes, but in that case we can double quote the whole string to avoid having to escape the single quotes:

'name' => "'html' and 'body' attributes",

And also in that case it might be helpful to add 'tag' or 'element' to give a little more context:

'name' => "'html' and 'body' tag attributes",

tstoeckler’s picture

Yeah, I introduced the < > brackets because "html attributes" could be attributes on any element/tag, whereas this is specifically about the <html> tag. I wasn't aware of the implications. I like the suggestion in #55, though.

dlu’s picture

Rerolled #49 and made the changes suggested by #51 and #52.

The reroll is significantly bigger than #49, this troubles me – but I'm not sure how to evaluate the changes. Here's what I did:

  1. In preparation for making these changes, I did a 'git reset --hard' and 'git pull' to make sure I was working with current code.
  2. Attempted to apply the patch from #49 – it failed.
  3. Rerolled patch. Git was able to do the rebase without manual intervention.
  4. Made changes and generated an interdiff and patch.
dlu’s picture

Status: Needs work » Needs review

Oops!

Status: Needs review » Needs work

The last submitted patch, drupal8.documentation.2049207-57.patch, failed testing.

star-szr’s picture

Yes, the patch now has many unrelated changes, so #49 needs another reroll (then you can apply the interdiff). You were right to be suspicious about the size. I will review the interdiff for now :)

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlAttributesTest.php
    @@ -10,7 +10,7 @@
    + * Functional tests for 'html' and 'body' attributes.
    
    @@ -23,20 +23,20 @@ class HtmlAttributesTest extends WebTestBase {
    +      'name' => "'html' and 'body' attributes",
    

    I would add 'element' to these ones.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/HtmlAttributesTest.php
    @@ -23,20 +23,20 @@ class HtmlAttributesTest extends WebTestBase {
    -   * Tests that the variables in the <html> and <body> tags can be altered.
    +   * Test that attributes in the 'html' and 'body' elements can be altered.
    

    I think this should stay as plural 'Tests' per http://drupal.org/node/1354#functions.

dlu’s picture

Status: Needs work » Needs review
FileSize
33.93 KB
1.16 KB

Sorted out what I did wrong with git in the #57, rerolled, and incorporated the changes from #60.

Status: Needs review » Needs work

The last submitted patch, drupal8.documentation.2049207-61.patch, failed testing.

dlu’s picture

Status: Needs work » Needs review
FileSize
33.93 KB
766 bytes

Oops, syntax error…

star-szr’s picture

Status: Needs review » Needs work

Thanks @dlu for your continued work here. Another reroll is needed, I think it's just for #1969916: Remove drupal_add_js/css from seven.theme so should be a quick one.

Also I noticed another .tpl.php snuck in (to core/modules/views/lib/Drupal/views/Plugin/views/PluginBase.php) as part of #1911492: Views try to find Custom StylePlugin template in core/modules/views/templates, just needs to be replaced with .html.twig.

Other than that, patch looks great to me!

dlu’s picture

Status: Needs work » Needs review
FileSize
33.58 KB
836 bytes

Did a reroll and fixed reference to .tpl.php mentioned in #64.

star-szr’s picture

Status: Needs review » Needs work

Thanks again @dlu!

So I compared the two patches (via visual diff), we did lose a couple fixes from #63 and I spotted one more docblock that can be fixed up by searching for "Implements hook_preprocess_HOOK() for ".

So these changes should be added back:

diff --git a/core/modules/forum/forum.module b/core/modules/forum/forum.module
index 41d2604..57c7617 100644
--- a/core/modules/forum/forum.module
+++ b/core/modules/forum/forum.module
@@ -947,7 +947,7 @@ function forum_get_topics($tid, $sortby, $forum_per_page) {
 }
 
 /**
- * Implements hook_preprocess_HOOK() for block.html.twig.
+ * Implements hook_preprocess_HOOK() for block templates.
  */
 function forum_preprocess_block(&$variables) {
   if ($variables['configuration']['module'] == 'forum') {
diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme
index 5735f29..7c7d286 100644
--- a/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme
@@ -8,11 +8,11 @@
 use Drupal\Core\Template\RenderWrapper;
 
 /**
- * Implements hook_preprocess_HOOK() for maintenance-page.html.twig.
+ * Implements hook_preprocess_HOOK() for maintenance page templates.
  */
 function seven_preprocess_maintenance_page(&$variables) {

And this is a new one that I spotted that could be brought in line since we are fixing all these anyway:

core/modules/user/user.module
549-    }
550-  }
551-}
552-
553-/**
554: * Implements hook_preprocess_HOOK() for block.html.twig.
555- */
556-function user_preprocess_block(&$variables) {
557-  if ($variables['configuration']['module'] == 'user') {
558-    switch ($variables['elements']['#plugin_id']) {
559-      case 'user_login_block':
dlu’s picture

Reroll of #65 without changes from #66. Intentionally left as 'needs work' since #68 will be a patch and interdiff to implement #66 and this should save the testbot some work.

dlu’s picture

Status: Needs work » Needs review
FileSize
33.65 KB
1.46 KB

Pick up changes from #66 on top of reroll in #67. I think #67 can be ignored, but just in case, it is there.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @dlu, I think this is ready to fly! I reviewed the whole patch again.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

:/

I did the grep for "Implements hook_preprocess_HOOK() for " again and spotted a couple more, then back to RTBC land:

core/modules/book/book.module
666: * Implements hook_preprocess_HOOK() for block.html.twig.
core/modules/system/tests/modules/theme_test/theme_test.module
80: * Implements hook_preprocess_HOOK() for html.tpl.php.
dlu’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
34.48 KB

Updates based on #70.

LinL’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies:

Checking patch core/modules/rdf/rdf.module...
Hunk #1 succeeded at 247 (offset 21 lines).
Hunk #2 succeeded at 262 (offset 21 lines).
error: while searching for:
}

/**
 * Implements hook_preprocess_HOOK() for field.tpl.php.
 */
function rdf_preprocess_field(&$variables) {
  $element = $variables['element'];

error: patch failed: core/modules/rdf/rdf.module:318

Otherwise, looks good.

Gaelan’s picture

Assigned: dlu » Gaelan

Rerolling.

Gaelan’s picture

Issue summary: View changes

Add issue link for Views Theming information fix issue

mradcliffe’s picture

I added links to the two recent issues where simultaneous effort by chris_hall_hu_cheng done to fix the .tpl.php references.

Issue summary is a bit outdated with regard to

  • Proposed resolution - it seems some later patches were missing things so this is something for reviewers to check.
  • Remaining tasks - re-roll, review
jhodgdon’s picture

I don't see where the issue summary is outdated... it looks like the task is correct. Maybe if you see something wrong you can update the summary mradcliffe?

ramlev’s picture

The patch is rerolled to work with core for now.

ramlev’s picture

Status: Needs work » Needs review
Moderate’s picture

Hi,
I tested the patch.

olivier@oli-computer:/var/www/drupal8$ grep -R ".tpl.php" .|grep -v 2049
[...]
./sites/default/default.settings.php: * theme. It is located inside 'core/modules/system/maintenance-page.tpl.php'.
./sites/default/settings.php: * theme. It is located inside 'core/modules/system/maintenance-page.tpl.php'.
./core/modules/system/tests/themes/test_theme_phptemplate/node--1.tpl.php:  // node--1.tpl.php - Dummy file for finding the template
[...]

There were some occurences of .tpl.php in default.settings.php and settings.php.
I edited the last patch.

But i don't know what to do with node--1.tpl.php?

Moderate’s picture

Issue summary: View changes

Added duplicate issues to issue summary to note simultaneous effort done by new core contributor.

dlu’s picture

This patch looks ok to me, but it might be better to split the reroll from the change to default.settings.php, so I reworked #79 so that we have an interdiff by applying #76 and then making the change from #78. Hopefully this is the right thing to do. The only other "issue" I noticed with this patch (really #76) was a change from templates to template (below) not sure if that was intentional or not – templates seems like the right one to me, but I'm OK with either.

299c299
< @@ -226,7 +226,7 @@ function rdf_theme() {
---
> @@ -249,7 +249,7 @@ function rdf_theme() {
304c304
< + * Implements hook_preprocess_HOOK() for HTML document templates.
---
> + * Implements hook_preprocess_HOOK() for HTML document template.

I think the thing to do with node--1.tpl.php is to leave it for now. There doesn't seem to be much point in changing just the comment and from the looks of it, that file may be going away.

As far as I can tell this is ready for RTBC.

star-szr’s picture

Assigned: Gaelan » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch needs another reroll.

node--1.tpl.php is fine because it is used for testing PHPTemplate.

  1. +++ b/core/modules/edit/edit.module
    @@ -161,7 +161,7 @@ function edit_field_formatter_info_alter(&$info) {
    - * Implements hook_preprocess_HOOK() for field.tpl.php.
    + * Implements hook_preprocess_HOOK() for field templates.
    

    We lost this from #71, please add it back. I diffed #71 and #79.

  2. +++ b/core/modules/rdf/rdf.module
    @@ -249,7 +249,7 @@ function rdf_theme() {
    - * Implements hook_preprocess_HOOK() for html.tpl.php.
    + * Implements hook_preprocess_HOOK() for HTML document template.
    

    This should be 'templates' plural as noted by @dlu and according to the standards: http://drupal.org/node/1354#themepreprocess

  3. +++ b/sites/default/default.settings.php
    @@ -578,7 +578,7 @@
    - * theme. It is located inside 'core/modules/system/maintenance-page.tpl.php'.
    + * theme. It is located inside 'core/modules/system/templates/maintenance-page.html.twig'.
    

    This is a great catch while we are updating this line, however the line is now too long and needs to be wrapped at 80 characters per http://drupal.org/node/1354#drupal.

dlu’s picture

Reroll of #79 prior to starting work on #81 – things must be hopping in Prague :-)

Figure the test bot might be busy, so left it as "needs work" (which it does).

dlu’s picture

Assigned: Unassigned » dlu
Status: Needs work » Needs review
FileSize
33.41 KB
1.34 KB

This is a patch based on @Cottser's comments in #81. I can't explain it, but the dropped segment that @Cottser notes as problem #1 isn't dropped in my version… So I didn't make that change. When making change #3 just moving the line break earlier in the line looked ugly, so I rewrote the comment a bit. I'm not sure if that was the right thing to do. Here is the change I made:

  * A custom theme can be set for the offline page. This applies when the site
  * is explicitly set to maintenance mode through the administration page or when
  * the database is inactive due to an error. It can be set through the
- * 'maintenance_theme' key. The template file should also be copied into the
- * theme. It is located inside 'core/modules/system/templates/maintenance-page.html.twig'.
+ * 'maintenance_theme' key. Also copy the template file into the theme.
+ * It is located ins 'core/modules/system/templates/maintenance-page.html.twig'.
  * Note: This setting does not apply to installation and update pages.

I'd appreciate knowing what the "right" thing to do here is – or better yet, where to go looking to see if there is already guidance on this.

Anyway, assuming the above is OK and I'm not delusional about #1, then I think this is ready for review and hopefully we can RTBC it
and get it in before those mad committers in Prague make another reroll necessary!

Status: Needs review » Needs work
Issue tags: -Novice, -Twig, -theme system cleanup, -Drupal-Camping 2013

The last submitted patch, drupal8.documentation.2049207-82.patch, failed testing.

dlu’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Twig, +theme system cleanup, +Drupal-Camping 2013
star-szr’s picture

I discussed this with @dlu in IRC, and I mentioned we probably shouldn't change the comment that much. Generally less is more and makes things easier to review.

Edit: And usually we keep awkward line breaks, erring on the side of code standards: http://drupal.org/node/1354 :)

dlu’s picture

Fixed the over edited default.settings.php. The interdiff is from #85, below is the diff relative to 8.x (that is, the change that should have been made in the first place).

drupal $ git diff 8.x sites/default/default.settings.php
diff --git c/sites/default/default.settings.php w/sites/default/default.settings.php
index a58d60e..db2303c 100644
--- c/sites/default/default.settings.php
+++ w/sites/default/default.settings.php
@@ -578,7 +578,8 @@
  * is explicitly set to maintenance mode through the administration page or when
  * the database is inactive due to an error. It can be set through the
  * 'maintenance_theme' key. The template file should also be copied into the
- * theme. It is located inside 'core/modules/system/maintenance-page.tpl.php'.
+ * theme. It is located inside
+ * 'core/modules/system/templates/maintenance-page.html.twig'.
  * Note: This setting does not apply to installation and update pages.
  */
 # $conf['maintenance_theme'] = 'bartik';
star-szr’s picture

I think that looks better, thanks @dlu! I think now we need to decide whether "Note: This setting…" gets a paragraph of its own or becomes part of the paragraph ending in ".html.twig'." https://drupal.org/node/1354#general

dlu’s picture

I was thinking it got its own paragraph, so that the note would draw more attention. Not a strong opinion either way.

dlu’s picture

Minor, minor, minor tweak to make note in default.settings.php its own paragraph. No reroll :-D

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick turnaround! I have reviewed the patch itself and grepped again to make sure we're not missing anything. RTBC :)

jhodgdon’s picture

I concur. Great work! This is ready to go in.

dlu’s picture

Yay! And the patch still applies to HEAD :-)

LinL’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Twig, +theme system cleanup, +Drupal-Camping 2013

The last submitted patch, drupal8.documentation.2049207-89.patch, failed testing.

dlu’s picture

Reroll of #89. Should be ready to RTBC.

dlu’s picture

Can't post a patch right now – attempts return this error: "An HTTP error 0 occurred /comment-upload/js" in both Chrome and Safari.

I've got a patch ready to go, or if anybody else can post, the fix is trivial, just a simple merge in core/themes/bartik/bartik.theme.

dlu’s picture

Here is the reroll I promised.

LinL’s picture

Status: Needs work » Needs review

Go testbot!

LinL’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like jhodgdon approves, so getting this in now while it still applies. Great work!!

Committed and pushed to 8.x. Thanks!

tstoeckler’s picture

Status: Fixed » Needs work

The latest patch missed the HtmlAttributesTest file which was added (renamed) in the previous patches.

star-szr’s picture

Well, HtmlAttributesTest is definitely there in the patch, but I don't see it or HtmlTplPhpAttributesTest in HEAD. Great catch @tstoeckler!

http://drupalcode.org/project/drupal.git/commit/4f59f97

I think a patch that just adds HtmlAttributesTest would be the quickest fix here.

jhodgdon’s picture

Assigned: dlu » webchick

What you are saying is that the commit missed adding the file, right? Maybe webchick still has it in her git repo and can commit it?

dlu’s picture

Assigned: webchick » dlu
Status: Needs work » Reviewed & tested by the community
FileSize
1.56 KB

My dear Dr. Watson, it is evident that someone purloined HtmlAttributesTest…

Perhaps this small extract from #97 will put things right. The ways of git are most mysterious.

All humor aside, it seemed like the easiest thing to do was just to grab the hunk of the patch from #97 that created HtmlAttributesTest.php and make it into its own patch. That's what this is. It applies on my system to a clean copy of HEAD, but I wanted to describe what I did just in case it is broken in some way.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.documentation.2049207-104.patch, failed testing.

webchick’s picture

Status: Needs work » Fixed

Thanks very much! :) And it was indeed on my machine yet somehow missed by git apply --index. NO idea.

Committed and pushed to 8.x for real this time. :)

webchick’s picture

Huh. Well that is an ominous cross-post. Though it looks like it is a random fail? Can't think of how this missing test would've caused problems in Drupal\views_ui\Tests\DefaultViewsTest->testSplitListing().

dlu’s picture

Indeed…

dlu’s picture

Status: Fixed » Needs review
Issue tags: -Novice, -Twig, -theme system cleanup, -Drupal-Camping 2013

Status: Needs review » Needs work
Issue tags: +Novice, +Twig, +theme system cleanup, +Drupal-Camping 2013

The last submitted patch, drupal8.documentation.2049207-104.patch, failed testing.

webchick’s picture

That fail is to be expected, since it's already committed to HEAD.

I'll still be up for an hour or so DEFINITELY NOT WORKING ON MY PNWDS PRESENTATION AT THE LAST MINUTE ;) :) so I'll keep an eye on https://qa.drupal.org/8.x-status and make sure there's no weirdness.

dlu’s picture

DEFINITELY NOT WORKING ON MY PNWDS PRESENTATION AT THE LAST MINUTE

Definitely not, way too stressful…

star-szr’s picture

Assigned: dlu » Unassigned
Status: Needs work » Fixed

:D

Thanks, everyone.

LinL’s picture

Title: Replace .tpl.php with .html.twig in documentation » [Follow up] Replace .tpl.php with .html.twig in documentation
Status: Fixed » Needs review
FileSize
696 bytes

Looks like the changes to sites/default/default.settings.php were also missing from the commit.

The file has been changed since by #1829170: Convert the Maintenance Theme variable to settings, so here's a new patch.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Indeed, thanks @LinL! Great catch.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 8.x. Issue marked fixed yet again. :)

webchick’s picture

Sheesh! :) The documentation patch that keeps on ticking!

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

Anonymous’s picture

Issue summary: View changes

Update issue summary to include latest updates and new proposals.