Task

Convert PHPTemplate templates to Twig templates.

Remaining

  • Patch needs review
  • Manual testing
Template path Conversion status
core/modules/system/templates/system-plugin-ui-form.tpl.php converted

#1987410: [meta] system.module - Convert theme_ functions to Twig
#1757550: [META-63] Convert core theme functions to Twig templates
#1938930: Convert system theme tables to table #type
#1987426: Convert maintenance-page.tpl.php to Twig

Files: 
CommentFileSizeAuthor
#62 1898454-62-twig-system-tpl.patch3.04 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 56,710 pass(es).
[ View ]
#62 interdiff.txt1.97 KBjoelpittet
#59 1898454-56.patch2.65 KBSean Charles
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es).
[ View ]
#56 1898454-56.patch2.65 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 56,605 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#53 drupal-1898454-53-interdiff.txt1.99 KBHydra
#53 drupal-1898454-53.patch1.84 KBHydra
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898454-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 drupal-1898454-48.patch2.04 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 55,822 pass(es).
[ View ]
#48 drupal-1898454-48.patch-interdiff.txt777 bytesc4rl
#44 drupal-1898454-44.patch2.08 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 55,538 pass(es).
[ View ]
#44 drupal-1898454-44.patch-interdiff.txt1.12 KBc4rl
#42 drupal-1898454-42.patch2.08 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 55,516 pass(es).
[ View ]
#41 drupal-1898454-41.patch37.9 KBgnuget
PASSED: [[SimpleTest]]: [MySQL] 55,731 pass(es).
[ View ]
#33 drupal-1898454-33.patch40.32 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898454-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 drupal-1898454-33.patch-interdiff.txt4.5 KBc4rl
#26 drupal-1898454-26.patch43.11 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 54,470 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#26 drupal-1898454-26.patch-interdiff.txt9.88 KBc4rl
#20 drupal-1898454-20.patch40.78 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 53,909 pass(es).
[ View ]
#20 drupal-1898454-20.patch-interdiff.txt647 bytesc4rl
#19 drupal-1898454-19.patch41.41 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 53,960 pass(es).
[ View ]
#19 drupal-1898454-19.patch-interdiff.txt7.07 KBc4rl
#14 drupal-1898454-14.patch47.51 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 54,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 drupal-1898454-14.patch-interdiff.txt1.87 KBc4rl
#11 drupal-1898454-11.patch47.76 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 53,845 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#9 drupal-1898454-9.patch48.85 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 53,791 pass(es), 58 fail(s), and 24 exception(s).
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» trrroy

Assigned:trrroy» c4rl

Re #4, we'll leave all the core/include conversions to their respective issues.

I will plan to deprecate theme_system_modules_incompatible() in the conversion here. This theme callback only provides one wrapper div with a single HTML class. The utility is trivial.

It is called by a parent function system_modules() that paradoxically provides hard-coded markup for this page. Arguably system_modules() should be refactored so that markup is provided in an overridable way.

Any news on this patch/issue? Even a rough work-in-progress patch would be a great start, or if you won't have time to look at this we can get some other folks working on it. Thanks!

I'll try to have a patch in the next 24 hours.

Status:Active» Needs review
StatusFileSize
new48.85 KB
FAILED: [[SimpleTest]]: [MySQL] 53,791 pass(es), 58 fail(s), and 24 exception(s).
[ View ]

Here's a rough first-pass. There's a lot to consolidate in this module, some theme functions are duplicates (see #1842232: Consolidate use of theme_admin_page() and theme_system_admin_index()) and things like admin_block and admin_block_content could likely be combined.

I'm also guessing #1876712: [meta] Convert all tables in core to new #type 'table' will have impact here as well as what I'm proposing in #202593-19: 'Hide descriptions' link on admin/config affects other pages.

Status:Needs review» Needs work

The last submitted patch, drupal-1898454-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new47.76 KB
FAILED: [[SimpleTest]]: [MySQL] 53,845 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Accidentally removed the datetime template, which should actually be resolved with #1905584: Move base theme system templates into /core/templates

Status:Needs review» Needs work

The last submitted patch, drupal-1898454-11.patch, failed testing.

Thanks for posting your patch. 4 fails, that's nothing! :D

I'd recommend removing the change to datetime.html.twig - I don't think we need @todos for every template affected by #1905584: Move base theme system templates into /core/templates, and this will be the third conversion issue touching datetime.html.twig and causing conflicts – the other issue is #1898480: [meta] form.inc - Convert theme_ functions to Twig. I'm hoping we can get it down to just #1939080: Convert theme_datetime() to Twig updating that template. Feel free to roll that @todo into the patch there if you like.

StatusFileSize
new1.87 KB
new47.51 KB
FAILED: [[SimpleTest]]: [MySQL] 54,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This should do it. drupal_render()/drupal_render_children() can be irritating, hoping that #1876712: [meta] Convert all tables in core to new #type 'table' solves these issues.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-1898454-14.patch, failed testing.

I think we can remove any theme function conversions from this patch that are converted to #type table via #1938930: Convert system theme tables to table #type:

theme_system_date_format_localize_form()
theme_system_modules_uninstall()

Status:Needs work» Needs review

#14: drupal-1898454-14.patch queued for re-testing.

StatusFileSize
new7.07 KB
new41.41 KB
PASSED: [[SimpleTest]]: [MySQL] 53,960 pass(es).
[ View ]

Good idea. Here's the patch without those theme callbacks converted.

StatusFileSize
new647 bytes
new40.78 KB
PASSED: [[SimpleTest]]: [MySQL] 53,909 pass(es).
[ View ]

Cottser just reminded me in IRC that the datetime template shouldn't have any changes as not to conflict with #1939080: Convert theme_datetime() to Twig. Reverting these then.

Status:Needs review» Needs work
Issue tags:-Twig

The last submitted patch, drupal-1898454-20.patch, failed testing.

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

#20: drupal-1898454-20.patch queued for re-testing.

Have I mentioned that green is my favourite colour? Nice work c4rl :)

