Issue #3364725 by omkar.podey, phenaproxima, Wim Leers: The $format_result...

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

@@ -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

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;
   }
 
   /**

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;
   }
 
 }

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);
     }
   }
 

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);
   }
 

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.
    */

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);
     };

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.
    */