From d9210f6f811e4662807e78b4dee4da6c7bbf7c30 Mon Sep 17 00:00:00 2001
From: omkar podey <58183-omkar.podey@users.noreply.drupalcode.org>
Date: Wed, 21 Jun 2023 16:49:37 +0000
Subject: [PATCH] Issue #3364725 by omkar.podey, phenaproxima, Wim Leers: The
$format_result closure in AdminStatusCheckMessages::displayResults() expects
to return a translatable string, which will not be the case for validation
results created from throwables
---
package_manager/package_manager.api.php | 5 +++-
.../src/Event/PreOperationStageEvent.php | 25 +++++++++++++---
.../src/Event/StatusCheckEvent.php | 10 ++++++-
.../src/EventSubscriber/TestSubscriber.php | 8 +----
.../Kernel/PackageManagerKernelTestBase.php | 8 +----
.../tests/src/Kernel/StageEventsTest.php | 30 +++++++++++++++++++
src/Validation/AdminStatusCheckMessages.php | 2 +-
tests/src/Functional/UpdateErrorTest.php | 15 ++++++++++
8 files changed, 82 insertions(+), 21 deletions(-)
diff --git a/package_manager/package_manager.api.php b/package_manager/package_manager.api.php
index 3b565fec8c..5adbc6711e 100644
--- a/package_manager/package_manager.api.php
+++ b/package_manager/package_manager.api.php
@@ -248,12 +248,15 @@
* cycle from proceeding any further. If a validator encounters an exception,
* it can use ::addErrorFromThrowable() instead of ::addError(). During status
* checks, validators can call ::addWarning() for less severe problems --
- * warnings will NOT stop the stage life cycle.
+ * warnings will NOT stop the stage life cycle. All three are convenience
+ * methods for equivalent \Drupal\package_manager\ValidationResult constructors,
+ * which can then be added to the event using ::addResult().
*
* @see \Drupal\package_manager\ValidationResult
* @see \Drupal\package_manager\Event\PreOperationStageEvent::addError()
* @see \Drupal\package_manager\Event\PreOperationStageEvent::addErrorFromThrowable()
* @see \Drupal\package_manager\Event\StatusCheckEvent::addWarning()
+ * @see \Drupal\package_manager\Event\PreOperationStageEvent::addResult()
*
* @section sec_excluded_paths Excluding files from stage operations
* Certain files are never copied into the stage directory because they are
diff --git a/package_manager/src/Event/PreOperationStageEvent.php b/package_manager/src/Event/PreOperationStageEvent.php
index 63d6997116..d451004353 100644
--- a/package_manager/src/Event/PreOperationStageEvent.php
+++ b/package_manager/src/Event/PreOperationStageEvent.php
@@ -40,7 +40,7 @@ abstract class PreOperationStageEvent extends StageEvent {
}
/**
- * Adds error information to the event.
+ * Convenience method, adds error validation result.
*
* @param \Drupal\Core\StringTranslation\TranslatableMarkup[] $messages
* The error messages.
@@ -49,11 +49,11 @@ abstract class PreOperationStageEvent extends StageEvent {
* is more than one message.
*/
public function addError(array $messages, ?TranslatableMarkup $summary = NULL): void {
- $this->results[] = ValidationResult::createError(array_values($messages), $summary);
+ $this->addResult(ValidationResult::createError(array_values($messages), $summary));
}
/**
- * Adds an error from a throwable.
+ * Convenience method, adds an error validation result from a throwable.
*
* @param \Throwable $throwable
* The throwable.
@@ -61,7 +61,24 @@ abstract class PreOperationStageEvent extends StageEvent {
* (optional) The summary of error messages.
*/
public function addErrorFromThrowable(\Throwable $throwable, ?TranslatableMarkup $summary = NULL): void {
- $this->results[] = ValidationResult::createErrorFromThrowable($throwable, $summary);
+ $this->addResult(ValidationResult::createErrorFromThrowable($throwable, $summary));
+ }
+
+ /**
+ * Adds a validation result to the event.
+ *
+ * @param \Drupal\package_manager\ValidationResult $result
+ * The validation result to add.
+ *
+ * @throws \InvalidArgumentException
+ * Thrown if the validation result is not an error.
+ */
+ public function addResult(ValidationResult $result): void {
+ // Only errors are allowed for this event.
+ if ($result->severity !== SystemManager::REQUIREMENT_ERROR) {
+ throw new \InvalidArgumentException('Only errors are allowed.');
+ }
+ $this->results[] = $result;
}
/**
diff --git a/package_manager/src/Event/StatusCheckEvent.php b/package_manager/src/Event/StatusCheckEvent.php
index 41dc77cde7..c50629af1c 100644
--- a/package_manager/src/Event/StatusCheckEvent.php
+++ b/package_manager/src/Event/StatusCheckEvent.php
@@ -60,7 +60,15 @@ final class StatusCheckEvent extends PreOperationStageEvent {
* message, optional otherwise.
*/
public function addWarning(array $messages, ?TranslatableMarkup $summary = NULL): void {
- $this->results[] = ValidationResult::createWarning($messages, $summary);
+ $this->addResult(ValidationResult::createWarning($messages, $summary));
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function addResult(ValidationResult $result): void {
+ // Override the parent to also allow warnings.
+ $this->results[] = $result;
}
}
diff --git a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
index 4a74187565..260c0761ab 100644
--- a/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
+++ b/package_manager/tests/modules/package_manager_test_validation/src/EventSubscriber/TestSubscriber.php
@@ -15,7 +15,6 @@ use Drupal\package_manager\Event\PreDestroyEvent;
use Drupal\package_manager\Event\PreRequireEvent;
use Drupal\package_manager\Event\StageEvent;
use Drupal\package_manager\Event\StatusCheckEvent;
-use Drupal\system\SystemManager;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
@@ -144,12 +143,7 @@ class TestSubscriber implements EventSubscriberInterface {
}
/** @var \Drupal\package_manager\ValidationResult $result */
foreach ($results as $result) {
- if ($result->severity === SystemManager::REQUIREMENT_ERROR) {
- $event->addError($result->messages, $result->summary);
- }
- else {
- $event->addWarning($result->messages, $result->summary);
- }
+ $event->addResult($result);
}
}
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index 909ab4017b..e3fec5d570 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -20,7 +20,6 @@ use Drupal\package_manager\PathLocator;
use Drupal\package_manager\StatusCheckTrait;
use Drupal\package_manager\Validator\DiskSpaceValidator;
use Drupal\package_manager\StageBase;
-use Drupal\system\SystemManager;
use Drupal\Tests\package_manager\Traits\AssertPreconditionsTrait;
use Drupal\Tests\package_manager\Traits\FixtureManipulatorTrait;
use Drupal\Tests\package_manager\Traits\FixtureUtilityTrait;
@@ -456,12 +455,7 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
*/
protected function createStageEventExceptionFromResults(array $expected_results, string $event_class = PreCreateEvent::class, StageBase $stage = NULL): StageEventException {
$event = new $event_class($stage ?? $this->createStage(), []);
-
- foreach ($expected_results as $result) {
- if ($result->severity === SystemManager::REQUIREMENT_ERROR) {
- $event->addError($result->messages, $result->summary);
- }
- }
+ array_walk($expected_results, $event->addResult(...));
return new StageEventException($event);
}
diff --git a/package_manager/tests/src/Kernel/StageEventsTest.php b/package_manager/tests/src/Kernel/StageEventsTest.php
index 2429dbb59e..1ba4a48f35 100644
--- a/package_manager/tests/src/Kernel/StageEventsTest.php
+++ b/package_manager/tests/src/Kernel/StageEventsTest.php
@@ -15,6 +15,7 @@ use Drupal\package_manager\Event\PreDestroyEvent;
use Drupal\package_manager\Event\PreOperationStageEvent;
use Drupal\package_manager\Event\PreRequireEvent;
use Drupal\package_manager\Event\StageEvent;
+use Drupal\package_manager\Event\StatusCheckEvent;
use Drupal\package_manager\Exception\StageEventException;
use Drupal\package_manager\ValidationResult;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -155,6 +156,35 @@ class StageEventsTest extends PackageManagerKernelTestBase implements EventSubsc
$this->assertResults([$result], $event_class);
}
+ /**
+ * Tests adding validation results to events.
+ */
+ public function testAddResult(): void {
+ $stage = $this->createStage();
+
+ $error = ValidationResult::createError([
+ t('Burn, baby, burn!'),
+ ]);
+ $warning = ValidationResult::createWarning([
+ t('The path ahead is scary...'),
+ ]);
+
+ // Status check events can accept both errors and warnings.
+ $event = new StatusCheckEvent($stage, NULL);
+ $event->addResult($error);
+ $event->addResult($warning);
+ $this->assertSame([$error, $warning], $event->getResults());
+
+ // Other stage events will accept errors, but throw an exception if you try
+ // to add a warning.
+ $event = new PreCreateEvent($stage, []);
+ $event->addResult($error);
+ $this->assertSame([$error], $event->getResults());
+ $this->expectException(\InvalidArgumentException::class);
+ $this->expectExceptionMessage('Only errors are allowed.');
+ $event->addResult($warning);
+ }
+
/**
* Tests that pre- and post-require events have access to the package lists.
*/
diff --git a/src/Validation/AdminStatusCheckMessages.php b/src/Validation/AdminStatusCheckMessages.php
index 4e0d9df309..47afe48ccb 100644
--- a/src/Validation/AdminStatusCheckMessages.php
+++ b/src/Validation/AdminStatusCheckMessages.php
@@ -201,7 +201,7 @@ final class AdminStatusCheckMessages implements ContainerInjectionInterface {
// multiple messages. This is because, on regular admin pages, we merely
// want to alert users that problems exist, but not burden them with the
// details. They can get those on the status report and updater form.
- $format_result = function (ValidationResult $result): TranslatableMarkup {
+ $format_result = function (ValidationResult $result): TranslatableMarkup|string {
$messages = $result->messages;
return $result->summary ?: reset($messages);
};
diff --git a/tests/src/Functional/UpdateErrorTest.php b/tests/src/Functional/UpdateErrorTest.php
index 803ebea0e8..2111614ce2 100644
--- a/tests/src/Functional/UpdateErrorTest.php
+++ b/tests/src/Functional/UpdateErrorTest.php
@@ -76,6 +76,21 @@ class UpdateErrorTest extends UpdaterFormTestBase {
$assert_session->buttonExists('Cancel update');
}
+ /**
+ * Tests that throwables will be displayed properly.
+ */
+ public function testDisplayErrorCreatedFromThrowable(): void {
+ $throwable = new \Exception("I want to be the pirate king because he's the freest man alive.");
+ $result = ValidationResult::createErrorFromThrowable($throwable);
+ TestSubscriber1::setTestResult([$result], StatusCheckEvent::class);
+ $this->drupalGet('/admin/reports/status');
+ $this->clickLink('Rerun readiness checks');
+ $this->drupalGet('/admin');
+ $assert_session = $this->assertSession();
+ $assert_session->statusCodeEquals(200);
+ $assert_session->statusMessageContains($throwable->getMessage(), 'error');
+ }
+
/**
* Tests the display of errors and warnings during status check.
*/
--
GitLab