Upgrade the dev toolchain - #4
Merged
Merged
Conversation
The static-analysis and test tooling had aged out of the supported PHP
range. vimeo/psalm ^5.10 caps at 5.26.1, whose last supported runtime is
PHP 8.3; on 8.4 it aborts during scanning with "InvalidArgumentException:
$value must be a scalar" while reflecting a core class. Because psalm
also hard-sets error_reporting(E_ALL) and converts every notice into an
exception, even loading the autoloader blew up on the PHP 8.4
deprecations in thecodingmachine/safe, a transitive infection 0.27
dependency. The practical effect was that the package could not be
installed on a PHP 8.4 host without --ignore-platform-req, and the gates
could not be run there at all.
The versions are brought in line with eventjet/adyen, which is already
green on 8.4:
infection/infection ^0.27.0 -> ^0.31.2 (0.31.9)
maglnet/composer-require-checker ^4.6 -> ^4.16 (4.20.0)
phpstan/extension-installer ^1.3 -> ^1.4 (1.4.3)
phpstan/phpstan ^1.10 -> ^2.1 (2.2.8)
phpstan/phpstan-phpunit ^1.3 -> ^2.0 (2.0.18)
phpstan/phpstan-strict-rules ^1.5 -> ^2.0 (2.0.12)
phpunit/phpunit ^10.2 -> ^12.3 (12.5.33)
psalm/plugin-phpunit ^0.18.4 -> ^0.19.5 (0.19.7)
vimeo/psalm ^5.10 -> ^6.13 (6.16.1)
eventjet/coding-standard and guzzlehttp/guzzle are left alone; their
existing constraints already resolve to the current releases.
Fallout, all of it in test code -- src/ is untouched by this change:
phpstan 2 caught a real invariant violation in TestHttpClient. The $map
property is declared list<...>, but sendRequest() removed a consumed
entry with unset(), which punches a hole in the array and leaves it a
non-list. Replaced with array_splice(), which removes and renumbers in
one step so the declared type stays true. The two differ only in key
renumbering, and every reader of $map either iterates it with fresh keys
or indexes it with a key taken from that same iteration, so behaviour is
unchanged. Verified by exercising the paths the suite does not cover:
removing the first of two mappings, the maxMatches counter decrementing,
and exhaustion.
psalm 6 added MissingOverrideAttribute, which fired on the two setUp()
methods and on TestHttpClient::sendRequest(). Added #[Override], matching
the convention already used throughout eventjet/adyen.
phpunit 12 needed nothing: this package has no data providers to migrate
to attributes, and phpunit.xml validates against the 12.x schema as-is.
Also enabled findUnusedPsalmSuppress, as eventjet/adyen does, so a
suppression that stops being necessary is reported instead of quietly
rotting. It confirmed that the one existing @psalm-suppress in
TestHttpClient is still required, so that stays.
No ignores, baseline entries, disabled rules or skipped tests were added.
Verified on the PHP 8.4.11 host with no --ignore-platform-req and no
container: composer update resolves cleanly, and check-deps, cs-check,
phpstan, psalm, phpunit and infection all exit 0 (6 tests, 1 skipped for
the missing OPENAI_API_KEY; infection 11/11 mutants killed, 100% MSI and
100% covered MSI).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The static-analysis and test tooling had aged out of the supported PHP range, to the point where this package could not be worked on with a current PHP.
vimeo/psalm ^5.10caps at 5.26.1, whose last supported runtime is PHP 8.3. On 8.4 it aborts during scanning:Psalm also hard-sets
error_reporting(E_ALL)and turns every notice into an exception, so even loading the autoloader blew up on the PHP 8.4 deprecations inthecodingmachine/safe(a transitiveinfection0.27 dependency), before Psalm read a single project file. The practical effect:composer updateneeded--ignore-platform-req=php+on a PHP 8.4 host, and the gates had to be run in aphp:8.3-clicontainer.What changed
require-devonly, brought in line witheventjet/adyen, which is already green on 8.4.requireis untouched — noeventjet/json, nophpconstraint change; those belong to #3.infection/infection^0.27.0^0.31.2maglnet/composer-require-checker^4.6^4.16phpstan/extension-installer^1.3^1.4phpstan/phpstan^1.10^2.1phpstan/phpstan-phpunit^1.3^2.0phpstan/phpstan-strict-rules^1.5^2.0phpunit/phpunit^10.2^12.3psalm/plugin-phpunit^0.18.4^0.19.5vimeo/psalm^5.10^6.13eventjet/coding-standardandguzzlehttp/guzzleare left alone — their existing constraints already resolve to the current releases (3.19.2 and 7.15.2), so bumping them would be noise.Fallout and how it was fixed
All of it in test code.
src/is untouched by this PR, so the shipped package is byte-identical.phpstan 2 → a real invariant violation in
TestHttpClient.$mapis declaredlist<array{Matcher, ResponseInterface, positive-int}>, butsendRequest()removed a consumed entry withunset($this->map[$index]), which punches a hole in the array and leaves it a non-list — so the declared type was a lie:Fixed with
array_splice($this->map, $index, 1), which removes and renumbers in one step, so the invariant holds. I chose this over widening the property toarray<int, …>because the list-ness is the intended design — entries are appended with$this->map[] =. The two differ only in key renumbering, and every reader of$mapeither iterates it with fresh keys or indexes it with a key taken from that same iteration, so behavior is unchanged. Verified by exercising the paths the suite does not cover: removing the first of two mappings, themaxMatchescounter decrementing, and exhaustion — all identical before and after.psalm 6 →
MissingOverrideAttributeon the twosetUp()methods and onTestHttpClient::sendRequest(). Added#[Override], matching the convention already used throughouteventjet/adyen.phpunit 12 → nothing. This package has no data providers to migrate to attributes, and
phpunit.xmlvalidates against the 12.x schema as-is.Also enabled
findUnusedPsalmSuppress(aseventjet/adyendoes) so a suppression that stops being necessary gets reported instead of quietly rotting. It confirmed the one existing@psalm-suppressinTestHttpClientis still required, so that stays. Drop this line if you'd rather keep the diff to dependencies alone.No ignores, baseline entries, disabled rules or skipped tests were added.
Verification
Run on the PHP 8.4.11 host, with no
--ignore-platform-reqand no container — which is the point of the change:composer updatecomposer check-depscomposer cs-checkcomposer phpstancomposer psalmcomposer phpunitOPENAI_API_KEY)composer infectionThe deprecation noise that previously flooded every command is gone as well.
Why there are no CI checks on this PR
GitHub Actions cannot run for this repo, and that is not caused by this change.
eventjet/openaiis not in the selected-repositories list for the org secretsCI_PAT,SATIS_PASSWORD,SATIS_USERandSSH_PRIVATE_KEY:The shared
eventjet/ciworkflow requires all four, so runs die before a job is even created — which is why recent runs show 0-second failures. Fixing it means adding this repo to those four secrets in org settings, which needs admin access and is out of scope here. The green signal for this PR is the local 8.4 run above.For the reviewer
This PR leaves
require.phpat>=8.2whilerequire-devnow needs 8.3+ (phpunit 12 and infection 0.31 both require>=8.3). Consumers are unaffected — the published package still installs on 8.2, and#[Override]only appears inautoload-devtest code — but a contributor on PHP 8.2 will not be able to install dev dependencies. #3 raisesrequire.phpto>=8.3and closes that gap, so the inconsistency lasts only for the window between the two merges. Landing #3 promptly after this one, or merging them in the other order, both resolve it.