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] Convert core theme functions to Twig templates
#1938930: Convert theme_system_modules_details() to #type table
#1987426: Convert maintenance-page.tpl.php to Twig

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

trrroy’s picture

Assigned: Unassigned » trrroy
c4rl’s picture

Assigned: trrroy » c4rl
c4rl’s picture

c4rl’s picture

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

c4rl’s picture

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.

star-szr’s picture

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!

c4rl’s picture

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

c4rl’s picture

Status: Active » Needs review
FileSize
48.85 KB

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 admin-page.html.twig and system-admin-index.html.twig) 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.

c4rl’s picture

Status: Needs work » Needs review
FileSize
47.76 KB

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.

star-szr’s picture

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.

c4rl’s picture

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.

c4rl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

star-szr’s picture

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

theme_system_date_format_localize_form()
theme_system_modules_uninstall()

star-szr’s picture

Status: Needs work » Needs review

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

c4rl’s picture

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

c4rl’s picture

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.

star-szr’s picture

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

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

star-szr’s picture

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

tstoeckler’s picture

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.)

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Issue summary: View changes

Update remaining

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

c4rl’s picture

star-szr’s picture

Assigned: c4rl » steveoliver

@steveoliver said he'd review this one :)

steveoliver’s picture

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

jenlampton’s picture

Status: Needs review » Needs work

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

duellj’s picture

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.

c4rl’s picture

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.

c4rl’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
40.32 KB

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 theme_system_modules_details() to #type table instead.

Interdiff is with respect to #26.

c4rl’s picture

Assigned: c4rl » Unassigned
c4rl’s picture

Issue summary: View changes

Update remaining

Fabianx’s picture

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.

star-szr’s picture

c4rl’s picture

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

Per #1757550-44: [Meta] 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.

c4rl’s picture

Issue tags: +Needs reroll

Missed tag?

c4rl’s picture

Issue summary: View changes

Remove sandbox link

c4rl’s picture

Issue summary: View changes

List of templates and related issu

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Status: Needs work » Needs review
FileSize
37.9 KB

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)

gnuget’s picture

Assigned: gnuget » Unassigned
FileSize
2.08 KB

A patch with only the PHPTemplate conversion.

steveoliver’s picture

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 %}
c4rl’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
2.08 KB

@steveoliver's nits from #43

star-szr’s picture

Issue tags: +needs profiling

Tagging for profiling.

star-szr’s picture

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.

star-szr’s picture

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&...

c4rl’s picture

Status: Needs work » Needs review
FileSize
777 bytes
2.04 KB

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

star-szr’s picture

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.

star-szr’s picture

…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>
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
LewisNyman’s picture

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

Hydra’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
1.84 KB
1.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.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

Assigning to myself for a reroll.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

Re-rolled so the patch applies.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned

Status: Needs review » Needs work

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

Sean Charles’s picture

FileSize
2.65 KB

Re-rolled (form variable reference)

Sean Charles’s picture

Status: Needs work » Needs review
Hydra’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
1.97 KB
3.04 KB

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>
Hydra’s picture

+1 for RTBC

star-szr’s picture

Issue tags: +needs profiling

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

c4rl’s picture

Assigned: Unassigned » c4rl
star-szr’s picture

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 :)

jerdavis’s picture

Assigned: Unassigned » jerdavis
jerdavis’s picture

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
Fabianx’s picture

I verified profiling results, +1 for RTBC.

alexpott’s picture

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

Title: [READY] system.module - Convert PHPTemplate templates to Twig » system.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.

Anonymous’s picture

Issue summary: View changes

Add maintenance page conversion issue