Issue with PHPStan configuration and 1.67

laryjulien commented 2 years ago

Hello,

thanks for your work by maintaining this utility.

Since the latest docker image build from yesterday (using jakzal/phpqa:php7.4 tag), PHPStan seems to ignore the level parameter on both command options and in NEON configuration file. By using the previous tag (jakzal/phpqa:1.66-php7.4), I do not encounter the issue.

Is this related to your image or PHPStan?

Thanks.

jakzal commented 2 years ago

The last release only introduced phpstan/extension-installer (see https://github.com/jakzal/toolbox/pull/422), but phpstan could've been updated too as we always install the latest available at the time.

@dgoosens is it possible phpstan/extension-installer caused the problem?

jakzal commented 2 years ago

Same phpstan version is present on both images:

$ docker run -it --rm jakzal/phpqa:php7.4 phpstan --version
PHPStan - PHP Static Analysis Tool 1.4.6

$ docker run -it --rm jakzal/phpqa:1.66-php7.4 phpstan --version
PHPStan - PHP Static Analysis Tool 1.4.6

dgoosens commented 2 years ago

hi @jakzal

you were faster...
I just ran the same check and came to the same conclusion

1.4.6 is the latest version of phpstan

I'm currently checking the level param and trying to reproduce @laryjulien's issue

dgoosens commented 2 years ago

@jakzal @laryjulien

so just ran it on some dummy project:

docker run -it --rm -v "$(pwd):/project" jakzal/phpqa:php7.4 phpstan analyse /project/src -l1
# reports 346 issues
docker run -it --rm -v "$(pwd):/project" jakzal/phpqa:php7.4 phpstan analyse /project/src -l9
# reports 541 issues

So that seems to work fine
(and show I've got some mess to cleanup there)

Same thing happens when I play around with the level in a phpstan.neon file

So no...
not related to https://github.com/jakzal/toolbox/pull/422 and AFAIK nothing wrong with phpqa or phpstan at all

nadl84 commented 2 years ago

Hi,

I ran into a similar issue.

I'm working with a 4 week old image version (tag: php8.0-alpine, image id: 3fdfaaff9642, phpstan version: 1.4.2) which worked just fine. When trying to use the php8.1-alpine on the same project, I had a huge number of phpstan issues...

jakzal commented 2 years ago

@nadl84 it makes more sense in your case since you went from phpstan 1.4.2 to 1.4.6. I imagine they introduce new rules between these two versions.

jakzal commented 2 years ago

Please note that php7.4 is built nightly, so it's already different to 1.67.0-php7.4.

dgoosens commented 2 years ago

mmmmh...

jakzal/phpqa:php7.4 does report more errors than jakzal/phpqa:1.66-php7.4
but I'm running it on a Symfony project with doctrine so the phpstan/extension-installer might explain that as it was not present in jakzal/phpqa:1.66-php7.4

jakzal commented 2 years ago

I've run phpstan against jakzal/toolbox and it seems to be that indeed phpstan/extension-installer affects the results.

1.66-php7.4

$ docker run -it --rm -v(pwd):/project -w /project jakzal/phpqa:1.66-php7.4 sh
$ phpstan -l1 analyze src

 [OK] No errors

1.67-php7.4 with phpstan/extension-installer

$ docker run -it --rm -v(pwd):/project -w /project jakzal/phpqa:1.67-php7.4 sh
$ phpstan -l1 analyze src

 [ERROR] Found 96 errors

1.67-php7.4 without phpstan/extension-installer

$ docker run -it --rm -v(pwd):/project -w /project jakzal/phpqa:1.67-php7.4 sh
$ composer global bin phpstan remove phpstan/extension-installer
$ phpstan -l1 analyze src

 [OK] No errors

dgoosens commented 2 years ago

well, that is quite normal actually phpqa comes with all the phpstan extensions

phpstan-deprecation-rules
phpstan-ergebnis-rules
phpstan-strict-rules
phpstan-doctrine
phpstan-phpunit
phpstan-symfony
phpstan-beberlei-assert
etc etc

By adding the phpstan/extension-installer a part of these extensions, that did not work previously, now all of a sudden do work and start reporting more issues with the code.

Ideally, only the phpstan/extension-installer should be installed by default....
and the extensions should be activated on demand.

Would that be possible somehow ?

jakzal commented 2 years ago

It seems that phpstan/extension-installer automatically enables all phpstan extensions.

laryjulien commented 2 years ago

Thank you for your analyze and your answers.

From my understanding:

if you add phpstan/extension-installer, it will install all others extensions added to composer.json when passing in composer post update or post install (see: https://github.com/phpstan/extension-installer/blob/master/src/Plugin.php)
The other way to use extensions is without the installer and by adding directly extensions in config files (see: https://phpstan.org/user-guide/extension-library#installing-extensions).

I may be wrong but I think that:

Either you add the installer without extensions and users of your images must add them using Composer. This can be done in building by extending your Dockerfile, in Docker entrypoint file or at runtime (like a script item in CI step, for example).
Or you add all extensions in composer.json except for phpstan/extension-installer and users of your images must add them manually in phpstan.neon to activate them.

For now, I have fixed the issue by blocking temporary the image to 1.66.

jakzal commented 2 years ago

Is the only way forward to remove the extension installer?

dgoosens commented 2 years ago

Is the only way forward to remove the extension installer?

I think it is...
but it would probably also be better to remove all the extensions

Part of them don't work without the extension installer... so basically they're broken

Once the installer is installed, which can be done manually very easily, all extension are enabled which is probably not what people want

I can add an article to the docs/cookbook explaining how to install the extensions (and only those one want) if that's of any help.

dgoosens

jakzal commented 2 years ago

Part of them don't work without the extension installer... so basically they're broken

Do you know why? Seems odd.

dgoosens commented 2 years ago

I have not tested all the extensions but at least for the symfony and doctrine extensions the extension-installer is required

Without it, the extensions won't work

jakzal commented 2 years ago

@dgoosens can you clarify what does it mean "the extensions won't work" ?

I'm not familiar with the way phpstan extensions work.

What's wrong with enabling extansions the way it's described in https://github.com/jakzal/phpqa/blob/f368bf6b9ab269e723d90277db1e23142658194e/docs/cookbook/customising-the-image.md#adding-phpstan-extensions ?

jakzal commented 2 years ago

Until we agree what needs to be done I reverted the addition of phpstan/extension-installer: https://github.com/jakzal/phpqa/releases/tag/v1.68.0

Wahib-L commented 1 year ago

Hi, I think we need to clarify a bit the behavior of PHPStan extensions.

Without phpstan/extension-installer, no extension is automatically activated. If you want to activate one, you have to include it manually.
With phpstan/extension-installer, all extensions installed (and supporting this feature) are automatically actived.

From the official PHPStan documentation (https://phpstan.org/user-guide/extension-library#installing-extensions):

Why wouldn’t I want to always use phpstan/extension-installer?

It always enables all the functionality that an extension offers. If you for example want to use only some of the rules from phpstan-strict-rules, or if you only want to use extension.neon (but not rules.neon) from phpstan-doctrine, you can’t use the extension installer plugin and must include chosen files manually.

Part of them don't work without the extension installer... so basically they're broken [...] I have not tested all the extensions but at least for the symfony and doctrine extensions the extension-installer is required

All extensions can be installed manually. the extension-installer will just load the configurations declared in the composer.json of the extensions (cf: https://github.com/phpstan/extension-installer#instructions-for-extension-developers) And the two mentioned extensions have a "manual installation" part in their README ;) :

[...] but it would probably also be better to remove all the extensions

Personally, I think it's better to have the extensions pre-installed (without the extension-installer). This will avoid having to modify the image before using it.

What's wrong with enabling extansions the way it's described in https://github.com/jakzal/phpqa/blob/f368bf6b9ab269e723d90277db1e23142658194e/docs/cookbook/customising-the-image.md#adding-phpstan-extensions ?

It works well for me :)

jakzal commented 1 year ago

Thanks @Wahib-L. I agree having extensions installed but disabled, so not using the extension-installer, is the safe default.

jakzal / phpqa

Issue with PHPStan configuration and 1.67 #349