Issue #3277007 by phenaproxima, chrisfromredfin, tedbow, Theresa.Grannum:...

Issue #3277007 by phenaproxima, chrisfromredfin, tedbow, Theresa.Grannum: Validation messages displayed on admin pages should be grouped together so it's clear they are all related to automatic updates

automatic_updates_extensions/src/Form/UpdaterForm.php

@@ -9,6 +9,7 @@ use Drupal\automatic_updates_extensions\ExtensionUpdater;
 use Drupal\Core\Batch\BatchBuilder;
 use Drupal\Core\Form\FormBase;
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\Core\Render\RendererInterface;
 use Drupal\system\SystemManager;
 use Drupal\update\UpdateManagerInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
@@ -38,6 +39,13 @@ final class UpdaterForm extends FormBase {
    */
   private $eventDispatcher;
 
+  /**
+   * The renderer service.
+   *
+   * @var \Drupal\Core\Render\RendererInterface
+   */
+  private $renderer;
+
   /**
    * {@inheritdoc}
    */
@@ -45,6 +53,7 @@ final class UpdaterForm extends FormBase {
     return new static(
       $container->get('automatic_updates_extensions.updater'),
       $container->get('event_dispatcher'),
+      $container->get('renderer')
     );
   }
 
@@ -55,10 +64,13 @@ final class UpdaterForm extends FormBase {
    *   The extension updater service.
    * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
    *   The extension event dispatcher service.
+   * @param \Drupal\Core\Render\RendererInterface $renderer
+   *   The renderer service.
    */
-  public function __construct(ExtensionUpdater $extension_updater, EventDispatcherInterface $event_dispatcher) {
+  public function __construct(ExtensionUpdater $extension_updater, EventDispatcherInterface $event_dispatcher, RendererInterface $renderer) {
     $this->extensionUpdater = $extension_updater;
     $this->eventDispatcher = $event_dispatcher;
+    $this->renderer = $renderer;
   }
 
   /**
@@ -120,7 +132,7 @@ final class UpdaterForm extends FormBase {
       $this->eventDispatcher->dispatch($event);
       $results = $event->getResults();
     }
-    $this->displayResults($results, $this->messenger());
+    $this->displayResults($results, $this->messenger(), $this->renderer);
     $security_level = $this->getOverallSeverity($results);
 
     if ($update_projects && $security_level !== SystemManager::REQUIREMENT_ERROR) {

src/Form/UpdaterForm.php

@@ -13,6 +13,7 @@ use Drupal\Core\Form\FormBase;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Link;
 use Drupal\Core\Messenger\MessengerInterface;
+use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\State\StateInterface;
 use Drupal\Core\Url;
 use Drupal\package_manager\Exception\StageException;
@@ -60,6 +61,13 @@ class UpdaterForm extends FormBase {
    */
   protected $releaseChooser;
 
+  /**
+   * The renderer service.
+   *
+   * @var \Drupal\Core\Render\RendererInterface
+   */
+  protected $renderer;
+
   /**
    * Constructs a new UpdaterForm object.
    *
@@ -71,12 +79,15 @@ class UpdaterForm extends FormBase {
    *   The event dispatcher service.
    * @param \Drupal\automatic_updates\ReleaseChooser $release_chooser
    *   The release chooser service.
+   * @param \Drupal\Core\Render\RendererInterface $renderer
+   *   The renderer service.
    */
-  public function __construct(StateInterface $state, Updater $updater, EventDispatcherInterface $event_dispatcher, ReleaseChooser $release_chooser) {
+  public function __construct(StateInterface $state, Updater $updater, EventDispatcherInterface $event_dispatcher, ReleaseChooser $release_chooser, RendererInterface $renderer) {
     $this->updater = $updater;
     $this->state = $state;
     $this->eventDispatcher = $event_dispatcher;
     $this->releaseChooser = $release_chooser;
+    $this->renderer = $renderer;
   }
 
   /**
@@ -94,7 +105,8 @@ class UpdaterForm extends FormBase {
       $container->get('state'),
       $container->get('automatic_updates.updater'),
       $container->get('event_dispatcher'),
-      $container->get('automatic_updates.release_chooser')
+      $container->get('automatic_updates.release_chooser'),
+      $container->get('renderer')
     );
   }
 
@@ -252,7 +264,7 @@ class UpdaterForm extends FormBase {
       $this->eventDispatcher->dispatch($event);
       $results = $event->getResults();
     }
-    $this->displayResults($results, $this->messenger());
+    $this->displayResults($results, $this->messenger(), $this->renderer);
 
     if ($stage_exists) {
       // If the form has been submitted, do not display this error message

src/Validation/AdminReadinessMessages.php

@@ -6,6 +6,7 @@ use Drupal\automatic_updates\CronUpdater;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
 use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\Messenger\MessengerTrait;
+use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Routing\AdminContext;
 use Drupal\Core\Routing\CurrentRouteMatch;
 use Drupal\Core\Routing\RedirectDestinationTrait;
@@ -65,6 +66,13 @@ final class AdminReadinessMessages implements ContainerInjectionInterface {
    */
   protected $cronUpdater;
 
+  /**
+   * The renderer service.
+   *
+   * @var \Drupal\Core\Render\RendererInterface
+   */
+  protected $renderer;
+
   /**
    * Constructs a ReadinessRequirement object.
    *
@@ -82,8 +90,10 @@ final class AdminReadinessMessages implements ContainerInjectionInterface {
    *   The current route match.
    * @param \Drupal\automatic_updates\CronUpdater $cron_updater
    *   The cron updater service.
+   * @param \Drupal\Core\Render\RendererInterface $renderer
+   *   The renderer service.
    */
-  public function __construct(ReadinessValidationManager $readiness_checker_manager, MessengerInterface $messenger, AdminContext $admin_context, AccountProxyInterface $current_user, TranslationInterface $translation, CurrentRouteMatch $current_route_match, CronUpdater $cron_updater) {
+  public function __construct(ReadinessValidationManager $readiness_checker_manager, MessengerInterface $messenger, AdminContext $admin_context, AccountProxyInterface $current_user, TranslationInterface $translation, CurrentRouteMatch $current_route_match, CronUpdater $cron_updater, RendererInterface $renderer) {
     $this->readinessCheckerManager = $readiness_checker_manager;
     $this->setMessenger($messenger);
     $this->adminContext = $admin_context;
@@ -91,6 +101,7 @@ final class AdminReadinessMessages implements ContainerInjectionInterface {
     $this->setStringTranslation($translation);
     $this->currentRouteMatch = $current_route_match;
     $this->cronUpdater = $cron_updater;
+    $this->renderer = $renderer;
   }
 
   /**
@@ -104,7 +115,8 @@ final class AdminReadinessMessages implements ContainerInjectionInterface {
       $container->get('current_user'),
       $container->get('string_translation'),
       $container->get('current_route_match'),
-      $container->get('automatic_updates.cron_updater')
+      $container->get('automatic_updates.cron_updater'),
+      $container->get('renderer')
     );
   }
 
@@ -167,7 +179,7 @@ final class AdminReadinessMessages implements ContainerInjectionInterface {
     if (empty($results)) {
       return FALSE;
     }
-    $this->displayResults($results, $this->messenger());
+    $this->displayResults($results, $this->messenger(), $this->renderer);
     return TRUE;
   }
 

src/Validation/ReadinessTrait.php

@@ -3,11 +3,17 @@
 namespace Drupal\automatic_updates\Validation;
 
 use Drupal\Core\Messenger\MessengerInterface;
+use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\StringTranslation\TranslatableMarkup;
 use Drupal\system\SystemManager;
 
 /**
  * Common methods for working with readiness checkers.
+ *
+ * @internal
+ *   This class implements logic to output the messages from readiness checkers
+ *   on admin pages. It may be changed or removed at any time without warning
+ *   and should not be used by external code.
  */
 trait ReadinessTrait {
 
@@ -59,33 +65,35 @@ trait ReadinessTrait {
    *   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): void {
+  protected function displayResults(array $results, MessengerInterface $messenger, RendererInterface $renderer): void {
     $severity = $this->getOverallSeverity($results);
 
     if ($severity === SystemManager::REQUIREMENT_OK) {
       return;
     }
 
-    $message = $this->getFailureMessageForSeverity($severity);
+    // 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),
+    ];
+    foreach ($results as $result) {
+      $messages = $result->getMessages();
+      // @todo Find a way to show all the messages, not just the summary, if
+      //   we're on the updater form in https://drupal.org/i/3284346.
+      $build['#items'][] = count($messages) === 1 ? reset($messages) : $result->getSummary();
+    }
+    $message = $renderer->renderRoot($build);
+
     if ($severity === SystemManager::REQUIREMENT_ERROR) {
       $messenger->addError($message);
     }
     else {
       $messenger->addWarning($message);
     }
-
-    foreach ($results as $result) {
-      $messages = $result->getMessages();
-      $message = count($messages) === 1 ? $messages[0] : $result->getSummary();
-
-      if ($result->getSeverity() === SystemManager::REQUIREMENT_ERROR) {
-        $messenger->addError($message);
-      }
-      else {
-        $messenger->addWarning($message);
-      }
-    }
   }
 
 }

tests/src/Unit/ReadinessTraitTest.php → tests/src/Kernel/ReadinessTraitTest.php

 <?php
 
-namespace Drupal\Tests\automatic_updates\Unit;
+namespace Drupal\Tests\automatic_updates\Kernel;
 
 use Drupal\automatic_updates\Validation\ReadinessTrait;
-use Drupal\Core\Messenger\Messenger;
-use Drupal\Core\PageCache\ResponsePolicy\KillSwitch;
-use Drupal\Core\StringTranslation\PluralTranslatableMarkup;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
-use Drupal\Core\StringTranslation\TranslatableMarkup;
-use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\package_manager\ValidationResult;
 use Drupal\system\SystemManager;
-use Drupal\Tests\UnitTestCase;
-use Symfony\Component\HttpFoundation\Session\Flash\FlashBag;
 
 /**
  * @coversDefaultClass \Drupal\automatic_updates\Validation\ReadinessTrait
  *
  * @group automatic_updates
  */
-class ReadinessTraitTest extends UnitTestCase {
+class ReadinessTraitTest extends AutomaticUpdatesKernelTestBase {
 
   use ReadinessTrait;
   use StringTranslationTrait;
@@ -49,47 +43,43 @@ class ReadinessTraitTest extends UnitTestCase {
    * @covers ::displayResults
    */
   public function testDisplayResults(): void {
-    $messenger = new Messenger(new FlashBag(), new KillSwitch());
-
-    $translation = new TestTranslationManager();
-    $this->setStringTranslation($translation);
+    $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(['Boo!']),
-      ValidationResult::createError(['Wednesday', 'Thursday'], $this->t('The Addams Family')),
+      ValidationResult::createError(['Wednesday', 'Lurch'], $this->t('The Addams Family')),
       ValidationResult::createWarning(['Moo!']),
       ValidationResult::createWarning(['Shaggy', 'The dog'], $this->t('Mystery Mobile')),
     ];
-    $this->displayResults($results, $messenger);
+    $this->displayResults($results, $messenger, $renderer);
 
+    $failure_message = (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_ERROR);
     $expected_errors = [
-      (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_ERROR),
-      'Boo!',
-      'The Addams Family',
+      "$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(Messenger::TYPE_ERROR));
+    $actual_errors = array_map('strval', $messenger->deleteByType(MessengerInterface::TYPE_ERROR));
     $this->assertSame($expected_errors, $actual_errors);
 
-    // Even though there were warnings, we shouldn't see the warning preamble.
-    $expected_warnings = ['Moo!', 'Mystery Mobile'];
-    $actual_warnings = array_map('strval', $messenger->deleteByType(Messenger::TYPE_WARNING));
-    $this->assertSame($expected_warnings, $actual_warnings);
+    // 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);
+    $this->displayResults($results, $messenger, $renderer);
 
+    $failure_message = (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_WARNING);
     $expected_warnings = [
-      (string) $this->getFailureMessageForSeverity(SystemManager::REQUIREMENT_WARNING),
-      'Moo!',
-      'Mystery Mobile',
+      "$failure_message<ul><li>Moo!</li><li>Mystery Mobile</li></ul>",
     ];
-    $actual_warnings = array_map('strval', $messenger->deleteByType(Messenger::TYPE_WARNING));
+    $actual_warnings = array_map('strval', $messenger->deleteByType(MessengerInterface::TYPE_WARNING));
     $this->assertSame($expected_warnings, $actual_warnings);
 
     // There shouldn't be any more messages.
@@ -97,31 +87,3 @@ class ReadinessTraitTest extends UnitTestCase {
   }
 
 }
-
-/**
- * Implements a translation manager in tests.
- */
-class TestTranslationManager implements TranslationInterface {
-
-  /**
-   * {@inheritdoc}
-   */
-  public function translate($string, array $args = [], array $options = []) {
-    return new TranslatableMarkup($string, $args, $options, $this);
-  }
-
-  /**
-   * {@inheritdoc}
-   */
-  public function translateString(TranslatableMarkup $translated_string) {
-    return $translated_string->getUntranslatedString();
-  }
-
-  /**
-   * {@inheritdoc}
-   */
-  public function formatPlural($count, $singular, $plural, array $args = [], array $options = []) {
-    return new PluralTranslatableMarkup($count, $singular, $plural, $args, $options, $this);
-  }
-
-}