Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Taking this one. My first patch so please needs review.

andypost’s picture

Status: Active » Needs review

Nice patch! Please set to 'needs review' when when new patch added

+++ b/.gitignoreundefined
@@ -0,0 +1 @@
+/nbproject/private/

useless netbeans file should be removed from patch

+++ b/core/modules/system/system.admin.incundefined
@@ -1438,73 +1438,6 @@
-  $severities = array(
-    REQUIREMENT_INFO => array(
-      'title' => t('Info'),
-      'class' => 'info',

+++ b/core/modules/system/templates/system-status-report.html.twigundefined
@@ -0,0 +1,50 @@
+        {% if requirement.severity %}
+           {% set severity = severities[(int) requirement.severity] %}

suppose, severities should be defined in preprocess

Status: Needs review » Needs work

The last submitted patch, theme_status_report-1938314-1.patch, failed testing.

Anonymous’s picture

Assigned: Unassigned »
Status: Needs work » Needs review
FileSize
5.46 KB

Reroll the patch as per #2 suggestions.

Status: Needs review » Needs work

The last submitted patch, theme_status_report-1938314-4.patch, failed testing.

andypost’s picture

Great, now needs to fix tests

+++ b/core/modules/system/templates/system-status-report.html.twigundefined
@@ -0,0 +1,48 @@
\ No newline at end of file

please add a new line after end

Anonymous’s picture

Assigned: » Unassigned
pplantinga’s picture

Status: Needs work » Needs review
FileSize
6.05 KB

How about this?

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

#5 looks good.

Eric_A’s picture

Status: Reviewed & tested by the community » Needs work
@@ -2505,6 +2505,36 @@ function template_preprocess_system_plugin_ui_form(&$variables) {
 }
 
 /**
+ * Prepares variables for system status report templates.
+ *
+ * Default template: system-status-report.html.twig.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - requirements: an array of requirements.
+ */
+function template_preprocess_system_status_report(&$variables) {
+  $variables['severites'] = array(
+    REQUIREMENT_INFO => array(
+      'title' => t('Info'),
+      'class' => 'info',
+    ),
+    REQUIREMENT_OK => array(
+      'title' => t('OK'),
+      'class' => 'ok',
+    ),
+    REQUIREMENT_WARNING => array(
+      'title' => t('Warning'),
+      'class' => 'warning',
+    ),
+    REQUIREMENT_ERROR => array(
+      'title' => t('Error'),
+      'class' => 'error',
+    ),
+  );
+}
+
+/**
  * Provide a single block on the administration overview page.
  *
  * @param $item
diff --git a/core/modules/system/templates/system-status-report.html.twig b/core/modules/system/templates/system-status-report.html.twig
new file mode 100644
index 0000000..b09cc1f
--- /dev/null
+++ b/core/modules/system/templates/system-status-report.html.twig
@@ -0,0 +1,49 @@
+{#
+/**
+ * @file
+ * Default theme implementation to present the theme status report.
+ *
+ * Available variables:
+ * - requirements: An array of requirements.
+ * - severities: An array of severites.

Misspelled severities (twice)

+++ b/core/modules/system/templates/system-status-report.html.twig
@@ -0,0 +1,49 @@
+{#
+/**
+ * @file
+ * Default theme implementation to present the theme status report.
+ *
+ * Available variables:
+ * - requirements: An array of requirements.
+ * - severities: An array of severites.
+ *
+ * @ingroup themeable
+ */
+#}
+<table class="system-status-report">
+  <thead>
+    <tr class="visually-hidden">
+      <th>{{ 'Status'|t }}</th>
+      <th>{{ 'Component'|t }}</th>
+      <th>{{ 'Details'|t }}</th>
+    </tr>
+  </thead>
+  <tbody>
+    {% for requirement in requirements %}
+      {% if requirement.type is empty %}

type is the wrong property.

thedavidmeister’s picture

type is the wrong property.

As part of this simplification can we try to remove checking the #type of requirements completely?

If it is checking for #type as a test for an array being a render element - this is a very flawed test as there's currently no requirement for render arrays to have a #type set and so a more robust test should be implemented.

If it is checking some kind of #type that has nothing to do with #type as defined for element_info(), this is super confusing and should be renamed or at the least documented way better.

If it is neither of these two things, can we please figure out what it is and remove it or document why it is required?

pplantinga’s picture

I'm not sure fixing the #type check fits within the scope of this issue, but here's my opinion anyway. I think we should remove the check altogether and just make the calling function format $requirements appropriately.

Here's two patches, one that fixes the spelling issues, and one that fixes the spelling and removes the #type check.

pplantinga’s picture

Status: Needs work » Needs review
thedavidmeister’s picture

I'm not sure fixing the #type check fits within the scope of this issue, but here's my opinion anyway.

Why would it be out of scope? This issue is specifically for simplifying the render process for theme_status_report().

Thanks for the patch :)

pplantinga’s picture

Fair enough. I somehow had it in my head that it was a straight theme() to twig conversion.

Eric_A’s picture

Status: Needs review » Needs work

There are dedicated Twig conversion issues already (#1987410: [meta] system.module - Convert theme_ functions to Twig / #1757550: [Meta] Convert core theme functions to Twig templates), so probably better to focus on just the simplification here?

pplantinga’s picture

Status: Needs work » Needs review
FileSize
5.96 KB

I think this issue is much more specific than #1987410: [meta] system.module - Convert theme_ functions to Twig and so can override it.

Here's a re-rolled patch.

pplantinga’s picture

Actually, #16 is right. Here's a patch that just does the simplification.

joelpittet’s picture

+++ b/core/modules/system/system.admin.inc
@@ -572,33 +572,31 @@ function theme_status_report($variables) {
-    if (empty($requirement['#type'])) {

@pplantinga re: #18

If I read this patch right, the only significant change is the removal of the $requirements['#type'] check inside the loop?

Is this all that's needed for this issue to simplify? If so, it's RTBC:)

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

See #11 and #12. Removing #type check seems to be consensus.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Yep this seems totally reasonable since https://api.drupal.org/api/drupal/core!modules!system!system.api.php/fun... makes no mention of a #type key.

Committed 0e100f3 and pushed to 8.x. Thanks!

git diff -w gives the following output

diff --git a/core/modules/system/system.admin.inc b/core/modules/system/system.admin.inc
index aa36ba7..05c67a7 100644
--- a/core/modules/system/system.admin.inc
+++ b/core/modules/system/system.admin.inc
@@ -572,7 +572,6 @@ function theme_status_report($variables) {
   $output .= '</tr></thead><tbody>';

   foreach ($requirements as $requirement) {
-    if (empty($requirement['#type'])) {
     // Always use the explicit requirement severity, if defined. Otherwise,
     // default to REQUIREMENT_OK in the installer to visually confirm that
     // installation requirements are met. And default to REQUIREMENT_INFO to
@@ -599,7 +598,6 @@ function theme_status_report($variables) {
     }
     $output .= '</td></tr>';
   }
-  }

   $output .= '</tbody></table>';
   return $output;

Status: Fixed » Closed (fixed)

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