- Issue created by @tim.plunkett
- Merge request !791Issue #3518231: New checking in mglaman/phpstan-drupal interfering with our usage of \Drupal\Component\Assertion\Inspector β (Merged) created by tim.plunkett
- πΊπΈUnited States mglaman WI, USA
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}>
Inspector::assertAllArrays($images)
tells PHPStan$images
isarray<int|string, array>
Inspector::assertAll(fn (array $i): bool => $i['file'] instanceof Url, $images)
should tell PHPStan it isarray<int|string, array{file: Url>
but this seems to break downInspector::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
- πΊπΈUnited States mglaman WI, USA
Opened https://github.com/mglaman/phpstan-drupal/issues/852 to take a look
- πΊπΈUnited States mglaman WI, USA
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
-
tim.plunkett β
committed 67099548 on 2.0.x
Issue #3518231 by tim.plunkett, mglaman: New checking in mglaman/phpstan...
-
tim.plunkett β
committed 67099548 on 2.0.x
- πΊπΈUnited States tim.plunkett Philadelphia
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
- πΊπΈUnited States tim.plunkett Philadelphia
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-drupal793 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 configuration794 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.
- π·πΊRussia niklan Russia, Perm
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 forInspector
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 currentignoreErrors
:
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.
- πΊπΈUnited States tim.plunkett Philadelphia
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