Issue #1898466 by joelpittet, webthingee, W1n5t0n45, ry5n, ezeedub | c4rl: Update.module - Convert theme_ functions to Twig.

Task

Use twig templates instead of theme_ functions for the update module.

To test this code

  • Visit admin/reports/status or Reports > Status report and make sure that your report renders correctly

Theme function name/template path Conversion status
theme_update_last_check converted
theme_update_manager_update_form Removed, needed markup moved into form callback.
theme_update_report converted, troubleshooting
theme_update_status_label converted
theme_update_version converted

#1757550: [META-63] Convert core theme functions to Twig templates
#1938934: Convert update theme tables to table #type
Followup: #2099293: Update Module Twig Clean Up

Files: 
CommentFileSizeAuthor
#115 1898466-twig-update-new-reroll-115.patch31.75 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,707 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#111 1898466-twig-update-new-reroll-111.patch31.63 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1898466-twig-update-new-reroll-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#108 1898466-twig-update-108.patch31.63 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es).
[ View ]
#99 interdiff.txt751 bytesjoelpittet
#99 1898466-99-twig-update.patch25.24 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]
#97 interdiff.txt3.43 KBCottser
#97 1898466-97.patch125.24 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,202 pass(es).
[ View ]
#96 interdiff.txt5.97 KBjoelpittet
#96 1898466-96-twig-update.patch24.65 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,380 pass(es).
[ View ]
#94 1898466-94.patch24.79 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,188 pass(es).
[ View ]
#88 twig-update_conversion-1898466-88.patch25.01 KBevanbarter
PASSED: [[SimpleTest]]: [MySQL] 58,037 pass(es).
[ View ]
#85 twig-update_conversion-1898466-84.patch39.65 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]
#84 interdiff-1898466-77-80.txt1.74 KBrichardj
#84 twig-update_conversion-1898466-84.patch24.92 KBrichardj
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es).
[ View ]
#80 twig-update_conversion-1898466-80.patch1.74 KBrichardj
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-update_conversion-1898466-80.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#80 interdiff-1898466-77-80.txt1.74 KBrichardj
#77 twig-update_conversion-1898466-77.patch24.99 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 59,102 pass(es).
[ View ]
#74 twig-update_conversion-1898466-74.patch24.99 KBdrupalmonkey
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]
#74 interdiff.txt4.44 KBdrupalmonkey
#74 update-testing-patch.txt2.06 KBdrupalmonkey
#73 twig-update_conversion-1898466-73.patch25.6 KBdrupalmonkey
PASSED: [[SimpleTest]]: [MySQL] 57,875 pass(es).
[ View ]
#67 twig-update_conversion-1898466-67.patch27.61 KBpplantinga
PASSED: [[SimpleTest]]: [MySQL] 56,803 pass(es).
[ View ]
#67 interdiff.txt829 bytespplantinga
#64 twig-update_conversion-1898466-64.patch27.54 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 58,181 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#60 twig-1898466-60.patch1.37 MBsiccababes
FAILED: [[SimpleTest]]: [MySQL] 56,626 pass(es), 572 fail(s), and 136 exception(s).
[ View ]
#57 1898466-57.patch28.92 KBtlattimore
PASSED: [[SimpleTest]]: [MySQL] 55,316 pass(es).
[ View ]
#57 interdiff.txt6.28 KBtlattimore
#54 1898466-54-twig-updatemodule.patch34.31 KBwebthingee
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]
#54 interdiff.txt3.25 KBwebthingee
#51 1898466-51-twig-updatemodule.patch28.91 KBwebthingee
PASSED: [[SimpleTest]]: [MySQL] 55,837 pass(es).
[ View ]
#51 interdiff.txt2.05 KBwebthingee
#49 1898466-49-updatemodule-reroll.patch28.52 KBwebthingee
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#49 interdiff.txt1.37 KBwebthingee
#47 1898466-47-twig-update.patch28.22 KBwebthingee
FAILED: [[SimpleTest]]: [MySQL] 56,204 pass(es), 6 fail(s), and 3 exception(s).
[ View ]
#47 innerdiff.txt28.42 KBwebthingee
#42 1898466-42-twig-update.patch28.2 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,773 pass(es).
[ View ]
#42 interdiff.txt938 bytesjoelpittet
#40 1898466-40-twig-update.patch28.9 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,522 pass(es).
[ View ]
#40 interdiff.txt2.24 KBjoelpittet
#39 interdiff.txt2.55 KBjoelpittet
#39 1898466-39-twig-update-with-1867090-25.patch28.63 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,466 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#37 interdiff.txt864 bytesjoelpittet
#37 1898466-37-twig-update-with-1867090-25.patch28.78 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,558 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt10.97 KBjoelpittet
#32 1898466-32.twig-update.patch28.81 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,478 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#28 interdiff.txt8.16 KBjoelpittet
#28 twig-update-1898466-28.patch27.72 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,391 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#26 twig-update-1898466-26.patch27.5 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,331 pass(es), 18 fail(s), and 1 exception(s).
[ View ]
#26 interdiff.txt18.83 KBjoelpittet
#24 interdiff.txt7.48 KBjoelpittet
#24 twig-update-1898466-24.patch14.99 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,351 pass(es).
[ View ]
#20 twig-update-report-20-inprogress.patch.txt36.39 KBry5n
#20 twig-update-report-15-to-20.interdiff.txt29.61 KBry5n
#15 interdiff.txt3.66 KBjoelpittet
#15 twig-update-1898466-15.patch8.75 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 53,889 pass(es).
[ View ]
#13 twig-update-1898466-13.patch6.98 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,646 pass(es), 11 fail(s), and 36 exception(s).
[ View ]
#7 update-twig.patch2.08 KBW1n5t0n45
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update-twig.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» designtodigital

