Issue #3230050 by phenaproxima, tedbow: Run Readiness Validation in UpdaterForm

src/Form/UpdaterForm.php

@@ -4,6 +4,7 @@ namespace Drupal\automatic_updates\Form;
 
 use Drupal\automatic_updates\BatchProcessor;
 use Drupal\automatic_updates\Updater;
+use Drupal\automatic_updates\Validation\ReadinessValidationManager;
 use Drupal\automatic_updates_9_3_shim\ProjectRelease;
 use Drupal\Core\Batch\BatchBuilder;
 use Drupal\Core\Extension\ModuleHandlerInterface;
@@ -12,6 +13,7 @@ use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Link;
 use Drupal\Core\State\StateInterface;
 use Drupal\Core\Url;
+use Drupal\system\SystemManager;
 use Drupal\update\UpdateManagerInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
@@ -44,6 +46,13 @@ class UpdaterForm extends FormBase {
    */
   protected $state;
 
+  /**
+   * The readiness validation manager service.
+   *
+   * @var \Drupal\automatic_updates\Validation\ReadinessValidationManager
+   */
+  protected $readinessValidationManager;
+
   /**
    * Constructs a new UpdaterForm object.
    *
@@ -53,11 +62,14 @@ class UpdaterForm extends FormBase {
    *   The state service.
    * @param \Drupal\automatic_updates\Updater $updater
    *   The updater service.
+   * @param \Drupal\automatic_updates\Validation\ReadinessValidationManager $readiness_validation_manager
+   *   The readiness validation manager service.
    */
-  public function __construct(ModuleHandlerInterface $module_handler, StateInterface $state, Updater $updater) {
+  public function __construct(ModuleHandlerInterface $module_handler, StateInterface $state, Updater $updater, ReadinessValidationManager $readiness_validation_manager) {
     $this->updater = $updater;
     $this->moduleHandler = $module_handler;
     $this->state = $state;
+    $this->readinessValidationManager = $readiness_validation_manager;
   }
 
   /**
@@ -74,7 +86,8 @@ class UpdaterForm extends FormBase {
     return new static(
       $container->get('module_handler'),
       $container->get('state'),
-      $container->get('automatic_updates.updater')
+      $container->get('automatic_updates.updater'),
+      $container->get('automatic_updates.readiness_validation_manager')
     );
   }
 
@@ -190,7 +203,17 @@ class UpdaterForm extends FormBase {
       ],
     ];
 
-    $form['actions'] = $this->actions();
+    // @todo Add a hasErrors() or getErrors() method to
+    // ReadinessValidationManager to make validation more introspectable.
+    // Re-running the readiness checks now should mean that when we display
+    // cached errors in automatic_updates_page_top(), we'll see errors that
+    // were raised during this run, instead of any previously cached results.
+    $errors = $this->readinessValidationManager->run()
+      ->getResults(SystemManager::REQUIREMENT_ERROR);
+
+    if (empty($errors)) {
+      $form['actions'] = $this->actions();
+    }
     return $form;
   }
 

tests/modules/automatic_updates_test/src/ReadinessChecker/TestChecker1.php

@@ -40,14 +40,22 @@ class TestChecker1 implements EventSubscriberInterface {
    * This method is static to enable setting the expected messages before the
    * test module is enabled.
    *
-   * @param \Drupal\automatic_updates\Validation\ValidationResult[]|\Throwable $checker_results
-   *   The test validation results, or an exception to throw.
+   * @param \Drupal\automatic_updates\Validation\ValidationResult[]|\Throwable|null $checker_results
+   *   The test validation results, or an exception to throw, or NULL to delete
+   *   stored results.
    * @param string $event_name
    *   (optional )The event name. Defaults to
    *   AutomaticUpdatesEvents::READINESS_CHECK.
    */
   public static function setTestResult($checker_results, string $event_name = AutomaticUpdatesEvents::READINESS_CHECK): void {
-    \Drupal::state()->set(static::STATE_KEY . ".$event_name", $checker_results);
+    $key = static::STATE_KEY . ".$event_name";
+
+    if (isset($checker_results)) {
+      \Drupal::state()->set($key, $checker_results);
+    }
+    else {
+      \Drupal::state()->delete($key);
+    }
   }
 
   /**

tests/src/Functional/ReadinessValidationTest.php

@@ -22,16 +22,6 @@ class ReadinessValidationTest extends BrowserTestBase {
   use CronRunTrait;
   use ValidationTestTrait;
 
-  /**
-   * Expected explanation text when readiness checkers return error messages.
-   */
-  const ERRORS_EXPLANATION = 'Your site does not pass some readiness checks for automatic updates. It cannot be automatically updated until further action is performed.';
-
-  /**
-   * Expected explanation text when readiness checkers return warning messages.
-   */
-  const WARNINGS_EXPLANATION = 'Your site does not pass some readiness checks for automatic updates. Depending on the nature of the failures, it might affect the eligibility for automatic updates.';
-
   /**
    * {@inheritdoc}
    */
@@ -131,15 +121,15 @@ class ReadinessValidationTest extends BrowserTestBase {
     // Assert that when the runners are run manually the message that updates
     // will not be performed because of errors is displayed on the top of the
     // page in message.
-    $assert->pageTextMatchesCount(2, '/' . preg_quote(static::ERRORS_EXPLANATION) . '/');
-    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0] . 'Run readiness checks now.', 'error', static::ERRORS_EXPLANATION);
+    $assert->pageTextMatchesCount(2, '/' . preg_quote(static::$errorsExplanation) . '/');
+    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0] . 'Run readiness checks now.', 'error', static::$errorsExplanation);
 
     // @todo Should we always show when the checks were last run and a link to
     //   run when there is an error?
     // Confirm a user without permission to run the checks sees the same error.
     $this->drupalLogin($this->reportViewerUser);
     $this->drupalGet('admin/reports/status');
