Replace theme() with drupal_render() in update module

Here's a patch

Hello.

It looks good to me. Maybe you could add a minor improvement.

+++ b/core/modules/update/update.report.incundefined
@@ -40,33 +44,51 @@ function theme_update_report($variables) {
+        $image['#uri'] = 'core/misc/watchdog-error.png';
+        $image['#alt'] = t('error');
+        $image['#title'] = t('error');

You could save some lines and avoid misspellings assigning values for '#alt' and '#title' in the same line $image['#alt'] = $image['#title'] = t('error');.

Regards.

Trying to patch the patch (core office hours - nick cconrad - assigned by heddn)

I have to say I disagree. For one, I don't think of "saving lines" as a worthwhile goal, since whitespace generally helps readability, and two, i think

$image['#alt'] = $image['#title'] = t('error');

is less clear (less readable) than

$image['#alt'] = t('error');
$image['#title'] = t('error');

Also, maybe someday we'll want to have different values or remove one of these. This makes it easier to change and makes the patch more readable too.

Implemented the improvement suggested by plopesc in #2

I patched this because I got it in Drupal Office Hours, but I tend to agree with pplantinga, the original patch is more readable.

How about this compromise - avoids misspellings, only one call to t(), and better readability?

$t_error = t('error');
$image['#alt'] = $t_error;
$image['#title'] = $t_error;

Looks like you tried to edit the patch directly? Trying to apply the patch in #7 gave: fatal: corrupt patch at line 128

Git tends to not like it when you edit a patch, so it's better to apply the patch, make the changes, and generate a new patch.

I could live with the changes suggested in #7, when there's a valid patch.

Right. I'll get back to this with a fixed patch.

Hello

@pplantinga:
I understand your point. It was a suggestion, that you can take or not. Maybe other more experienced contributors could give their opinion in order to follow one approach or the other.

@cconrad:
Thanks for your approach, It's a different point and it will be take into account.

Different points of view are good and that's the way to improve the Drupal code, everyone can say and then, decide the best option.

Regards

In case you decide to use the "compromise" from #7, here's a fixed patch that hopefully passes.

Hi,

Here are my two cents about this issue. This patch moves the $image array definition below the switch call. Then, the whole renderable array is built in a single step, and its properties are declared in that switch depending on the project status.

What do you think about it?

Regards.

Sorry, my previous interdiff was bad named.

Attaching with correct name.

Looks good to me. Let's see if testbot likes it.

Looks like testbot likes it too.

+++ b/core/modules/update/update.report.incundefined
@@ -43,30 +47,48 @@ function theme_update_report($variables) {
+        $icon = drupal_render($image);
...
+        $icon = drupal_render($image);
...
+        $icon = drupal_render($image);
...
+        $icon = drupal_render($image);
...
+    $image = array(
+      '#theme' => 'image',
+      '#width' => 18,
+      '#height' => 18,
+      '#uri' => $uri,
+      '#alt' => $text,
+      '#title' => $text,
+    );

This seems to be occurring in the wrong place. the first time around $image is not going to exist...

+++ b/core/modules/update/update.report.incundefined
@@ -43,30 +47,48 @@ function theme_update_report($variables) {
     $row .= '<span class="icon">' . $icon . '</span>';

Just do this instead

  $row .= '<span class="icon">' . drupal_render($image) . '</span>';

D'oh!

It was fast patch without the necessary review from my side.

Re-rolled.

Thanks for your hard work @alexpott

Thanks for the quick update @plopesc

+++ b/core/modules/update/update.manager.incundefined
@@ -257,8 +257,13 @@ function update_manager_update_form($form, $form_state = array(), $context) {
     $form['manual_updates'] = array(
-      '#markup' => theme('table', array('header' => $headers, 'rows' => $projects['manual'])),

Is it really necessary to use #markup, why not have the table as a child and let it get rendered later?

@catch, you're right, It's not necessary ;)

Re-rolling using '#type' => 'table' instead of '#markup' => drupal_render()

Thanks

Why is one of the tables using #theme and another using #type?

Both should probably use #type, as per #1876712: [meta] Convert all tables in core to new #type 'table'.

Also, as I understand from reading over that issue, #type 'table' expects rows to be provided as child elements rather than in #rows.

Actually, the patches would be easier to review if we kept the tables as #theme 'table' for now, and let the other meta issue handle converting from #theme to #type - that's how other patches on sibling issues to this issue have handled tables.

How about this?

seems fair.

Committed 53731f9 and pushed to 8.x. Thanks!