Issue #3247478 by tedbow, phenaproxima: Only pre-operation events should be...

Issue #3247478 by tedbow, phenaproxima: Only pre-operation events should be able to flag errors that stop an update

package_manager/src/EventSubscriber/WritableFileSystemValidator.php

@@ -3,8 +3,7 @@
 namespace Drupal\package_manager\EventSubscriber;
 
 use Drupal\package_manager\Event\PreCreateEvent;
-use Drupal\package_manager\Event\StageEvent;
-use Drupal\package_manager\ValidationResult;
+use Drupal\package_manager\Event\PreOperationStageEvent;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\package_manager\PathLocator;
@@ -12,7 +11,7 @@ use Drupal\package_manager\PathLocator;
 /**
  * Checks that the file system is writable.
  */
-class WritableFileSystemValidator implements StageValidatorInterface {
+class WritableFileSystemValidator implements PreOperationStageValidatorInterface {
 
   use StringTranslationTrait;
 
@@ -54,7 +53,7 @@ class WritableFileSystemValidator implements StageValidatorInterface {
    *   false negatives/positives due to things like SELinux, exotic file
    *   systems, and so forth.
    */
-  public function validateStage(StageEvent $event): void {
+  public function validateStagePreOperation(PreOperationStageEvent $event): void {
     $messages = [];
 
     if (!is_writable($this->appRoot)) {
@@ -69,8 +68,7 @@ class WritableFileSystemValidator implements StageValidatorInterface {
     }
 
     if ($messages) {
-      $result = ValidationResult::createError($messages, $this->t('The file system is not writable.'));
-      $event->addValidationResult($result);
+      $event->addError($messages, $this->t('The file system is not writable.'));
     }
   }
 
@@ -79,7 +77,7 @@ class WritableFileSystemValidator implements StageValidatorInterface {
    */
   public static function getSubscribedEvents() {
     return [
-      PreCreateEvent::class => 'validateStage',
+      PreCreateEvent::class => 'validateStagePreOperation',
     ];
   }
 

package_manager/src/Stage.php

@@ -12,7 +12,6 @@ use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreDestroyEvent;
 use Drupal\package_manager\Event\PreRequireEvent;
 use Drupal\package_manager\Event\StageEvent;
-use Drupal\system\SystemManager;
 use PhpTuf\ComposerStager\Domain\BeginnerInterface;
 use PhpTuf\ComposerStager\Domain\CleanerInterface;
 use PhpTuf\ComposerStager\Domain\CommitterInterface;
@@ -240,9 +239,9 @@ class Stage {
     try {
       $this->eventDispatcher->dispatch($event);
 
-      $errors = $event->getResults(SystemManager::REQUIREMENT_ERROR);
-      if ($errors) {
-        throw new StageException($errors);
+      $results = $event->getResults();
+      if ($results) {
+        throw new StageException($results);
       }
     }
     catch (\Throwable $error) {

package_manager/tests/modules/package_manager_test_validation/src/TestSubscriber.php

@@ -12,6 +12,7 @@ use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreDestroyEvent;
 use Drupal\package_manager\Event\PreRequireEvent;
 use Drupal\package_manager\Event\StageEvent;
+use Drupal\system\SystemManager;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
@@ -101,8 +102,14 @@ class TestSubscriber implements EventSubscriberInterface {
     if ($results instanceof \Throwable) {
       throw $results;
     }
+    /** @var \Drupal\package_manager\ValidationResult $result */
     foreach ($results as $result) {
-      $event->addValidationResult($result);
+      if ($result->getSeverity() === SystemManager::REQUIREMENT_ERROR) {
+        $event->addError($result->getMessages(), $result->getSummary());
+      }
+      else {
+        $event->addWarning($result->getMessages(), $result->getSummary());
+      }
     }
   }
 

package_manager/tests/src/Kernel/StageEventsTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\package_manager\Event\ErrorEventInterface;
 use Drupal\package_manager\Event\PostApplyEvent;
 use Drupal\package_manager\Event\PostCreateEvent;
 use Drupal\package_manager\Event\PostDestroyEvent;
@@ -11,9 +12,9 @@ use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreDestroyEvent;
 use Drupal\package_manager\Event\PreRequireEvent;
 use Drupal\package_manager\Event\StageEvent;
+use Drupal\package_manager\Event\WarningEventInterface;
 use Drupal\package_manager\Stage;
 use Drupal\package_manager\StageException;
-use Drupal\package_manager\ValidationResult;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
@@ -75,7 +76,7 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
   /**
    * Handles a staging area life cycle event.
    *
-   * @param \Drupal\package_manager\Event\StageEvent $event
+   * @param \Drupal\package_manager\Event\PreOperationStageEvent|\Drupal\package_manager\Event\PostOperationStageEvent $event
    *   The event object.
    */
   public function handleEvent(StageEvent $event): void {
@@ -83,12 +84,6 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
 
     // The event should have a reference to the stage which fired it.
     $this->assertSame($event->getStage(), $this->stage);
-
-    // Adding a warning to the event, should not trigger an exception.
-    $result = ValidationResult::createWarning([
-      'This is a public service announcement, this is only a test.',
-    ]);
-    $event->addValidationResult($result);
   }
 
   /**
@@ -115,12 +110,12 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
   }
 
   /**
-   * Data provider for ::testError().
+   * Data provider for ::testValidationResults().
    *
    * @return string[][]
    *   Sets of arguments to pass to the test method.
    */
-  public function providerError(): array {
+  public function providerValidationResults(): array {
     return [
       [PreCreateEvent::class],
       [PostCreateEvent::class],
@@ -134,20 +129,24 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
   }
 
   /**
-   * Tests that an exception is thrown if an event collects an error.
+   * Tests that an exception is thrown if an event has validation results.
    *
    * @param string $event_class
    *   The event class to test.
    *
-   * @dataProvider providerError
+   * @dataProvider providerValidationResults
    */
-  public function testError(string $event_class): void {
+  public function testValidationResults(string $event_class): void {
     // Set up an event listener which will only flag an error for the event
     // class under test.
     $handler = function (StageEvent $event) use ($event_class): void {
       if (get_class($event) === $event_class) {
-        $result = ValidationResult::createError(['Burn, baby, burn']);
-        $event->addValidationResult($result);
+        if ($event instanceof ErrorEventInterface) {
+          $event->addError([['Burn, baby, burn']]);
+        }
+        elseif ($event instanceof WarningEventInterface) {
+          $event->addWarning(['Be careful about fires.']);
+        }
       }
     };
     $this->container->get('event_dispatcher')

src/Event/ReadinessCheckEvent.php

@@ -3,7 +3,10 @@
 namespace Drupal\automatic_updates\Event;
 
 use Drupal\automatic_updates\Updater;
-use Drupal\package_manager\Event\StageEvent;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
+use Drupal\package_manager\Event\PreOperationStageEvent;
+use Drupal\package_manager\Event\WarningEventInterface;
+use Drupal\package_manager\ValidationResult;
 
 /**
  * Event fired when checking if the site could perform an update.
@@ -17,7 +20,7 @@ use Drupal\package_manager\Event\StageEvent;
  *
  * @see \Drupal\automatic_updates\Validation\ReadinessValidationManager
  */
-class ReadinessCheckEvent extends StageEvent {
+class ReadinessCheckEvent extends PreOperationStageEvent implements WarningEventInterface {
 
   /**
    * The desired package versions to update to, keyed by package name.
@@ -50,4 +53,11 @@ class ReadinessCheckEvent extends StageEvent {
     return $this->packageVersions;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function addWarning(array $messages, ?TranslatableMarkup $summary = NULL) {
+    $this->results[] = ValidationResult::createWarning($messages, $summary);
+  }
+
 }

src/Validator/CoreComposerValidator.php

@@ -3,7 +3,6 @@
 namespace Drupal\automatic_updates\Validator;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
-use Drupal\package_manager\ValidationResult;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
@@ -29,10 +28,9 @@ class CoreComposerValidator implements EventSubscriberInterface {
       ['drupal/core', 'drupal/core-recommended']
     );
     if (empty($core_requirements)) {
-      $error = ValidationResult::createError([
+      $event->addError([
         $this->t('Drupal core does not appear to be required in the project-level composer.json.'),
       ]);
-      $event->addValidationResult($error);
     }
   }
 

src/Validator/CronFrequencyValidator.php

@@ -11,7 +11,6 @@ use Drupal\Core\State\StateInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Drupal\Core\Url;
-use Drupal\package_manager\ValidationResult;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
@@ -136,12 +135,10 @@ class CronFrequencyValidator implements EventSubscriberInterface {
     $interval = $this->configFactory->get('automated_cron.settings')->get('interval');
 
     if ($interval > static::ERROR_INTERVAL) {
-      $error = ValidationResult::createError([$message]);
-      $event->addValidationResult($error);
+      $event->addError([$message]);
     }
     elseif ($interval > static::WARNING_INTERVAL) {
-      $warning = ValidationResult::createWarning([$message]);
-      $event->addValidationResult($warning);
+      $event->addWarning([$message]);
     }
   }
 
@@ -162,13 +159,12 @@ class CronFrequencyValidator implements EventSubscriberInterface {
     //   last cron run time is recorded after cron runs. Address this in
     //   https://www.drupal.org/project/automatic_updates/issues/3248544.
     if ($this->time->getRequestTime() - $cron_last > static::WARNING_INTERVAL) {
-      $error = ValidationResult::createError([
+      $event->addError([
         $this->t('Cron has not run recently. For more information, see the online handbook entry for <a href=":cron-handbook">configuring cron jobs</a> to run at least every @frequency hours.', [
           ':cron-handbook' => 'https://www.drupal.org/cron',
           '@frequency' => static::SUGGESTED_INTERVAL,
         ]),
       ]);
-      $event->addValidationResult($error);
     }
   }
 

src/Validator/PackageManagerReadinessCheck.php

@@ -3,7 +3,7 @@
 namespace Drupal\automatic_updates\Validator;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
-use Drupal\package_manager\EventSubscriber\StageValidatorInterface;
+use Drupal\package_manager\EventSubscriber\PreOperationStageValidatorInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
 /**
@@ -18,17 +18,17 @@ class PackageManagerReadinessCheck implements EventSubscriberInterface {
   /**
    * The validator to run.
    *
-   * @var \Drupal\package_manager\EventSubscriber\StageValidatorInterface
+   * @var \Drupal\package_manager\EventSubscriber\PreOperationStageValidatorInterface
    */
   protected $validator;
 
   /**
    * Constructs a PackageManagerReadinessCheck object.
    *
-   * @param \Drupal\package_manager\EventSubscriber\StageValidatorInterface $validator
+   * @param \Drupal\package_manager\EventSubscriber\PreOperationStageValidatorInterface $validator
    *   The Package Manager validator to run during readiness checking.
    */
-  public function __construct(StageValidatorInterface $validator) {
+  public function __construct(PreOperationStageValidatorInterface $validator) {
     $this->validator = $validator;
   }
 
@@ -39,7 +39,7 @@ class PackageManagerReadinessCheck implements EventSubscriberInterface {
    *   The event object.
    */
   public function validate(ReadinessCheckEvent $event): void {
-    $this->validator->validateStage($event);
+    $this->validator->validateStagePreOperation($event);
   }
 
   /**

src/Validator/StagedProjectsValidator.php

@@ -3,7 +3,6 @@
 namespace Drupal\automatic_updates\Validator;
 
 use Drupal\package_manager\Event\PreApplyEvent;
-use Drupal\package_manager\ValidationResult;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -38,10 +37,9 @@ final class StagedProjectsValidator implements EventSubscriberInterface {
       $staged_packages = $stage->getStageComposer()->getDrupalExtensionPackages();
     }
     catch (\Throwable $e) {
-      $result = ValidationResult::createError([
+      $event->addError([
         $e->getMessage(),
       ]);
-      $event->addValidationResult($result);
       return;
     }
 
@@ -69,7 +67,7 @@ final class StagedProjectsValidator implements EventSubscriberInterface {
         'The update cannot proceed because the following Drupal project was installed during the update.',
         'The update cannot proceed because the following Drupal projects were installed during the update.'
       );
-      $event->addValidationResult(ValidationResult::createError($new_packages_messages, $new_packages_summary));
+      $event->addError($new_packages_messages, $new_packages_summary);
     }
 
     // Check if any Drupal projects were removed.
@@ -89,7 +87,7 @@ final class StagedProjectsValidator implements EventSubscriberInterface {
         'The update cannot proceed because the following Drupal project was removed during the update.',
         'The update cannot proceed because the following Drupal projects were removed during the update.'
       );
-      $event->addValidationResult(ValidationResult::createError($removed_packages_messages, $removed_packages_summary));
+      $event->addError($removed_packages_messages, $removed_packages_summary);
     }
 
     // Get all the packages that are neither newly installed or removed to
@@ -115,7 +113,7 @@ final class StagedProjectsValidator implements EventSubscriberInterface {
           'The update cannot proceed because the following Drupal project was unexpectedly updated. Only Drupal Core updates are currently supported.',
           'The update cannot proceed because the following Drupal projects were unexpectedly updated. Only Drupal Core updates are currently supported.'
         );
-        $event->addValidationResult(ValidationResult::createError($version_change_messages, $version_change_summary));
+        $event->addError($version_change_messages, $version_change_summary);
       }
     }
   }

src/Validator/UpdateVersionValidator.php

@@ -6,8 +6,7 @@ use Composer\Semver\Semver;
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
 use Drupal\automatic_updates\Updater;
 use Drupal\package_manager\Event\PreCreateEvent;
-use Drupal\package_manager\Event\StageEvent;
-use Drupal\package_manager\ValidationResult;
+use Drupal\package_manager\Event\PreOperationStageEvent;
 use Drupal\Core\Extension\ExtensionVersion;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
@@ -48,10 +47,10 @@ class UpdateVersionValidator implements EventSubscriberInterface {
   /**
    * Validates that core is not being updated to another minor or major version.
    *
-   * @param \Drupal\package_manager\Event\StageEvent $event
+   * @param \Drupal\package_manager\Event\PreOperationStageEvent $event
    *   The event object.
    */
-  public function checkUpdateVersion(StageEvent $event): void {
+  public function checkUpdateVersion(PreOperationStageEvent $event): void {
     $stage = $event->getStage();
     // We only want to do this check if the stage belongs to Automatic Updates.
     if (!$stage instanceof Updater) {
@@ -85,32 +84,28 @@ class UpdateVersionValidator implements EventSubscriberInterface {
         '@to_version' => $to_version_string,
         '@from_version' => $from_version_string,
       ]);
-      $error = ValidationResult::createError($messages);
-      $event->addValidationResult($error);
+      $event->addError($messages);
     }
     elseif ($from_version->getVersionExtra() === 'dev') {
       $messages[] = $this->t('Drupal cannot be automatically updated from its current version, @from_version, to the recommended version, @to_version, because automatic updates from a dev version to any other version are not supported.', [
         '@to_version' => $to_version_string,
         '@from_version' => $from_version_string,
       ]);
-      $error = ValidationResult::createError($messages);
-      $event->addValidationResult($error);
+      $event->addError($messages);
     }
     elseif ($from_version->getMajorVersion() !== $to_version->getMajorVersion()) {
       $messages[] = $this->t('Drupal cannot be automatically updated from its current version, @from_version, to the recommended version, @to_version, because automatic updates from one major version to another are not supported.', [
         '@to_version' => $to_version_string,
         '@from_version' => $from_version_string,
       ]);
-      $error = ValidationResult::createError($messages);
-      $event->addValidationResult($error);
+      $event->addError($messages);
     }
     elseif ($from_version->getMinorVersion() !== $to_version->getMinorVersion()) {
       $messages[] = $this->t('Drupal cannot be automatically updated from its current version, @from_version, to the recommended version, @to_version, because automatic updates from one minor version to another are not supported.', [
         '@from_version' => $this->getCoreVersion(),
         '@to_version' => $package_versions[$core_package_name],
       ]);
-      $error = ValidationResult::createError($messages);
-      $event->addValidationResult($error);
+      $event->addError($messages);
     }
   }
 

tests/src/Functional/UpdaterFormTest.php

@@ -204,14 +204,6 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     // Since there's only one message, we shouldn't see the summary.
     $assert_session->pageTextNotContains($expected_results[0]->getSummary());
     $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
-
-    // If a validator flags a warning, but doesn't throw, the update should
-    // continue.
-    $expected_results = $this->testResults['checker_1']['1 warning'];
-    TestChecker1::setTestResult($expected_results, PreCreateEvent::class);
-    $page->pressButton('Update');
-    $this->checkForMetaRefresh();
-    $assert_session->pageTextContains('Ready to update');
   }
 
   /**

tests/src/Kernel/ReadinessValidation/PackageManagerReadinessChecksTest.php

@@ -4,7 +4,7 @@ namespace Drupal\Tests\automatic_updates\Kernel\ReadinessValidation;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
-use Drupal\package_manager\EventSubscriber\StageValidatorInterface;
+use Drupal\package_manager\EventSubscriber\PreOperationStageValidatorInterface;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
 use Prophecy\Argument;
 
@@ -65,8 +65,8 @@ class PackageManagerReadinessChecksTest extends AutomaticUpdatesKernelTestBase {
     // that it gets called with a readiness check event, when we run readiness
     // checks.
     $event = Argument::type(ReadinessCheckEvent::class);
-    $validator = $this->prophesize(StageValidatorInterface::class);
-    $validator->validateStage($event)->shouldBeCalled();
+    $validator = $this->prophesize(PreOperationStageValidatorInterface::class);
+    $validator->validateStagePreOperation($event)->shouldBeCalled();
     $this->container->set($service_id, $validator->reveal());
 
     $this->container->get('automatic_updates.readiness_validation_manager')