Issue #1898466 by joelpittet, webthingee, W1n5t0n45, ry5n, ezeedub, lokeoke, Cottser | 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/updates 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 Moved to it's own issue: #2264833: Convert theme_update_version to update-version.html.twig

#1757550: [Meta] Convert core theme functions to Twig templates
#1938934: Convert update theme tables to table #type
#2298039: Remove all PNG fallbacks for SVG
Followup: #2099293: Replace table with available module updates with better markup
Followup: #2287031: Change lcase translatable strings to ucfirst

Files: 
CommentFileSizeAuthor
#151 update_module_convert-1898466-151.patch27.94 KBsteveoliver
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,284 pass(es). View
#151 update_module_convert-1898466-148-151--interdiff-do-not-test.patch3.54 KBsteveoliver
#149 update-report.png247.21 KBjoelpittet
#149 updates-after-norm.txt24.51 KBjoelpittet
#149 updates-before-norm.txt24.27 KBjoelpittet
#149 updates-after.txt24.93 KBjoelpittet
#149 updates-before.txt24.6 KBjoelpittet
#148 update_module_convert-1898466-148.patch27.56 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,971 pass(es). View
#148 interdiff.txt1.43 KBjoelpittet
#147 update_module_convert-1898466-147.patch27.54 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,969 pass(es). View
#147 interdiff.txt2.49 KBjoelpittet
#146 interdiff-1898466-142-143.txt2.91 KBl0ke
#144 1898466-143.patch26.33 KBl0ke
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,224 pass(es). View
#135 interdiff-1898466-133-134.txt741 byteslokapujya
#134 1898466-134.patch31.73 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,727 pass(es). View
#133 1898466-133.patch31.74 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,821 pass(es), 1 fail(s), and 28 exception(s). View
#126 1898466-twig-reroll-correct-version.patch31.75 KBbdragon
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,716 pass(es). View
#124 1898466-twig-update-124.patch26.13 KBbdragon
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,717 pass(es). View
#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

c4rl’s picture

Issue tags: +Twig

Tagging

designtodigital’s picture

Assigned: Unassigned » designtodigital

Assigning this to me.

myers_d’s picture

Assigned: designtodigital » myers_d

Reassigning this issue to my personal account.

tlattimore’s picture

Assigned: myers_d » tlattimore

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

myers_d’s picture

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.

carsonblack’s picture

Assigned: tlattimore » carsonblack
W1n5t0n45’s picture

Assigned: carsonblack » tlattimore
Status: Active » Needs review
FileSize
2.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.

carsonblack’s picture

Assigned: tlattimore » carsonblack
Status: Needs work » Active

I have theme_update_last_check() and theme_update_version() converted.

tlattimore’s picture

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

carsonblack’s picture

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

Cottser’s picture

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.

joelpittet’s picture

Status: Active » Needs review
FileSize
6.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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.75 KB
PASSED: [[SimpleTest]]: [MySQL] 53,889 pass(es). View
3.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.

Cottser’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

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

joelpittet’s picture

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

joelpittet’s picture

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

ry5n’s picture

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.

ry5n’s picture

Issue summary: View changes

added type table for update_version

joelpittet’s picture

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!

ry5n’s picture

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

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

joelpittet’s picture

FileSize
14.99 KB
PASSED: [[SimpleTest]]: [MySQL] 54,351 pass(es). View
7.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()

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

FileSize
18.83 KB
27.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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
27.72 KB
FAILED: [[SimpleTest]]: [MySQL] 54,391 pass(es), 6 fail(s), and 1 exception(s). View
8.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.

carsonblack’s picture

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!

joelpittet’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
28.81 KB
FAILED: [[SimpleTest]]: [MySQL] 54,478 pass(es), 3 fail(s), and 0 exception(s). View
10.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.

Anonymous’s picture

Issue summary: View changes

Add conversion summary table

Cottser’s picture

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.

joelpittet’s picture

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

Cottser’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
28.78 KB
FAILED: [[SimpleTest]]: [MySQL] 54,558 pass(es), 2 fail(s), and 0 exception(s). View
864 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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
28.63 KB
FAILED: [[SimpleTest]]: [MySQL] 54,466 pass(es), 2 fail(s), and 0 exception(s). View
2.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

joelpittet’s picture

FileSize
2.24 KB
28.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?

Cottser’s picture

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

joelpittet’s picture

FileSize
938 bytes
28.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,773 pass(es). View
tlattimore’s picture

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

thedavidmeister’s picture

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.

thedavidmeister’s picture

Issue summary: View changes

Update conversion table

c4rl’s picture

Title: Convert update module to Twig » update.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, which are lower in priority than PHPTemplate conversion issues.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue tags: +Novice
webthingee’s picture

Assigned: Unassigned » webthingee
FileSize
28.42 KB
28.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.

webthingee’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
28.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.

webthingee’s picture

FileSize
2.05 KB
28.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,837 pass(es). View

update comments

webthingee’s picture

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

tlattimore’s picture

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.

webthingee’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
34.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....

ezeedub’s picture

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

ezeedub’s picture

Issue summary: View changes

Remove sandbox link

joelpittet’s picture

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

tlattimore’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
28.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.

