From 06304129cd7eef65e71cdc538dac922ed9af33bc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Mar 2026 15:06:36 +0200 Subject: [PATCH 1/2] Introduce PHP_CodeSniffer ruleset based on PSR12 Follow-up on PRs 200, 201, 202 and 203. This commit introduces [PHP_CodeSniffer](https://github.com/PHPCSStandards/PHP_CodeSniffer) (PHPCS) as the tooling to enforce code style consistency for the Patchwork codebase. Notes: * As discussed in 180, the ruleset is based on [PSR-12](https://www.php-fig.org/psr/psr-12/). At a later point in time, we may want to update that to PERCS (once available), but that's not that relevant for this codebase as the rule additions in PERCS are mostly related to new PHP syntax, which can't be used in this codebase at this time anyway due to the PHP 7.1 minimum. * Additionally, the [PHPCompatibility](https://github.com/PHPCompatibility/PHPCompatibility) package, an external ruleset for PHPCS, has been added at the 10.0.0-alpha2 release to detect PHP cross-version compatibility issues. While this is an alpha release, I'd still recommend using it over the last stable release as a) it's pretty damn stable anyway and b) it has much much more detection of issues than any previous release. * For PHPCS to recognize the external PHPCompatibility ruleset, the `dealerdirect/phpcodesniffer-composer-installer` composer plugin needs permission to run. This has been granted. * Also note that PHPCS is allowed at version 3 and 4. This is because v4 has a PHP 7.2 minimum (v3 PHP 5.4), which would otherwise impede running of our tests on PHP 7.1. Ruleset specific notes: * At this time, only the `.php` files can be checked. `.phpt` files cannot be analyzed at this time by PHPCS. An issue is open about this upstream and if that ever gets addressed, we could consider scanning the `.phpt` test files too. * The test fixtures are explicitly excluded from the scans as those may contain code written in a non-compliant way explicitly to trigger a specific test situation and we don't want to limit the test fixtures in what can be tested. * There are two sniffs, both related to file organisation, which have been explicitly excluded. Whether or not this codebase should follow the "one class per file", PSR-4 and "no side-effects" rules is a different conversation and should be discussed separately. This should not block this PR, which is why I've excluded the rules for the time being. This is related to the work previously started in 98. * Additionally, I've made some selective exclusions (= exclude a specific sniff for one specific file), where the changes PSR12 expects would lead to breaking changes otherwise. It could be considered to address these in a future major release, but making these changes at this time is incompatible with Semver. * I've also elected to make the line length rule a little more lenient. Includes: * A GitHub Actions workflow job to enforce the code style on all pushes and pull requests. When the workflow runs for a pull request, code style violations will be shown in the PR code view via inline annotations using the `cs2pr` tool. * Some simple Composer scripts to make it straight-forward for contributors to know what CI checks are being run and how to run these locally. * Excluding the PHPCS ruleset file from being included in distribution archives. * Git-ignoring typical PHPCS ruleset overload files (which can be used by devs locally to try additional rules). A --- .gitattributes | 1 + .github/workflows/qa.yml | 30 ++++++++++++ .gitignore | 2 + .phpcs.xml.dist | 99 ++++++++++++++++++++++++++++++++++++++++ composer.json | 30 +++++++++++- 5 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 .phpcs.xml.dist diff --git a/.gitattributes b/.gitattributes index 4a24ebf..c8442fa 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,5 +1,6 @@ /.gitattributes export-ignore /.gitignore export-ignore +/.phpcs.xml.dist export-ignore /box.json export-ignore /.github export-ignore /tests export-ignore diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index 3c26aa8..981e2eb 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -35,3 +35,33 @@ jobs: uses: docker://rhysd/actionlint:latest with: args: -color + + phpcs: + name: 'PHPCS' + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Install PHP + uses: shivammathur/setup-php@accd6127cb78bee3e8082180cb391013d204ef9f # 2.37.0 + with: + php-version: 'latest' + coverage: none + tools: cs2pr + + # Install dependencies and handle caching in one go. + # @link https://github.com/marketplace/actions/install-composer-dependencies + - name: Install Composer dependencies + uses: "ramsey/composer-install@65e4f84970763564f46a70b8a54b90d033b3bdda" # 4.0.0 + + # Check the code-style consistency of the PHP files. + - name: Check PHP code style + continue-on-error: true + run: composer checkcs -- --report-full --report-checkstyle=./phpcs-report.xml + + - name: Show PHPCS results in PR + run: cs2pr ./phpcs-report.xml diff --git a/.gitignore b/.gitignore index 5c319eb..82e3acb 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,8 @@ tests/cache vendor/ composer.phar .DS_Store +.phpcs.xml +phpcs.xml # Ignore PhpStorm project related files. .idea diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist new file mode 100644 index 0000000..d1d7929 --- /dev/null +++ b/.phpcs.xml.dist @@ -0,0 +1,99 @@ + + + + Patchwork rules for PHP_CodeSniffer + + + + + . + + + */vendor/* + + + */tests/includes/* + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + /src/CodeManipulation/Stream\.php$ + + + + + /src/CodeManipulation/Actions/RedefinitionOfNew\.php$ + + + diff --git a/composer.json b/composer.json index f41c39e..25ecb1c 100644 --- a/composer.json +++ b/composer.json @@ -10,14 +10,40 @@ "email": "ignas.rudaitis@gmail.com" } ], - "minimum-stability": "stable", "require": { "php": ">=7.1.0" }, "require-dev": { - "phpunit/phpunit": ">=4" + "phpcompatibility/php-compatibility": "^10.0.0@alpha", + "phpunit/phpunit": ">=4", + "squizlabs/php_codesniffer": "^3.13.5 || ^4.0" }, + "minimum-stability": "alpha", + "prefer-stable": true, "config": { + "allow-plugins": { + "dealerdirect/phpcodesniffer-composer-installer": true + }, "lock": false + }, + "scripts": { + "checkcs": [ + "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs" + ], + "fixcs": [ + "@php ./vendor/squizlabs/php_codesniffer/bin/phpcbf" + ], + "test": [ + "@php ./vendor/phpunit/phpunit/phpunit --no-coverage" + ], + "coverage": [ + "@php ./vendor/phpunit/phpunit/phpunit" + ] + }, + "scripts-descriptions": { + "checkcs": "Check the PHP files for code style violations and best practices.", + "fixcs": "Auto-fix code style violations in the PHP files.", + "test": "Run the unit tests without code coverage.", + "coverage": "Run the unit tests with code coverage." } } From c456ebcd21e7940d70dfbf54754f5ee97f3c4c0c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Mar 2026 15:11:07 +0200 Subject: [PATCH 2/2] PHPCS: add some targetted inline ignore annotations * `CallRerouting`: I've verified that the issue flagged will not occur. * `Namespaces`: I've verified that the issue flagged will not occur. * `RedefinitionOfNew`: I've elected not to address the line length issue for this code snippet. --- src/CallRerouting.php | 1 + src/CodeManipulation/Actions/Namespaces.php | 5 +++++ src/CodeManipulation/Actions/RedefinitionOfNew.php | 1 + 3 files changed, 7 insertions(+) diff --git a/src/CallRerouting.php b/src/CallRerouting.php index 121329a..50fc557 100644 --- a/src/CallRerouting.php +++ b/src/CallRerouting.php @@ -360,6 +360,7 @@ function connectOnHHVM($function, Handle $handle) } elseif (is_object($obj)) { $calledClass = get_class($obj); } + // phpcs:ignore PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.NeedsInspection -- Verified okay. $frame = count(debug_backtrace(0)) - 1; $result = null; $done = dispatch($class, $calledClass, $method, $frame, $result, $args); diff --git a/src/CodeManipulation/Actions/Namespaces.php b/src/CodeManipulation/Actions/Namespaces.php index edc2e67..41968ac 100644 --- a/src/CodeManipulation/Actions/Namespaces.php +++ b/src/CodeManipulation/Actions/Namespaces.php @@ -52,6 +52,7 @@ function getNamespaceAt(Source $s, $pos) function collectNamespaceBoundaries(Source $s) { + // phpcs:disable PHPCompatibility.FunctionDeclarations.NewClosure.ThisFoundOutsideClass -- Using $this is okay. Source::cache() binds the closure. return $s->cache([], function () { if (!$this->has(T_NAMESPACE)) { return ['' => [[0, INF]]]; @@ -71,6 +72,7 @@ function collectNamespaceBoundaries(Source $s) } return $result; }); + // phpcs:enable } function collectUseDeclarations(Source $s, $begin) @@ -84,6 +86,8 @@ function collectUseDeclarations(Source $s, $begin) } } } + + // phpcs:disable PHPCompatibility.FunctionDeclarations.NewClosure.ThisFoundOutsideClass -- Using $this is okay. Source::cache() binds the closure. return $s->cache([$begin], function ($begin) { $result = ['class' => [], 'function' => [], 'const' => []]; # only tokens that are siblings bracket-wise are considered, @@ -98,6 +102,7 @@ function collectUseDeclarations(Source $s, $begin) } return $result; }); + // phpcs:enable } function parseUseDeclaration(Source $s, $pos, array &$aliases, $prefix = '', $type = 'class') diff --git a/src/CodeManipulation/Actions/RedefinitionOfNew.php b/src/CodeManipulation/Actions/RedefinitionOfNew.php index 304b98e..4629bf4 100644 --- a/src/CodeManipulation/Actions/RedefinitionOfNew.php +++ b/src/CodeManipulation/Actions/RedefinitionOfNew.php @@ -16,6 +16,7 @@ const STATIC_INSTANTIATION_REPLACEMENT = '\Patchwork\CallRerouting\getInstantiator(\'%s\', %s)->instantiate(%s)'; const DYNAMIC_INSTANTIATION_REPLACEMENT = '\Patchwork\CallRerouting\getInstantiator(%s, %s)->instantiate(%s)'; +// phpcs:ignore Generic.Files.LineLength.TooLong const CALLED_CLASS = '((__CLASS__ && __FUNCTION__ !== (__NAMESPACE__ ? __NAMESPACE__ . "\\\\{closure}" : "\\\\{closure}")) ? \get_called_class() : null)'; const spliceAllInstantiations = 'Patchwork\CodeManipulation\Actions\RedefinitionOfNew\spliceAllInstantiations';