Replace theme() with drupal_render() in system module for system_status()

Can you pot a patch and we'll see how many tests are broken first.
// code wins

I am working on this. screenshot of errors displayed after these modifications attached along with the patch.

So here's the patch.

+++ b/core/modules/system/system.admin.incundefined
@@ -1304,7 +1304,12 @@ function system_status($check = FALSE) {
-  return theme('status_report', array('requirements' => $requirements));
+  $status_report = array('#theme' => 'status_report');

should return render array

return array(
  '#theme' => 'status_report',
  'requirements' => $requirements,
);

+++ b/core/modules/system/system.admin.incundefined
@@ -1556,14 +1561,16 @@ function theme_status_report($variables) {
-      $output .= '<tr class="' . $severity['class'] . '">';
...
+      if(!empty($requirement['title'])) {
+        $output .= '<tr class="' . $severity['class'] . '">';

any reason to check title?
Please try not touch this

related issue #1938930-10: Convert theme_system_modules_details() to #type table

Suppose better of theme_status_report() at all

@Joe9 please re-roll #7

For theme function filed #2013258: Simplify render for theme_status_report()

@andypost I would need to target status() instead of system_status() now.

@andypost Here's the re-roll for #7.

+++ b/core/modules/system/system.admin.incundefined
@@ -1556,14 +1561,16 @@ function theme_status_report($variables) {
-      $output .= '<tr class="' . $severity['class'] . '">';
...
+      if(!empty($requirement['title'])) {
+        $output .= '<tr class="' . $severity['class'] . '">';

was to avoid blank rows rendered at the bottom (pls ref. attach image)

Better to make Controller to return render array

+++ b/core/modules/system/system.module
@@ -161,7 +161,7 @@ function system_theme() {
+      'variables' => array('requirements' => NULL),

Super minor, but would an empty array make more sense here?

Otherwise makes total sense!

@tstoeckler following hook_theme() all core defines default as NULL, so not sure it makes sense to introduce new kind of default value

OK, fine with me. Looks RTBC to me, but I'll let someone else pull the trigger. :-)

Shouldn't we be using drupal_render?

instead of:

-    return theme('status_report', array('requirements' => $requirements));
+    return array('#theme' => 'status_report', '#requirements' => $requirements);

use:

$status_report('#theme' => 'status_report', '#requirements' => $requirements);
return drupal_render($status_report);

Shouldn't we be using drupal_render?

instead of:

-    return theme('status_report', array('requirements' => $requirements));
+    return array('#theme' => 'status_report', '#requirements' => $requirements);

use:

$status_report('#theme' => 'status_report', '#requirements' => $requirements);
return drupal_render($status_report);

@pplantinga: We try to render things as late as possible, because as long as they are not rendered one can alter the render array. As soon as they are rendered you only have a string and there is no way to alter it anymore. So whenever simply passing a render array works, we should do it.

Fine by me. I understand that it's better, I was just thinking of guidelines on #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() where it says:

Don't try to do anything new/fancy like returning the renderable array instead of rendering it where theme() was, despite this probably being "better" in many cases it will also incur quite a bit of extra testing overhead. We're trying to minimise the scope of this issue. Don't change the name/structure of any existing variables or try to add/remove/merge any existing functions. Touch nothing but the lamp (@see Aladdin). If the patch does change anything else, it will need manual testing and therefore slow/stall what should be relatively simple tasks. @thedavidmeister, who wrote this rule, is aware of the irony/hypocrisy in that the patches he submitted (before writing this rule) all refactored things slightly when they probably shouldn't have :P

Ahh, OK. I still think we're fine in this case. Controllers returning render arrays is really standard behavior. Don't feel strongly either way, though.

Changing requirements array to a variable is (hopefully) going to happen here:

#2041283: theme_status_report() is severely broken

#2041283: theme_status_report() is severely broken is in

Updated patch

All issues gone and a perfect one-liner remained.

Committed and pushed to 8.x. Thanks!