Issue #3338666 by omkar.podey, phenaproxima, tedbow, Wim Leers: Add functional...

Issue #3338666 by omkar.podey, phenaproxima, tedbow, Wim Leers: Add functional test that proves there is reasonable UX whenever a stage event subscriber has an exception

src/BatchProcessor.php

@@ -32,6 +32,13 @@ final class BatchProcessor {
    */
   public const MAINTENANCE_MODE_SESSION_KEY = '_automatic_updates_maintenance_mode';
 
+  /**
+   * The session key which stores error messages that occur in processing.
+   *
+   * @var string
+   */
+  private const ERROR_MESSAGES_SESSION_KEY = '_automatic_updates_errors';
+
   /**
    * Gets the update stage service.
    *
@@ -47,15 +54,24 @@ final class BatchProcessor {
    *
    * @param \Throwable $error
    *   The caught exception.
-   * @param array $context
-   *   The current context of the batch job.
    *
    * @throws \Throwable
    *   The caught exception, which will always be re-thrown once its messages
    *   have been recorded.
    */
-  protected static function handleException(\Throwable $error, array &$context): void {
-    $context['results']['errors'][] = $error->getMessage();
+  protected static function handleException(\Throwable $error): never {
+    // TRICKY: We need to store error messages in the session because the batch
+    // context becomes a dangling reference when static variables are globally
+    // reset by drupal_flush_all_caches(), which is called during the post-apply
+    // phase of the update. Which means that, when ::postApply() is called, any
+    // data added to the batch context in the current request is lost. On the
+    // other hand, data stored in the session is not affected.
+    /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */
+    $session = \Drupal::service('session');
+    $errors = $session->get(self::ERROR_MESSAGES_SESSION_KEY, []);
+    $errors[] = $error->getMessage();
+    $session->set(self::ERROR_MESSAGES_SESSION_KEY, $errors);
+
     throw $error;
   }
 
@@ -64,37 +80,32 @@ final class BatchProcessor {
    *
    * @param string[] $project_versions
    *   The project versions to be staged in the update, keyed by package name.
-   * @param array $context
-   *   The current context of the batch job.
    *
    * @see \Drupal\automatic_updates\UpdateStage::begin()
    */
-  public static function begin(array $project_versions, array &$context): void {
+  public static function begin(array $project_versions): void {
     try {
       $stage_id = static::getStage()->begin($project_versions);
       \Drupal::service('session')->set(static::STAGE_ID_SESSION_KEY, $stage_id);
     }
     catch (\Throwable $e) {
-      static::handleException($e, $context);
+      static::handleException($e);
     }
   }
 
   /**
    * Calls the update stage's stage() method.
    *
-   * @param array $context
-   *   The current context of the batch job.
-   *
    * @see \Drupal\automatic_updates\UpdateStage::stage()
    */
-  public static function stage(array &$context): void {
+  public static function stage(): void {
     $stage_id = \Drupal::service('session')->get(static::STAGE_ID_SESSION_KEY);
     try {
       static::getStage()->claim($stage_id)->stage();
     }
     catch (\Throwable $e) {
-      static::clean($stage_id, $context);
-      static::handleException($e, $context);
+      static::clean($stage_id);
+      static::handleException($e);
     }
   }
 
@@ -103,12 +114,10 @@ final class BatchProcessor {
    *
    * @param string $stage_id
    *   The stage ID.
-   * @param array $context
-   *   The current context of the batch job.
    *
    * @see \Drupal\automatic_updates\UpdateStage::apply()
    */
-  public static function commit(string $stage_id, array &$context): void {
+  public static function commit(string $stage_id): void {
     try {
       static::getStage()->claim($stage_id)->apply();
       // The batch system does not allow any single request to run for longer
@@ -120,7 +129,7 @@ final class BatchProcessor {
       sleep(1);
     }
     catch (\Throwable $e) {
-      static::handleException($e, $context);
+      static::handleException($e);
     }
   }
 
@@ -129,17 +138,15 @@ final class BatchProcessor {
    *
    * @param string $stage_id
    *   The stage ID.
-   * @param array $context
-   *   The current context of the batch job.
    *
    * @see \Drupal\automatic_updates\UpdateStage::postApply()
    */
-  public static function postApply(string $stage_id, array &$context): void {
+  public static function postApply(string $stage_id): void {
     try {
       static::getStage()->claim($stage_id)->postApply();
     }
     catch (\Throwable $e) {
-      static::handleException($e, $context);
+      static::handleException($e);
     }
   }
 
@@ -148,17 +155,15 @@ final class BatchProcessor {
    *
    * @param string $stage_id
    *   The stage ID.
-   * @param array $context
-   *   The current context of the batch job.
    *
    * @see \Drupal\automatic_updates\UpdateStage::destroy()
    */
-  public static function clean(string $stage_id, array &$context): void {
+  public static function clean(string $stage_id): void {
     try {
       static::getStage()->claim($stage_id)->destroy();
     }
     catch (\Throwable $e) {
-      static::handleException($e, $context);
+      static::handleException($e);
     }
   }
 
@@ -167,12 +172,8 @@ final class BatchProcessor {
    *
    * @param bool $success
    *   Indicate that the batch API tasks were all completed successfully.
-   * @param array $results
-   *   An array of all the results.
-   * @param array $operations
-   *   A list of the operations that had not been completed by the batch API.
    */
-  public static function finishStage(bool $success, array $results, array $operations): ?RedirectResponse {
+  public static function finishStage(bool $success): ?RedirectResponse {
     if ($success) {
       $stage_id = \Drupal::service('session')->get(static::STAGE_ID_SESSION_KEY);
       $url = Url::fromRoute('automatic_updates.confirmation_page', [
@@ -180,7 +181,7 @@ final class BatchProcessor {
       ]);
       return new RedirectResponse($url->setAbsolute()->toString());
     }
-    static::handleBatchError($results);
+    static::handleBatchError();
     return NULL;
   }
 
@@ -189,12 +190,8 @@ final class BatchProcessor {
    *
    * @param bool $success
    *   Indicate that the batch API tasks were all completed successfully.
-   * @param array $results
-   *   An array of all the results.
-   * @param array $operations
-   *   A list of the operations that had not been completed by the batch API.
    */
-  public static function finishCommit(bool $success, array $results, array $operations): ?RedirectResponse {
+  public static function finishCommit(bool $success): ?RedirectResponse {
     \Drupal::service('session')->remove(static::STAGE_ID_SESSION_KEY);
 
     if ($success) {
@@ -203,21 +200,21 @@ final class BatchProcessor {
         ->toString();
       return new RedirectResponse($url);
     }
-    static::handleBatchError($results);
+    static::handleBatchError();
     return NULL;
   }
 
   /**
    * Handles a batch job that finished with errors.
-   *
-   * @param array $results
-   *   The batch results.
    */
-  protected static function handleBatchError(array $results): void {
-    if (isset($results['errors'])) {
-      foreach ($results['errors'] as $error) {
-        \Drupal::messenger()->addError($error);
-      }
+  protected static function handleBatchError(): void {
+    /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */
+    $session = \Drupal::service('session');
+    $errors = $session->get(self::ERROR_MESSAGES_SESSION_KEY);
+    $session->remove(self::ERROR_MESSAGES_SESSION_KEY);
+
+    if (is_array($errors)) {
+      array_walk($errors, \Drupal::messenger()->addError(...));
     }
     else {
       \Drupal::messenger()->addError("Update error");

src/Form/UpdaterForm.php

@@ -152,7 +152,13 @@ final class UpdaterForm extends UpdateFormBase {
       $results = [];
     }
     else {
-      $results = $this->runStatusCheck($this->stage, $this->eventDispatcher);
+      try {
+        $results = $this->runStatusCheck($this->stage, $this->eventDispatcher);
+      }
+      catch (\Throwable $e) {
+        $this->messenger()->addError($e->getMessage());
+        return $form;
+      }
     }
     $this->displayResults($results, $this->renderer);
     $project = $project_info->getProjectInfo();

tests/src/Functional/UpdateErrorTest.php

@@ -4,8 +4,14 @@ declare(strict_types = 1);
 
 namespace Drupal\Tests\automatic_updates\Functional;
 
+use Drupal\package_manager\Event\PostApplyEvent;
+use Drupal\package_manager\Event\PostCreateEvent;
+use Drupal\package_manager\Event\PostDestroyEvent;
 use Drupal\package_manager\Event\PostRequireEvent;
+use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
+use Drupal\package_manager\Event\PreDestroyEvent;
+use Drupal\package_manager\Event\PreRequireEvent;
 use Drupal\package_manager\Event\StatusCheckEvent;
 use Drupal\package_manager\ValidationResult;
 use Drupal\automatic_updates_test\EventSubscriber\TestSubscriber1;
@@ -16,9 +22,23 @@ use Drupal\system\SystemManager;
  * @covers \Drupal\automatic_updates\Form\UpdaterForm
  * @group automatic_updates
  * @internal
+ *
+ * @todo Consolidate and remove duplicate test coverage in
+ *   https://drupal.org/i/3354325.
  */
 class UpdateErrorTest extends UpdaterFormTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+
+    $this->config('system.logging')
+      ->set('error_level', ERROR_REPORTING_DISPLAY_VERBOSE)
+      ->save();
+  }
+
   /**
    * Tests that the update stage is destroyed if an error occurs during require.
    */
@@ -164,4 +184,144 @@ class UpdateErrorTest extends UpdaterFormTestBase {
     $assert_session->pageTextNotContains($cached_message);
   }
 
+  /**
+   * Tests handling of exceptions thrown by event subscribers.
+   *
+   * @param string $event
+   *   The event that should throw an exception.
+   *
+   * @dataProvider providerExceptionFromEventSubscriber
+   */
+  public function testExceptionFromEventSubscriber(string $event): void {
+    $exception = new \Exception('Bad news bears!');
+    TestSubscriber::setException($exception, $event);
+
+    // Only simulate a staged update if we're going to get far enough that the
+    // stage directory will be created.
+    if ($event !== StatusCheckEvent::class && $event !== PreCreateEvent::class) {
+      $this->getStageFixtureManipulator()->setCorePackageVersion('9.8.1');
+    }
+
+    $session = $this->getSession();
+    $page = $session->getPage();
+    $assert_session = $this->assertSession();
+
+    $this->mockActiveCoreVersion('9.8.0');
+    $this->checkForUpdates();
+    $this->drupalGet('/admin/modules/update');
+
+    // StatusCheckEvent runs very early, before we can even start the update.
+    // If it raises the error we're expecting, we're done.
+    if ($event === StatusCheckEvent::class) {
+      $assert_session->statusMessageContains($exception->getMessage(), 'error');
+      // We shouldn't be able to start the update.
+      $assert_session->buttonNotExists('Update to 9.8.1');
+      return;
+    }
+
+    // Start the update.
+    $page->pressButton('Update to 9.8.1');
+    $this->checkForMetaRefresh();
+    // If the batch job fails, proceed to the error page. If it failed because
+    // of the exception we set up, we're done.
+    if ($page->hasLink('the error page')) {
+      // We should see the exception's backtrace.
+      $assert_session->responseContains('<pre class="backtrace">');
+      $page->clickLink('the error page');
+      $assert_session->statusMessageContains($exception->getMessage(), 'error');
+      // We should be on the start page.
+      $assert_session->addressEquals('/admin/modules/update');
+
+      // If we failed during post-create, the stage is not destroyed, so we
+      // should not be able to start the update anew without destroying the
+      // stage first. In all other cases, the stage should have been destroyed
+      // and we should be able to try again.
+      // @todo Delete the existing update on behalf of the user in
+      //   https://drupal.org/i/3346644.
+      if ($event === PostCreateEvent::class) {
+        $assert_session->buttonNotExists('Update to 9.8.1');
+        $assert_session->buttonExists('Delete existing update');
+      }
+      else {
+        $assert_session->buttonExists('Update to 9.8.1');
+        $assert_session->buttonNotExists('Delete existing update');
+      }
+      return;
+    }
+
+    // We should now be ready to finish the update...
+    $this->assertStringContainsString('/admin/automatic-update-ready/', $session->getCurrentUrl());
+    // ...but if we set it up to fail on PostRequireEvent, and we see the error
+    // message from that, we're done.
+    // @todo In https://drupal.org/i/3346644, ensure that PostRequireEvent
+    //   behaves the same way as PreCreateEvent, PostCreateEvent, and
+    //   PreRequireEvent.
+    if ($event === PostRequireEvent::class) {
+      $assert_session->statusMessageContains($exception->getMessage(), 'error');
+      $assert_session->buttonNotExists('Continue');
+      return;
+    }
+
+    // Ensure that we are expecting a failure from an event that is dispatched
+    // during the second phase (apply and destroy) of the update.
+    $this->assertContains($event, [
+      PreApplyEvent::class,
+      PostApplyEvent::class,
+      PreDestroyEvent::class,
+      PostDestroyEvent::class,
+    ]);
+    // Try to finish the update.
+    $page->pressButton('Continue');
+    $this->checkForMetaRefresh();
+    // As we did before, proceed to the error page if the batch job fails. If it
+    // failed because of the exception we set up, we're done here.
+    if ($page->hasLink('the error page')) {
+      // We should see the exception's backtrace.
+      $assert_session->responseContains('<pre class="backtrace">');
+      $page->clickLink('the error page');
+      $assert_session->statusMessageContains($exception->getMessage(), 'error');
+      // We should be back on the "ready to update" page.
+      $this->assertStringContainsString('/admin/automatic-update-ready/', $session->getCurrentUrl());
+      return;
+    }
+    $this->fail('Expected to encounter an error message during the update process.');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function tearDown(): void {
+    // Ensure that any pre- or post-destroy exception we set up during testing
+    // does not interfere with the parent class' ability to destroy the stage.
+    TestSubscriber::setException(NULL, PreDestroyEvent::class);
+    TestSubscriber::setException(NULL, PostDestroyEvent::class);
+
+    parent::tearDown();
+  }
+
+  /**
+   * Data provider for ::testExceptionFromEventSubscriber().
+   *
+   * @return array[]
+   *   The test cases.
+   */
+  public function providerExceptionFromEventSubscriber(): array {
+    $events = [
+      StatusCheckEvent::class,
+      PreCreateEvent::class,
+      PostCreateEvent::class,
+      PreRequireEvent::class,
+      PostRequireEvent::class,
+      PreApplyEvent::class,
+      PostApplyEvent::class,
+      PreDestroyEvent::class,
+      // @todo PostDestroyEvent leads to an exception with "This operation was
+      //   already canceled". This is because the batch processor redirects to
+      //   the UpdateReady form, which tries to claim the stage...which has been
+      //   destroyed. Fix this in https://drupal.org/i/3354003.
+      // PostDestroyEvent::class,
+    ];
+    return array_combine($events, array_map(fn ($event) => [$event], $events));
+  }
+
 }