Switch from PHPStan to Psalm #63
Conversation
- Replace usage of Prophecy with native PHPUnit mock objects, adapting tests to accommodate multiple invocations properly. - Ensure `composer.json` is copied to temp dir so that it is found. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Removes phpstan dependencies - Removes phpstan from scripts - Removes phpstan-related installer actions/metadata Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Adds Psalm as dev dependency - Adds Psalm configuration Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Because no plugins/scripts will be run during update operations, we need to manually run the phpcs plugin that aggregates phpcs plugins. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
…pipeline - Adds the `--no-scripts` and `--no-plugins` flags for Composer - Adds a pre-run script that enables the phpcs plugins. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Bumps to 2.1, which ensures we get a compatible symfony/console version. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Picks up a fix for autoloading when running discrete processes. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Issues with duplicate and/or undeclared classes, largely from the fact this is a SKELETON. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
First that supports 8.1 properly. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Ensures PHP 8.1 compat. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
No longer necessary, and ensures we get deps that actually work with them when testing against lowest supported versions. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
| "ignore_php_platform_requirements": { | ||
| "8.1": true | ||
| "8.0": false, | ||
| "8.1": false |
There was a problem hiding this comment.
I've indicated that the --ignore-platform-requirement flag should NOT be used with PHP 8.0 and 8.1. composer update operations run normally without the flag, and, when using --prefer-lowest, we need to get requirements that are appropriate for those versions (otherwise, tests fail due to syntax errors!).
There was a problem hiding this comment.
AFAIK, if the property ignore_php_platform_requirements is available, all PHP versions will have false by default.
So it would be enough to simply create the property as an empty object to ensure that platform reqs won't be ignored.
If the ignore_php_platform_requirements property is not available, PHP 8.0 will always have ignored platform reqs.
| "enable-codestandard": "Dealerdirect\\Composer\\Plugin\\Installers\\PHPCodeSniffer\\Plugin::run", | ||
| "cs-check": [ | ||
| "@enable-codestandard", | ||
| "phpcs -s" | ||
| ], | ||
| "cs-fix": [ | ||
| "@enable-codestandard", | ||
| "phpcbf" | ||
| ], |
There was a problem hiding this comment.
TBH, we could automate some of this in the laminas CI setup (to auto-push to contributor branches after message confirmation, potentially)
There was a problem hiding this comment.
This is only necessary on projects where we are using the --disable-plugins and/or --disable-scripts switches. We might want to detect that, but I think there's only going to be a very small number of places where it will be useful; since there are ways to work around it with the existing tooling, I'm not sure if it makes sense.
There was a problem hiding this comment.
I wonder if this is somehow solvable in another way.
I remember that phpstan has some similar magic and most of my colleagues which do use phpstan in other projects (at work) got rid of that composer plugin magic and added the plugins path to their config.
Yes, I know that would mean we would have to change all repositories if we wanted to roll that out for all components but IMHO its only necessary here due to the fact that we do, in fact, ignore composer plugins intentional.
So instead of still relying on composer, we could simply change the path to the coding standard in the codestyle config instead. WDYT?
| @@ -0,0 +1,348 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <files psalm-version="4.17.0@6f4707aa41c9174353a6434bba3fc8840f981d9c"> | |||
| <file src="config/pipeline.php"> | |||
There was a problem hiding this comment.
I wonder if it's possible to fix any of these 🤔
There was a problem hiding this comment.
I actually fixed quite a few (spent a few hours on it this morning). The vast majority (as in all but maybe 5-10) were MixedAssignment-style warnings. This is to be expected, as we're testing how the application consumes our configuration and/or the Composer file itself. The amount of effort required really would be better spent on creating a different configuration system that had a schema or type system built-in; anything else is just playing whack-a-mole at this point.
|
Thanks @weierophinney ! |
This patch switches from PHPStan to Psalm for static analysis, via the following changes:
--no-pluginsand--no-scriptsflags to Composer.Finally, this patch also applies a number of auto-detected Psalm updates, as well as a few that were flagged and easily fixed. The majority of what is in the baseline are of the
Mixed*-style warnings.Fixes #36
Fixes #61
Replaces #57
Replaces #50