Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type inference for Plugin Managers #165

Merged
merged 14 commits into from Jul 10, 2022
Merged

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jul 8, 2022

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.

  • 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
  • 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 🤷‍♂️
  • Navigation is looking for non-existent classes - should probably sort that out separately

gsteel added 11 commits June 13, 2022 13:50
Signed-off-by: George Steel <george@net-glue.co.uk>
Link: laminas/laminas-servicemanager#137
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
…lable

Signed-off-by: George Steel <george@net-glue.co.uk>
… types can be returned by implemetors

Signed-off-by: George Steel <george@net-glue.co.uk>
… elsewhere

Signed-off-by: George Steel <george@net-glue.co.uk>
…d of the renderer

Signed-off-by: George Steel <george@net-glue.co.uk>
Enables the removal of a number of assertions and var annotations

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel
Copy link
Member Author

gsteel commented Jul 8, 2022

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…

…/console until 3.0

Signed-off-by: George Steel <george@net-glue.co.uk>
@Ocramius
Copy link
Member

Ocramius commented Jul 8, 2022

  • 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

@Ocramius Ocramius added this to the 2.21.0 milestone Jul 8, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's expand the baseline with the new issues: easier to discuss them on the psalm-baseline.xml diff, than within comments in the PR

.laminas-ci.json Show resolved Hide resolved
.psr-container.php.stub Outdated Show resolved Hide resolved
composer.lock Show resolved Hide resolved
src/Helper/Navigation/PluginManager.php Show resolved Hide resolved
…xisting implementors such as those found in service manager - this silences unusual errors from psalm.

Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel
Copy link
Member Author

gsteel commented Jul 8, 2022

@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
Copy link
Member

Ocramius commented Jul 8, 2022

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
Copy link
Member

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
Copy link

@Ocramius Yes, something like this: phpstan/phpdoc-parser#127

It's fixed in Slevomat CS ^8.0

@Ocramius
Copy link
Member

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

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

@kukulich
Copy link

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

@Ocramius
Copy link
Member

That's a viable solution for upstream, I suppose 🤔

@gsteel
Copy link
Member Author

gsteel commented Jul 10, 2022

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
Copy link
Member

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

@gsteel
Copy link
Member Author

gsteel commented Jul 10, 2022

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
Copy link
Member

  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

…oding standard 7.2 and phpdoc-parser 1.6.x

Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel
Copy link
Member Author

gsteel commented Jul 10, 2022

Thanks @Ocramius - I finally got there 🙄

@Ocramius Ocramius self-assigned this Jul 10, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 this brings us forward!

@Ocramius Ocramius merged commit b25e4a4 into laminas:2.21.x Jul 10, 2022
@gsteel gsteel deleted the qa/psr-stub branch July 10, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants