search

phpstan / extension-installer

Composer plugin for automatic installation of PHPStan extensions.
MIT License
391 stars 27 forks source link

Install phpstan extension also for root package #39

Closed mvorisek closed 1 year ago

mvorisek commented 2 years ago

fixes https://github.com/phpstan/extension-installer/issues/36

mvorisek commented 2 years ago

PR is done, Build / PHPStan (8.0, highest) CI error seems unrelated

mvorisek commented 2 years ago

@ondrejmirtes can you please review this PR?

ondrejmirtes commented 2 years ago

1) This PR does too many unrelated changes 2) I'm not even sure that https://github.com/phpstan/extension-installer/issues/36 is a real problem that needs solving. The whole point of this package is that it saves a few lines in your phpstan.neon.

mvorisek commented 2 years ago
  1. This PR does too many unrelated changes

I have separated the verbose logging to https://github.com/phpstan/extension-installer/pull/42, all other changes are related

  1. I'm not even sure that composer.json's extra section must be honored also for root-package #36 is a real problem that needs solving. The whole point of this package is that it saves a few lines in your phpstan.neon.

Exactly that is the point. When phpstan config is added to composer.json, it must always apply.

Currently, for the root project, all configs must be included on two locations - like in https://github.com/atk4/data/blob/b0b50a0f0ae5ce280a180beb0dfd63f08100e02f/phpstan.neon.dist#L1-L3 . And if such config is then run from another project, it will even fail as included twice, steps to reproduce:

  1. create some repo with this ext, require package x/y with phpstan config phpstan-ext.neon specified in composer.json and also include that config in phpstan.neon
  2. run composer install
  3. run phpstan from the "some repo" on vendor/x/y
  4. currently, phpstan will fail as phpstan-ext.neon is loaded twice
  5. with this PR, phpstan-ext.neon is enough to be specified in a single place - composer.json, phpstan always finish no matter if run from x/y project directly or from root project
mvorisek commented 2 years ago

@ondrejmirtes can you please review?

ondrejmirtes commented 2 years ago

I'm still not sure about the implications and the usefulness. I'd rather not do it.

mvorisek commented 2 years ago

yes, repos including some extra conf manually need to be updated (otherwise phpstan will throw for config included twice), but this PR unifies how the composer config behaves - like for ex. autoload config - it works for root project and also when included

other than this change needed, do you have any other implication in head?

to maintain compatibility with existing projects, what about releasing this as 2.x version?

ondrejmirtes commented 1 year ago

I feel like there isn't any demand for this, and I feel against it too. So closing. Thanks for understanding.