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

I don't think this can happen even for throwables since we have a test for it too \Drupal\Tests\package_manager\Unit\ValidationResultTest::testMessagesMustBeTranslatable if i'm not wrong.

So the next thing to do is to restructure \Drupal\package_manager\ValidationResult so that it allows createErrorFromThrowable to use strings instead of Translatable Markup.

I ran into this 4 days ago, I reported in Slack, but am repeating it in full here.

WSOD:

The website encountered an unexpected error. Please try again later.

TypeError: Drupal\automatic_updates\Validation\AdminStatusCheckMessages::Drupal\automatic_updates\Validation\{closure}(): Return value must be of type Drupal\Core\StringTranslation\TranslatableMarkup, string returned in Drupal\automatic_updates\Validation\AdminStatusCheckMessages->Drupal\automatic_updates\Validation\{closure}() (line 206 of modules/contrib/automatic_updates/src/Validation/AdminStatusCheckMessages.php).
array_map(Object, Array) (Line: 212)
Drupal\automatic_updates\Validation\AdminStatusCheckMessages->displayResults(Array, Object, Object) (Line: 149)
Drupal\automatic_updates\Validation\AdminStatusCheckMessages->displayResultsForSeverity(2) (Line: 108)
Drupal\automatic_updates\Validation\AdminStatusCheckMessages->displayAdminPageMessages() (Line: 128)
automatic_updates_page_top(Array) (Line: 352)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}(Object, 'automatic_updates') (Line: 388)
Drupal\Core\Extension\ModuleHandler->invokeAllWith('page_top', Object) (Line: 353)
Drupal\Core\Render\MainContent\HtmlRenderer->buildPageTopAndBottom(Array) (Line: 146)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 168)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Obj

… by just having AU installed. I’m on the latest commit.

Reason this happens:

array(2) {
  [0]=>
  object(Drupal\package_manager\ValidationResult)#7193 (3) {
    ["severity"]=>
    int(2)
    ["messages":"Drupal\package_manager\ValidationResult":private]=>
    array(1) {
      [0]=>
      object(Drupal\Core\StringTranslation\TranslatableMarkup)#7271 (5) {
        ["string":protected]=>
        string(53) "Drupal multisite is not supported by Package Manager."
        ["arguments":protected]=>
        array(0) {
        }
        ["translatedMarkup":protected]=>
        NULL
        ["options":protected]=>
        array(0) {
        }
        ["stringTranslation":protected]=>
        NULL
      }
    }
    ["summary"]=>
    NULL
  }
  [1]=>
  object(Drupal\package_manager\ValidationResult)#7097 (3) {
    ["severity"]=>
    int(2)
    ["messages":"Drupal\package_manager\ValidationResult":private]=>
    array(1) {
      [0]=>
      string(147) "The active directory at "/Users/wim.leers/core" contains absolute links, which is not supported. The first one is "/Users/wim.leers/core/test.php"."
    }
    ["summary"]=>
    NULL
  }
}

👆 the second validation result’s message is not translatable markup.

The error is accurate though:

$ ls -al ../../test.php
lrwxr-xr-x  1 wim.leers  staff  31 Apr 21 15:40 ../../test.php -> /Users/wim.leers/Sites/test.php

Turns out this MR is only aiming to fix $format_result = function (ValidationResult $result): TranslatableMarkup { … } in AdminStatusCheckMessages.

Apparently 🐛 Many callers of ValidationResult::createError() violate its interface due to lack of strict typing Fixed introduced:

   */
  private function __construct(
    public readonly int $severity,
    private array $messages,
    public readonly ?TranslatableMarkup $summary,
    bool $assert_translatable
  ) {
    if ($assert_translatable) {
      assert(Inspector::assertAll(fn ($message) => $message instanceof TranslatableMarkup, $messages));
    }

and

  public static function createErrorFromThrowable(\Throwable $throwable, ?TranslatableMarkup $summary = NULL): static {
    return new static(SystemManager::REQUIREMENT_ERROR, [$throwable->getMessage()], $summary, FALSE);
  }

Note that FALSE at the end of the second line.

That means the instanceof TranslatableMarkup check it is not executed for throwables, but is for all other public constructors.

Weird, but fine, and forced by external factors.

However, AFAICT string|\Stringable is the wrong type because the constructor says:

   * @param \Drupal\Core\StringTranslation\TranslatableMarkup[]|string[] $messages
   *   The result messages.

Letting @phenaproxima judge that.

The above aside, AFAICT 90% of this MR is unnecessary, if we'd just use TestSubscriber::setException() instead of TestSubscriber::setResults(), assigning back to @omkar.podey for that.

I do not agree, so i did try TestSubscriber::setException() at first but it didn't work as \Drupal\package_manager\Event\PreOperationStageEvent::addErrorFromThrowable and that's why i changed the TestSubscriber

Using TestSubscriber::setException() i got

#12: but

        $event->addErrorFromThrowable(new \Exception($result->messages[0]), $result->summary);

is really atypical too: it's pretending there isn't an exception, and instead there is a hardcoded assumption it lives on the first result. What about subsequent results?

Also, the array_pop() modifies the provided $results, which undermines the reliability of the tests.

So, I dug in. And I then think I found the more nuanced reason for why you went this way! 😊 Effectively, you do need setException(), but you need it to not get thrown, but instead be converted into a validation result. And in that sense, the modifications you made to setTestResult() make sense!

However, it unavoidably leads to the code smells I've pointed out above. AFAICT the root cause for this is ValidationResult being a public API but it being explicitly hidden by the API surface of PreOperationStageEvent and StatusCheckEvent. Which makes sense, because those APIs date back to May 2022, whereas ValidationResult was only officially marked as an API in 📌 Define the Package Manager API (package_manager.api.php is outdated) Fixed ~3 months ago.

So I think the solution is pretty simple: slightly expand what PreOperationStageEvent and StatusCheckEvent allow you to do, relabel the existing methods as convenience methods (plenty examples of that in Drupal core!), and we're off to a much simpler implementation!

Just pushed a commit that does that.

I don't mind this approach; I agree it's simpler. But it's not quite ready; there's some missing test coverage, first and foremost, as well as some stuff that needs to be cleaned up.

Looking really good -- I think we can simplify the test (I had originally asked for a unit test because trying to test it through the stage life cycle is needlessly complicated for this case), and then there's a couple of other tiny things.

Assigning to Wim for final review.

Looks good.

Looking for places we can use the new `addResult`

1. \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::createStageEventExceptionFromResults
   

   $event->addError($result->messages, $result->summary);
   

   can be simplified

Actually I guess that is all I could find

Looks good!

Looks like I wasn't really needed here 👍😄

Automatically closed - issue fixed for 2 weeks with no activity.