[meta] system.module - Convert theme_ functions to Twig

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

Updated issue summary.

Patch attached

#2: system_theme_functions_to_Twig-1987410-1.patch queued for re-testing.

This patchs needs a re-roll i guess... i'm going to work on it.

Here a rerolled version.

Thanks @gnuget.

Here is a few nitpiks.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+ * Available variables:
+ * - content: An array containing information about the block. Each element
+ *   of the array represents an administrative menu item, and must at least
+ *   contain the keys 'title', 'href', and 'localized_options', which are

Array shouldn't be in the description, change to 'A list'

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,26 @@
+{% if block.show %}
+<div class="admin-panel">
+  {% if block.title %}<h3>{{ block.title }}</h3>{% endif %}
+  {% if block.content %}<div class="body">{{ block.content }}</div>
+  {% elseif block.description %}<div class="description">{{ block.description }}</div>{% endif %}
+</div>

Indenting on this should be in line with other twig indentation.
@see http://twig.sensiolabs.org/doc/coding_standards.html

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,58 @@
+ * Each theme_groups[group] contains an array of theme groups.
+ *
+ * Each group in theme_groups[group] contains:
+ * - group.attributes: Element attributes specific to this group.
+ * - group.title: Title for the theme group.
+ * - group.state: State of the theme group.
+ * - group.themes: An array of themes within that group.
+ *
+ * Each group.themes[theme] contains an array of themes.
+ *
+ * Each theme in group.themes[theme] contains:
+ * - theme.attributes: Element attributes specific to this theme.
+ * - theme.screenshot: Render of theme screenshot.
+ * - theme.description: Description of the theme.
+ * - theme.name: Name of theme.
+ * - theme.version: Verions number of theme.
+ * - theme.notes: Identifies what context this theme is being used.
+ *   eg. (default theme, admin theme)
+ * - theme.compatibility: Description of any incompatibility issues,
+ *   if the theme is compatible, provides a list of links.

These sub items should be indented without the name.
ie.
- attributes
not group.attributes.
@see https://drupal.org/node/1823416

Re-roll and nitpicks from #7

interdiff from after re-roll.

botched that re-roll. Attached re-roll only to test before adding the "nitpicks."

with patch :)

Thank you @IshaDakota:)
FYI, there is a bunch more nitpicks and performance improvements on this patch as it's fairly big. Mostly to do with attributes but I'll wait till the patch turns green before I write them up.

OK @joelpittet - I'll wait for a write up before submitting a new patch.

