diff --git a/src/BatchProcessor.php b/src/BatchProcessor.php index ce81085c4a00e0eba81d73b5ca39a8667b6c42f2..aa816be4949c9c4cd06f4803e17afc653df6b918 100644 --- a/src/BatchProcessor.php +++ b/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); diff --git a/tests/src/Functional/UpdateErrorTest.php b/tests/src/Functional/UpdateErrorTest.php index 52cfd10f2d8478146f5f424b63baa99b6be8fc36..eba186952512b1dfbef8000c0c4169de3d910a02 100644 --- a/tests/src/Functional/UpdateErrorTest.php +++ b/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)); }