From 5f652d458c863af5068833d074cfaedb9de39650 Mon Sep 17 00:00:00 2001 From: Adam G-H <32250-phenaproxima@users.noreply.drupalcode.org> Date: Thu, 9 Feb 2023 17:03:41 +0000 Subject: [PATCH] Issue #3339657 by phenaproxima, tedbow: Always show summary of validation result if exists --- src/Form/UpdateFormBase.php | 7 +- src/Validation/AdminStatusCheckMessages.php | 42 +++++++++++ src/Validation/StatusCheckRequirements.php | 5 +- .../ValidationResultDisplayTrait.php | 51 ------------- tests/src/Functional/StatusCheckTest.php | 54 +++++++------ tests/src/Functional/UpdateErrorTest.php | 33 +++++--- tests/src/Functional/UpdateWarningTest.php | 12 ++- tests/src/Functional/UpdaterFormTestBase.php | 18 +++++ .../ValidationResultDisplayTraitTest.php | 75 ------------------- 9 files changed, 130 insertions(+), 167 deletions(-) delete mode 100644 tests/src/Kernel/ValidationResultDisplayTraitTest.php diff --git a/src/Form/UpdateFormBase.php b/src/Form/UpdateFormBase.php index 461e31f646..ea0bb61262 100644 --- a/src/Form/UpdateFormBase.php +++ b/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, ]; } diff --git a/src/Validation/AdminStatusCheckMessages.php b/src/Validation/AdminStatusCheckMessages.php index 14776fd9ec..43b9215f50 100644 --- a/src/Validation/AdminStatusCheckMessages.php +++ b/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); + } + } + } diff --git a/src/Validation/StatusCheckRequirements.php b/src/Validation/StatusCheckRequirements.php index 245d483ad0..d1504dfe61 100644 --- a/src/Validation/StatusCheckRequirements.php +++ b/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', diff --git a/src/Validation/ValidationResultDisplayTrait.php b/src/Validation/ValidationResultDisplayTrait.php index d7eb2279f8..dd85a2c1d3 100644 --- a/src/Validation/ValidationResultDisplayTrait.php +++ b/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(); - } - } diff --git a/tests/src/Functional/StatusCheckTest.php b/tests/src/Functional/StatusCheckTest.php index 8fa61c0cab..f74d7f8f89 100644 --- a/tests/src/Functional/StatusCheckTest.php +++ b/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); } diff --git a/tests/src/Functional/UpdateErrorTest.php b/tests/src/Functional/UpdateErrorTest.php index f681d7300b..f7e77c9583 100644 --- a/tests/src/Functional/UpdateErrorTest.php +++ b/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); } diff --git a/tests/src/Functional/UpdateWarningTest.php b/tests/src/Functional/UpdateWarningTest.php index bc83f2a5f7..0465782ce8 100644 --- a/tests/src/Functional/UpdateWarningTest.php +++ b/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'); } } diff --git a/tests/src/Functional/UpdaterFormTestBase.php b/tests/src/Functional/UpdaterFormTestBase.php index a0ce76a115..a0851535e7 100644 --- a/tests/src/Functional/UpdaterFormTestBase.php +++ b/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); + } + } + } diff --git a/tests/src/Kernel/ValidationResultDisplayTraitTest.php b/tests/src/Kernel/ValidationResultDisplayTraitTest.php deleted file mode 100644 index 71e03c825a..0000000000 --- a/tests/src/Kernel/ValidationResultDisplayTraitTest.php +++ /dev/null @@ -1,75 +0,0 @@ -<?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()); - } - -} -- GitLab