Issue #3354003 by phenaproxima: If a PostDestroyEvent subscriber throws an...

Issue #3354003 by phenaproxima: If a PostDestroyEvent subscriber throws an exception, users are redirected to a WSOD

src/BatchProcessor.php

@@ -50,16 +50,12 @@ final class BatchProcessor {
   }
 
   /**
-   * Records messages from a throwable, then re-throws it.
+   * Stores an error message for later display.
    *
-   * @param \Throwable $error
-   *   The caught exception.
-   *
-   * @throws \Throwable
-   *   The caught exception, which will always be re-thrown once its messages
-   *   have been recorded.
+   * @param string $error_message
+   *   The error message.
    */
-  protected static function handleException(\Throwable $error): never {
+  protected static function storeErrorMessage(string $error_message): void {
     // 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
@@ -69,10 +65,8 @@ final class BatchProcessor {
     /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */
     $session = \Drupal::service('session');
     $errors = $session->get(self::ERROR_MESSAGES_SESSION_KEY, []);
-    $errors[] = $error->getMessage();
+    $errors[] = $error_message;
     $session->set(self::ERROR_MESSAGES_SESSION_KEY, $errors);
-
-    throw $error;
   }
 
   /**
@@ -89,7 +83,8 @@ final class BatchProcessor {
       \Drupal::service('session')->set(static::STAGE_ID_SESSION_KEY, $stage_id);
     }
     catch (\Throwable $e) {
-      static::handleException($e);
+      static::storeErrorMessage($e->getMessage());
+      throw $e;
     }
   }
 
@@ -105,7 +100,8 @@ final class BatchProcessor {
     }
     catch (\Throwable $e) {
       static::clean($stage_id);
-      static::handleException($e);
+      static::storeErrorMessage($e->getMessage());
+      throw $e;
     }
   }
 
@@ -129,7 +125,8 @@ final class BatchProcessor {
       sleep(1);
     }
     catch (\Throwable $e) {
-      static::handleException($e);
+      static::storeErrorMessage($e->getMessage());
+      throw $e;
     }
   }
 
@@ -146,7 +143,8 @@ final class BatchProcessor {
       static::getStage()->claim($stage_id)->postApply();
     }
     catch (\Throwable $e) {
-      static::handleException($e);
+      static::storeErrorMessage($e->getMessage());
+      throw $e;
     }
   }
 
@@ -156,14 +154,24 @@ final class BatchProcessor {
    * @param string $stage_id
    *   The stage ID.
    *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse|null
+   *   A redirect response, or NULL to proceed to the normal finish page.
+   *
    * @see \Drupal\automatic_updates\UpdateStage::destroy()
    */
-  public static function clean(string $stage_id): void {
+  public static function clean(string $stage_id): ?RedirectResponse {
     try {
       static::getStage()->claim($stage_id)->destroy();
+      return NULL;
     }
     catch (\Throwable $e) {
-      static::handleException($e);
+      static::storeErrorMessage($e->getMessage());
+      static::displayStoredErrorMessages();
+      // If we failed to destroy the stage, the update still (mostly) succeeded,
+      // so forward the user to the finish page. They won't be able to start
+      // another update (or, indeed, any other Package Manager operation) until
+      // they destroy the existing stage anyway.
+      return static::finishCommit(TRUE);
     }
   }
 
@@ -181,7 +189,7 @@ final class BatchProcessor {
       ]);
       return new RedirectResponse($url->setAbsolute()->toString());
     }
-    static::handleBatchError();
+    static::displayStoredErrorMessages();
     return NULL;
   }
 
@@ -200,14 +208,16 @@ final class BatchProcessor {
         ->toString();
       return new RedirectResponse($url);
     }
-    static::handleBatchError();
+    static::displayStoredErrorMessages();
     return NULL;
   }
 
   /**
-   * Handles a batch job that finished with errors.
+   * Displays any error messages that were stored in the session.
+   *
+   * @see ::storeErrorMessage()
    */
-  protected static function handleBatchError(): void {
+  protected static function displayStoredErrorMessages(): void {
     /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */
     $session = \Drupal::service('session');
     $errors = $session->get(self::ERROR_MESSAGES_SESSION_KEY);

tests/src/Functional/UpdateErrorTest.php

@@ -279,12 +279,33 @@ class UpdateErrorTest extends UpdaterFormTestBase {
       // 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.
+      // We should be back on the "ready to update" page, and the exception
+      // message should be visible.
       $this->assertStringContainsString('/admin/automatic-update-ready/', $session->getCurrentUrl());
+      $assert_session->statusMessageContains($exception->getMessage(), 'error');
       return;
     }
-    $this->fail('Expected to encounter an error message during the update process.');
+
+    // If we got any further, ensure we were expecting an exception during the
+    // destroy phase.
+    $this->assertStringEndsWith('DestroyEvent', $event);
+    // We should have been forwarded to the main update page, and the exception
+    // message should be visible.
+    $assert_session->addressEquals('/admin/reports/updates');
+    $assert_session->statusMessageContains($exception->getMessage(), 'error');
+
+    // If the exception was thrown during PreDestroyEvent, the stage was not
+    // destroyed, so pretend there's another available update, and ensure that
+    // the user cannot update to it before they delete the existing update.
+    if ($event === PreDestroyEvent::class) {
+      $this->mockActiveCoreVersion('9.8.0');
+      $this->checkForUpdates();
+      $this->drupalGet('/admin/reports/updates/update');
+
+      $assert_session->statusMessageContains('Cannot begin an update because another Composer operation is currently in progress.', 'error');
+      $assert_session->buttonNotExists('Update to 9.8.1');
+      $assert_session->buttonExists('Delete existing update');
+    }
   }
 
   /**
@@ -315,11 +336,7 @@ class UpdateErrorTest extends UpdaterFormTestBase {
       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,
+      PostDestroyEvent::class,
     ];
     return array_combine($events, array_map(fn ($event) => [$event], $events));
   }