Assigning this to me.

Reassigning this issue to my personal account.

Assigned:myers_d» tlattimore

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.

Assigned:tlattimore» carsonblack

Assigned:carsonblack» tlattimore
Status:Active» Needs review
StatusFileSize
new2.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update-twig.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, update-twig.patch, failed testing.

Assigned:tlattimore» carsonblack
Status:Needs work» Active

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.

Status:Active» Needs review
StatusFileSize
new6.98 KB
FAILED: [[SimpleTest]]: [MySQL] 53,646 pass(es), 11 fail(s), and 36 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, twig-update-1898466-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.75 KB
PASSED: [[SimpleTest]]: [MySQL] 53,889 pass(es).
[ View ]
new3.66 KB

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.

Status:Needs review» Needs work

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

Assigned:carsonblack» Unassigned

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.

Issue summary:View changes

added type table for update_version

Assigned:Unassigned» joelpittet

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.

Issue tags:+Needs manual testing

Tagging.

StatusFileSize
new14.99 KB
PASSED: [[SimpleTest]]: [MySQL] 54,351 pass(es).
[ View ]
new7.48 KB

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

Status:Needs work» Needs review

StatusFileSize
new18.83 KB
new27.5 KB
FAILED: [[SimpleTest]]: [MySQL] 54,331 pass(es), 18 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, twig-update-1898466-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.72 KB
FAILED: [[SimpleTest]]: [MySQL] 54,391 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
new8.16 KB

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.

Status:Needs review» Needs work

The last submitted patch, twig-update-1898466-28.patch, failed testing.

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.

Status:Needs work» Needs review
Issue tags:-Needs manual testing
StatusFileSize
new28.81 KB
FAILED: [[SimpleTest]]: [MySQL] 54,478 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new10.97 KB

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.

Status:Needs review» Needs work

The last submitted patch, 1898466-32.twig-update.patch, failed testing.

Issue summary:View changes

Add conversion summary table

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.

Status:Needs work» Needs review
StatusFileSize
new28.78 KB
FAILED: [[SimpleTest]]: [MySQL] 54,558 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new864 bytes

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

Still needs #34 With syntax #26

Status:Needs review» Needs work

The last submitted patch, 1898466-37-twig-update-with-1867090-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.63 KB
FAILED: [[SimpleTest]]: [MySQL] 54,466 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.55 KB

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

StatusFileSize
new2.24 KB
new28.9 KB
PASSED: [[SimpleTest]]: [MySQL] 54,522 pass(es).
[ View ]

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.

<?php
$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.

StatusFileSize
new938 bytes
new28.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,773 pass(es).
[ View ]

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

Issue tags:+@todo clean-up

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

Issue summary:View changes

Update conversion table

Title:Convert update module to Twigupdate.module - Convert theme_ functions to Twig

Per #1757550-44: [META-63] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

Assigned:joelpittet» Unassigned
Issue tags:+Novice