Found a bunch of nitpicks. Looks like very nice clean-up. Leaving at needs review, as this can be fixed post-commit, if needed.

+++ b/core/modules/system/system.admin.inc
@@ -2173,32 +2206,13 @@ function system_batch_page() {
+function template_preprocess_admin_block(&$variables) {
+  // This doesn't do anything extra to prepare the template, but is given
+  // simply for example purposes.
}

I'd like to meet the themer that goes strolling around line 2000 of system.admin.inc for template/theming examples. :-)

Seriously, though, I think this should be removed.

+++ b/core/modules/system/system.admin.inc
@@ -2242,100 +2254,82 @@ function theme_admin_block_content($variables) {
+ * Prepare variables for admin-system-index template.
+ * It is used for the admin index page.

There needs to be a blank (" *") line between these two. Also "It" should probably be "The template" (or "This template"?).

+++ b/core/modules/system/system.admin.inc
@@ -2242,100 +2254,82 @@ function theme_admin_block_content($variables) {
  * @ingroup themeable
+ * @todo: remove this file once http://drupal.org/node/1842232 is resolved.

There should be a blank line here as well. Also no colon after the @todo and start *R*emove with a capital letter.

More importantly, I think you mean remove this *function*, not this file, right?

+++ b/core/modules/system/system.admin.inc
@@ -2542,48 +2518,67 @@ function theme_system_themes_page($variables) {
+          '#theme' => 'links', ¶

Trailing whitespace.

+++ b/core/modules/system/system.module
@@ -2690,10 +2701,12 @@ function system_preprocess_block(&$variables) {
+ * @todo Fix drupal_render() and drupal_render_children() calls.
+ * See http://drupal.org/node/1920886 for more information.

This should be preceded by a blank (" *") line. Also, the second line should be indented by 2 chars.

+++ b/core/modules/system/system.module
@@ -3640,30 +3653,31 @@ function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_t
+function template_preprocess_system_powered_by(&$variables) {
+  // This doesn't do anything extra to prepare the template, but is given
+  // simply for example purposes.
}

I was about to say that this should be removed as well, but then I realized that people might search for this on api.drupal.org and for them this would be pretty nice. So maybe we should keep this. (This applies to the above function as well, of course.)

Issue summary:View changes

Add conversion summary table

Issue summary:View changes

Update remaining

Issue tags:+Needs manual testing

Tagging.

StatusFileSize
new9.88 KB
new43.11 KB
FAILED: [[SimpleTest]]: [MySQL] 54,470 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Assigned:c4rl» steveoliver

@steveoliver said he'd review this one :)

Update: will be reviewing this Thursday, incase anyone else wants to review in the meantime.

Status:Needs review» Needs work

changing back to needs work since this patch won't apply anymore.

Status:Needs work» Needs review
Issue tags:-Needs manual testing, -Twig

#26: drupal-1898454-26.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Twig

The last submitted patch, drupal-1898454-26.patch, failed testing.

Assigned:steveoliver» c4rl

I'll re-roll. Also see #1876712-35: [meta] Convert all tables in core to new #type 'table'.

I kind of also want to kill template_preprocess_confirm_form and template_preprocess_system_settings_form since they are just examples.

Status:Needs work» Needs review
StatusFileSize
new4.5 KB
new40.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898454-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've re-rolled with the following changes:

* I've removed confirm_form and system_settings_form since they were just empty examples and aren't really helpful.
* I've left out the conversion of theme_system_modules_details since it seems that should be handled by #1938930: Convert system theme tables to table #type instead.

Interdiff is with respect to #26.

Assigned:c4rl» Unassigned

Issue summary:View changes

Update remaining

Issue tags:-Needs manual testing, -Twig

#33: drupal-1898454-33.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Twig

The last submitted patch, drupal-1898454-33.patch, failed testing.

Title:Convert system module to Twigsystem.module - Convert PHPTemplate templates to Twig
Issue tags:-Needs reroll

Per #1757550-44: [META-63] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987410: [meta] system.module - Convert theme_ functions to Twig for theme_ function conversion.

Issue tags:+Needs reroll

Missed tag?

Issue summary:View changes

Remove sandbox link

Issue summary:View changes

List of templates and related issu

Assigned:Unassigned» gnuget

Status:Needs work» Needs review
StatusFileSize
new37.9 KB
PASSED: [[SimpleTest]]: [MySQL] 55,731 pass(es).
[ View ]

Before to start with #38

I did a reroll because of this #1964044: Change module list in install/uninstall confirm form to render array and this #1696224: Remove system_settings_form()

(I'm going to put this issue as needs review for the testbot check if anything is broken, after that i will provide a new patch only with the tpl.php files conversion)

Assigned:gnuget» Unassigned
StatusFileSize
new2.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,516 pass(es).
[ View ]

A patch with only the PHPTemplate conversion.

Status:Needs review» Needs work

Wow, this is the only template in system? Right on.

Small nitpicks:

+++ b/core/modules/system/templates/system-plugin-ui-form.html.twig
@@ -0,0 +1,31 @@
+ * Available variables:
+ * - $left: Any form array elements that should appear in the left hand column.
+ * - $right: Any form array elements that should appear in the right hand column.
+ * - $form_submit: Form submit button.

Lose the dollar signs.

+++ b/core/modules/system/templates/system-plugin-ui-form.html.twig
@@ -0,0 +1,31 @@
+  {% if form_submit %}
+  <div class="bottom-bar">{{ form_submit }}</div>
+  {% endif %}
+</div>

Indent div and use whitespace dashes, like this:
{% if form_submit -%}
  <div class="bottom-bar">{{ form_submit }}</div>
{%- endif %}

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
new2.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,538 pass(es).
[ View ]

@steveoliver's nits from #43

Issue tags:+Needs profiling

Tagging for profiling.

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

+++ b/core/modules/system/templates/system-plugin-ui-form.html.twigundefined
@@ -0,0 +1,31 @@
+ * - left: Any form array elements that should appear in the left hand column.
+ * - right: Any form array elements that should appear in the right hand column.

Can we just say 'form elements' instead of 'form array elements' here?

Going to do some testing and profiling here.

Markup looks great. Happy to RTBC this after the minor docs tweak in #46 is done.


Profiling

I added a views block displaying a node to the sidebar to isolate the profiling to just this conversion - this way Twig would be loaded for both scenarios.

Stark theme, APC class loader enabled - tested the page at admin/structure/block/list/block_plugin_ui%3Astark/add.

With Twig already loaded, this conversion is on par or even slightly faster than the PHPTemplate version.

=== 8.x..8.x compared (51901ff69a8a6..51902129e4489):
ct  : 41,863|41,863|0|0.0%
wt  : 264,143|264,089|-54|-0.0%
cpu : 231,228|231,303|75|0.0%
mu  : 17,928,248|17,928,248|0|0.0%
pmu : 18,075,784|18,075,784|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51901ff69a8a6&...

=== 8.x..system-1898454-44 compared (51901ff69a8a6..5190217a23e77):
ct  : 41,863|41,924|61|0.1%
wt  : 264,143|263,932|-211|-0.1%
cpu : 231,228|231,222|-6|-0.0%
mu  : 17,928,248|17,954,008|25,760|0.1%
pmu : 18,075,784|18,098,344|22,560|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51901ff69a8a6&...

Status:Needs work» Needs review
StatusFileSize
new777 bytes
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,822 pass(es).
[ View ]

Tweaks from #46 (Eventually these columns should just die and be replaced with a layout, IMHO -- separate issue). RTBC?

Status:Needs review» Reviewed & tested by the community
Issue tags:-Novice

RTBC (when it comes back green), thanks @c4rl!

The only other thing is we will need to rework template_preprocess_system_plugin_ui_form() after #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is resolved - only the drupal_add_css() line would be needed.

…and the template would look like this, so template docs would need tweaking as well:

<div id="block-library" class="container">
  <div class="left-col">
    <div class="inside">
      {{ form.left }}
    </div>
  </div>
  <div class="right-col">
    <div class="inside">
      {{ form.right }}
    </div>
  </div>
  {% if form -%}
    <div class="bottom-bar">{{ form }}</div>
  {%- endif %}
</div>

I got this error after applying the patch, is there something else I need to do?

PHP Fatal error:  Class '__TwigTemplate_b34682e870a2789958fdc767a660c4e1' not found in /Users/Lewis/Sites/DrupalCoreIssues/drupal/core/lib/Drupal/Core/Template/TwigEnvironment.php on line 107

Status:Needs work» Needs review
Issue tags:+Needs manual testing
StatusFileSize
new1.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898454-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.99 KB

@LewisNyman: The patch in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead was not commited/pushed properly, this may cause your problems?

Anyhow, this will be done hopefully soon, so here the updated patch. Keep in mind, it's still depending on the patch in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead.

Status:Needs review» Needs work

The last submitted patch, drupal-1898454-53.patch, failed testing.

Assigned:Unassigned» mr.baileys

Assigning to myself for a reroll.

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
FAILED: [[SimpleTest]]: [MySQL] 56,605 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Re-rolled so the patch applies.

Assigned:mr.baileys» Unassigned

Status:Needs review» Needs work

The last submitted patch, 1898454-56.patch, failed testing.

StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,725 pass(es).
[ View ]

Re-rolled (form variable reference)

Status:Needs work» Needs review

#56: 1898454-56.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing
StatusFileSize
new1.97 KB
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 56,710 pass(es).
[ View ]

Doc updates and did manual testing between .

http://d8.dev/admin/structure/block/list/block_plugin_ui%3Astark/add
http://d8clean.dev/admin/structure/block/list/block_plugin_ui%3Astark/add

And there was only minor whitespace diffs.

diff --git a/d8-clean-add.htm b/d8-add.htm
index 0697bc2..f999671 100644
--- a/d8-clean-add.htm
+++ b/d8-add.htm
@@ -2,6 +2,7 @@
   <div class="left-col">
     <div class="inside">
       <table class="sticky-enabled responsive-enabled">...
+
     </div>
   </div>
   <div class="right-col">
@@ -9,8 +10,8 @@
       <div class="form-item form-type-textfield form-item-block">
  <label for="edit-block">Search</label> <input class="form-autocomplete form-text" type="text" id="edit-block" name="block" value="" size="60" maxlength="128" /><input class="autocomplete" id="edit-block-autocomplete" type="hidden" value="http://d8.dev/system/autocomplete/block_plugin_ui%3Astark" />
</div>
+
     </div>
   </div>
-    <div class="bottom-bar">...</div>
-  </div>
+  <div class="bottom-bar">...</div></div>
</div>

+1 for RTBC

Issue tags:+Needs profiling

Let's re-profile this one now that we have the drupal_render_children() update.

Assigned:Unassigned» c4rl

Assigned:c4rl» Unassigned

@c4rl - this is one of the only ones left so I'm going to unassign so anyone can take this on, since I'm not sure if you got results yet. Feel free to still go through this yourself, more profiling data is not a bad thing :)

Assigned:Unassigned» jerdavis

Assigned:jerdavis» Unassigned
Issue tags:-Needs profiling

Profiled and uploaded.

I had some odd results on this one, most half runs were -0.4% to 0.3%, a couple were in the 1.4%-2.2% range. I'm uploading the one that seemed the cleanest.

Scenario:
* Full node block placed in content region
* 50 nodes are generated on site
* Applied patch
* Gave Anonymous user Administer blocks
* Pointed test to admin/structure/block/list/block_plugin_ui%3Astark/add

Results:

=== 8.x..8.x compared (519d31ef1945d..519d322fe3bf3):
ct  : 39,395|39,395|0|0.0%
wt  : 194,709|194,756|47|0.0%
cpu : 182,764|183,533|769|0.4%
mu  : 14,348,032|14,348,032|0|0.0%
pmu : 14,526,776|14,526,776|0|0.0%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d31ef1945d&run2=519d322fe3bf3&source=drupal-perf-drupalcon&extra=8.x..8.x
Run 519d31ef1945d uploaded successfully for drupal-perf-drupalcon.
Run 519d324f874a4 uploaded successfully for drupal-perf-drupalcon.
=== 8.x..1898454-twig-system compared (519d31ef1945d..519d324f874a4):
ct  : 39,395|39,489|94|0.2%
wt  : 194,709|194,020|-689|-0.4%
cpu : 182,764|182,437|-327|-0.2%
mu  : 14,348,032|14,375,392|27,360|0.2%
pmu : 14,526,776|14,554,816|28,040|0.2%
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d31ef1945d&run2=519d324f874a4&source=drupal-perf-drupalcon&extra=8.x..1898454-twig-system

I verified profiling results, +1 for RTBC.

Title:system.module - Convert PHPTemplate templates to Twig[READY] system.module - Convert PHPTemplate templates to Twig
Status:Reviewed & tested by the community» Closed (duplicate)

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

Committed 67b166c and pushed to 8.x. Thanks!

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

Issue summary:View changes

Add maintenance page conversion issue