search

laminas / laminas-view

Flexible view layer supporting and providing multiple view layers, helpers, and more
https://docs.laminas.dev/laminas-view/
BSD 3-Clause "New" or "Revised" License
74 stars 46 forks source link

Improve type inference for Plugin Managers #165

Closed gsteel closed 2 years ago

gsteel commented 2 years ago
Q A
Documentation yes
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

General type inference improvements

There's a couple of outstanding psalm issues that I think should probably be baselined.

gsteel commented 2 years ago

phpcs is broken … conflicting with "phpstan/phpdoc-parser": ">=1.6" solves the issue - not sure if the problem is with the doc parser or slevomat - I'm guessing that this is going to affect anything using coding standard v2.3…

Ocramius commented 2 years ago
  • HeadLink - psalm now complains about stdClass, which can be annotated as object for the most part, but HeadLink has parameter types explicitly set to stdClass

If we use @psalm-param object, it takes priority over any native types

  • Psalm is moaning about the plugin managers with MethodSignatureMismatch for Method Laminas\ServiceManager\ServiceManager::has with return type '' is different to return type 'bool' - This is really weird because bool is the return type all the way up the inheritance chain man_shrugging

Potential regression in psalm? Perhaps we can reduce this into an example on https://psalm.dev/ and report it

  • Navigation is looking for non-existent classes - should probably sort that out separately

Indeed, let's keep it out for now

gsteel commented 2 years ago

@Ocramius - Current phpcs failures only happen on 7.4 - is it possible to run phpcs on 8.0 or 8.1 instead? It would save adding a conflict or pinning versions (slevomat or phpdoc-parser)

Ocramius commented 2 years ago

is it possible to run phpcs on 8.0 or 8.1 instead?

We can drop PHP 7.4 support perhaps: I really don't mind on my end, but I'm fairly sure that more people need to have a say in this.

Ocramius commented 2 years ago 
Running ./vendor/bin/phpcs -q --report=checkstyle | cs2pr
PHP Fatal error:  Uncaught TypeError: Argument 5 passed to SlevomatCodingStandard\Helpers\Annotation\ParameterAnnotation::__construct() must be an instance of PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode or null, instance of PHPStan\PhpDocParser\Ast\PhpDoc\TypelessParamTagValueNode given, called in /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/AnnotationHelper.php on line 357 and defined in /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/Annotation/ParameterAnnotation.php:31
Stack trace:
#0 /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/AnnotationHelper.php(357): SlevomatCodingStandard\Helpers\Annotation\ParameterAnnotation->__construct('@param', 265, 267, '$mode', Object(PHPStan\PhpDocParser\Ast\PhpDoc\TypelessParamTagValueNode))
#1 /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/SniffLocalCache.php(42): SlevomatCodingStandard\Helpers\AnnotationHelper::SlevomatCodingStandard\Helpers\{clos in /github/workspace/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/Annotation/ParameterAnnotation.php on line 31

This looks like a BC break in PHPStan-doc-parser perhaps? /cc @kukulich

kukulich commented 2 years ago

@Ocramius Yes, something like this: https://github.com/phpstan/phpdoc-parser/pull/127

It's fixed in Slevomat CS ^8.0

Ocramius commented 2 years ago

Hmm, upgrading to next slevomat CS is most likely BC breaking all over the place, so that will take a lot of time :thinking:

Oh well, renovate-bot should be able to help on that.

kukulich commented 2 years ago

Locking phpstan/phpdoc-parser to 1.5.1 should probably work as well.

Ocramius commented 2 years ago

That's a viable solution for upstream, I suppose :thinking:

gsteel commented 2 years ago

I added a conflict for 1.6.x which fixed things locally for me - it's just remembering to remove it again in future… I'll add it to the patch

Ocramius commented 2 years ago

I think we can update composer.lock (only) and stick to that, for now.

gsteel commented 2 years ago

I think we can update composer.lock (only) and stick to that, for now.

How do you change deps in the lock without modifying composer.json?

Ocramius commented 2 years ago
  1. add the "the-dependency": "1.2.3" to composer.json
  2. run composer update "the-dependency"
  3. remove "the-dependency" from composer.json
  4. run composer update nothing
gsteel commented 2 years ago

Thanks @Ocramius - I finally got there 🙄