update.module - Convert theme_ functions to Twig

Assigning this to me.

Reassigning this issue to my personal account.

Does look like this has been worked on in a month. I'll take a crack at it.

My apologies for not getting to this sooner... Just switched jobs and my free time is much less. tlattimore... thanks for picking up my slack.

I made a patch for this, I believe it works but I am having trouble testing it.

I have theme_update_last_check() and theme_update_version() converted.

@carsonblack - any progress on this? I'd be glad to help out on itif you'd like.

A little bit, still on theme_update_report(). Should have this wrapped up by Sunday, 3/24/2013

Any progress you can share @carsonblack? Even posting a work-in-progress patch would be a great step forward for this issue. If not, we can have someone else (maybe @tlattimore?) take a crack at it.

update_last_check is very similar to locale locale_translation_last_check
Save an extra <p> tag... maybe these two theme functions could be consolidated?

This patch does that one conversion anyways.

In a bit of a rush so this doesn't even touch
theme_update_report()

But this patch should apply for anybody who want's to take this further. Also running into some errors with "Undefined index: reason" which I am sure will show up in the testbot

So this will probably pass, strange it needed the file in the update_theme definition but didn't for the theme_ functions...

Made a bit of a start on update_report, but still not converted... so if this passes, turn it back to needs work.

Thanks @joelpittet, you rock. Back to CNW as requested in #15.

Forgot to mention update_version was removed from this conversion due to #type=>table duplicate stuff.
#1938934: Convert update theme tables to table #type

@ry5n is going t add update_version to the twig again... discussed a bit with @Cottser on IRC and for now a straight twig conversion of this would be fine... possible down the road of making it a private function that calls #theme=>table__update_version would be more ideal but it's not a form table so doesn't need type=>table conversion. Closed that issue as won't fix for now. Hope we made the right move there.

theme_update_manager_update_form was removed because it was not necessary and moved the code to the form callback.

Worked on this with @joelpittet today (thanks Joel!). Got much of the conversion done for theme_update_report, but it’s not working yet. I won’t have time to come back to this for a while, so am posting progress here.

added type table for update_version

Picking this up because I worked through much of it with @ry5n. update_reports was a beast to get through and this is a great start. Thanks for helping start that conversion!

Thanks Joel! Hopefully it’s on the right track.

Tagging.

So there is an extra class attribute in the template_preprocess_update_version() which would be fixed by
http://drupal.org/node/1938430#comment-7240452
#1938430: Don't add a default theme hook class in template_preprocess() which adds the module name as a class.

Here's just a patch of the update_version() bit that was tweaked from #20, to see if it will pass. Still working on the update_report()

Ok the beast is in: update_report(). This may not pass but it's pretty close, if it doesn't I'll pick it up over the weekend.

So having trouble with context within loops again...
#1867090: Nested Twig 'for' loops behaving strange

I left one trick in this patch to "fix" a few errors but this really needs to be looked into.

oof , sorry Cottser, and others... Just got slammed with a bunch of other work. I'll check out the patches and see what I can do!

@carsonblack no worries, thanks for getting it started. Could you look at that exception? Most of the fails are related to #1867090: Nested Twig 'for' loops behaving strange which I am working on tracking down right now... it looks like it's a drupal issue not a twig issue.

Slightly better but still a couple of issues. I had to put a theme() call back in and write all the nested loop 'set' hacks to get things working.

Down to 3 fails is quite an achievement I'd say, fantastic work on this @joelpittet :)

Hate to bring this up now, but…

