With the new design of the Status Report, when you have an error that the cron job hasn't run in a long time, it's not obvious what the issue is. See the attached screenshot. The error is listed in the count at the top of the page, but not in the Errors Found section. The error text is in the Last Cron Run box, but it's not highlighted in any way.

The same thing can happen for the PHP memory limit requirement. It should be displayed as a warning, but it doesn't show up in the Warnings Found sections of the report. It only shows up as normal text in the PHP box.

CommentFileSizeAuthor
#8 status_report_is_not-2874878-8-complete.patch2.33 KBlomo
#7 status_report_is_not-2874878-7-test.patch1.28 KBlomo
#5 status_report_is_not-2874878-5.patch2.33 KBzengenuity
#2 status_report_unclear-2874878-2.patch1.12 KBzengenuity
Screen Shot 2017-05-02 at 8.51.51 AM.png129.41 KBzengenuity

Comments

zengenuity created an issue. See original summary.

zengenuity’s picture

zengenuity commented
Status: Active » Needs review
StatusFileSize
new status_report_unclear-2874878-2.patch1.12 KB

Attached patch restores warning and errors for general info items, if they have severity >= REQUIREMENTS_WARNING. This makes them appear in the Errors Found and Warnings Found sections, though it does also duplicate the text in the general info section at the top.

bander2’s picture

bander2 commented
Status: Needs review » Reviewed & tested by the community

I was able to reproduce both the PHP Memory limit and cron issues. The patch applied cleanly and fixes the issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Usability, +Needs tests

For me this is a major usability bug because you are been told there is an error but the error is not listed in the expected place.

  1. +++ b/core/modules/system/src/Element/StatusReportPage.php
@@ -50,7 +50,9 @@ public static function preRenderGeneralInfo($element) {
           $element['#general_info']['#' . $key] = $requirement;
-          unset($element['#requirements'][$key]);
+          if (isset($requirement['severity']) && $requirement['severity'] < REQUIREMENT_WARNING) {
+            unset($element['#requirements'][$key]);
+          }
           break;

    We could just replace all of this with // Intentional fall-through.. So the entire switch statement would look like this:

          switch ($key) {
        case 'cron':
          foreach ($requirement['description'] as &$description_elements) {
            foreach ($description_elements as &$description_element) {
              if (isset($description_element['#url']) && $description_element['#url']->getRouteName() == 'system.run_cron') {
                $description_element['#attributes']['class'][] = 'button';
                $description_element['#attributes']['class'][] = 'button--small';
                $description_element['#attributes']['class'][] = 'button--primary';
                $description_element['#attributes']['class'][] = 'system-status-general-info__run-cron';
              }
            }
          }
          // Intentional fall-through.
        case 'drupal':
        case 'webserver':
        case 'database_system':
        case 'database_system_version':
        case 'php':
        case 'php_memory_limit':
          $element['#general_info']['#' . $key] = $requirement;
          if (isset($requirement['severity']) && $requirement['severity'] < REQUIREMENT_WARNING) {
            unset($element['#requirements'][$key]);
          }
          break;
      }

    I think this would make the code less brittle because there is less duplication right next to each other.

  2. We should add test coverage of this change in \Drupal\Tests\system\Functional\System\StatusTest. A way to cause the cron requirement error is to do \Drupal::state()->set('system.cron_last', 0) in the test.
zengenuity’s picture

zengenuity commented
Status: Needs work » Needs review
StatusFileSize
new status_report_is_not-2874878-5.patch2.33 KB

I've made the suggested changes, and added test code to \Drupal\Tests\system\Functional\System\StatusTest.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@zengenuity nice work!

I've run the test locally - without the fix it fails

    1) Drupal\Tests\system\Functional\System\StatusTest::testStatusPage
    Cron has not run recently error is being displayed.
    Failed asserting that an array is not empty.

@zengenuity when writing a test for a bug fix such as this it's great t upload a test-only patch too. However, you need to ensure that the patch with the fix is the last one on the issue so that the automated rtbc retest works as expected (it assumes the last patch on the issue is rtbc).

lomo’s picture

lomo commented
StatusFileSize
new status_report_is_not-2874878-7-test.patch1.28 KB

Tests-only patch separated out from #5

lomo’s picture

lomo commented
StatusFileSize
new status_report_is_not-2874878-8-complete.patch2.33 KB

We should see the test-only patch fail (#7) and this patch (same as #5, just renamed), should get us an RTBC-worthy test result. :-)

The last submitted patch, 7: status_report_is_not-2874878-7-test.patch, failed testing.

lomo’s picture

lomo commented
Status: Needs review » Reviewed & tested by the community

I think this is looking good for RTBC. ;-)

  • catch committed 2b7b8b3 on 8.4.x 
    Issue #2874878 by zengenuity, LoMo, alexpott: Status Report is not clear...

  • catch committed 625e0bf on 8.3.x 
    Issue #2874878 by zengenuity, LoMo, alexpott: Status Report is not clear...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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