Ok here they come:) @IshaDakota if you have any questions I should be around on IRC (#drupal-twig) if you have any questions or leave the questions here.

+++ b/core/modules/system/system.admin.inc
@@ -11,6 +11,7 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Drupal\Core\Datetime\DrupalDateTime;
+use Drupal\Core\Template\Attribute;
 use Drupal\system\Form\ModulesInstallConfirmForm;

This is a task in itself but we don't need to instantiate Attribute for anything in this patch because they should all be lazy loaded through #1982024: Lazy-load Attribute objects later in the rendering process only if needed

This would mean removing all the new Attribute() calls form this patch and leaving them as arrays.

+++ b/core/modules/system/system.module
@@ -152,46 +157,49 @@ function system_theme() {
     ),
     'admin_page' => array(
-      'variables' => array('blocks' => NULL),
+      'variables' => array('blocks' => array()),

None of the variables in system_theme() need to be converted to an empty array from NULL. I have a good feeling that was a twig bug that was fixed here: #1975462: Allow and test for NULL and integer 0 values in Twig templates.

+++ b/core/modules/system/system.module
@@ -3515,30 +3523,38 @@ function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_t
 
 /**
- * Returns HTML for the Powered by Drupal text.
+ * Prepares variables for the Powered by Drupal template.
+ *
+ * Default template: system-powered-by.html.twig.
+ *
+ * This doesn't do anything extra to prepare the template, but is given simply
+ * for example purposes.
  *
  * @ingroup themeable
  */
-function theme_system_powered_by() {
-  return '<span>' . t('Powered by <a href="@poweredby">Drupal</a>', array('@poweredby' => 'http://drupal.org')) . '</span>';
+function template_preprocess_system_powered_by(&$variables) {
 }

if this is an empty preprocess function, it can be deleted.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+{% if content %}
+<dl{{ attributes }}>
+  {% for item in content %}
+    <dt>{{ item.link }}</dt>
+    {% if item.description %}
+      <dd>{{ item.description }}</dd>
+    {% endif %}
+  {% endfor %}
+</dl>

dl needs one more indent.

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,26 @@
+{% if block.show %}
+<div class="admin-panel">
+  {% if block.title %}<h3>{{ block.title }}</h3>{% endif %}
+  {% if block.content %}<div class="body">{{ block.content }}</div>
+  {% elseif block.description %}<div class="description">{{ block.description }}</div>{% endif %}
+</div>

contents of if block nees one more indent and if statements shouldn't be on oneline.

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -0,0 +1,25 @@
+ * - containers: An array of administrative blocks keyed by position: left or right.

80 charachters and 'An array of' should be 'A list of'.

+++ b/core/modules/system/templates/status-report.html.twig
@@ -0,0 +1,40 @@
+ * - requirements: contains multiple requirement instances. Each requirement
+ *     has .title, .value and .description.
+ * - severity: The severity of error. Contains .title and .class.

the indent and formatting of the requirements properties is incorrect. @see https://drupal.org/node/1823416

+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -0,0 +1,26 @@
+ @todo: remove this file once http://drupal.org/node/1842232 is resolved.

Remove the todo, the issue should be good enough. We can't commit with @todos.

+++ b/core/modules/system/templates/system-modules-details.html.twig
@@ -0,0 +1,15 @@
+ * - content: file form element html.

Capital F for the sentence.

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,58 @@
+ * Each theme in group.themes[theme] contains:
+ * - theme.attributes: Element attributes specific to this theme.
+ * - theme.screenshot: Render of theme screenshot.
+ * - theme.description: Description of the theme.
+ * - theme.name: Name of theme.
+ * - theme.version: Verions number of theme.
+ * - theme.notes: Identifies what context this theme is being used.
+ *   eg. (default theme, admin theme)

The main item 'group' and 'theme' and all their sub items need to be formatted according to the twig standards here: https://drupal.org/node/1823416

OK, I think maybe I got everything from #7 and #14, thanks to @joelpittet in IRC.

Thank you @IshaDakota!

There is still quite a bit to do on this patch as I skimmed through so we need a fresh pair of eyes I bet.

I was wrong on some of those array() to NULL. Here's a patch that inches this a bit further.

Ok @IshaDakota! I was a little heavy handed on the Attributes note but it's still much better (even though it's red:)

This may still not pass, but again, I promise it's in the correct direction.

So the Attributes here that I had to leave in as Attribute objects are not $variables['attributes'] that you changed to arrays. Those ones are cool, and also not $whatever['#attributes'] which is a renderable array that will get themed. It's only the ones that are $whatever['attributes]. It's subtle, confusing and important:)

@IshaDakota, checkout how I cleaned up the twig files with those |keys nonesense and split out attributes.class. There are a number of other templates that need the same cleanup. And whatever errors I left, if any:)

activate testbot! and tag, also added a few new @todos that need some filling out in the twig file.

See #21 for notes on what further needs doing.

...and re-rolled.

-function system_admin_index() removed in #1987812: Convert system_admin_index() to a new style controller
-div container and class added to "no-screenshot" in #1861702: Add a responsive grid to the Appearance page. Resolved as such:

---- a/core/modules/system/templates/system-themes-page.html.twig
+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -43,7 +43,9 @@
           {% if theme.screenshot %}
             {{ theme.screenshot }}
           {% else %}
