From ada7b68ba4a1866cd0f7af8b40b16167c1715d67 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Aug 2023 19:42:21 +0200 Subject: [PATCH] Add PHPStan to QA checks PHPStan is a good addition to our QA toolkit and with improvements PHPStan has made over the years is now a viable tool for us to use (previously it would give way too many false positives). This commit: * Adds a separate job to the `basic-qa` workflow in GH Actions. Notes: - I've chosen **not** to add PHPStan to the Compose dependencies for two reasons: 1. It doesn't allow for installation on PHP < 7.2, which would break/block the `composer install` for our test runs. 2. It would add dependencies which could conflict/cause problems for our test runs due to those defining token constants too. - We could potentially use [Phive](https://phar.io/) to still have a setup which can be used locally, but just running locally from a PHPStan PHAR file should work just fine. - For CI, PHPStan will be installed as a PHAR file by `setup-php` now. This does carry a risk _if_ PHPStan would make breaking changes or if a new release adds rules for the levels being scanned as, in that case, builds could unexpectedly start failing. We could fix the version `setup-php` action installs to the current release `1.10.29`, but that adds an additional maintenance burden of having to keep updating the version as PHPStan releases pretty often. So, for now, I've elected to run the risk of random failures. If and when those start happening, we can re-evaluate. - The PHP version for the CI run is set to PHP 7.4 to prevent PHPStan throwing some errors/notices related to the outdated PHPUnit version being used. * Adds a configuration file for PHPStan. Notes: - PHPStan needs to know about our dependencies (PHPCS et al), so I'm (re-)using the bootstrap file we have for our tests to load the PHPCS autoloader and register the standard with the PHPCS autoloader as we can't add an `autoload` directive to our `composer.json` file as it would cause problems with the PHPCS autoloader. - PHPStan flags type checks on properties with a documented type, while - especially for the `public` properties - we cannot always be sure the properties set will be of the correct type. For that reason, I've set `treatPhpDocTypesAsCertain` to `false` (which silences those notices). - I've very selectively silenced a number of errors. - Level 0: this is about the use of `return $this->method()` when the return type of the `process_token()` method is `int|void`. For consistency, I believe keeping those returns makes for less confusing code (they will still be `void`). - Level 4: I believe those are false positives. - Level 5: This error is not relevant as we don't use `strict_types`. * Adds the configuration file to `.gitattributes` and the typical overload file for the configuration file to `.gitignore`. Refs: * https://phpstan.org/ * https://phpstan.org/config-reference --- .gitattributes | 1 + .github/workflows/basic-qa.yml | 28 ++++++++++++++++++++++++++++ .gitignore | 1 + phpstan.neon.dist | 26 ++++++++++++++++++++++++++ 4 files changed, 56 insertions(+) create mode 100644 phpstan.neon.dist diff --git a/.gitattributes b/.gitattributes index 77023da83a..224ffba10a 100644 --- a/.gitattributes +++ b/.gitattributes @@ -9,6 +9,7 @@ /.codecov.yml export-ignore /.phpcs.xml.dist export-ignore /CODE_OF_CONDUCT.md export-ignore +/phpstan.neon.dist export-ignore /phpunit.xml.dist export-ignore /.github export-ignore /Tests export-ignore diff --git a/.github/workflows/basic-qa.yml b/.github/workflows/basic-qa.yml index 8e0b604763..859cc0566d 100644 --- a/.github/workflows/basic-qa.yml +++ b/.github/workflows/basic-qa.yml @@ -160,3 +160,31 @@ jobs: - name: Fail the build on fixer conflicts and other errors if: ${{ steps.phpcbf.outputs.EXITCODE != 0 && steps.phpcbf.outputs.EXITCODE != 1 }} run: exit ${{ steps.phpcbf.outputs.EXITCODE }} + + phpstan: + name: "PHPStan" + + runs-on: "ubuntu-latest" + + steps: + - name: Checkout code + uses: actions/checkout@v3 + + - name: Install PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '7.4' + coverage: none + tools: phpstan + + # Install dependencies and handle caching in one go. + # Dependencies need to be installed to make sure the PHPCS and PHPUnit classes are recognized. + # @link https://github.com/marketplace/actions/install-composer-dependencies + - name: Install Composer dependencies + uses: "ramsey/composer-install@v2" + with: + # Bust the cache at least once a month - output format: YYYY-MM. + custom-cache-suffix: $(date -u "+%Y-%m") + + - name: Run PHPStan + run: phpstan analyse diff --git a/.gitignore b/.gitignore index 9e9c75d11d..f4a8c2e298 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ build/ phpunit.xml phpcs.xml .phpcs.xml +phpstan.neon diff --git a/phpstan.neon.dist b/phpstan.neon.dist new file mode 100644 index 0000000000..b4774d4a95 --- /dev/null +++ b/phpstan.neon.dist @@ -0,0 +1,26 @@ +parameters: + #phpVersion: 50400 # Needs to be 70100 or higher... sigh... + level: 5 + paths: + - /WordPress/ + bootstrapFiles: + - /Tests/bootstrap.php + treatPhpDocTypesAsCertain: false + + ignoreErrors: + # Level 0 + - '#^Result of method \S+ \(void\) is used\.$#' + + # Level 4 + - '#^Property \S+::\$\S+ \([^)]+\) in isset\(\) is not nullable\.$#' + - + count: 1 + message: '#^Result of && is always true\.$#' + path: /WordPress/Sniffs/Security/EscapeOutputSniff.php + - + count: 1 + message: '#^Strict comparison using === between true and false will always evaluate to false\.$#' + path: /WordPress/Sniffs/Utils/I18nTextDomainFixerSniff.php + + # Level 5 + - '#^Parameter \#3 \$value of method \S+File::recordMetric\(\) expects string, \(?(float|int|bool)(\|(float|int|bool))*\)? given\.$#'