From 83bf1d58b06e7ecf29196bc224c982fad8bd07b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 11:29:49 +0000 Subject: [PATCH 1/2] Test the lowest allowed dependency set in CI composer.lock is not committed, so every CI job resolved to the newest release allowed by composer.json: the lower bounds of the declared constraints were never executed, even though consumers installing arkitect as a dev dependency often end up on them. Adding that job to the matrix surfaced three problems: - with phpstan/phpdoc-parser v1 the docblock nodes carried no line numbers, because on v1 they are opt-in through the $usedAttributes constructor argument (on v2 they are already requested through ParserConfig). Dependencies declared in docblocks were still detected, but reported on the wrong line, so violations pointed at the wrong place - phpstan/phpdoc-parser below 1.20 lexes `@ORM\Id` as the `ORM` tag, so doctrine-like annotations were collected under a truncated FQCN and the rules on them silently did not match - nikic/php-parser below 5.3 cannot parse property hooks, so any file using them produced no class description at all despite 8.4 being an accepted --target-php-version The version constraints are raised to the lowest release that actually works, and mikey179/vfsstream to the first one that runs on PHP 8: 1.6.0 uses curly brace string offsets, removed in PHP 8. The Composer cache key was keyed on a composer.lock that does not exist, which made it a constant shared by every job; it now includes the PHP version and the resolution strategy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WMkvCCKkaZe1CU1cUREKQN --- .github/workflows/build.yml | 30 +++++++++++++++++++++----- CONTRIBUTING.md | 19 ++++++++++++++++ composer.json | 6 +++--- src/Analyzer/DocblockParserFactory.php | 11 ++++++++-- 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ad31f47e..f4338bf7 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -17,6 +17,14 @@ jobs: fail-fast: false matrix: php-versions: [ '8.0', '8.1', '8.2', '8.3', '8.4', '8.5' ] + dependencies: [ 'highest' ] + include: + # The lowest allowed dependency set needs to be verified only once, and the + # oldest supported PHP is where consumers are most likely to end up with it. + - php-versions: '8.0' + dependencies: 'lowest' + + name: "PHP ${{ matrix.php-versions }} (${{ matrix.dependencies }} deps)" steps: - uses: actions/checkout@v7 @@ -40,13 +48,25 @@ jobs: path: | vendor tools/psalm/vendor - key: ${{ runner.os }}-php-${{ hashFiles('**/composer.lock') }} + # composer.lock is not committed, so it cannot take part in the cache key: + # the installed set depends on the PHP version and on the resolution + # strategy, and both must be part of the key to keep the caches separate + key: ${{ runner.os }}-php-${{ matrix.php-versions }}-${{ matrix.dependencies }}-${{ hashFiles('composer.json') }} restore-keys: | - ${{ runner.os }}-php- + ${{ runner.os }}-php-${{ matrix.php-versions }}-${{ matrix.dependencies }}- - name: Install dependencies + if: ${{ matrix.dependencies == 'highest' }} run: composer install --prefer-dist + # Without a committed composer.lock every other job resolves to the newest + # allowed release, so the lower bounds declared in composer.json are never + # exercised. This job installs the other end of those ranges: the set a + # consumer with older pinned dependencies actually gets. + - name: Install lowest allowed dependencies + if: ${{ matrix.dependencies == 'lowest' }} + run: composer update --prefer-dist --prefer-lowest --prefer-stable + # Psalm 6 needs PHP >= 8.1 to run; the analysis target stays 8.0 (phpVersion in psalm.xml) - name: Static Analysis if: ${{ matrix.php-versions == '8.1' }} @@ -55,15 +75,15 @@ jobs: tools/psalm/vendor/bin/psalm --no-cache - name: Test with coverage - if: ${{ matrix.php-versions == '8.0' }} + if: ${{ matrix.php-versions == '8.0' && matrix.dependencies == 'highest' }} run: ./bin/phpunit -d memory_limit=-1 --coverage-clover clover.xml - name: Test without coverage - if: ${{ matrix.php-versions != '8.0' }} + if: ${{ matrix.php-versions != '8.0' || matrix.dependencies == 'lowest' }} run: ./bin/phpunit -d memory_limit=-1 - name: Upload coverage to Codecov - if: ${{ matrix.php-versions == '8.0' }} + if: ${{ matrix.php-versions == '8.0' && matrix.dependencies == 'highest' }} uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: token: ${{ secrets.CODECOV_TOKEN }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a9f57122..0081da4a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,3 +50,22 @@ make csfix # run code style fixer make psalm # run static analysis ``` +### Testing against the lowest allowed dependencies + +`composer.lock` is not committed, so `composer install` always gives you the +newest release allowed by `composer.json`. Arkitect is installed as a dependency +of other projects, though, where the resolver often picks something older, so CI +also runs the test suite against the lower bound of every declared constraint. + +To reproduce that job locally: + +```shell +composer update --prefer-lowest --prefer-stable +make test +composer update # restore the newest allowed dependencies +``` + +If a change needs a feature that is not in the lowest allowed version of a +dependency, raise the constraint in `composer.json` instead of working around +it. + diff --git a/composer.json b/composer.json index d4c6cc75..c1d6e3a5 100644 --- a/composer.json +++ b/composer.json @@ -23,16 +23,16 @@ ], "require": { "php": "^8.0", - "nikic/php-parser": "~5", + "nikic/php-parser": "^5.3", "ondram/ci-detector": "^4.2", - "phpstan/phpdoc-parser": "^1.2|^2.0", + "phpstan/phpdoc-parser": "^1.20|^2.0", "symfony/console": "^5.4|^6.0|^7.0|^8.0", "symfony/finder": "^5.4|^6.0|^7.0|^8.0", "webmozart/assert": "^1.12|^2.0" }, "require-dev": { "friendsofphp/php-cs-fixer": "^3.95", - "mikey179/vfsstream": "^1.6", + "mikey179/vfsstream": "^1.6.8", "phpspec/prophecy": "^1.10", "phpspec/prophecy-phpunit": "^2.5", "phpunit/phpunit": "^9.6|^10.0|^11.0", diff --git a/src/Analyzer/DocblockParserFactory.php b/src/Analyzer/DocblockParserFactory.php index abdf2bd2..51ab383a 100644 --- a/src/Analyzer/DocblockParserFactory.php +++ b/src/Analyzer/DocblockParserFactory.php @@ -12,7 +12,11 @@ class DocblockParserFactory { /** + * Psalm only ever sees one of the two installed versions of phpstan/phpdoc-parser, + * so the constructor calls of the other branch always look wrong to it. + * * @psalm-suppress TooFewArguments + * @psalm-suppress TooManyArguments * @psalm-suppress InvalidArgument */ public static function create(): DocblockParser @@ -20,7 +24,7 @@ public static function create(): DocblockParser $phpDocParser = null; $phpDocLexer = null; - // this if is to allow using v 1.2 or v2 + // this if is to allow using v1 or v2 if (class_exists(ParserConfig::class)) { $parserConfig = new ParserConfig(['lines' => true]); $constExprParser = new ConstExprParser($parserConfig); @@ -30,7 +34,10 @@ public static function create(): DocblockParser } else { $typeParser = new TypeParser(); $constExprParser = new ConstExprParser(); - $phpDocParser = new PhpDocParser($typeParser, $constExprParser); + // on v1 line numbers are opt-in through $usedAttributes, the 5th constructor + // argument: without it every node reports no line and violations would be + // reported on the wrong line + $phpDocParser = new PhpDocParser($typeParser, $constExprParser, false, false, ['lines' => true]); $phpDocLexer = new Lexer(); } From 835bf37994f19b5be0c67036d8c11f56f7926ac2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 18:33:58 +0000 Subject: [PATCH 2/2] Collect coverage from both dependency sets codecov/patch reported 0.00% of the previous commit's diff as hit: the only executable line it added is the phpstan/phpdoc-parser v1 branch of DocblockParserFactory, and coverage was measured only in the job installing the newest allowed dependencies, where that branch is dead code by construction. Any change to version compatibility code would have been reported the same way. Coverage is now measured on both PHP 8.0 jobs and uploaded with a flag naming the dependency set, so the merge of the two reports describes src. Codecov waits for both uploads before computing the status, otherwise it would compute it against whichever report arrives first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WMkvCCKkaZe1CU1cUREKQN --- .github/workflows/build.yml | 10 +++++++--- codecov.yml | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f4338bf7..3e97e374 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -74,19 +74,23 @@ jobs: composer install --prefer-dist -d tools/psalm tools/psalm/vendor/bin/psalm --no-cache + # Both dependency sets report coverage: code that supports one version of a + # dependency is dead code under the other one, so a single set can never cover + # all of src. Codecov merges the two reports, and codecov.yml waits for both. - name: Test with coverage - if: ${{ matrix.php-versions == '8.0' && matrix.dependencies == 'highest' }} + if: ${{ matrix.php-versions == '8.0' }} run: ./bin/phpunit -d memory_limit=-1 --coverage-clover clover.xml - name: Test without coverage - if: ${{ matrix.php-versions != '8.0' || matrix.dependencies == 'lowest' }} + if: ${{ matrix.php-versions != '8.0' }} run: ./bin/phpunit -d memory_limit=-1 - name: Upload coverage to Codecov - if: ${{ matrix.php-versions == '8.0' && matrix.dependencies == 'highest' }} + if: ${{ matrix.php-versions == '8.0' }} uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: token: ${{ secrets.CODECOV_TOKEN }} + flags: ${{ matrix.dependencies }} phar: runs-on: "ubuntu-22.04" diff --git a/codecov.yml b/codecov.yml index 74e42d9d..7fef4984 100644 --- a/codecov.yml +++ b/codecov.yml @@ -29,13 +29,27 @@ comment: layout: "reach, diff, flags, files" behavior: default require_changes: false + # Same reason as codecov.notify.after_n_builds below. + after_n_builds: 2 -# Wait for the (single) coverage upload before computing the status, so the -# check doesn't run against a partial report. +# Wait for both coverage uploads before computing the status, so the check +# doesn't run against a partial report. The build uploads one report per +# dependency set (see the `flags` below): the code supporting one version of a +# dependency is dead code under the other, so only the merge of the two reports +# describes the coverage of src. codecov: require_ci_to_pass: true notify: wait_for_ci: true + after_n_builds: 2 + +# The flags exist to tell the two uploads apart, not to gate them separately: +# neither dependency set covers src on its own, so a per-flag status would be +# meaningless. Nothing is carried forward from older commits either, both +# reports are uploaded on every build. +flag_management: + default_rules: + carryforward: false ignore: - "tests"