+++ b/core/modules/update/update.report.incundefined
@@ -134,68 +189,94 @@ function theme_update_report($variables) {
+      $row['includes'] = array(
+        '#prefix' => t('Includes:'),
+        '#theme' => 'item_list',
+        '#items' => $includes_items,
+      );
     }
     else {
-      $row .= t('Includes: %includes', array('%includes' => implode(', ', $project['includes'])));
+      $row['includes'] = t('Includes: %includes', array('%includes' => implode(', ', $project['includes'])));

If at all possible we should be using t and join filters in the template for this.

@Cottser, I think the answer is yes, if you look at previous patches it was in there, the reason I kept that in preprocess was because t('Includes: %includes') and t('Includes:') were different and wasn't sure I should change them both to {{ 'Includes'|t }}: with the colon on the outside or inside for fear of the i18n stuff not passing as well. But worst case I could put both in with an if statement.

We should be maintaining the exact same strings within t() calls, so it sounds like an {% if %} in the template will be the way to go.

Rolled with http://drupal.org/node/1867090#comment-7317770

Still needs #34 With syntax #26

Ok this still fails those two, but at least http://drupal.org/node/1867090#comment-7317770 is fixing the TwigReference in loop issue!

This is changes from #34

Ok this should pass the tests now. I removed the theme() call and replaced both with drupal_render(). I know we don't want to use that, but TwigReference will not return false in a condition because it's an array with '#theme' and other keys(ie. not an empty array or rendered empty string). Suggestions here?

+++ b/core/modules/update/update.report.incundefined
@@ -59,53 +71,97 @@ function theme_update_report($variables) {
+    // Rendered status label here because we can't check that a TwigReference
+    // is empty in the twig file.
+    $status_label = array(
+      '#theme' => 'update_status_label',
+      '#status' => $project['status'],
+    );
+    $row['status_label'] = drupal_render($status_label);

If you weren't passing the variable to t() later, you could just do something like this - or put the empty array in an 'else' if you prefer.

$row['status_label'] = array();
if (!empty($project['status'])) {
  $row['status_label'] = array(
    '#theme' => 'update_status_label',
    '#status' => $project['status'],
  );
}

…but since you pass it to t() later, that won't work. I don't know a way around that - at this time anyway if it goes through t() it needs to be a string. So the way you did it is fine, I can't think of any better way.

re-rolled as #1867090: Nested Twig 'for' loops behaving strange landed

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,100 @@
+ *     - reason: @todo.
+ *     - icon: The project status version indicator icon.
+ *     - existing_version: @todo.
+ *     - versions: @todo.
+ *     - install_type: @todo.
+ *     - datestamp: @todo.
+ *     - extra: @todo.
+ *       - attributes: HTML attributes for the extra items.
+ *       - label: @todo.
+ *       - data: @todo.
+ *     - includes: @todo.
+ *     - base_themes: @todo.
+ *     - sub_themes: @todo.

This seems like a lot of @todo's here.

#43 - yeah, that's been happening in a few patches where theme functions were badly/not documented in the original code - especially in some of the Views conversion patches. Not mentioning a variable used in a Twig template is not ok but if we really don't know what each one does in detail, leaving @todo is ok for now.

Update conversion table

Re Roll from #42

Will be working on the updates next.

Failed because the link 'Check Manually' was missing on the reports/updates page.

This patch is a reroll of the patch from #42, with the necessary update that allow it to pass testing.

update comments

The previous patch did not pass...
This last one is passing all my local simpletests... w00t!

ready for some review!! :P

Great job on this re-roll @webthingee! Here's a comment of adjustments that need to be made to this:

+++ b/core/modules/update/templates/update-last-check.html.twigundefined
@@ -0,0 +1,24 @@
+ * Default theme implementation for the last time we checked for update data.

Could we say something like "..displaying the last time we checked for update data" or something like that?

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,103 @@
+      for the installed projects.

Need an extra "*" at the beginning of this line.

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,103 @@
+            <div class="version-status">
+              {{- project.status_label|default(project.reason) -}}
+              <span class="icon">{{ project.icon }}</span>
+            </div>

Indent inside spaceless here.

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,103 @@
+              {% for version in project.versions %}
+                {{ version }}

Indentation needs to be 2 spaces inside div class="versions"

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,103 @@
+                {{ 'Depends on: !basethemes'|t({'!basethemes': project.base_themes|join(', ')}) }}

Indentation needs to be adjusted.

Updated with http://drupal.org/node/1898466#comment-7457564, as well as found a typo in one of the form variables.
Checked locally with simpletest... fingers crossed, here it comes again....

Point the home page to admin/reports/updates

=== 8.x..8.x compared (51a4463d308ce..51a447be01e16):

ct  : 164,330|164,330|0|0.0%
wt  : 1,377,577|1,372,960|-4,617|-0.3%
cpu : 1,072,067|1,080,067|8,000|0.7%
mu  : 7,991,352|7,991,352|0|0.0%
pmu : 8,099,236|8,099,236|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4463d308ce&...

=== 8.x..update-1898466-54 compared (51a4463d308ce..51a448a326782):

ct  : 164,330|164,800|470|0.3%
wt  : 1,377,577|1,361,964|-15,613|-1.1%
cpu : 1,072,067|1,080,068|8,001|0.7%
mu  : 7,991,352|8,103,432|112,080|1.4%
pmu : 8,099,236|8,207,472|108,236|1.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4463d308ce&...

Remove sandbox link

Profiling looks great, after the image style stuff get's removed or ... explained maybe needs re-profiling.

diff --git a/core/modules/image/image.module b/core/modules/image/image.module
index 2cc6f6b..00f21b6 100644
--- a/core/modules/image/image.module
+++ b/core/modules/image/image.module
@@ -693,10 +693,9 @@ function image_style_transform_dimensions($style_name, array &$dimensions) {
  *   An image style array.
...
   // Let other modules update as necessary on flush.
diff --git a/core/modules/image/lib/Drupal/image/Tests/ImageStyleFlushTest.php b/core/modules/image/lib/Drupal/image/Tests/ImageStyleFlushTest.php

not sure how this image module stuff got in here.

+++ b/core/modules/update/templates/update-report.html.twig
@@ -0,0 +1,104 @@
+ * - project_types: A list of product types.
+ *   - label: The product type label.
+ *   - projects: Data about each project's status.
+ *     - title: The project tile.
+ *     - attributes: HTML attributes for the product row.

Should be project not product. Whoops, I probably did that:S

Here's an updated patch that removes the image related stuff @joelpittet mentioned in #56, also fixes the comment in `update-report.html.twig` as well.

Few more patch cleanups and @todo cleanups. Thanks @tlattimore for removing the image stuff and that typo fix.

+++ b/core/modules/update/templates/update-report.html.twig
@@ -0,0 +1,104 @@
+ * - project_types: A list of product types.
+ *   - label: The product type label.
+ *   - projects: Data about each project's status.

Couple of 'product' instead of 'project' typos in there... you're welcome:P

+++ b/core/modules/update/templates/update-version.html.twig
@@ -0,0 +1,30 @@
+ *   - attributes: HTML attributes for the update versions table.
+ *   - version_link: @todo.
+ *   - version_date: The date of the release.
+ *   - version_links: @todo.

Need to find some words to fill these @todos.

+++ b/core/modules/update/update.report.inc
@@ -276,36 +382,33 @@ function theme_update_status_label($variables) {
+  // Remove 'update_version' from the attributes array. This is added through
+  // template_preprocess() but we do not need it.
+  // @todo Remove after http://drupal.org/node/1938430 is resolved.
+  $attributes = array();
+  $attributes['class'] = $variables['class'];
+  $attributes['class'][] = 'version';
+  $variables['attributes'] = new Attribute($attributes);

This needs to be removed and just the 'version' class should stay.

+++ b/core/modules/update/templates/update-report.html.twig
@@ -0,0 +1,104 @@
+ * - project_types: A list of product types.
+ *   - label: The product type label.
+ *   - projects: Data about each project's status.

Couple of 'product' instead of 'project' typos in there... you're welcome:P

hello i am calling dibs

I tried to reroll the patch. Hope it works!

testbot go!

rerolled, and took care of most of joelpittet's comments in #58 except for the @todos.

Also:
- removed update_last_check
- update_status_label
These should not be their own templates.

If this fails tests tho I'm happy to move these off into a sub-issue.

Here's an updated patch for #64 that fixes the 2 test fails, plus interdiff.

The {%--%}'s are the result of filter_xss having issues with extra whitespace...

The status report page renders correctly.

un tagging

+++ b/core/modules/update/templates/update-report.html.twigundefined
@@ -0,0 +1,116 @@
+  <table class="update">
+    <tbody>
+      {% for project in project_type.projects %}
+        <tr{{ project.attributes }}>
+          <td>
+            {% spaceless %}
+              <div class="version-status">
+                  {%- if project.status_label -%}
+                    <span{{ project.status_attributes }}>{{ project.status_label|t }}</span>
+                  {%- else -%}
+                    {{ project.reason }}
+                  {%- endif -%}
+                <span class="icon">{{ project.icon }}</span>
+              </div>
+            {% endspaceless %}
+
+            <div class="project">{{ project.title }} {{ project.existing_version }}
+              {% if project.install_type == 'dev' and project.datestamp %}
+                <span class="version-date">({{ project.datestamp }})</span>
+              {% endif %}
+            </div>
+
+            {% if project.versions %}
+              <div class="versions">
+                {% for version in project.versions %}
+                  {{ version }}
+                {% endfor %}
+              </div>
+            {% endif %}
+
+            <div class="info">
+              {% if project.extra %}
+                <div class="extra">
+                  {% for extra in project.extra %}
+                    <div{{ extra.attributes }}>
+                      {{ extra.label }}: {{ extra.data }}
+                    </div>
+                  {% endfor %}
+                </div>
+              {% endif %}
+              <div class="includes">
+                {% if project.disabled %}
+                  {{ 'Includes:'|t }}
+                  <ul>
+                    <li class="first odd">{{ 'Enabled: %includes'|t({'%includes': project.includes|join(', ')}) }}</li>
+                    <li class="last even">{{ 'Disabled: %disabled'|t({'%disabled': project.disabled|join(', ')}) }}</li>
+                  </ul>
+                {% else %}
+                  {{ 'Includes: %includes'|t({'%includes': project.includes|join(', ')}) }}
+                {% endif %}
+              </div>
+
+              {% if project.base_themes %}
+                <div class="basethemes">
+                  {{ 'Depends on: !basethemes'|t({'!basethemes': project.base_themes|join(', ')}) }}
+                </div>
+              {% endif %}
+
+              {% if project.sub_themes %}
+                <div class="subthemes">
+                  {{ 'Required by: %subthemes'|t({'%subthemes': project.sub_themes|join(', ')}) }}
+                </div>
+              {% endif %}
+            </div>
+          </td>
+        </tr>
+      {% endfor %}
+    </tbody>
+  </table>

+++ b/core/modules/update/update.report.incundefined
@@ -224,49 +342,23 @@ function theme_update_report($variables) {
-      $output .= theme('table', array('header' => $header, 'rows' => $rows[$type_name], 'attributes' => array('class' => array('update'))));

It seems weird that we've replaced a theme table with such explicit markup. How come?

Also this patch conflicts with #2048933: Replace theme() with drupal_render() in update module

Let's not add this new template. Instead, let's update to a render array with #type = table.
Let's remove this conversion from here and use drupal_render. Also, see https://drupal.org/node/1938934.

@alexpott the reason I went this way was the thought that this template doesn't need to be a table at all and in hopes to make it easier for themers to change it to something better. It's definitely overkill and a bunch of work to get it like that but when talking to other themers they seemed to agree that if they wanted to change the template it was a choice of manipulating a large renderable array or changing a twig template it would be much more practical to do the latter.

The flip side to the coin though is that if this never get's converted to divs there is really no point to theme_update_report and converting to #type=>table or whatever would be the way it would go... so probably just me dreaming;)

Patch rerolled.

• Got rid of the todos in update-version.html.twig.
• Removed update_status function which was removed in #1938822: Convert update_status to a new-style Controller
• Removed unused $update_last_check.
• Removed references to update_status_label callback, this function has been removed.

Included a patch to get a module and theme to show up on the updates page. To use it:
Install Nivo Slider module version 8.x-1.3 to the DRUPAL_ROOT/modules/ directory:
drush dl nivo_slider-8.x-1.3

Install the Red theme version 8.x-1.0-alpha1 to the DRUPAL_ROOT/themes/ directory:
drush dl red-8.x-1.0-alpha1

Apply the update-testing-patch.txt file. Load: admin/reports/updates

Thank you for the cleanup @drupalmonkey! We should probably do another round on manual testing on this using your steps!

Tagging for reroll.

Reroll

Thanks for the re-roll @pplantinga!

Minor nitpicks.

+++ b/core/modules/update/templates/update-report.html.twig
@@ -0,0 +1,116 @@
+ * @see template_preprocess()

+++ b/core/modules/update/templates/update-version.html.twig
@@ -0,0 +1,30 @@
+ * @see template_preprocess()

These @see template_preprocess() can be removed.

+++ b/core/modules/update/templates/update-report.html.twig
@@ -0,0 +1,116 @@
+                  {%- if project.status_label -%}
+                    <span{{ project.status_attributes }}>{{ project.status_label|t }}</span>
+                  {%- else -%}
+                    {{ project.reason }}
+                  {%- endif -%}

This block is indented one indent too far.

I'm more and more convinced that this approach of building the tables in the twig file is better for DX, better for themers to replace later and has some performance over #type=>table. This also keeps to my goal of having less to no markup in PHP.

- Removed @see template_preprocess
- Fixed indenting

#80: twig-update_conversion-1898466-80.patch queued for re-testing.

Trying this with a new patch (very new to interdiffing), same changes as in #80

Another shot at it.

Whoops looks like we posted at the same time. Yours looks more like the right size than mine...

#84: twig-update_conversion-1898466-84.patch queued for re-testing.

Took a look at richardj's patch and compared the markup to pre-conversion markup, there was a couple of classes missing, new patch attached.

how to test

88: twig-update_conversion-1898466-88.patch queued for re-testing.

added a follow-up issue to description #2099293: Replace table with available module updates with better markup

Is the remaining task in the issue description still up-to-date? Or is this issue ready for manual testing?

@Nebel54 This is ready for manual testing in #88.

This needs a reroll, I'll take care of it.

Here's a reroll, I'm going to review the patch now.

Here is a first pass at reviewing the code and docs, I'll see about updating the patch with all or most of these changes.

1. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   + *     - title: The project tile.
   

   s/tile/title/

2. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   + *     - install_type: The type of project (i.e. dev).
   

   I think this should be "(e.g., dev)".

3. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   + *     - datestamp: The date/time of a project versions release.
   

   s/versions/version's/

4. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   + *     - includes: The projects within project.
   

   "within a project" maybe?

5. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   + *     - sub_themes: The sub themes declared for the project.
   

   We say 'subtheme', not 'sub theme'.

6. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   +    {{ 'Last checked: @time ago'|t({'@time': time}) }}
   ...
   +                    <li class="first odd">{{ 'Enabled: %includes'|t({'%includes': project.includes|join(', ')}) }}</li>
   +                    <li class="last even">{{ 'Disabled: %disabled'|t({'%disabled': project.disabled|join(', ')}) }}</li>
   ...
   +                  {{ 'Includes: %includes'|t({'%includes': project.includes|join(', ')}) }}
   ...
   +                  {{ 'Depends on: !basethemes'|t({'!basethemes': project.base_themes|join(', ')}) }}
   ...
   +                  {{ 'Required by: %subthemes'|t({'%subthemes': project.sub_themes|join(', ')}) }}
   

   These would all be a great fit for a trans block! https://drupal.org/node/2047135

7. +++ b/core/modules/update/templates/update-report.html.twig
   @@ -0,0 +1,115 @@
   +                  <span{{ project.status_attributes }}>{{ project.status_label|t }}</span>
   
   +++ b/core/modules/update/update.report.inc
   @@ -80,32 +77,63 @@ function theme_update_report($variables) {
   +        $row['status_label'] = 'Security update required!';
   ...
   +        $row['status_label'] = 'Revoked!';
   ...
   +        $row['status_label'] = 'Not supported!';
   ...
   +        $row['status_label'] = 'Update available';
   ...
   +        $row['status_label'] = 'Up to date';
   

   I see that these are translated in the template but that seems a strange approach, we don't usually put variables through t() and we are trained to put strings like this through t().

8. +++ b/core/modules/update/templates/update-version.html.twig
   @@ -0,0 +1,29 @@
   + * Available variables:
   + *   - attributes: HTML attributes for the update versions table.
   + *   - version_link: Link to this version's release notes on drupal.org.
   + *   - version_date: The date of the release.
   + *   - version_links: Links to download this version and to this version's release notes.
   + *   - tag: The title of the project.
   

   Everything underneath "Available variables" is indented 2 spaces too many.

9. +++ b/core/modules/update/update.report.inc
   @@ -80,32 +77,63 @@ function theme_update_report($variables) {
   +    if (!empty($project['reason'])) {
   +      $row['reason'] = check_plain($project['reason']);
   +    }
   +    else {
   +      $row['reason'] = '';
   +    }
   

   Seems like a good fit for a ternary.

10. +++ b/core/modules/update/update.report.inc
    @@ -302,42 +315,12 @@ function theme_update_report($variables) {
    + * Prepare variables for the version display of a project.
    

    s/Prepare/Prepares/ per https://drupal.org/node/1354#themepreprocess.

11. +++ b/core/modules/update/update.report.inc
    @@ -347,40 +330,28 @@ function theme_update_status_label($variables) {
    + *   - class: A array containing extra classes for the wrapping table.
    

    s/A/An/

12. +++ b/core/modules/update/update.report.inc
    @@ -347,40 +330,28 @@ function theme_update_status_label($variables) {
    +  $attributes = array('class' => array_merge(array('version'), $variables['class']));
    +  $variables['attributes'] = new Attribute($attributes);
    

    These lines can be combined and the Attribute object doesn't need to be instantiated here as of #1982024: Lazy-load Attribute objects later in the rendering process only if needed.

Did all the fixes up in except item 6 in #95. Thanks for the review @Cottser!

I don't think transblock is good for short text like that, it's great for long hunks of text but you're welcome to tackle that if you feel it will suite it better.

Here's the trans block stuff, I could go either way but I think this reads better. Also wrapping a slightly long comment at 80 chars.

+++ b/core/modules/update/update.report.inc
@@ -330,14 +325,12 @@ function template_preprocess_update_report(&$variables) {
- *   - class: A array containing extra classes for the wrapping table.
+ *   - class: An array containing extra classes for the wrapping table.
...
-  $attributes = array('class' => array_merge(array('version'), $variables['class']));
-  $variables['attributes'] = new Attribute($attributes);
-
+  $variables['attributes']['class'][] = 'version';

I think we need to pass in the 'class' variable for the attributes here.

Something like this maybe?

$variables['attributes']['class'] = $variables['class'];
$variables['attributes']['class'][] = 'version';

Anyway unassigning and back in your court @joelpittet :)

+++ b/core/modules/update/templates/update-report.html.twig
@@ -84,26 +86,44 @@
-                  {{ 'Includes: %includes'|t({'%includes': project.includes|join(', ')}) }}
...
+                    Includes: {{ includes|placeholder }}
...
-                  {{ 'Depends on: !basethemes'|t({'!basethemes': project.base_themes|join(', ')}) }}
...
+                    Depends on: {{ basethemes|passthrough }}
...
-                  {{ 'Required by: %subthemes'|t({'%subthemes': project.sub_themes|join(', ')}) }}
...
+                    Required by: {{ subthemes|placeholder }}

Also I think maybe base_themes should be placeholder instead of passthrough here unless there is something special about base_themes.

base_theme was sent through placeholder for the items and then through passthrough, so it is a special case. Thanks for spotting the class was missing. I unioned the arrays, that should do the trick and won't clobber any attributes passed through.

Sorry for my ignorance, but why are we introducing custom theme templates for those tables here?

There's hardly a case for overriding these tables in any way, so IMO those should simply be built + rendered via #type table, and done.

FWIW, that aspect applies to pretty much all tables. There are use-cases for manipulating classes, injecting/manipulating/removing table rows and columns, which is a typical preprocess/alter task, but otherwise, a table is a table. There's no use-case and thus no point in duplicating the basic HTML output of a table in individual templates.

The main reason we decided this for this table is that it's a single column table with lots of markup inside the cell. The idea was to make it easier for themers to replace the markup with a div or ul list structure that better suited it. Though a Views listing page would allow that too... and may better suit this if that is possible?

Also it's easier for us to get pass the performance gate with this then #type=>table currently as we are required to have each template profiled.

Maybe there is a compromise here... I don't want to hand code all the update detail markup in a controller but maybe that whole chunk in twig can be a new theme function and we can replace the table surrounding it with a #type=>table. And update_version would be a straight forward #type=>table. Deal?

Not sure whether this is what you meant, but

1. Can we move the inner markup (of a table cell in the loop; i.e., the output for a single project) into a separate template, so that it is decoupled from the outer/wrapping table?

   → That way, it would be easier to get rid of the wrapping table in a separate follow-up issue. (since I can't see why the wrapping output is a table to begin with)

2. Make update_version a simple #type table and just supply it as theme variable to aforementioned single project template?

Is that what you meant? :)

Lastly, @webchick recently worked on the port of Upgrade Status module to D7, in which we identified that the module still has to copy/fork a lot of code of Update module in core... Assigning this issue to her, in the hope that she might be able to give some feedback with regard to the template variable preprocessing (→ ideally the Upgrade Status module should be able to re-use this theme template as-is)

Yes, that is exactly what I meant:)

If you two are in agreement, sounds good to me. :) It's true that I did port upgrade status, but it was awhile back on weekends in between baby naps. :)

I'm SO sorry for letting this sit so long. :(

Assigning to tackle #102/103.

Not sure what to do with update_version so I just left it as a theme function. Any suggestions for that theme function please let me know, maybe a helper function or just leave it as a theme_function, or maybe extend #type=>table suggestion?.

This simplifies the preprocess by splitting the update_report into two templates. There is still some markup in the update-report.html.twig so I left the twig file for that.

Deal with 'theme_update_version'. I'm thinking private helper function is the way to go because it's just helping build a #type=>table now and likely doesn't need to drupal_render it.

Tagging for reroll.

Re-rolled.

The status report is rendering right for me

111: 1898466-twig-update-new-reroll-111.patch queued for re-testing.

rerol

please review since I'm learning here at #drupaldevdays

+++ b/core/modules/update/update.report.inc
@@ -346,41 +356,55 @@ function theme_update_status_label($variables) {
 function theme_update_version($variables) {
   $version = $variables['version'];
-  $tag = $variables['tag'];

We still have this theme_ function in there... not sure what to do with it yet... But maybe we can deal with it in a follow-up issue as this still get's rid of the markup that it was writing in strings in this. And all the other theme conversions?

Thanks for the re-roll @rodrigoaguilera!

Assigning to @webchick to see if that would be cool/kosher?

115: 1898466-twig-update-new-reroll-115.patch queued for re-testing.

I think given how massive this patch is already, it's fine to split that off into a separate issue. OTOH if it's easy to knock out, let's do it here. :)

I think this fail was a fleeting issue with the testbot so marking back to needs-review.

I'm planning to split off all these templates one by one because the re-roll is complicated and it's a big patch.

Here is #2264753: Convert theme_update_last_check to update-last-check.html.twig the first for theme_update_last_check, which was removed, but that may be too soon and can be consolidated later.

quickie reroll just in case because I was poking around this stuff for chx sandbox autoescape reasons.

aw hell, was working off the -99 patch. Please disregard that one.

In any case, it would be nice to have this stuff in because it will fix some of the breakage in the chx sandbox autoescape branch...

Reroll correct patch. Only change is fuzz removal...

There are several strings that need to be ucfirst. These are: warning, error, ok

@hass, this is a conversion issue. If that needs to happen it can be in a followup/separate issue. The before and after for manual testing should be identical in a conversion issue.

We convert D7 to D8, isn't it? This ucfirst is a D8 rule.

This is not D7 to D8 conversion. This is theme_* function to twig template conversion. For this type of issue, we try to avoid re-rolls by not doing scope changes that maybe applicable to other D8 issues as much as possible. Mostly because it effects manual testing and also may have an effect on xhprof profiling results as well.

When you create a followup could you also point in the issue summary of the new issue where ucfirst is a rule so we can point people to that standards change?

Tagging for another reroll.

Rerolled.

Reroll. I messed up on a conflict.

Attaching the interdiff. Accidentally attached the previous patch instead of the interdiff to the last post.

1. +++ b/core/modules/update/update.report.inc
   @@ -304,38 +122,243 @@ function theme_update_report($variables) {
   +  $project =& $variables['project'];
   

   I don't think there is a coding standard set, but = &$ is usually preferred.

2. +++ b/core/modules/update/update.report.inc
   @@ -304,38 +122,243 @@ function theme_update_report($variables) {
   +      $uri = 'core/misc/watchdog-warning.png';
   

   The image paths have changed.

@lokapujya you are totally right on the referenced variable convention, there is 34 of the way I did it and 168 of the way you mentioned. = &$ wins:)

Thanks for catching that image patch change, that will need to go.

I've split out theme_update_version into it's own contentious issue: #2264833: Convert theme_update_version to update-version.html.twig So it no longer needs to live in this patch.

I'd like to maybe move some of the other theme functions to their own issue as well to slim jim this patch down a bit for easier review. Thoughts?

In #1825952: Turn on twig autoescape by default, in theme_update_report(), we are marking the $data array of project status (wrapped in a <p> tag) as safe. Once that goes in, we should check with whatever is providing the $data variable to ensure that we don't run into double-escaping. Adding a reference to this issue in that patch.

Adding issue relationship to #1825952: Turn on twig autoescape by default.

Reroll of 134 patch, considered the notes above.

Messed up a little in previous patch, fix exceptions.

Thanks @lokeoke, great to add interdiffs when updating patches, as a rule upload them every time you change a patch, unless you are doing a reroll. Thanks again :D

Here is interdiff, i messed up with an Attribute objects and didn't remove theme_update_status_label when rerolling the patch.

@lokeoke That's awsome thank you! Here is a few more changes from that. I did a manual test which seemed to look good but probably needs a few more scenarios added to the issue summary because by default you only get that Drupal Core is out of date at the moment.

Also I added a nice syntax sugar in twig the for..else
I so wanted to use it and seems like a nice place to try it out.

http://twig.sensiolabs.org/doc/tags/for.html#the-else-clause

Forgot some p tags after testing the no updates works. And changed the variable name because it didn't make much sense before.

Here's a round of manual testing. The markup doesn't change.
I normalized the whitespace for comparison and uploading both versions. (norm = normalize;)

Noticed the images are broken but that is broken before this patch too so just a note and I'll likely need to open up a new issue if there isn't one already.

Reviewing this in the next hour or so.

Manual testing looks good -- one extra whitespace in output seems worth it for a clean template.

Attached patch addresses these points:

1. --- /dev/null
   +++ b/core/modules/update/templates/update-project-status.html.twig
   @@ -0,0 +1,112 @@
   +{#
   +/**
   + * @file
   + * Default theme implementation for the project status report.
   + *
   + * Available variables:
   + * - title: The project title.
   + * - url: The project url.
   + * - status: The project status.
   + *   - label: The project status label.
   + *   - attributes: HTML attributes for the project status.
   + *   - reason: The reason you should update the project.
   + *   - icon: The project status version indicator icon.
   + * - existing_version: The version of the installed project.
   + * - versions: The available versions of the project.
   + * - install_type: The type of project (e.g., dev).
   + * - datestamp: The date/time of a project version's release.
   + * - extras: HTML attributes and additional information about the project.
   + *   - attributes: HTML attributes for the extra item.
   + *   - label: The label for an extra item.
   + *   - data: The data about an extra item.
   + * - includes: The projects within a project.
   + * - disabled: The disabled included projects.
   + * - base_themes: The base theme declared for the project.
   + * - sub_themes: The subthemes declared for the project.
   + *
   + * @see template_preprocess_update_project_status()
   + *
   + * @ingroup themeable
   + */
   +#}
   +<div class="version-status">
   +  {%- if status.label -%}
   +    <span{{ status.attributes }}>{{ status.label }}</span>
   +  {%- else -%}
   +    {{ status.reason|e }}
   +  {%- endif %}
   +  <span class="icon">{{ status.icon }}</span>
   +</div>
   +
   +<div class="project">
   +  {%- if url -%}
   +    <a href="{{ url }}">{{ title|e }}</a>
   +  {%- else -%}
   +    {{ title|e }}
   +  {%- endif %}
   +  {{ existing_version|e }}
   +  {% if install_type == 'dev' and datestamp %}
   +    <span class="version-date">({{ datestamp }})</span>
   +  {% endif %}
   +</div>
   +
   +{% if versions %}
   +  <div class="versions">
   +    {% for version in versions %}
   +      {{ version }}
   +    {% endfor %}
   +  </div>
   +{% endif %}
   +
   +<div class="info">
   +  {% if extras %}
   +    <div class="extra">
   +      {% for extra in extras %}
   +        <div{{ extra.attributes }}>
   +          {{ extra.label|e }}: {{ extra.data }}
   +        </div>
   +      {% endfor %}
   +    </div>
   +  {% endif %}
   +  <div class="includes">
   +    {% set includes = includes|join(', ') %}
   +    {% if disabled %}
   +      {{ 'Includes:'|t }}
   +      <ul>
   +        <li class="first odd">
   +          {% trans %}
   +            Enabled: {{ includes|placeholder }}
   +          {% endtrans %}
   +        </li>
   +        <li class="last even">
   +          {% set disabled = disabled|join(', ') %}
   +          {% trans %}
   +            Disabled: {{ disabled|placeholder }}
   +          {% endtrans %}
   +        </li>
   +      </ul>
   +    {% else %}
   +      {% trans %}
   +        Includes: {{ includes|placeholder }}
   +      {% endtrans %}
   +    {% endif %}
   +  </div>
   +
   +  {% if base_themes %}
   +    {% set basethemes = base_themes|join(', ') %}
   +    <div class="basethemes">
   +      {% trans %}
   +        Depends on: {{ basethemes|passthrough }}
   +      {% endtrans %}
   +    </div>
   +  {% endif %}
   +
   +  {% if sub_themes %}
   +    {% set subthemes = sub_themes|join(', ') %}
   +    <div class="subthemes">
   +      {% trans %}
   +        Required by: {{ subthemes|placeholder }}
   +      {% endtrans %}
   +    </div>
   +  {% endif %}
   +</div>
   

   1. Available variables, minor grammar:
   - inludes: "The projects within a project." ==> "The projects within the project."
   - disabled: "The disabled included projects." ==> "The currently disabled projects in the project."
   - base_themes: "The base theme declared for the project." ==> "The base themes supplied by the project."
   - sub_themes: "The subthemes declared for the project." ==> "The subthemes supplied by the project."
   2. |e escape filters can be removed here because of #1825952: Turn on twig autoescape by default
   3. Since it looks strange, add a comment about why we use |passthrough instead of |placeholder here?

2. +++ b/core/modules/update/update.report.inc
   @@ -49,298 +53,302 @@ function theme_update_report($variables) {
   +/**
   + * Prepares variables for update project status templates.
   + *
   + * Default template: update-project-status.html.twig.
   + *
   + * @param array $variables
   + *   An associative array containing:
   + *   - data: An array of data about each project's status.
   + *   - project_status:
   + *
   + * @ingroup themeable
   + */
   

   @param $variables not finished.

3. Uninitialized $status variable in template_preprocess_update_report().

Since all the changes in my last review-patch are minor grammar changes and removal of unneeded |escape filters, I think this is ready to go! Nice work, everyone! (also linked this to the broken images issue).

Committed c2e66f9 and pushed to 8.x. Thanks!

diff --git a/core/modules/update/update.report.inc b/core/modules/update/update.report.inc
index 1dd2e76..bf3d6ae 100644
--- a/core/modules/update/update.report.inc
+++ b/core/modules/update/update.report.inc
@@ -5,8 +5,6 @@
  * Code required only when rendering the available updates report.
  */

-use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Component\Utility\String;
 use Drupal\Core\Template\Attribute;

 /**

Fixed on commit.