From af056ae6839d4bac670df1d45fc26eb3fc22dbf5 Mon Sep 17 00:00:00 2001
From: omkar podey <58183-omkar.podey@users.noreply.drupalcode.org>
Date: Mon, 17 Apr 2023 19:48:10 +0000
Subject: [PATCH] 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 | 89 ++++++-------
src/Form/UpdaterForm.php | 8 +-
tests/src/Functional/UpdateErrorTest.php | 160 +++++++++++++++++++++++
3 files changed, 210 insertions(+), 47 deletions(-)
diff --git a/src/BatchProcessor.php b/src/BatchProcessor.php
index c4656c126f..ce81085c4a 100644
--- a/src/BatchProcessor.php
+++ b/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");
diff --git a/src/Form/UpdaterForm.php b/src/Form/UpdaterForm.php
index ee7768166c..79c1f558c9 100644
--- a/src/Form/UpdaterForm.php
+++ b/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();
diff --git a/tests/src/Functional/UpdateErrorTest.php b/tests/src/Functional/UpdateErrorTest.php
index b5666fb012..52cfd10f2d 100644
--- a/tests/src/Functional/UpdateErrorTest.php
+++ b/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));
+ }
+
}
--
GitLab