Issue #3339657 by phenaproxima, tedbow: Always show summary of validation result if exists

src/Form/UpdateFormBase.php

@@ -66,10 +66,13 @@ abstract class UpdateFormBase extends FormBase {
     foreach ($results as $result) {
       $messages = $result->getMessages();
 
-      if (count($messages) > 1) {
+      // If there's a summary, there's guaranteed to be at least one message,
+      // so render the result as a nested list.
+      $summary = $result->getSummary();
+      if ($summary) {
         $build['#items'][] = [
           '#theme' => $build['#theme'],
-          '#prefix' => $result->getSummary(),
+          '#prefix' => $summary,
           '#items' => $messages,
         ];
       }

src/Validation/AdminStatusCheckMessages.php

@@ -14,6 +14,7 @@ use Drupal\Core\Routing\CurrentRouteMatch;
 use Drupal\Core\Routing\RedirectDestinationTrait;
 use Drupal\Core\Session\AccountProxyInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\Core\Url;
 use Drupal\package_manager\ValidationResult;
@@ -222,4 +223,45 @@ final class AdminStatusCheckMessages implements ContainerInjectionInterface {
     }
   }
 
+  /**
+   * Adds a set of validation results to the messages.
+   *
+   * @param \Drupal\package_manager\ValidationResult[] $results
+   *   The validation results.
+   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
+   *   The messenger service.
+   * @param \Drupal\Core\Render\RendererInterface $renderer
+   *   The renderer service.
+   */
+  protected function displayResults(array $results, MessengerInterface $messenger, RendererInterface $renderer): void {
+    $severity = ValidationResult::getOverallSeverity($results);
+
+    if ($severity === SystemManager::REQUIREMENT_OK) {
+      return;
+    }
+
+    // Display a single message for each validation result, even if it has
+    // multiple messages. This is because, on regular admin pages, we merely
+    // want to alert users that problems exist, but not burden them with the
+    // details. They can get those on the status report and updater form.
+    $format_result = function (ValidationResult $result): TranslatableMarkup {
+      $messages = $result->getMessages();
+      return $result->getSummary() ?: reset($messages);
+    };
+    // Format the results as a single item list prefixed by a preamble message.
+    $build = [
+      '#theme' => 'item_list__automatic_updates_validation_results',
+      '#prefix' => $this->getFailureMessageForSeverity($severity),
+      '#items' => array_map($format_result, $results),
+    ];
+    $message = $renderer->renderRoot($build);
+
+    if ($severity === SystemManager::REQUIREMENT_ERROR) {
+      $messenger->addError($message);
+    }
+    else {
+      $messenger->addWarning($message);
+    }
+  }
+
 }

src/Validation/StatusCheckRequirements.php

@@ -119,13 +119,14 @@ final class StatusCheckRequirements implements ContainerInjectionInterface {
     }
     foreach ($results as $result) {
       $checker_messages = $result->getMessages();
-      if (count($checker_messages) === 1) {
+      $summary = $result->getSummary();
+      if (empty($summary)) {
         $severity_messages[] = ['#markup' => array_pop($checker_messages)];
       }
       else {
         $severity_messages[] = [
           '#type' => 'details',
-          '#title' => $result->getSummary(),
+          '#title' => $summary,
           '#open' => FALSE,
           'messages' => [
             '#theme' => 'item_list',

src/Validation/ValidationResultDisplayTrait.php

@@ -4,10 +4,7 @@ declare(strict_types = 1);
 
 namespace Drupal\automatic_updates\Validation;
 
-use Drupal\Core\Messenger\MessengerInterface;
-use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\StringTranslation\TranslatableMarkup;
-use Drupal\package_manager\ValidationResult;
 use Drupal\system\SystemManager;
 
 /**
@@ -41,52 +38,4 @@ trait ValidationResultDisplayTrait {
       $this->t('Your site does not pass some readiness checks for automatic updates. It cannot be automatically updated until further action is performed.');
   }
 
-  /**
-   * Adds a set of validation results to the messages.
-   *
-   * @param \Drupal\package_manager\ValidationResult[] $results
-   *   The validation results.
-   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
-   *   The messenger service.
-   * @param \Drupal\Core\Render\RendererInterface $renderer
-   *   The renderer service.
-   */
-  protected function displayResults(array $results, MessengerInterface $messenger, RendererInterface $renderer): void {
-    $severity = ValidationResult::getOverallSeverity($results);
-
-    if ($severity === SystemManager::REQUIREMENT_OK) {
-      return;
-    }
-
-    // Format the results as a single item list prefixed by a preamble message.
-    $build = [
-      '#theme' => 'item_list__automatic_updates_validation_results',
-      '#prefix' => $this->getFailureMessageForSeverity($severity),
-      '#items' => array_map([$this, 'formatResult'], $results),
-    ];
-    $message = $renderer->renderRoot($build);
-
-    if ($severity === SystemManager::REQUIREMENT_ERROR) {
-      $messenger->addError($message);
-    }
-    else {
-      $messenger->addWarning($message);
-    }
-  }
-
-  /**
-   * Formats a single validation result as an item in an item list.
-   *
-   * @param \Drupal\package_manager\ValidationResult $result
-   *   A validation result.
-   *
-   * @return \Drupal\Core\StringTranslation\TranslatableMarkup|array
-   *   The validation result, formatted for inclusion in a themed item list as
-   *   either a translated string, or a renderable array.
-   */
-  protected function formatResult(ValidationResult $result) {
-    $messages = $result->getMessages();
-    return count($messages) === 1 ? reset($messages) : $result->getSummary();
-  }
-
 }

tests/src/Functional/StatusCheckTest.php

@@ -12,6 +12,7 @@ use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1;
 use Drupal\automatic_updates_test2\EventSubscriber\TestSubscriber2;
 use Drupal\Core\Url;
 use Drupal\package_manager\Event\StatusCheckEvent;
+use Drupal\package_manager\ValidationResult;
 use Drupal\package_manager_test_validation\EventSubscriber\TestSubscriber;
 use Drupal\system\SystemManager;
 use Drupal\Tests\automatic_updates\Traits\ValidationTestTrait;
@@ -194,12 +195,17 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     TestSubscriber1::setTestResult($expected_results, StatusCheckEvent::class);
     // Confirm a new message is displayed if the page is reloaded.
     $this->getSession()->reload();
-    // Confirm that on the status page if there is only 1 warning or error the
-    // summaries will not be displayed.
+    // On the status page, we should see the summaries and messages, even if
+    // there is only 1 message.
     $this->assertErrors([$expected_results['error']]);
     $this->assertWarnings([$expected_results['warning']]);
-    $assert->pageTextNotContains($expected_results['error']->getSummary());
-    $assert->pageTextNotContains($expected_results['warning']->getSummary());
+
+    // If there's a result with only one message, but no summary, ensure that
+    // message is displayed.
+    $result = ValidationResult::createError([t('A lone message, with no summary.')]);
+    TestSubscriber1::setTestResult([$result], StatusCheckEvent::class);
+    $this->getSession()->reload();
+    $this->assertErrors([$result]);
 
     $expected_results = [
       'error' => $this->createValidationResult(SystemManager::REQUIREMENT_ERROR, 2),
@@ -293,7 +299,7 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     $assert->pageTextContainsOnce('Your site has not recently run an update readiness check. Rerun readiness checks now.');
     $this->clickLink('Rerun readiness checks now.');
     $assert->addressEquals(Url::fromRoute($admin_route));
-    $assert->pageTextContainsOnce($expected_results[0]->getMessages()[0]);
+    $assert->pageTextContainsOnce($expected_results[0]->getSummary());
 
     $expected_results = [
       '1 error' => $this->createValidationResult(SystemManager::REQUIREMENT_ERROR),
@@ -305,11 +311,10 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     $this->cronRun();
     $this->drupalGet(Url::fromRoute($admin_route));
     $assert->pageTextContainsOnce(static::$errorsExplanation);
-    // Confirm on admin pages that a single error will be displayed instead of a
-    // summary.
+    // Confirm on admin pages that the summary will be displayed.
     $this->assertSame(SystemManager::REQUIREMENT_ERROR, $expected_results['1 error']->getSeverity());
-    $assert->pageTextContainsOnce($expected_results['1 error']->getMessages()[0]);
-    $assert->pageTextNotContains($expected_results['1 error']->getSummary());
+    $assert->pageTextContainsOnce((string) $expected_results['1 error']->getSummary());
+    $assert->pageTextNotContains($expected_results['1 error']->getMessages()[0]);
     // Warnings are not displayed on admin pages if there are any errors.
     $this->assertSame(SystemManager::REQUIREMENT_WARNING, $expected_results['1 warning']->getSeverity());
     $assert->pageTextNotContains($expected_results['1 warning']->getMessages()[0]);
@@ -326,7 +331,7 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     $this->cronRun();
     $this->drupalGet(Url::fromRoute($admin_route));
     $assert->pageTextNotContains($unexpected_results['2 errors']->getSummary());
-    $assert->pageTextContainsOnce($expected_results['1 error']->getMessages()[0]);
+    $assert->pageTextContainsOnce((string) $expected_results['1 error']->getSummary());
     $assert->pageTextNotContains($unexpected_results['2 warnings']->getSummary());
     $assert->pageTextNotContains($expected_results['1 warning']->getMessages()[0]);
 
@@ -373,8 +378,8 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     // pages if there is only 1 warning and there are no errors.
     $this->assertSame(SystemManager::REQUIREMENT_WARNING, $expected_results[0]->getSeverity());
     $assert->pageTextContainsOnce(static::$warningsExplanation);
-    $assert->pageTextContainsOnce($expected_results[0]->getMessages()[0]);
-    $assert->pageTextNotContains($expected_results[0]->getSummary());
+    $assert->pageTextContainsOnce((string) $expected_results[0]->getSummary());
+    $assert->pageTextNotContains($expected_results[0]->getMessages()[0]);
 
     // Confirm status check messages are not displayed when cron updates are
     // disabled.
@@ -408,7 +413,7 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     TestSubscriber2::setTestResult($expected_results, StatusCheckEvent::class);
     $this->container->get('module_installer')->install(['automatic_updates_test2']);
     $this->drupalGet('admin/structure');
-    $assert->pageTextContainsOnce($expected_results[0]->getMessages()[0]);
+    $assert->pageTextContainsOnce((string) $expected_results[0]->getSummary());
 
     // Confirm that installing a module runs the checkers, even if the new
     // module does not provide any validators.
@@ -447,15 +452,15 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     // Check for message on 'admin/structure' instead of the status report
     // because checkers will be run if needed on the status report.
     $this->drupalGet('admin/structure');
-    $assert->pageTextContainsOnce($expected_results_1[0]->getMessages()[0]);
-    $assert->pageTextContainsOnce($expected_results_2[0]->getMessages()[0]);
+    $assert->pageTextContainsOnce($expected_results_1[0]->getSummary());
+    $assert->pageTextContainsOnce($expected_results_2[0]->getSummary());
 
     // Confirm that when on of the module is uninstalled the other module's
     // checker result is still displayed.
     $this->container->get('module_installer')->uninstall(['automatic_updates_test2']);
     $this->drupalGet('admin/structure');
-    $assert->pageTextNotContains($expected_results_2[0]->getMessages()[0]);
-    $assert->pageTextContainsOnce($expected_results_1[0]->getMessages()[0]);
+    $assert->pageTextNotContains($expected_results_2[0]->getSummary());
+    $assert->pageTextContainsOnce($expected_results_1[0]->getSummary());
 
     // Confirm that when on of the module is uninstalled the other module's
     // checker result is still displayed.
@@ -478,10 +483,11 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     // version.
     $this->mockActiveCoreVersion('9.8.0');
 
-    // Flag a validation error, which will be displayed in the messages area.
+    // Flag a validation error, whose summary will be displayed in the messages
+    // area.
     $results = [$this->createValidationResult(SystemManager::REQUIREMENT_ERROR)];
     TestSubscriber1::setTestResult($results, StatusCheckEvent::class);
-    $message = $results[0]->getMessages()[0];
+    $message = $results[0]->getSummary();
 
     $this->container->get('module_installer')->install([
       'automatic_updates',
@@ -527,10 +533,11 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
   public function testStoredResultsClearedAfterConfigChanges(): void {
     $this->drupalLogin($this->checkerRunnerUser);
 
-    // Flag a validation error, which will be displayed in the messages area.
+    // Flag a validation error, whose summary will be displayed in the messages
+    // area.
     $result = $this->createValidationResult(SystemManager::REQUIREMENT_ERROR);
     TestSubscriber1::setTestResult([$result], StatusCheckEvent::class);
-    $message = $result->getMessages()[0];
+    $message = $result->getSummary();
 
     $this->container->get('module_installer')->install([
       'automatic_updates',
@@ -641,8 +648,9 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     $expected_messages = [];
     foreach ($expected_results as $result) {
       $messages = $result->getMessages();
-      if (count($messages) > 1) {
-        $expected_messages[] = $result->getSummary();
+      $summary = $result->getSummary();
+      if ($summary) {
+        $expected_messages[] = $summary;
       }
       $expected_messages = array_merge($expected_messages, $messages);
     }

tests/src/Functional/UpdateErrorTest.php

@@ -69,10 +69,15 @@ class UpdateErrorTest extends UpdaterFormTestBase {
     $error = ValidationResult::createError($error_messages, $summary);
     TestSubscriber::setTestResult([$error], StatusCheckEvent::class);
     $this->getSession()->reload();
-    $assert_session->pageTextContains($summary);
-    foreach ($error_messages as $message) {
-      $assert_session->pageTextContains($message);
-    }
+    $this->assertStatusMessageContainsResult($error);
+    $assert_session->buttonNotExists('Continue');
+    $assert_session->buttonExists('Cancel update');
+
+    // An error with only one message should also show the summary.
+    $error = ValidationResult::createError([t('Yet another smarmy error.')], $summary);
+    TestSubscriber::setTestResult([$error], StatusCheckEvent::class);
+    $this->getSession()->reload();
+    $this->assertStatusMessageContainsResult($error);
     $assert_session->buttonNotExists('Continue');
     $assert_session->buttonExists('Cancel update');
   }
@@ -106,9 +111,19 @@ class UpdateErrorTest extends UpdaterFormTestBase {
     // thanks to automatic_updates_page_top(). The status checks were re-run
     // during the form build, which means the new error should be cached and
     // displayed instead of the previously cached error.
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[1]);
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getSummary());
+    $this->assertStatusMessageContainsResult($expected_results[0]);
+    $assert_session->pageTextContainsOnce(static::$errorsExplanation);
+    $assert_session->pageTextNotContains(static::$warningsExplanation);
+    $assert_session->pageTextNotContains($cached_message);
+    TestSubscriber1::setTestResult(NULL, StatusCheckEvent::class);
+
+    // Set up an error with one message and a summary. We should see both when
+    // we refresh the form.
+    $expected_result = $this->createValidationResult(SystemManager::REQUIREMENT_ERROR, 1);
+    TestSubscriber1::setTestResult([$expected_result], StatusCheckEvent::class);
+    $this->getSession()->reload();
+    $this->assertNoUpdateButtons();
+    $this->assertStatusMessageContainsResult($expected_result);
     $assert_session->pageTextContainsOnce(static::$errorsExplanation);
     $assert_session->pageTextNotContains(static::$warningsExplanation);
     $assert_session->pageTextNotContains($cached_message);
@@ -145,9 +160,7 @@ class UpdateErrorTest extends UpdaterFormTestBase {
     $this->assertUpdateStagedTimes(0);
     $assert_session->pageTextContainsOnce('An error has occurred.');
     $page->clickLink('the error page');
-    $assert_session->pageTextContainsOnce($expected_results[0]->getSummary());
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[1]);
+    $this->assertStatusMessageContainsResult($expected_results[0]);
     $assert_session->pageTextNotContains($cached_message);
   }
 

tests/src/Functional/UpdateWarningTest.php

@@ -40,10 +40,14 @@ class UpdateWarningTest extends UpdaterFormTestBase {
 
     $assert_session = $this->assertSession();
     $assert_session->buttonExists('Continue');
-    $assert_session->pageTextContains($summary);
-    foreach ($messages as $message) {
-      $assert_session->pageTextContains($message);
-    }
+    $this->assertStatusMessageContainsResult($warning);
+
+    // A warning with only one message should also show its summary.
+    $warning = ValidationResult::createWarning([t("I'm still warning you.")], $summary);
+    TestSubscriber::setTestResult([$warning], StatusCheckEvent::class);
+    $session->reload();
+    $this->assertStatusMessageContainsResult($warning);
+    $assert_session->buttonExists('Continue');
   }
 
 }

tests/src/Functional/UpdaterFormTestBase.php

@@ -8,6 +8,7 @@ use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1;
 use Drupal\Core\StringTranslation\TranslatableMarkup;
 use Drupal\package_manager\Event\StatusCheckEvent;
 use Drupal\package_manager\ValidationResult;
+use Drupal\system\SystemManager;
 use Drupal\Tests\automatic_updates\Traits\ValidationTestTrait;
 use Drupal\Tests\package_manager\Traits\PackageManagerBypassTestTrait;
 
@@ -128,4 +129,21 @@ abstract class UpdaterFormTestBase extends AutomaticUpdatesFunctionalTestBase {
     $this->assertSame($is_primary, $button->hasClass('button--primary'));
   }
 
+  /**
+   * Asserts that a status message containing a given validation result exists.
+   *
+   * @param \Drupal\package_manager\ValidationResult $result
+   *   A validation result.
+   */
+  protected function assertStatusMessageContainsResult(ValidationResult $result): void {
+    $assert_session = $this->assertSession();
+    $type = $result->getSeverity() === SystemManager::REQUIREMENT_ERROR ? 'error' : 'warning';
+    $assert_session->statusMessageContains((string) $result->getSummary(), $type);
+    $assert_session->pageTextContainsOnce((string) $result->getSummary());
+    foreach ($result->getMessages() as $message) {
+      $assert_session->statusMessageContains((string) $message, $type);
+      $assert_session->pageTextContainsOnce((string) $message);
+    }
+  }
+
 }

tests/src/Kernel/ValidationResultDisplayTraitTest.php

-<?php
-
-declare(strict_types = 1);
-
-namespace Drupal\Tests\automatic_updates\Kernel;
-
-use Drupal\automatic_updates\Validation\ValidationResultDisplayTrait;
-use Drupal\Core\Messenger\MessengerInterface;
-use Drupal\Core\StringTranslation\StringTranslationTrait;
-use Drupal\package_manager\ValidationResult;
-use Drupal\system\SystemManager;
-
-/**
- * @coversDefaultClass \Drupal\automatic_updates\Validation\ValidationResultDisplayTrait
- * @group automatic_updates
- * @internal
- */
-class ValidationResultDisplayTraitTest extends AutomaticUpdatesKernelTestBase {
-
-  use ValidationResultDisplayTrait;
-  use StringTranslationTrait;
-
-  /**
-   * {@inheritdoc}
-   */
-  protected static $modules = ['automatic_updates'];
-
-  /**
-   * @covers ::displayResults
-   */
-  public function testDisplayResults(): void {
-    $messenger = $this->container->get('messenger');
-    $renderer = $this->container->get('renderer');
-
-    // An error and a warning should display the error preamble, and the result
-    // messages as errors and warnings, respectively.
-    $results = [
-      ValidationResult::createError([t('Boo!')]),
-      ValidationResult::createError([t('Wednesday'), t('Lurch')], $this->t('The Addams Family')),
-      ValidationResult::createWarning([t('Moo!')]),
-      ValidationResult::createWarning([t('Shaggy'), t('The dog')], $this->t('Mystery Mobile')),
-    ];
-    $this->displayResults($results, $messenger, $renderer);
-
-    $failure_message = (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_ERROR);
-    $expected_errors = [
-      "$failure_message<ul><li>Boo!</li><li>The Addams Family</li><li>Moo!</li><li>Mystery Mobile</li></ul>",
-    ];
-    $actual_errors = array_map('strval', $messenger->deleteByType(MessengerInterface::TYPE_ERROR));
-    $this->assertSame($expected_errors, $actual_errors);
-
-    // Even though there were warnings, they should have been included with the
-    // errors.
-    $actual_warnings = array_map('strval', $messenger->deleteByType(MessengerInterface::TYPE_WARNING));
-    $this->assertEmpty($actual_warnings);
-
-    // There shouldn't be any more messages.
-    $this->assertEmpty($messenger->all());
-
-    // If there are only warnings, we should see the warning preamble.
-    $results = array_slice($results, -2);
-    $this->displayResults($results, $messenger, $renderer);
-
-    $failure_message = (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_WARNING);
-    $expected_warnings = [
-      "$failure_message<ul><li>Moo!</li><li>Mystery Mobile</li></ul>",
-    ];
-    $actual_warnings = array_map('strval', $messenger->deleteByType(MessengerInterface::TYPE_WARNING));
-    $this->assertSame($expected_warnings, $actual_warnings);
-
-    // There shouldn't be any more messages.
-    $this->assertEmpty($messenger->all());
-  }
-
-}