From ee34db2123633d8511e1734664a62dface4e1dcd Mon Sep 17 00:00:00 2001
From: phenaproxima <phenaproxima@205645.no-reply.drupal.org>
Date: Wed, 19 Jan 2022 17:05:13 +0000
Subject: [PATCH] Issue #3258056 by phenaproxima, tedbow: Simplify
Stage::dispatch exception handling
---
.../src/Exception/StageException.php | 2 ++
.../src/Exception/StageOwnershipException.php | 2 ++
.../Exception/StageValidationException.php | 2 ++
package_manager/src/Stage.php | 24 +++++++-----------
src/Exception/UpdateException.php | 6 +++++
tests/src/Functional/UpdaterFormTest.php | 25 +++++++++----------
6 files changed, 33 insertions(+), 28 deletions(-)
diff --git a/package_manager/src/Exception/StageException.php b/package_manager/src/Exception/StageException.php
index e8011bac34..9e5e06ebc1 100644
--- a/package_manager/src/Exception/StageException.php
+++ b/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 {
}
diff --git a/package_manager/src/Exception/StageOwnershipException.php b/package_manager/src/Exception/StageOwnershipException.php
index 8a8e868ac9..5e44c8c360 100644
--- a/package_manager/src/Exception/StageOwnershipException.php
+++ b/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 {
}
diff --git a/package_manager/src/Exception/StageValidationException.php b/package_manager/src/Exception/StageValidationException.php
index 1bb0a9c696..2654615bf9 100644
--- a/package_manager/src/Exception/StageValidationException.php
+++ b/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 {
diff --git a/package_manager/src/Stage.php b/package_manager/src/Stage.php
index b1b9483f6d..756a45a23f 100644
--- a/package_manager/src/Stage.php
+++ b/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;
}
}
diff --git a/src/Exception/UpdateException.php b/src/Exception/UpdateException.php
index 390e899b2f..8ea6a9f3b0 100644
--- a/src/Exception/UpdateException.php
+++ b/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 {
}
diff --git a/tests/src/Functional/UpdaterFormTest.php b/tests/src/Functional/UpdaterFormTest.php
index f8e289ae21..e98f123d6f 100644
--- a/tests/src/Functional/UpdaterFormTest.php
+++ b/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();
--
GitLab