-            <div class="no-screenshot">{{ "no screenshot"|t }}</div>
+            <div class="no-screenshot">
+              <div class="no-screenshot__text">{{ "no screenshot"|t }}</div>
+            </div>
           {% endif %}
           <div class="theme-info">
             <h3>{{ theme.name }} {{ theme.version }} {{ theme.notes }}</h3>

@joelpittet: Is this what you had in mind for for cleaning up the twig files? These were the only two cases I could find (in addition to the 2 you fixed).

--- a/core/modules/system/templates/admin-page.html.twig
+++ b/core/modules/system/templates/admin-page.html.twig
@@ -18,8 +18,8 @@
   {{ system_compact_link }}
   {% for position, container in containers %}
     <div class="{{ position }} clearfix">
-      {% for i in container.blocks|keys %}
-        {{ container.blocks[i] }}
+      {% for container.block in container.blocks %}
+        {{ container.block }}
       {% endfor %}
     </div>
   {% endfor %}

and

--- a/core/modules/system/templates/system-admin-index.html.twig
+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -17,8 +17,8 @@
   {{ system_compact_link }}
   {% for position, container in containers %}
     <div class="{{ position }} clearfix">
-      {% for i in container.blocks|keys %}
-        {{ container.blocks[i] }}
+      {% for container.block in container.blocks %}
+        {{ container.block }}
       {% endfor %}
     </div>
   {% endfor %}

@IshaDakota Very close and in the right direction.