-    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0], 'error', static::ERRORS_EXPLANATION);
+    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0], 'error', static::$errorsExplanation);
 
     $expected_results = $this->testResults['checker_1']['1 error 1 warning'];
     TestChecker1::setTestResult($expected_results);
@@ -148,8 +138,8 @@ class ReadinessValidationTest extends BrowserTestBase {
     $this->drupalGet('admin/reports/status');
     // Confirm that on the status page if there is only 1 warning or error the
     // the summaries will not be displayed.
-    $this->assertReadinessReportMatches($expected_results['1:error']->getMessages()[0], 'error', static::ERRORS_EXPLANATION);
-    $this->assertReadinessReportMatches($expected_results['1:warning']->getMessages()[0], 'warning', static::WARNINGS_EXPLANATION);
+    $this->assertReadinessReportMatches($expected_results['1:error']->getMessages()[0], 'error', static::$errorsExplanation);
+    $this->assertReadinessReportMatches($expected_results['1:warning']->getMessages()[0], 'warning', static::$warningsExplanation);
     $assert->pageTextNotContains($expected_results['1:error']->getSummary());
     $assert->pageTextNotContains($expected_results['1:warning']->getSummary());
 
@@ -159,8 +149,8 @@ class ReadinessValidationTest extends BrowserTestBase {
     $this->drupalGet('admin/reports/status');
     // Confirm that both messages and summaries will be displayed on status
     // report when there multiple messages.
-    $this->assertReadinessReportMatches($expected_results['1:errors']->getSummary() . ' ' . implode('', $expected_results['1:errors']->getMessages()), 'error', static::ERRORS_EXPLANATION);
-    $this->assertReadinessReportMatches($expected_results['1:warnings']->getSummary() . ' ' . implode('', $expected_results['1:warnings']->getMessages()), 'warning', static::WARNINGS_EXPLANATION);
+    $this->assertReadinessReportMatches($expected_results['1:errors']->getSummary() . ' ' . implode('', $expected_results['1:errors']->getMessages()), 'error', static::$errorsExplanation);
+    $this->assertReadinessReportMatches($expected_results['1:warnings']->getSummary() . ' ' . implode('', $expected_results['1:warnings']->getMessages()), 'warning', static::$warningsExplanation);
 
     $key_value->delete('readiness_validation_last_run');
     $expected_results = $this->testResults['checker_1']['2 warnings'];
@@ -169,14 +159,14 @@ class ReadinessValidationTest extends BrowserTestBase {
     $assert->pageTextContainsOnce('Update readiness checks');
     // Confirm that warnings will display on the status report if there are no
     // errors.
-    $this->assertReadinessReportMatches($expected_results[0]->getSummary() . ' ' . implode('', $expected_results[0]->getMessages()), 'warning', static::WARNINGS_EXPLANATION);
+    $this->assertReadinessReportMatches($expected_results[0]->getSummary() . ' ' . implode('', $expected_results[0]->getMessages()), 'warning', static::$warningsExplanation);
 
     $key_value->delete('readiness_validation_last_run');
     $expected_results = $this->testResults['checker_1']['1 warning'];
     TestChecker1::setTestResult($expected_results);
     $this->drupalGet('admin/reports/status');
     $assert->pageTextContainsOnce('Update readiness checks');
-    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0], 'warning', static::WARNINGS_EXPLANATION);
+    $this->assertReadinessReportMatches($expected_results[0]->getMessages()[0], 'warning', static::$warningsExplanation);
   }
 
   /**
@@ -230,7 +220,7 @@ class ReadinessValidationTest extends BrowserTestBase {
     $this->delayRequestTime();
     $this->cronRun();
     $this->drupalGet('admin/structure');
-    $assert->pageTextContainsOnce(static::ERRORS_EXPLANATION);
+    $assert->pageTextContainsOnce(static::$errorsExplanation);
     // Confirm on admin pages that a single error will be displayed instead of a
     // summary.
     $this->assertSame(SystemManager::REQUIREMENT_ERROR, $expected_results['1:error']->getSeverity());
@@ -265,7 +255,7 @@ class ReadinessValidationTest extends BrowserTestBase {
     $assert->pageTextNotContains($expected_results['1:errors']->getMessages()[0]);
     $assert->pageTextNotContains($expected_results['1:errors']->getMessages()[1]);
     $assert->pageTextContainsOnce($expected_results['1:errors']->getSummary());
-    $assert->pageTextContainsOnce(static::ERRORS_EXPLANATION);
+    $assert->pageTextContainsOnce(static::$errorsExplanation);
     // Warnings are not displayed on admin pages if there are any errors.
     $this->assertSame(SystemManager::REQUIREMENT_WARNING, $expected_results['1:warnings']->getSeverity());
     $assert->pageTextNotContains($expected_results['1:warnings']->getMessages()[0]);
@@ -279,11 +269,11 @@ class ReadinessValidationTest extends BrowserTestBase {
     $this->drupalGet('admin/structure');
     // Confirm that the warnings summary is displayed on admin pages if there
     // are no errors.
-    $assert->pageTextNotContains(static::ERRORS_EXPLANATION);
+    $assert->pageTextNotContains(static::$errorsExplanation);
     $this->assertSame(SystemManager::REQUIREMENT_WARNING, $expected_results[0]->getSeverity());
     $assert->pageTextNotContains($expected_results[0]->getMessages()[0]);
     $assert->pageTextNotContains($expected_results[0]->getMessages()[1]);
-    $assert->pageTextContainsOnce(static::WARNINGS_EXPLANATION);
+    $assert->pageTextContainsOnce(static::$warningsExplanation);
     $assert->pageTextContainsOnce($expected_results[0]->getSummary());
 
     $expected_results = $this->testResults['checker_1']['1 warning'];
@@ -291,11 +281,11 @@ class ReadinessValidationTest extends BrowserTestBase {
     $this->delayRequestTime();
     $this->cronRun();
     $this->drupalGet('admin/structure');
-    $assert->pageTextNotContains(static::ERRORS_EXPLANATION);
+    $assert->pageTextNotContains(static::$errorsExplanation);
     // Confirm that a single warning is displayed and not the summary on admin
     // pages if there is only 1 warning and there are no errors.
     $this->assertSame(SystemManager::REQUIREMENT_WARNING, $expected_results[0]->getSeverity());
-    $assert->pageTextContainsOnce(static::WARNINGS_EXPLANATION);
+    $assert->pageTextContainsOnce(static::$warningsExplanation);
     $assert->pageTextContainsOnce($expected_results[0]->getMessages()[0]);
     $assert->pageTextNotContains($expected_results[0]->getSummary());
   }

tests/src/Functional/UpdaterFormTest.php

@@ -4,6 +4,7 @@ namespace Drupal\Tests\automatic_updates\Functional;
 
 use Drupal\automatic_updates\AutomaticUpdatesEvents;
 use Drupal\automatic_updates\Exception\UpdateException;
+use Drupal\automatic_updates\Validation\ValidationResult;
 use Drupal\automatic_updates_test\ReadinessChecker\TestChecker1;
 use Drupal\Tests\automatic_updates\Traits\ValidationTestTrait;
 use Drupal\Tests\BrowserTestBase;
@@ -109,27 +110,57 @@ class UpdaterFormTest extends BrowserTestBase {
     $assert_session = $this->assertSession();
     $page = $session->getPage();
 
+    // Store a fake readiness error, which will be cached.
+    $message = t("You've not experienced Shakespeare until you have read him in the original Klingon.");
+    $error = ValidationResult::createError([$message]);
+    TestChecker1::setTestResult([$error]);
+
+    $this->drupalLogin($this->rootUser);
+    $this->drupalGet('/admin/reports/status');
+    $page->clickLink('Run readiness checks');
+    $assert_session->pageTextContainsOnce((string) $message);
+    // Ensure that the fake error is cached.
+    $session->reload();
+    $assert_session->pageTextContainsOnce((string) $message);
+
     $this->setCoreVersion('9.8.0');
-    $this->createTestValidationResults();
+    $this->checkForUpdates();
 
+    // Set up a new fake error.
+    $this->createTestValidationResults();
     $expected_results = $this->testResults['checker_1']['1 error'];
+    TestChecker1::setTestResult($expected_results);
+
+    // If a validator raises an error during readiness checking, the form should
+    // not have a submit button.
+    $this->drupalGet('/admin/automatic-update');
+    $assert_session->buttonNotExists('Download these updates');
+    // Since this is an administrative page, the error message should be visible
+    // thanks to automatic_updates_page_top(). The readiness 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(static::$errorsExplanation);
+    $assert_session->pageTextNotContains(static::$warningsExplanation);
+    $assert_session->pageTextNotContains((string) $message);
+    TestChecker1::setTestResult(NULL);
+
     // Repackage the validation error as an exception, so we can test what
-    // happens if a validator throws.
+    // happens if a validator throws once the update has started.
     $error = new UpdateException($expected_results, 'The update exploded.');
     TestChecker1::setTestResult($error, AutomaticUpdatesEvents::PRE_START);
-
-    $this->drupalLogin($this->rootUser);
-    $this->checkForUpdates();
-    $this->drupalGet('/admin/automatic-update');
+    $session->reload();
+    $assert_session->pageTextNotContains(static::$errorsExplanation);
+    $assert_session->pageTextNotContains(static::$warningsExplanation);
     $page->pressButton('Download these updates');
     $this->checkForMetaRefresh();
-    $assert_session->pageTextContains('An error has occurred.');
+    $assert_session->pageTextContainsOnce('An error has occurred.');
     $page->clickLink('the error page');
-    $assert_session->pageTextContains((string) $expected_results[0]->getMessages()[0]);
+    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
     // Since there's only one error message, we shouldn't see the summary...
     $assert_session->pageTextNotContains($expected_results[0]->getSummary());
     // ...but we should see the exception message.
-    $assert_session->pageTextContains('The update exploded.');
+    $assert_session->pageTextContainsOnce('The update exploded.');
 
     // If a validator flags an error, but doesn't throw, the update should still
     // be halted.
@@ -137,11 +168,11 @@ class UpdaterFormTest extends BrowserTestBase {
     $this->deleteStagedUpdate();
     $page->pressButton('Download these updates');
     $this->checkForMetaRefresh();
-    $assert_session->pageTextContains('An error has occurred.');
+    $assert_session->pageTextContainsOnce('An error has occurred.');
     $page->clickLink('the error page');
     // Since there's only one message, we shouldn't see the summary.
     $assert_session->pageTextNotContains($expected_results[0]->getSummary());
-    $assert_session->pageTextContains((string) $expected_results[0]->getMessages()[0]);
+    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
 
     // If a validator flags a warning, but doesn't throw, the update should
     // continue.

tests/src/Traits/ValidationTestTrait.php

@@ -9,6 +9,20 @@ use Drupal\automatic_updates\Validation\ValidationResult;
  */
 trait ValidationTestTrait {
 
+  /**
+   * Expected explanation text when readiness checkers return error messages.
+   *
+   * @var string
+   */
+  protected static $errorsExplanation = 'Your site does not pass some readiness checks for automatic updates. It cannot be automatically updated until further action is performed.';
+
+  /**
+   * Expected explanation text when readiness checkers return warning messages.
+   *
+   * @var string
+   */
+  protected static $warningsExplanation = 'Your site does not pass some readiness checks for automatic updates. Depending on the nature of the failures, it might affect the eligibility for automatic updates.';
+
   /**
    * Test validation results.
    *