From 6f26b9e2d9de93b7ef639a36a7d3a3fe68cb22a3 Mon Sep 17 00:00:00 2001
From: Ted Bowman <41201-tedbow@users.noreply.drupalcode.org>
Date: Wed, 31 May 2023 04:40:19 +0000
Subject: [PATCH] Issue #3355074 by Wim Leers, phenaproxima, tedbow: Failure
marker message does not contain exception message + backtrace if available
---
package_manager/src/FailureMarker.php | 68 ++++++++++++++++---
package_manager/src/StageBase.php | 4 +-
.../tests/src/Kernel/FailureMarkerTest.php | 53 ++++++++++++---
.../tests/src/Kernel/StageBaseTest.php | 29 ++++----
src/CronUpdateStage.php | 6 ++
tests/src/Kernel/CronUpdateStageTest.php | 4 +-
.../ScaffoldFilePermissionsValidatorTest.php | 2 +-
7 files changed, 133 insertions(+), 33 deletions(-)
diff --git a/package_manager/src/FailureMarker.php b/package_manager/src/FailureMarker.php
index d174613379..04cf6c51a5 100644
--- a/package_manager/src/FailureMarker.php
+++ b/package_manager/src/FailureMarker.php
@@ -58,35 +58,87 @@ final class FailureMarker {
* The stage.
* @param \Drupal\Core\StringTranslation\TranslatableMarkup $message
* Failure message to be added.
+ * @param \Throwable $throwable
+ * (optional) The throwable that caused the failure.
*/
- public function write(StageBase $stage, TranslatableMarkup $message): void {
+ public function write(StageBase $stage, TranslatableMarkup $message, \Throwable $throwable = NULL): void {
$data = [
'stage_class' => get_class($stage),
'stage_file' => (new \ReflectionObject($stage))->getFileName(),
- 'message' => $message->render(),
+ 'message' => (string) $message,
+ 'throwable_class' => $throwable ? get_class($throwable) : FALSE,
+ 'throwable_message' => $throwable?->getMessage() ?? 'Not available',
+ 'throwable_backtrace' => $throwable?->getTraceAsString() ?? 'Not available.',
];
file_put_contents($this->getPath(), Yaml::dump($data));
}
/**
- * Asserts the failure file doesn't exist.
+ * Gets the data from the file if it exists.
+ *
+ * @return array|null
+ * The data from the file if it exists.
*
* @throws \Drupal\package_manager\Exception\StageFailureMarkerException
- * Thrown if the marker file exists.
+ * Thrown if failure marker exists but cannot be decoded.
*/
- public function assertNotExists(): void {
+ private function getData(): ?array {
$path = $this->getPath();
-
if (file_exists($path)) {
$data = file_get_contents($path);
try {
- $data = Yaml::parse($data);
+ return Yaml::parse($data);
+
}
catch (ParseException $exception) {
throw new StageFailureMarkerException('Failure marker file exists but cannot be decoded.', $exception->getCode(), $exception);
}
+ }
+ return NULL;
+ }
+
+ /**
+ * Gets the message from the file if it exists.
+ *
+ * @param bool $include_backtrace
+ * Whether to include the backtrace in the message. Defaults to TRUE. May be
+ * set to FALSE in a context where it does not make sense to include, such
+ * as e-mails.
+ *
+ * @return string|null
+ * The message from the file if it exists, otherwise NULL.
+ *
+ * @throws \Drupal\package_manager\Exception\StageFailureMarkerException
+ * Thrown if failure marker exists but cannot be decoded.
+ */
+ public function getMessage(bool $include_backtrace = TRUE): ?string {
+ $data = $this->getData();
+ if ($data === NULL) {
+ return NULL;
+ }
+ $message = $data['message'];
+ if ($data['throwable_class']) {
+ $message .= sprintf(
+ ' Caused by %s, with this message: %s',
+ $data['throwable_class'],
+ $data['throwable_message'],
+ );
+ if ($include_backtrace) {
+ $message .= "\nBacktrace:\n" . $data['throwable_backtrace'];
+ }
+ }
+ return $message;
+ }
- throw new StageFailureMarkerException($data['message']);
+ /**
+ * Asserts the failure file doesn't exist.
+ *
+ * @throws \Drupal\package_manager\Exception\StageFailureMarkerException
+ * Thrown if the marker file exists.
+ */
+ public function assertNotExists(): void {
+ if ($message = $this->getMessage()) {
+ throw new StageFailureMarkerException($message);
}
}
diff --git a/package_manager/src/StageBase.php b/package_manager/src/StageBase.php
index 49d57c828e..1aa124a85b 100644
--- a/package_manager/src/StageBase.php
+++ b/package_manager/src/StageBase.php
@@ -470,7 +470,9 @@ abstract class StageBase implements LoggerAwareInterface {
// applying, because in this situation, the site owner should probably
// restore everything from a backup.
$this->setNotApplying()();
- throw new ApplyFailedException($this, (string) $this->getFailureMarkerMessage(), $throwable->getCode(), $throwable);
+ // Update the marker file with the information from the throwable.
+ $this->failureMarker->write($this, $this->getFailureMarkerMessage(), $throwable);
+ throw new ApplyFailedException($this, $this->failureMarker->getMessage(), $throwable->getCode(), $throwable);
}
$this->failureMarker->clear();
$this->setMetadata(self::TEMPSTORE_CHANGES_APPLIED, TRUE);
diff --git a/package_manager/tests/src/Kernel/FailureMarkerTest.php b/package_manager/tests/src/Kernel/FailureMarkerTest.php
index c246845964..a56ef95759 100644
--- a/package_manager/tests/src/Kernel/FailureMarkerTest.php
+++ b/package_manager/tests/src/Kernel/FailureMarkerTest.php
@@ -4,7 +4,6 @@ declare(strict_types = 1);
namespace Drupal\Tests\package_manager\Kernel;
-use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\package_manager\Exception\StageFailureMarkerException;
use Drupal\package_manager\FailureMarker;
@@ -14,18 +13,42 @@ use Drupal\package_manager\FailureMarker;
* @internal
*/
class FailureMarkerTest extends PackageManagerKernelTestBase {
- use StringTranslationTrait;
/**
- * @covers ::assertNotExists
+ * @covers ::getMessage
+ * @testWith [true]
+ * [false]
*/
- public function testExceptionIfExists(): void {
+ public function testGetMessageWithoutThrowable(bool $include_backtrace): void {
$failure_marker = $this->container->get(FailureMarker::class);
- $failure_marker->write($this->createStage(), $this->t('Disastrous catastrophe!'));
+ $failure_marker->write($this->createStage(), t('Disastrous catastrophe!'));
- $this->expectException(StageFailureMarkerException::class);
- $this->expectExceptionMessage('Disastrous catastrophe!');
- $failure_marker->assertNotExists();
+ $this->assertMatchesRegularExpression('/^Disastrous catastrophe!$/', $failure_marker->getMessage($include_backtrace));
+ }
+
+ /**
+ * @covers ::getMessage
+ * @testWith [true]
+ * [false]
+ */
+ public function testGetMessageWithThrowable(bool $include_backtrace): void {
+ $failure_marker = $this->container->get(FailureMarker::class);
+ $failure_marker->write($this->createStage(), t('Disastrous catastrophe!'), new \Exception('Witchcraft!'));
+
+ $expected_pattern = $include_backtrace
+ ? <<<REGEXP
+/^Disastrous catastrophe! Caused by Exception, with this message: Witchcraft!
+Backtrace:
+#0 .*FailureMarkerTest->testGetMessageWithThrowable\(true\)
+#1 .*
+#2 .*
+#3 .*/
+REGEXP
+ : '/^Disastrous catastrophe! Caused by Exception, with this message: Witchcraft!$/';
+ $this->assertMatchesRegularExpression(
+ $expected_pattern,
+ $failure_marker->getMessage($include_backtrace)
+ );
}
/**
@@ -43,4 +66,18 @@ class FailureMarkerTest extends PackageManagerKernelTestBase {
$failure_marker->assertNotExists();
}
+ /**
+ * Tests that the failure marker can contain an exception message.
+ *
+ * @covers ::assertNotExists
+ */
+ public function testAssertNotExists(): void {
+ $failure_marker = $this->container->get(FailureMarker::class);
+ $failure_marker->write($this->createStage(), t('Something wicked occurred here.'), new \Exception('Witchcraft!'));
+
+ $this->expectException(StageFailureMarkerException::class);
+ $this->expectExceptionMessageMatches('/^Something wicked occurred here. Caused by Exception, with this message: Witchcraft!\nBacktrace:\n#0 .*/');
+ $failure_marker->assertNotExists();
+ }
+
}
diff --git a/package_manager/tests/src/Kernel/StageBaseTest.php b/package_manager/tests/src/Kernel/StageBaseTest.php
index 01e4c500c3..843b4e8422 100644
--- a/package_manager/tests/src/Kernel/StageBaseTest.php
+++ b/package_manager/tests/src/Kernel/StageBaseTest.php
@@ -341,10 +341,14 @@ class StageBaseTest extends PackageManagerKernelTestBase {
// \Drupal\package_manager\Stage::getFailureMarkerMessage() when throwing
// ApplyFailedException.
if ($expected_class == ApplyFailedException::class) {
- $thrown_message = 'Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.';
+ $class_in_message = get_class($throwable);
+ $thrown_message = "/^Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup. Caused by $class_in_message, with this message: A very bad thing happened\nBacktrace:\n#0 .*/";
+ }
+ else {
+ $thrown_message = "/^$thrown_message$/";
}
$this->assertInstanceOf($expected_class, $exception);
- $this->assertSame($thrown_message, $exception->getMessage());
+ $this->assertMatchesRegularExpression($thrown_message, $exception->getMessage());
$this->assertSame(123, $exception->getCode());
$failure_marker = $this->container->get(FailureMarker::class);
@@ -368,14 +372,14 @@ class StageBaseTest extends PackageManagerKernelTestBase {
// Make the committer throw an exception, which should cause the failure
// marker to be present.
- $thrown = new \Exception('Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.');
+ $thrown = new \Exception('Thrown by the committer.');
LoggingCommitter::setException($thrown);
try {
$stage->apply();
$this->fail('Expected an exception.');
}
catch (ApplyFailedException $e) {
- $this->assertSame($thrown->getMessage(), $e->getMessage());
+ $this->assertStringContainsString($thrown->getMessage(), $e->getMessage());
$this->assertFalse($stage->isApplying());
}
$stage->destroy();
@@ -388,7 +392,7 @@ class StageBaseTest extends PackageManagerKernelTestBase {
$this->fail('Expected an exception.');
}
catch (StageFailureMarkerException $e) {
- $this->assertSame('Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.', $e->getMessage());
+ $this->assertMatchesRegularExpression('/^Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup. Caused by Exception, with this message: ' . $thrown->getMessage() . "\nBacktrace:\n#0 .*/", $e->getMessage());
$this->assertFalse($stage->isApplying());
}
@@ -573,18 +577,17 @@ class StageBaseTest extends PackageManagerKernelTestBase {
* @dataProvider providerFailureDuringComposerStagerOperations
*/
public function testFailureDuringComposerStagerOperations(string $throwing_class): void {
+ $exception = new \Exception("$throwing_class is angry!", 1024);
+ $throwing_class::setException($exception);
+
+ $expected_message = preg_quote($exception->getMessage());
if ($throwing_class === LoggingCommitter::class) {
- // If the committer throws an exception, Stage will always re-throw it
- // with a predetermined failure message.
- $expected_message = 'Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup.';
+ $expected_message = "/^Staged changes failed to apply, and the site is in an indeterminate state. It is strongly recommended to restore the code and database from a backup. Caused by Exception, with this message: $expected_message\nBacktrace:\n#0 .*/";
}
else {
- $expected_message = "$throwing_class is angry!";
+ $expected_message = "/^$expected_message$/";
}
- $exception = new \Exception($expected_message, 1024);
- $throwing_class::setException($exception);
-
$stage = $this->createStage();
try {
$stage->create();
@@ -593,7 +596,7 @@ class StageBaseTest extends PackageManagerKernelTestBase {
$this->fail('Expected an exception to be thrown, but it was not.');
}
catch (StageException $e) {
- $this->assertSame($expected_message, $e->getMessage());
+ $this->assertMatchesRegularExpression($expected_message, $e->getMessage());
$this->assertSame(1024, $e->getCode());
$this->assertSame($exception, $e->getPrevious());
}
diff --git a/src/CronUpdateStage.php b/src/CronUpdateStage.php
index 2f1c90d096..f305e01cab 100644
--- a/src/CronUpdateStage.php
+++ b/src/CronUpdateStage.php
@@ -14,6 +14,7 @@ use Drupal\Core\Url;
use Drupal\package_manager\ComposerInspector;
use Drupal\package_manager\Exception\ApplyFailedException;
use Drupal\package_manager\Exception\StageEventException;
+use Drupal\package_manager\Exception\StageFailureMarkerException;
use Drupal\package_manager\FailureMarker;
use Drupal\package_manager\PathLocator;
use Drupal\package_manager\ProjectInfo;
@@ -215,6 +216,11 @@ class CronUpdateStage extends UpdateStage {
'target_version' => $target_version,
'error_message' => $e->getMessage(),
];
+ // Omit the backtrace in e-mails. That will be visible on the site, and is
+ // also stored in the failure marker.
+ if ($e instanceof StageFailureMarkerException || $e instanceof ApplyFailedException) {
+ $mail_params['error_message'] = $this->failureMarker->getMessage(FALSE);
+ }
if ($e instanceof ApplyFailedException) {
$mail_params['urgent'] = TRUE;
$key = 'cron_failed_apply';
diff --git a/tests/src/Kernel/CronUpdateStageTest.php b/tests/src/Kernel/CronUpdateStageTest.php
index cfc4f09ae4..efe2111c90 100644
--- a/tests/src/Kernel/CronUpdateStageTest.php
+++ b/tests/src/Kernel/CronUpdateStageTest.php
@@ -564,14 +564,14 @@ END;
*/
public function testApplyFailureEmail(): void {
$this->getStageFixtureManipulator()->setCorePackageVersion('9.8.1');
- $error = new \Exception('I drink your milkshake!');
+ $error = new \LogicException('I drink your milkshake!');
LoggingCommitter::setException($error);
$this->container->get('cron')->run();
$expected_body = <<<END
Drupal core failed to update automatically from 9.8.0 to 9.8.1. The following error was logged:
-Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup.
+Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup. Caused by LogicException, with this message: {$error->getMessage()}
This e-mail was sent by the Automatic Updates module. Unattended updates are not yet fully supported.
diff --git a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
index c7d9d2389f..9236e3eca5 100644
--- a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
+++ b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
@@ -345,7 +345,7 @@ class ScaffoldFilePermissionsValidatorTest extends AutomaticUpdatesKernelTestBas
// If we try to overwrite any write-protected paths, even if they're not
// scaffold files, we'll get an ApplyFailedException.
catch (ApplyFailedException $e) {
- $this->assertSame("Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup.", $e->getMessage());
+ $this->assertStringStartsWith("Automatic updates failed to apply, and the site is in an indeterminate state. Consider restoring the code and database from a backup.", $e->getMessage());
}
catch (StageEventException $e) {
$this->assertExpectedResultsFromException($expected_results, $e);
--
GitLab