Assigned:Unassigned» webthingee
StatusFileSize
new28.42 KB
new28.22 KB
FAILED: [[SimpleTest]]: [MySQL] 56,204 pass(es), 6 fail(s), and 3 exception(s).
[ View ]

Re Roll from #42

Will be working on the updates next.

Status:Needs review» Needs work

The last submitted patch, 1898466-47-twig-update.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
new28.52 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1898466-49-updatemodule-reroll.patch, failed testing.

StatusFileSize
new2.05 KB
new28.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,837 pass(es).
[ View ]

update comments

Status:Needs work» Needs review

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

ready for some review!! :P

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new3.25 KB
new34.31 KB
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]

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

Issue summary:View changes

Remove sandbox link

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.28 KB
new28.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,316 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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

Assigned:webthingee» siccababes

hello i am calling dibs

StatusFileSize
new1.37 MB
FAILED: [[SimpleTest]]: [MySQL] 56,626 pass(es), 572 fail(s), and 136 exception(s).
[ View ]

I tried to reroll the patch. Hope it works!

Assigned:siccababes» Unassigned

Status:Needs work» Needs review

testbot go!

Status:Needs review» Needs work

The last submitted patch, twig-1898466-60.patch, failed testing.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new27.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,181 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Novice, -Twig, -@todo clean-up

The last submitted patch, twig-update_conversion-1898466-64.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +Twig, +@todo clean-up
StatusFileSize
new829 bytes
new27.61 KB
PASSED: [[SimpleTest]]: [MySQL] 56,803 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

The status report page renders correctly.

Issue tags:-@todo clean-up

un tagging

Status:Reviewed & tested by the community» Needs review

+++ 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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new25.6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,875 pass(es).
[ View ]

Patch rerolled.

StatusFileSize
new2.06 KB
new4.44 KB
new24.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,163 pass(es).
[ View ]

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

Issue tags:+Needs manual testing

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

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

Tagging for reroll.

Status:Needs work» Needs review
StatusFileSize
new24.99 KB
PASSED: [[SimpleTest]]: [MySQL] 59,102 pass(es).
[ View ]

Reroll

Issue tags:-Needs 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.

Assigned:Unassigned» richardj

StatusFileSize
new1.74 KB
new1.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-update_conversion-1898466-80.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

- Removed @see template_preprocess
- Fixed indenting

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

The last submitted patch, twig-update_conversion-1898466-80.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, twig-update_conversion-1898466-80.patch, failed testing.

StatusFileSize
new24.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es).
[ View ]
new1.74 KB

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

Status:Needs work» Needs review
StatusFileSize
new39.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]

Another shot at it.

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

StatusFileSize
new25.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,037 pass(es).
[ View ]

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.

Issue summary:View changes

how to test

Issue summary:View changes

added a follow-up issue to description #2099293: Update Module Twig Clean Up

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.

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

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

Status:Needs work» Needs review
StatusFileSize
new24.79 KB
PASSED: [[SimpleTest]]: [MySQL] 59,188 pass(es).
[ View ]

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

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new24.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,380 pass(es).
[ View ]
new5.97 KB

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.

Assigned:Cottser» Unassigned
Issue tags:-Novice
StatusFileSize
new125.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,202 pass(es).
[ View ]
new3.43 KB

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new25.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]
new751 bytes

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?

Assigned:Unassigned» webchick

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

Issue tags:+Twig conversion

Assigned:webchick» Unassigned

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. :(

Assigned:Unassigned» joelpittet

Assigning to tackle #102/103.

Assigned:joelpittet» Unassigned
StatusFileSize
new31.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es).
[ View ]

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.

Status:Needs review» Needs work

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.

Issue tags:+Needs reroll

Tagging for reroll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new31.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1898466-twig-update-new-reroll-111.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled.

Status:Needs review» Reviewed & tested by the community

The status report is rendering right for me

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 111: 1898466-twig-update-new-reroll-111.patch, failed testing.

StatusFileSize
new31.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,707 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

rerol

Status:Needs work» Needs review

please review since I'm learning here at #drupaldevdays

Assigned:Unassigned» webchick

+++ 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?

Status:Needs review» Needs work

The last submitted patch, 115: 1898466-twig-update-new-reroll-115.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 115: 1898466-twig-update-new-reroll-115.patch, failed testing.

Assigned:webchick» Unassigned

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

Status:Needs work» Needs review

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