-
Notifications
You must be signed in to change notification settings - Fork 40
Add PHPStan checks #382
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
base: main
Are you sure you want to change the base?
Add PHPStan checks #382
Conversation
5859af1 to
7cf5f57
Compare
| with: | ||
| show-progress: ${{ runner.debug == '1' && 'true' || 'false' }} | ||
| key: ${{ runner.os }}-${{ hashFiles('composer.json') }} # Note that lock file will change between runs. | ||
| path: .cache |
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.
This is where both phpcs and phpstan store their caches.
| - name: Make Composer packages available globally | ||
| run: echo "${PWD}/vendor/bin" >> $GITHUB_PATH | ||
| - name: Start the development environment | ||
| run: npm run env start |
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.
Run the checks inside the same development environment as used locally. Ensures consistent results across local dev environments and the CI runs.
I've tested this with updated PHPUnit runs and they take the same ~1 min. as the runs currently.
| run: phpcs . -n --report-full | ||
| - name: Lint PHPCS | ||
| # TODO: Remove the flag once all warnings are fixed. | ||
| run: npm run lint:php:phpcs -- -- -- --warning-severity=0 |
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.
There are several existing warnings and errors reported (not sure why previously they didn't come up). I chose to not fix them as part of this pull request to make merging this easier.
FILE: inc/packages/admin/info.php
------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
498 | WARNING | Strings should not be wrapped in HTML (WordPress.WP.I18n.NoHtmlWrappedStrings)
------------------------------------------------------------------------------------------------
FILE: inc/packages/admin/namespace.php
------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------
502 | WARNING | strip_tags() is discouraged. Use the more comprehensive wp_strip_all_tags() instead.
| | (WordPress.WP.AlternativeFunctions.strip_tags_strip_tags)
------------------------------------------------------------------------------------------------------------------------------------
FILE: inc/packages/namespace.php
------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------
627 | WARNING | strip_tags() is discouraged. Use the more comprehensive wp_strip_all_tags() instead.
| | (WordPress.WP.AlternativeFunctions.strip_tags_strip_tags)
------------------------------------------------------------------------------------------------------------------------------------
FILE: inc/site-health/namespace.php
------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------
1 | WARNING | A file should declare new symbols (classes, functions, constants, etc.) and cause no other side effects, or it
| | should execute logic with side effects, but should not do both. The first symbol is defined on line 13 and the first
| | side effect is on line 110. (PSR1.Files.SideEffects.FoundWithSymbols)
------------------------------------------------------------------------------------------------------------------------------------
Time: 241ms; Memory: 16MB
| @@ -0,0 +1,109 @@ | |||
| parameters: | |||
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.
To avoid making changes outside of adding the phpstan tooling to this pull request, we generate a baseline of "known issues" which can be fixed in follow-up pull requests.
| "format": "@php ./vendor/bin/phpcbf .", | ||
| "lint:phpcs": "phpcs .", | ||
| "lint:phpstan": "phpstan analyse --verbose --memory-limit=2G", | ||
| "lint": [ |
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.
We keep the existing composer lint as the combined task and introduce individual scripts for just phpcs and phpstan for convenience.
| # Stop the development server | ||
| $ npm run env stop | ||
| ``` | ||
| - `npm run lint:php:phpcs` to run PHPCS (configured in [`phpcs.xml.dist`](phpcs.xml.dist)). |
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.
Documented a bunch of existing scripts along with pointers to their configuration.
Added instructions for overriding PHP and WP versions locally for running the tooling with any kind of setup. Also useful when debugging version-specific bugs and issues.
| </rule> | ||
|
|
||
| <!-- Cache for PHPStan and PHPCS --> | ||
| <exclude-pattern>.cache</exclude-pattern> |
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.
These were being caught by the phpcs.
| @@ -0,0 +1,10 @@ | |||
| includes: | |||
| - tests/phpstan-baseline.neon # TODO: Fix all these issues. | |||
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.
This can be removed once we've fixed all the reported issues.
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
12d798e to
e14a813
Compare
Signed-off-by: Kaspars Dambis <hi@kaspars.net>
|
@toderash Noticed this was moved to icebox. I'm not familiar with the triage/planning lingo for the project. Could you please confirm if there is any interest in merging this (or solving the associated #376) in near term? Or if there is anything required to get this merged? I have a other follow-up pull requests ready that would fix #383, for example, and it relies on the cleanup and documentation updates in this pull request. Wanted to get a sense of when and how that could happen, if at all. Thanks! |
|
Noting that #394 would have been caught by phpstan if I'm not mistaken. |
Fixes #376.
Add
szepeviktor/phpstan-wordpress(includes both phpstan and the WP core stubs) atv2.0which supports the lowest supported PHP version 7.4 by the plugin.Update the existing coding standard CI checks to run the PHPStan checks inside the same predictable
wp-envenvironment. This is part of larger strategy of running all CI tasks using the same tooling as used for local development (wp-envin this case instead of a soup of bash scripts that rely on global mysql installs, etc.)Update contributor documentation with all of the available helper scripts and their usage. Explain how to configure specific WP and PHP versions.