-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Use stricter PHPStan checking for better coverage. #17448
Conversation
composer.json
Outdated
@@ -130,7 +131,7 @@ | |||
"phpstan-tests": "tools/phpstan analyze -c tests/phpstan.neon", | |||
"phpstan-baseline": "tools/phpstan --generate-baseline", | |||
"psalm-baseline": "tools/psalm --set-baseline=psalm-baseline.xml", | |||
"stan-setup": "phive install", | |||
"stan-setup": "phive install && cp composer.json composer.backup && composer require --dev phpstan/extension-installer symplify/phpstan-rules && mv composer.backup composer.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't these be standard dev deps? Having untracked changes sitting around isn't great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could, if PHPStan would also already be IMO.
But as it is, phive is installing that one.
We could split it sure, not sure if that works, as those might be dependant on it.
"require": {
...
"phpstan/phpstan": "^1.10.30",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if phive would be able to install those, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also https://github.com/phar-io/phar.io/issues/142#issuecomment-1826863632
Sounds more like we should remove that part from phive for now then.
This should work once we set the config here
It now also runs the other validation tasks only once, in the stan run |
"phpstan/phpstan": "^1.10.30", | ||
"phpstan/extension-installer": "^1.3", | ||
"symplify/phpstan-rules": "^12.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much does this ruin your day @ADmad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not thrilled about it, but since @dereuromark has ideas on how to streamline/automate our CI tasks I don't want to hold back the changes.
I'll let you guys be the judge on whether the simplyfy/phpstan-rules
rules bring enough value to warrant this change.
@dereuromark Do you plan to install phpstan through composer for other repos too or just this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the idea was only for this repo.
If you want to do that for others too in order to have it in general more available without 2nd install command, that is sure something we could do and would make it easier again to have a common install script.
For now the splitoff of core sounds warranted though, as there are more special needs for validation here, then for others like "just" plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't want to bloat our dev dependencies we could create a separate composer-test.json
and set a enviromment variable to let composer know to use that one instead of the default one.
https://getcomposer.org/doc/03-cli.md#composer
As far as I can tell phive doesn't allow phpstan extensions (unfortunately...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldnt want to duplicate the dependencies across those files.
I think it should be fair enough for now to have those extra ones in dev, they dont leak into any dependant repo.
Can we move forward with this? |
a6b1e59
to
058f5a1
Compare
We still would need the github token to be added |
6d6fc72
to
e4cb387
Compare
With https://github.com/phpstan/phpstan/releases/tag/1.10.54 being released this would now be fully mergeable again. cc @markstory |
Completes protection discussed and finalized in #17443 etc
I wonder how we can make PHPStan in CI work with this, though
Since we are using this split off task
I still think we should undo that and move CI validation back here directly, it would also help with #17439
There is no need in trying to keep this too agnostic IMO