New checking in mglaman/phpstan-drupal interfering with our usage of \Drupal\Component\Assertion\Inspector

Checking a few things:

phpdoc:

   * @param array $images
   *   Images of the project. Each item needs to be an array with two elements:
   *   `file`, which is a \Drupal\Core\Url object pointing to the image, and
   *   `alt`, which is the alt text.

So PHPStan has no idea what the iterator contains

    assert(
      Inspector::assertAllArrays($images) &&
      Inspector::assertAll(fn (array $i): bool => $i['file'] instanceof Url, $images) &&
      Inspector::assertAllHaveKey($images, 'alt')
    ) or throw new \InvalidArgumentException('The project images must be arrays with `file` and `alt` elements.');

The latest changes to Inspector help do type narrowing so that PHPStan understands the array as if the phpdoc had list<array{file: Url, alt: string}>

1. Inspector::assertAllArrays($images) tells PHPStan $images is array<int|string, array>
2. Inspector::assertAll(fn (array $i): bool => $i['file'] instanceof Url, $images) should tell PHPStan it is array<int|string, array{file: Url> but this seems to break down
3. Inspector::assertAllHaveKey($images, 'alt') should flush out the array shape, but now PHPStan doesn't think $images is an array at all because the previous assert failed

I'll see if I can get a test case in phpstan-drupal and quick fix

Opened https://github.com/mglaman/phpstan-drupal/issues/852 to take a look

phpstan-drupal should be fixed: https://github.com/mglaman/phpstan-drupal/releases/tag/2.0.4

no idea if CI pulls latest or if it needs core to

Merging for now, the 2.0.4 PHPStan-Drupal release solved some but not all of the problems, and I want to unblock other MRs.

Merged !791 🎉, leaving open

Two new MRs (!793 and !794) to finish this issue.

Both of them fix a logic "bug" in SortHelper, there's two redundant checks.
Both of them bump our composer.json to the latest version of phpstan-drupal

793 takes the opinion that PHPStan knows best and that we don't need to enforce the types via assert, and removes the extraneous ones.
This has the side benefit of making the calling code actually ensure it is passing what it thinks it's passing, see the change to DrupalCore.php that actually enforces what's in the configuration

794 takes the opinion that while PHPStan might know best, that doubling up with asserts is okay too.
This codifies the doubled asserts in a new baseline file, that is only because PHPStan 1 (or maybe phpstan-drupal 1) aren't flagging this yet.

I slightly prefer 793 because it has less maintenance burden, but I'd be fine with 794.

794 is adding new ignores to phpstan-baseline-phpstan2-only.neon. Is this a correct name for such ignores? Because, as I understand it, baseline is used for ignoring errors that should be fixed in the future, but until then, they shouldn't be reported. These ignores for Inspector are not that type of ignores; they are intentionally disabled for a specific reason and not meant to be fixed in the future.

So, from my point of view, these ignores should be in phpstan.neon or some kind of "extras". Like the current ignoreErrors:
https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/phpstan....
they are not meant to be fixed, so they are not in the baseline.

I might be wrong about this assumption about baseline, but I'm interested why they go into baseline.

We can rename the file anyway we want, but it has to be separate because of the conditional loading while we are also supporting PHPStan 1 (as these errors are only reported in PHPStan 2)
Agreed we can add some docs if we pursue 794

Fixed via 793 - trust the tools!

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

tim.plunkett → closed merge request !794