The first variable in the {% for ... is something you 'make up'. While container.block may work it's a bit confusing, the thing I was thinking of making may also be a bit confusing.

+      {% for block in container.blocks %}
+        {{ block.block }}

Which reads: For each block in container's blocks... print block's block.
The reason this is confusing, IMO, isn't my change but the property name block. So I'd prefer:

+      {% for block in container.blocks %}
+        {{ block.content }}

Which reads: For each block in container's blocks... print block's content.

This would mean changing a variable in the preprocces some place but by the looks of things you should be able to get away with this:

+      {% for block in container.blocks %}
+        {{ block }}

Which would be even better, so maybe give that a try and see if the admin page still does what it should.

OK. I'll test that out.

The following seems to work in both the admin and admin index pages (see screenshots):

<div class="admin clearfix">
  {{ system_compact_link }}
  {% for position, container in containers %}
    <div class="{{ position }} clearfix">
      {% for block in container.blocks %}
        {{ block }}
      {% endfor %}
    </div>
  {% endfor %}
</div>

Admin Index Page:

Admin Config Page:

I'll check out those @todos in the twig files.

Great news @IshaDakota! Could you add the patch with that up?

This patch is getting pretty close, would like to get another set of eyes to review after that patch. Then we can move on to manual testing and profiling. (pre-emptive tagging.)

This is is the patch with #29.

There are still @todos in the status-report.html.twig file. Are those just to fill out the descriptions in the docblock? Or does it refer to the code? To me it looks OK.

#31: 1987410-31-twig-system-theme.patch queued for re-testing.

@IshaDakota, thank you and you are right, yeah, just to fill out the descriptions of what those variable are.

Below are another round of nitpikery:

+++ b/core/modules/system/system.admin.inc
@@ -1354,100 +1335,86 @@ function theme_admin_block_content($variables) {
+ * @todo Remove this once http://drupal.org/node/1842232 is resolved.

Remove @todo, the issue it self can deal with it's removal

+++ b/core/modules/system/system.module
@@ -3785,6 +3788,7 @@ function theme_system_config_form($variables) {
  * @ingroup themeable
+ * @todo Move this out of system.module and back into theme.inc.

This should have it's own issue and not be a @todo. (may already exist, have a search before creating a new issue)

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+ *   passed to l(). A 'description' key may also be provided.
+ * - attributes: Remaining HTML attributes to be aded to the element.

Remove 'Remaining '.

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,31 @@
+ * - block: An array of information about the block, including:
+ *   - show: A boolean flag indicating if the block should be displayed.
+ *   - title: The block title.

Should sa 'A list' and not refernce 'An array' @see https://drupal.org/node/1823416#datatypes

+++ b/core/modules/system/templates/status-report.html.twig
@@ -0,0 +1,46 @@
+ *   Each requirement contains:
+ *   - title: @todo.
+ *   - value: @todo.
+ *   - description: @todo.
+ * - severity: The severity of error.
+ *   Contains:
+ *   - title: @todo.
+ *   - class: @todo.

These todo's need to be filed out describing what these variables contain.

+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -0,0 +1,25 @@
+ * - system_compact_link: Themed link to toggle compact view.
+ * - container: Container for admin blocks.

Need to add 'position' and 'block' to the available variables here.

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,60 @@
+ * - attributes: HTML element attributes.
+ * - theme_groups: An array of theme groups.
...
+ * - state: State of the theme group.
+ * - themes: An array of themes within that group.

Data types in twig dockblocks again @see https://drupal.org/node/1823416#datatypes

dibs

dibs!

first patch failed here is a re roll!

test bot go

Re-roll again.

Doc cleanups from #34

I installed the patch, and checked all of the fixed subjects noted at the top under "Theme function name". Everything ran smoothly and it appears as though the patch worked swimmingly.

Also, be aware that this is going to need a reroll after #1833932: Convert theme_system_compact_link() into a #type link

List of functions

I looked at the theme_status_report() part and noticed two things in the patch from #42:
This patch converts direct calls to theme() for the status_report hook to calls to drupal_render(). (Why?). Also, this is already happening in #2009674: Replace theme() with drupal_render() in system module. See also #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
This patch changes the status_report theme hook to take variables rather than a render element. (Why?) As it happens this has already happened in #2041283: theme_status_report() is severely broken, so the patch will need a re-roll anyways.

Also, theme_status_report() only prints the icon and rows when the #type key is empty, if I'm not mistaken. The template nor the new preprocess function take care of this now, it seems. Related: #2013258: Simplify render for theme_status_report().

Reroll.

Needs work per #47 and #46.

Reverted unnecessary theme() to drupal_render() changes, and removed two templates that only had {{ variable }} in them (they're unnecessary and slow). As for #47, that issue is outside of scope for this issue, let's let #2013258: Simplify render for theme_status_report() take care of it.

In #51 the code for the two templates was reverted but not the code for the preprocessors and theme functions. Also, #1939090: Convert theme_tablesort_indicator() to Twig had gone in already.

#2013258: Simplify render for theme_status_report() and #2010982: Replace theme() with drupal_render() in system module for system_status() are is in, too.

Sorry, #2010982: Replace theme() with drupal_render() in system module for system_status() is not in and not relevant here. Edited my earlier comment.

Ok I think there may have been some miscommunication here... #51 revert went a bit too far.

I took #49 which needed another re-roll. There aren't any theme to drupal_render switches that I can see.

@Eric_A could you try again with your review?

Looks like some things got dropped in favour of the routes, I like it:)

Here's the re-roll.

Re-roll

re-roll

updaed

Updated issue summary.

Re-roll the #61.

Forget to change status to 'need review'.

Tags

This is missing the removal of theme_system_modules_details And hopefully the template is not {{ content }} as it is now, if so try to remove the template all together.

I will try to fix it :)

Updated issue summary.

never mind updated issue summary.
theme_system_modules_details will be removed in #1938930: Convert theme_system_modules_details() to #type table

Updated issue summary.

Tagging for #2094585: [policy, no patch] Core review bonus.

didn't mean to add that tag back on :(

Updated issue summary.

Updated issue summary.

Needs a re-roll, patch doesn't apply.

Rerolled #63. Minor comment cleanup, and effectively reverted #1143922: Undefined index: value in theme_status_report() as explicitly checking for 'value' is not needed now we have Twig.

Also changed .element-invisible to .visually-hidden.

Nice catch on the .visually-hidden. I didn't even see that conversion get in. For anybody looking that change record is here: https://drupal.org/node/2022859