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

Issue fork drupal-3322743

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Spokje created an issue. See original summary.

Spokje’s picture

Spokje’s picture

Spokje’s picture

Status: Active » Needs review

The 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 and Drupal\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:

 ------ ----------------------------------------------------------------------------------------------------------------------------------------------- 
  [32mLine[39m   [32mcore\lib\Drupal\Component\Plugin\Discovery\StaticDiscoveryDecorator.php[39m                                                                        
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------- 
  22     Property Drupal\Component\Plugin\Discovery\StaticDiscoveryDecorator::$registerDefinitions has unknown class Callable as its type.              
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols                                                                            
  33     Parameter $registerDefinitions of method Drupal\Component\Plugin\Discovery\StaticDiscoveryDecorator::__construct() has invalid type Callable.  
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------- 

Putting this on NR to get more feedback from Bigger Brains on the two questions above.
Spokje’s picture

Hmmm, we have seem to fix some level 1 issues along the way.

Spokje’s picture

FileSize
30.67 KB
2.52 KB

...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

Spokje’s picture

Assigned: Spokje » Unassigned
mondrake’s picture

Status: Needs review » Needs work

It 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.

  1. +++ b/core/lib/Drupal/Core/Archiver/Tar.php
    @@ -81,7 +81,7 @@ public function listContents() {
        * Archive_Tar object for implementation-specific logic. This is for advanced
        * use only as it is not shared by other implementations of ArchiveInterface.
        *
    -   * @return Archive_Tar
    +   * @return ArchiveTar
        *   The Archive_Tar object used by this object.
        */
    

    need to adjust the docs above and below, too.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
    @@ -406,7 +406,7 @@ public function createFromStorageRecord(array $values) {
    -   * @return \Drupal\Core\Config\ConfigEntityInterface
    +   * @return \Drupal\Core\Entity\EntityInterface
    

    shouldn't it be \Drupal\Core\Config\Entity\ConfigEntityInterface?

  3. +++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php
    @@ -15,7 +15,7 @@
    - * @see \Drupal\Core\Plugin\ContextAwarePluginInterface
    + * @see \Drupal\KernelTests\Core\Plugin\Context\TestContextAwarePlugin
    

    mmm sounds strange to refer a runtime trait to a test class...

  4. +++ b/core/modules/jsonapi/src/Normalizer/FieldNormalizer.php
    @@ -83,7 +83,7 @@ public function denormalize($data, $class, $format = NULL, array $context = []):
    -   * @return \Drupal\jsonapi\Normalizer\Value\FieldItemNormalizerValue[]
    +   * @return array
    

    this is losing information, could it be avoided?

  5. +++ b/core/modules/migrate/src/Plugin/Discovery/ProviderFilterDecorator.php
    @@ -55,7 +55,7 @@ public function __construct(DiscoveryInterface $decorated, callable $provider_ex
    -   * @return array|\mixed[]
    +   * @return array|mixed[]
    

    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

  6. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
    @@ -415,7 +415,7 @@ public function testUriRelationships() {
    -   *   The class name to mock. Should be \Drupal\Core\Entity\Entity or a
    +   *   The class name to mock. Should be \Drupal\Tests\Core\Entity\UrlTestEntity or a
    

    comment should be wrapped on 80 chars

mondrake’s picture

Not 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.

Spokje’s picture

FileSize
30.13 KB
9.83 KB
Spokje’s picture

FileSize
31.05 KB
11.68 KB

Addressed #8 and #9.

Spokje’s picture

FileSize
30.81 KB
11.51 KB
Spokje’s picture

Status: Needs work » Needs review
Spokje’s picture

FileSize
2.18 MB
2.09 MB

Added 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.

mondrake’s picture

Issue tags: +PHPStan-1
+++ b/core/lib/Drupal/Core/Plugin/CategorizingPluginManagerTrait.php
@@ -74,7 +74,7 @@ public function getModuleHandler() {
-    /** @var \Drupal\Core\Plugin\CategorizingPluginManagerTrait|\Drupal\Component\Plugin\PluginManagerInterface $this */
+    /** @var \Drupal\Component\Plugin\PluginManagerInterface $this */

@@ -88,7 +88,7 @@ public function getCategories() {
-    /** @var \Drupal\Core\Plugin\CategorizingPluginManagerTrait|\Drupal\Component\Plugin\PluginManagerInterface $this */
+    /** @var \Drupal\Component\Plugin\PluginManagerInterface $this */

This 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.

Spokje’s picture

This 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'.

mondrake’s picture

I 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)

Spokje’s picture

I suppose you meant PHPStan, I can't see how this could fail PHPUnit.

Sorry about that, was actually running the trait in phpunit, which will of course fail... *facepalm*

What happens if we remove the typehint completely?

Let's find out in the attached patch...

Spokje’s picture

FileSize
2.12 MB

Works for me :)

Spokje’s picture

Rerolled 3322743-18.patch in a new MR, let's get this reviewed for 10.1.x first, instead of rerolling 2 patches endlessly.

Manoj Raj.R’s picture

Looks good catch to me of all those rectifying the issues,spelling,typo etc..

mondrake’s picture

Status: Needs review » Needs work

I see one last thing, commented in the MR.

Spokje’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Needs work

See inline comment.

Spokje’s picture

mondrake’s picture

Same here. https://www.drupal.org/project/drupal/issues/3191623#comment-14861657

I switched back to patch workflow.

Spokje’s picture

I'm going to walk away from (at least this one) until this is fixed. Not in the mood to go back to 2010 tech :)

Spokje’s picture

Status: Needs work » Needs review

I'm going to walk away from (at least this one) until this is fixed. Not in the mood to go back to 2010 tech :)

Aaaaand...We're back in business.

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

Removing credit from myself as I just rebased. Will mark after that runs.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

No issues LGTM

longwave’s picture

Status: Reviewed & tested by the community » Needs work

One question about the session handler parameter but otherwise this looks ready to go.

Spokje’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Improvements of docs. LGTM

  • longwave committed de09fc95 on 10.1.x
    Issue #3322743 by Spokje, mondrake: Fix PHPStan L2 errors "Parameter $...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 10.1.x, thanks!

Status: Fixed » Closed (fixed)

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