flarum / docs

Flarum documentation.
https://docs.flarum.org
MIT License
66 stars 131 forks source link

Document PHPStan usage in extensions #441

Closed SychO9 closed 1 year ago

SychO9 commented 1 year ago

Goes hand-in-hand with: https://github.com/flarum/framework/pull/3666 This also needs updating the Flarum CLI before merging.

n-peugnet commented 1 year ago

This is very cool, can't wait to use it!

It seems another thing missing, besides the CLI changes, is to publish the flarum/phpstan package to Packagist repository. Would it be possible to do it, even without tagged version? I would love to try this out.

n-peugnet commented 1 year ago

I just remembered that I can simply install the package from the git repo while it is not released yet. So I tested it and it seems there is a problem with the stubFiles that are included from the vendor directory.

First I had to set the include path as so as I said above https://github.com/flarum/docs/pull/441#discussion_r1097712669:

# phpstan.neon
includes:
  - vendor/flarum/phpstan/extension.neon

But then I had the following error:

Note: Using configuration file /home/nicolas/Source/www/flarum/packages/cross-references/phpstan.neon.
Stub file /home/nicolas/Source/www/flarum/packages/cross-references/vendor/vendor/nunomaduro/larastan/stubs/Enumerable.stub does not exist.

And indeed, the paths of the stub files are wrong:

# vendor/flarum/phpstan/larastan-extension.neon
parameters:
    stubFiles:
        - ../../vendor/nunomaduro/larastan/stubs/Enumerable.stub
        - ../../vendor/nunomaduro/larastan/stubs/EloquentBuilder.stub
        - ../../vendor/nunomaduro/larastan/stubs/Collection.stub
        - ../../vendor/nunomaduro/larastan/stubs/EloquentCollection.stub

        [...]

This is because the way its used internally in Flarum source tree is one less level deep than what it will be once installed through composer.

One easy way I see to fix this without having to move the directory of the extension in flarum/framwork is to use the %rootDir% path expansion from PHPStan (for instance %rootDir%/../../nunomaduro/larastan/stubs/Enumerable.stub).

n-peugnet commented 1 year ago

I also have the following error messages:

Ignored error pattern #Relation '[A-z_-]+' is not found in [A-z ]+ model.# was not matched in reported errors.                                            
Ignored error pattern #^Parameter \#1 \$query of method [A-z_<>\\]+\:\:union\(\) expects [A-z_<> .,|\\]+ given\.$# was not matched in reported errors.    
Ignored error pattern #^Parameter \#1 \$query of method [A-z_<>\\]+\:\:joinSub\(\) expects [A-z_<> .,|\\]+ given\.$# was not matched in reported errors.  
Ignored error pattern #Template type T of function resolve[()]{2} is not referenced in a parameter.# was not matched in reported errors.                  
Ignored error pattern #^Call to an undefined method Illuminate\\Database\\ConnectionInterface\:\:[A-z0-9_]+\(\)\.$# was not matched in reported errors.   

This comes from the phpstan-baseline.neon:

# vendor/flarum/phpstan/phpstan-baseline.neon
parameters:
    ignoreErrors:
        # Remove this group below with larastan 2.0 (i.e Flarum 2.0)
        - "#Relation '[A-z_-]+' is not found in [A-z\_]+ model.#"
        - '#^Parameter \#1 \$query of method [A-z_<>\\]+\:\:union\(\) expects [A-z_<> .,|\\]+ given\.$#'
        - '#^Parameter \#1 \$query of method [A-z_<>\\]+\:\:joinSub\(\) expects [A-z_<> .,|\\]+ given\.$#'

        #   We ignore this because resolve can either take a class name as the generic return type or just a binding name.
        - "#Template type T of function resolve[()]{2} is not referenced in a parameter.#"

        #   We ignore new static errors because we want extensibility.
        #   @TODO: needs discussion.
        - "#^Unsafe usage of new static[()]{2}.$#"

        # ConnectionInterface lacks methods that exist in the implementation,
        # yet we don't want to inject the implementation.
        - '#^Call to an undefined method Illuminate\\Database\\ConnectionInterface\:\:[A-z0-9_]+\(\)\.$#'

It might be a good thing to selectively disable reporting unused ignore for each of these ignoreErrors like this:

parameters:
    ignoreErrors:
        # Remove this group below with larastan 2.0 (i.e Flarum 2.0)
        - message: "#Relation '[A-z_-]+' is not found in [A-z\_]+ model.#"
          reportUnmatched: false

        [...]