joelpittet’s picture

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

siccababes’s picture

Assigned: webthingee » siccababes

hello i am calling dibs

siccababes’s picture

FileSize
1.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!

siccababes’s picture

Assigned: siccababes » Unassigned
siccababes’s picture

Status: Needs work » Needs review

testbot go!

Status: Needs review » Needs work

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
27.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.

pplantinga’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Twig, +@todo clean-up
FileSize
829 bytes
27.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...

adamcowboy’s picture

Status: Needs review » Reviewed & tested by the community

The status report page renders correctly.

jenlampton’s picture

Issue tags: -@todo clean-up

un tagging

alexpott’s picture

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

siccababes’s picture

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.

joelpittet’s picture

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

drupalmonkey’s picture

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

Patch rerolled.

drupalmonkey’s picture

FileSize
2.06 KB
4.44 KB
24.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

joelpittet’s picture

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!

Cottser’s picture

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

Tagging for reroll.

pplantinga’s picture

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

Reroll

joelpittet’s picture

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.

richardj’s picture

Assigned: Unassigned » richardj
richardj’s picture

FileSize
1.74 KB
1.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.

richardj’s picture

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.

richardj’s picture

FileSize
24.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es). View
1.74 KB

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

pplantinga’s picture

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

Another shot at it.

pplantinga’s picture

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

richardj’s picture

evanbarter’s picture

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

drumm’s picture

Issue summary: View changes

how to test

joelpittet’s picture

Nebel54’s picture

Issue summary: View changes

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?

joelpittet’s picture

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

joelpittet’s picture

Cottser’s picture

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

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

Cottser’s picture

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

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

Cottser’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
24.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,380 pass(es). View
5.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.

Cottser’s picture

Assigned: Cottser » Unassigned
Issue tags: -Novice
FileSize
125.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,202 pass(es). View
3.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 :)

Cottser’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
25.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es). View
751 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.

sun’s picture

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.

joelpittet’s picture

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.

joelpittet’s picture

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?

sun’s picture

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)

joelpittet’s picture

Yes, that is exactly what I meant:)

Cottser’s picture

Issue tags: +Twig conversion
webchick’s picture

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

joelpittet’s picture

Assigned: Unassigned » joelpittet

Assigning to tackle #102/103.

joelpittet’s picture

Assigned: joelpittet » Unassigned
FileSize
31.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.

joelpittet’s picture

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.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
31.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.

rodrigoaguilera’s picture

Status: Needs review » Reviewed & tested by the community

The status report is rendering right for me

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

rodrigoaguilera’s picture

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

rerol

rodrigoaguilera’s picture

Status: Needs work » Needs review

please review since I'm learning here at #drupaldevdays

joelpittet’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

webchick’s picture

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

cwells’s picture

Status: Needs work » Needs review

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

joelpittet’s picture

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.

bdragon’s picture

FileSize
26.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,717 pass(es). View

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

bdragon’s picture

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

bdragon’s picture

FileSize
31.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,716 pass(es). View

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

hass’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review

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

hass’s picture

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

joelpittet’s picture

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?

hass’s picture

Issue summary: View changes
Cottser’s picture

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

Tagging for another reroll.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
31.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,821 pass(es), 1 fail(s), and 28 exception(s). View

Rerolled.

lokapujya’s picture

FileSize
31.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,727 pass(es). View
31.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,737 pass(es), 0 fail(s), and 26 exception(s). View

Reroll. I messed up on a conflict.

lokapujya’s picture

FileSize
741 bytes

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

The last submitted patch, 133: 1898466-133.patch, failed testing.

lokapujya’s picture

Status: Needs review » Needs work
  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.

The last submitted patch, 134: 1898466-133.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Issue tags: +Novice

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

xjm’s picture

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.

steveoliver’s picture

l0ke’s picture

Status: Needs work » Needs review
FileSize
25.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,006 pass(es), 0 fail(s), and 38 exception(s). View

Reroll of 134 patch, considered the notes above.

Status: Needs review » Needs work

The last submitted patch, 142: 1898466-142.patch, failed testing.

l0ke’s picture

Status: Needs work » Needs review
FileSize
26.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,224 pass(es). View

Messed up a little in previous patch, fix exceptions.

Cottser’s picture

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

l0ke’s picture

Issue summary: View changes
FileSize
2.91 KB

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

joelpittet’s picture

Issue summary: View changes
FileSize
2.49 KB
27.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,969 pass(es). View

@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

joelpittet’s picture

FileSize
1.43 KB
27.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,971 pass(es). View

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

joelpittet’s picture

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.

steveoliver’s picture

Assigned: Unassigned » steveoliver

Reviewing this in the next hour or so.

steveoliver’s picture

Assigned: steveoliver » Unassigned
FileSize
3.54 KB
27.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,284 pass(es). View

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().
steveoliver’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Novice
Related issues: +#2298039: Remove all PNG fallbacks for SVG

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed c2e66f9 on 8.0.x
    Issue #1898466 by joelpittet, webthingee, lokapujya, pplantinga, lokeoke...

Status: Fixed » Closed (fixed)

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

m1r1k’s picture

Issue tags: +#ams2014contest