Issue #3258056 by phenaproxima, tedbow: Simplify Stage::dispatch exception handling

package_manager/src/Exception/StageException.php

@@ -4,6 +4,8 @@ namespace Drupal\package_manager\Exception;
 
 /**
  * Base class for all exceptions related to staging operations.
+ *
+ * Should not be thrown by external code.
  */
 class StageException extends \RuntimeException {
 }

package_manager/src/Exception/StageOwnershipException.php

@@ -4,6 +4,8 @@ namespace Drupal\package_manager\Exception;
 
 /**
  * Exception thrown if a stage encounters an ownership or locking error.
+ *
+ * Should not be thrown by external code.
  */
 class StageOwnershipException extends StageException {
 }

package_manager/src/Exception/StageValidationException.php

@@ -4,6 +4,8 @@ namespace Drupal\package_manager\Exception;
 
 /**
  * Exception thrown if a stage has validation errors.
+ *
+ * Should not be thrown by external code.
  */
 class StageValidationException extends StageException {
 

package_manager/src/Stage.php

@@ -333,9 +333,8 @@ class Stage {
    *   The event object.
    *
    * @throws \Drupal\package_manager\Exception\StageValidationException
-   *   If the event collects any validation errors, or a subscriber throws a
-   *   StageValidationException directly.
-   * @throws \RuntimeException
+   *   If the event collects any validation errors.
+   * @throws \Drupal\package_manager\Exception\StageException
    *   If any other sort of error occurs.
    */
   protected function dispatch(StageEvent $event): void {
@@ -344,25 +343,20 @@ class Stage {
 
       $results = $event->getResults();
       if ($results) {
-        throw new StageValidationException($results);
+        $error = new StageValidationException($results);
       }
     }
     catch (\Throwable $error) {
-      // @todo Simplify exception handling in https://www.drupal.org/i/3258056.
-      // If we are not going to be able to create the staging area, mark it as
-      // available.
+      $error = new StageException($error->getMessage(), $error->getCode(), $error);
+    }
+
+    if (isset($error)) {
+      // If we won't be able to create the staging area, mark it as available.
       // @see ::create()
       if ($event instanceof PreCreateEvent) {
         $this->markAsAvailable();
       }
-
-      // Wrap the exception to preserve the backtrace, and re-throw it.
-      if ($error instanceof StageValidationException) {
-        throw new StageValidationException($error->getResults(), $error->getMessage(), $error->getCode(), $error);
-      }
-      else {
-        throw new \RuntimeException($error->getMessage(), $error->getCode(), $error);
-      }
+      throw $error;
     }
   }
 

src/Exception/UpdateException.php

@@ -6,6 +6,12 @@ use Drupal\package_manager\Exception\StageValidationException;
 
 /**
  * Defines a custom exception for a failure during an update.
+ *
+ * Should not be thrown by external code. This is only used to identify
+ * validation errors that occurred during a staging operation performed by
+ * Automatic Updates.
+ *
+ * @see \Drupal\automatic_updates\Updater::dispatch()
  */
 class UpdateException extends StageValidationException {
 }

tests/src/Functional/UpdaterFormTest.php

@@ -3,7 +3,6 @@
 namespace Drupal\Tests\automatic_updates\Functional;
 
 use Drupal\automatic_updates\Event\ReadinessCheckEvent;
-use Drupal\automatic_updates\Exception\UpdateException;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\ValidationResult;
 use Drupal\automatic_updates_test\ReadinessChecker\TestChecker1;
@@ -176,29 +175,29 @@ class UpdaterFormTest extends AutomaticUpdatesFunctionalTestBase {
     $assert_session->pageTextNotContains((string) $message);
     TestChecker1::setTestResult(NULL, ReadinessCheckEvent::class);
 
-    // Repackage the validation error as an exception, so we can test what
-    // happens if a validator throws once the update has started.
-    $error = new UpdateException($expected_results, 'The update exploded.');
+    // Make the validator throw an exception during pre-create.
+    $error = new \Exception('The update exploded.');
     TestChecker1::setException($error, PreCreateEvent::class);
     $session->reload();
     $assert_session->pageTextNotContains(static::$errorsExplanation);
     $assert_session->pageTextNotContains(static::$warningsExplanation);
     $page->pressButton('Update');
     $this->checkForMetaRefresh();
-
-    // If a validator flags an error, but doesn't throw, the update should still
-    // be halted.
     $this->assertUpdateStagedTimes(0);
     $assert_session->pageTextContainsOnce('An error has occurred.');
     $page->clickLink('the error page');
-    $assert_session->pageTextContainsOnce((string) $expected_results[0]->getMessages()[0]);
-    // Since there's only one error message, we shouldn't see the summary...
+    // We should see the exception message, but not the validation result's
+    // messages or summary, because exceptions thrown directly by event
+    // subscribers are wrapped in simple exceptions and re-thrown.
+    $assert_session->pageTextContainsOnce($error->getMessage());
+    $assert_session->pageTextNotContains((string) $expected_results[0]->getMessages()[0]);
     $assert_session->pageTextNotContains($expected_results[0]->getSummary());
-    // ...but we should see the exception message.
-    $assert_session->pageTextContainsOnce('The update exploded.');
-    // If the error is thrown in PreCreateEvent the update stage will not have
-    // been created.
+    // Since the error occurred during pre-create, there should be no existing
+    // update to delete.
     $assert_session->buttonNotExists('Delete existing update');
+
+    // If a validator flags an error, but doesn't throw, the update should still
+    // be halted.
     TestChecker1::setTestResult($expected_results, PreCreateEvent::class);
     $page->pressButton('Update');
     $this->checkForMetaRefresh();