Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install phpstan extension also for root package #39

Closed

Conversation

mvorisek
Copy link

@mvorisek mvorisek commented Nov 26, 2021

fixes #36

@mvorisek mvorisek changed the title Install phpstan extension also for self/root project Install phpstan extension also for self/root package Nov 26, 2021
@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch 3 times, most recently from abb5b42 to 5e39cd7 Compare November 26, 2021 13:23
@mvorisek mvorisek marked this pull request as ready for review November 26, 2021 13:23
@@ -8,7 +8,7 @@
"require": {
"php": "^7.1 || ^8.0",
"composer-plugin-api": "^1.1 || ^2.0",
"phpstan/phpstan": ">=0.11.6"
"phpstan/phpstan": ">=0.12.19"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to support infile @phpstan-ignore-line

@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch 2 times, most recently from 3c102b0 to 997c743 Compare November 26, 2021 13:34
@mvorisek mvorisek marked this pull request as draft November 26, 2021 13:35
@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch from 997c743 to 7ffbb85 Compare November 26, 2021 13:57
@mvorisek mvorisek changed the title Install phpstan extension also for self/root package Install phpstan extension also for root package Nov 26, 2021
@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch 5 times, most recently from cd8dd63 to de379c0 Compare November 26, 2021 14:09
@mvorisek mvorisek marked this pull request as ready for review November 26, 2021 14:11
@mvorisek
Copy link
Author

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

@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch 2 times, most recently from b96b797 to fc412bd Compare November 26, 2021 14:13
@mvorisek
Copy link
Author

@ondrejmirtes can you please review this PR?

@ondrejmirtes
Copy link
Member

  1. This PR does too many unrelated changes
  2. 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.

@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch from fc412bd to ee1d0a6 Compare December 15, 2021 15:13
@mvorisek
Copy link
Author

mvorisek commented Dec 15, 2021

  1. This PR does too many unrelated changes

I have separated the verbose logging to #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 mvorisek force-pushed the fix_config_build_for_root_project branch from 4d4801e to 67acd2a Compare June 3, 2022 12:28
@mvorisek mvorisek force-pushed the fix_config_build_for_root_project branch from 67acd2a to e80be09 Compare June 3, 2022 12:28
@mvorisek
Copy link
Author

mvorisek commented Jun 3, 2022

@ondrejmirtes can you please review?

@ondrejmirtes
Copy link
Member

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

@mvorisek
Copy link
Author

mvorisek commented Jun 3, 2022

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

composer.json's extra section must be honored also for root-package
2 participants