Skip to content

Commit

Permalink
Add PHPStan to QA checks
Browse files Browse the repository at this point in the history
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 `basics` workflow in GH Actions.
    Notes:
    - I've **not** added PHPStan to the Composer 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.
    - [Phive](https://phar.io/) could potentially be used 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.
        Fixing the version for the `setup-php` action installs to the current release `1.10.31` could prevent that, 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, this should be re-evaluated.
* Adds a configuration file for PHPStan.
    Notes:
    - PHPStan needs to know about the dependencies (PHPCS et al), so I'm (re-)using the bootstrap file for the tests to load the PHPCS autoloader and register the standard with the PHPCS autoloader as adding an `autoload` directive to the `composer.json` file would cause problems with the PHPCS autoloader.
    - PHPStan flags type checks on properties with a documented type, while without `strict_types` 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 gone through the scan results up to level 6 with a fine toothcomb and fixed what was reasonable to fix and ignored those things which should *not* be fixed (or, for level 6, are irrelevant to fix).
* 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
  • Loading branch information
jrfnl committed Aug 25, 2023
1 parent 21d1993 commit bfb89ca
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 0 deletions.
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
phpcs.xml.dist export-ignore
phpdoc.dist.xml export-ignore
phpdoc.xml.dist export-ignore
phpstan.neon.dist export-ignore
phpunit.xml.dist export-ignore
phpunit10.xml.dist export-ignore
/.github/ export-ignore
Expand Down
27 changes: 27 additions & 0 deletions .github/workflows/basics.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,33 @@ jobs:
if: ${{ always() && steps.phpcs.outcome == 'failure' }}
run: cs2pr ./phpcs-report.xml

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: 'latest'
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

markdownlint:
name: 'Lint Markdown'
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ vendor/
/.phpunit.result.cache
/docs/phpdoc/
!/docs/phpdoc/.nojekyll
phpstan.neon
95 changes: 95 additions & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
parameters:
#phpVersion: 50400 # Needs to be 70100 or higher... sigh...
level: 6
paths:
- phpcsutils-autoload.php
- .github/GHPages
- PHPCSUtils
- Tests
excludePaths:
# This file needs to be excluded as the availability of the traits depends on which PHPUnit Polyfills version is loaded/installed.
- Tests/PolyfilledTestCase.php
bootstrapFiles:
- Tests/bootstrap.php
treatPhpDocTypesAsCertain: false

ignoreErrors:
# Level 0
# Ignoring this as availability depends on which PHPUnit Polyfills version is loaded/installed. This is 100% okay.
-
message: '`^Call to an undefined method \S+UtilityMethodTestCase::setExpectedException\(\)\.$`'
path: PHPCSUtils/TestUtils/UtilityMethodTestCase.php
count: 1

# Level 1
# These are on purpose for testing the magic method. This is 100% okay.
-
message: '`^Call to an undefined static method PHPCSUtils\\BackCompat\\BCTokens::notATokenArray\(\)\.$`'
path: Tests/BackCompat/BCTokens/UnchangedTokenArraysTest.php
count: 1
-
message: '`^Call to an undefined static method PHPCSUtils\\Tokens\\Collections::notATokenArray\(\)\.$`'
path: Tests/Tokens/Collections/PropertyBasedTokenArraysTest.php
count: 1

# Level 2
# Ignoring as this refers to a non-mocked method on the original class. This is 100% okay.
-
message: '`^Call to an undefined method PHPUnit\\Framework\\MockObject\\MockObject::process\(\)\.$`'
path: Tests/AbstractSniffs/AbstractArrayDeclaration/AbstractArrayDeclarationSniffTest.php
# Ignoring as availability depends on which PHPUnit version is loaded/installed. This is 100% okay.
-
message: '`^Call to an undefined method PHPUnit\\Framework\\MockObject\\MockBuilder<[^>]+>::setMethods\(\)\.$`'
path: Tests/AbstractSniffs/AbstractArrayDeclaration/AbstractArrayDeclarationSniffTest.php
count: 1

# Level 3
# Ignoring as `null` is the initial value for config settings in PHPCS which this test is resetting to.
# The PHPCS docs just don't reflect that. This is 100% okay.
-
message: '`^Property PHP_CodeSniffer\\Config::\$\S+ \([^\)]+\) does not accept null\.$`'
path: Tests/BackCompat/Helper/GetCommandLineDataTest.php

# Level 4
# This is by design.
-
message: '`^Static method PHPCSUtils\\Tokens\\Collections::triggerDeprecation\(\) is unused\.$`'
path: PHPCSUtils/Tokens/Collections.php
count: 1

# This depends on the PHP version on which PHPStan is being run, so not valid.
-
message: "`^Comparison operation \"\\>\\=\" between '[0-9\\. -]+' and 10 is always true\\.$`"
path: Tests/Utils/Orthography/FirstCharTest.php
count: 1
-
message: '`^Else branch is unreachable because previous condition is always true\.$`'
path: Tests/Utils/Orthography/FirstCharTest.php
count: 1

# Level 5
# This is by design to test handling of incorrect input.
-
message: '`^Parameter #[0-9]+ \$\S+ of static method PHPCSUtils\\(?!Tests)[A-Za-z]+\\[A-Za-z]+::[A-Za-z]+\(\) expects [^,]+, \S+ given\.$`'
paths:
- Tests/BackCompat/BCFile/GetTokensAsStringTest.php
- Tests/Exceptions/TestTargetNotFound/TestTargetNotFoundTest.php
- Tests/Fixers/SpacesFixer/SpacesFixerExceptionsTest.php
- Tests/Utils/GetTokensAsString/GetTokensAsStringTest.php

# Ignoring as this is fine.
-
message: '`^Parameter #1 \$exception of method PHPUnit\\Framework\\TestCase::expectException\(\) expects class-string<Throwable>, string given\.$`'
path: Tests/TestUtils/UtilityMethodTestCase/SkipJSCSSTestsOnPHPCS4Test.php
count: 1

# Level 6
# Test data providers.
-
message: '`^Method PHPCSUtils\\Tests\\[^: ]+Test(Case)?::data\S+\(\) return type has no value type specified in iterable type array\.$`'
path: Tests/*

# Test methods.
-
message: '`^Method PHPCSUtils\\Tests\\[^: ]+Test(Case)?::\S+\(\) has parameter \$\S* with no value type specified in iterable type array\.$`'
path: Tests/*

0 comments on commit bfb89ca

Please sign in to comment.