Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Amongst of the "new" issues found when running PHPStan on level 2 are: Parameter $foo of method Foo::bar() has invalid type Foo\Baz.
and Method Foo::bar() has invalid return type Foo\Baz.
.
This child-issue exists to fix all of those.
Steps to reproduce
- Run PHPStan on level 2 and see the above 2 issues amongst all others.
Proposed resolution
- Solve all of the reported issues for both of the above mentioned.
- Run PHPStan on level 2 and don't see the above 2 issues any more.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|
Issue fork drupal-3322743
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3322743-fix-phpstan-l2 changes, plain diff MR !3177
Comments
Comment #2
SpokjeComment #3
SpokjeComment #4
SpokjeThe attached level2.txt is the output of
vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist
after setting PHPStan to level 2.We're down from the original
[ERROR] Found 9440 errors
to[ERROR] Found 9237 errors
.There are a few questions raised however during this "exercise":
1) I didn't touch issues in the namespaces
Drupal\Component\Annotation\Doctrine
andDrupal\Tests\Component\Annotation\Doctrine\Fixtures
.Classes in these namespaces are all (almost) 1-on-1 copies of the Doctrine project.
Personally I think we should exclude this files from being scrutinized by PHPStan.
2) Not sure how to handle this one:
Comment #5
SpokjeHmmm, we have seem to fix some level 1 issues along the way.
Comment #6
Spokje...and PHPStan doesn't show all the changes at once.
I regenerated the baseline:
vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist --generate-baseline ./core/phpstan-baseline.neon
Comment #7
SpokjeComment #8
mondrakeIt would be good to have a coding standard change to allow using non-FQCN in comments, rather
use
imports on file headers and then just the class names. PHPStan fully supports that, and IMHO readibility would greatly improve.need to adjust the docs above and below, too.
shouldn't it be
\Drupal\Core\Config\Entity\ConfigEntityInterface
?mmm sounds strange to refer a runtime trait to a test class...
this is losing information, could it be avoided?
kinda redundant, could it be just one of the two? actually with #3309010: Support PHPDoc Types in @param @var @return annotations all this could be vastly improved
comment should be wrapped on 80 chars
Comment #9
mondrakeNot a big brain, still I'd give some feedback :)
#4.1 IMHO we should do nothing. #3252386: Use PHP attributes instead of doctrine annotations would probably make these classes redundant at some point in time. Until then, I see no issue to keep these errors in a baseline (so that any change on that code in the meatime at least would not add errors).
#4.2 'callable' is a basic type, it should be all lowercase and no backslash in front.
Comment #10
SpokjeComment #11
SpokjeAddressed #8 and #9.
Comment #12
SpokjeComment #13
SpokjeComment #14
SpokjeAdded the output of
$ vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist --xdebug --no-ansi
on lvl2 before and after the patch has been applied.Comment #15
mondrakeThis seems rather strange... does it fail if we totally remove the hint? One may wonder why we need to have a trait if $this is an implementation of an interface at this point?
Adding the PHPStan-1 tag as it seems this fixes some L1 baseline errors too.
Comment #16
SpokjeThis seems rather strange... does it fail if we totally remove the hint?
It fails PHPUnit:
Class 'CategorizingPluginManagerTrait' could not be found in 'D:\htdocs\drupal\core\lib\Drupal\Core\Plugin\CategorizingPluginManagerTrait.php'.
Comment #17
mondrakeI suppose you meant PHPStan, I can't see how this could fail PHPUnit. What happens if we remove the typehint completely? What's strange to me is typehinting $this (which is the current state, not the change itself)
Comment #18
SpokjeSorry about that, was actually running the trait in phpunit, which will of course fail... *facepalm*
Let's find out in the attached patch...
Comment #19
SpokjeWorks for me :)
Comment #20
SpokjeRerolled 3322743-18.patch in a new MR, let's get this reviewed for 10.1.x first, instead of rerolling 2 patches endlessly.
Comment #22
Manoj Raj.R CreditAttribution: Manoj Raj.R as a volunteer and at ITT Digital commentedLooks good catch to me of all those rectifying the issues,spelling,typo etc..
Comment #23
mondrakeI see one last thing, commented in the MR.
Comment #24
SpokjeComment #25
mondrakeSee inline comment.
Comment #26
SpokjeDeath by tooling, see https://drupal.slack.com/archives/C51GNJG91/p1673375643741729
Comment #27
mondrakeSame here. https://www.drupal.org/project/drupal/issues/3191623#comment-14861657
I switched back to patch workflow.
Comment #28
SpokjeI'm going to walk away from (at least this one) until this is fixed. Not in the mood to go back to 2010 tech :)
Comment #29
SpokjeAaaaand...We're back in business.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedRemoving credit from myself as I just rebased. Will mark after that runs.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedNo issues LGTM
Comment #33
longwaveOne question about the session handler parameter but otherwise this looks ready to go.
Comment #34
SpokjeComment #35
mondrakeImprovements of docs. LGTM
Comment #37
longwaveCommitted and pushed to 10.1.x, thanks!