build: upgrade psalm to 6.16, fix newly reported issues#660
Merged
Conversation
Psalm 6 requires PHP >= 8.1 to run, so the CI static-analysis step moves from the 8.0 to the 8.1 matrix job; the analysis target stays the lowest supported PHP via phpVersion=8.0 in psalm.xml. This also makes psalm runnable again on dev machines with PHP >= 8.4, where the 5.x phar refused to start. Two new opinionated checks are configured off: ClassMustBeFinal (making library classes final is an API decision for consumers, not a static-analysis one) and ensureOverrideAttribute (#[Override] is only enforced from PHP 8.3, below our 8.0 floor). The remaining findings are real and fixed: json_encode sites in the printers and baseline now use JSON_THROW_ON_ERROR plus JSON_INVALID_UTF8_SUBSTITUTE, so invalid UTF-8 coming from analyzed sources is substituted instead of silently emitting empty output; Baseline::loadFromFile surfaces unreadable files with a clear message; toKebabCase handles preg_replace's null error return; TargetPhpVersion uses the PHP_VERSION constant instead of the false-able phpversion(); FullyQualifiedClassName tolerates a null array_pop result; the PHPUnit constraint adapter declares native mixed params, valid across the untyped (PHPUnit 9) and mixed (10/11) parent signatures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Centralizes the JSON_THROW_ON_ERROR/JSON_INVALID_UTF8_SUBSTITUTE flags required by Psalm 6 into a single helper instead of repeating them at every json_encode call site, and pretty-prints all JSON output (baseline file, --format json, --format gitlab) for readability.
…d narrowing Violation has no setters and every "modification" (withoutLineNumber) returns a new instance, so it's already immutable in practice. The annotation lets Psalm carry null-narrowing across repeated getter calls without introducing throwaway local variables at every call site, and keeps TextPrinter unchanged from main.
…String() Implicit __toString via string concatenation is idiomatic PHP, not a real correctness risk, so it's not worth requiring an explicit call at every call site.
array_pop() on the filtered path segments can never be null here: the regex above already rejects an empty or trailing-backslash \$fqcn, so there's always at least one non-empty segment to pop. A '?? ""' fallback would silently build an invalid FullyQualifiedClassName if the invariant were ever violated instead of surfacing the bug.
Codifies the approach taken throughout this Psalm 6 upgrade: don't add runtime code whose only purpose is to satisfy the analyzer, prefer a targeted or global suppression with an explanatory comment when the flagged case is unreachable or low-value.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #660 +/- ##
============================================
- Coverage 98.28% 98.24% -0.04%
- Complexity 737 739 +2
============================================
Files 94 95 +1
Lines 2100 2111 +11
============================================
+ Hits 2064 2074 +10
- Misses 36 37 +1
🚀 New features to boost your workflow:
|
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
1 participant
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.
Follow-up to #659 — now that Psalm runs via composer in
tools/psalm, nothing stops us from upgrading it independently of the runtime PHP version used to analyze the code.Why: the committed
bin/psalm.pharwas stuck on Psalm 5 and unable to even start on PHP ≥ 8.4 (this repo runs PHP 8.5 locally). Psalm 6.16 requires PHP ~8.1+ to run, but still analyzes code down to our declared floor viaphpVersion="8.0"inpsalm.xml— the runtime and the analysis target are independent settings.What changes:
tools/psalm/composer.json:vimeo/psalm^5.26→^6.16, platform pin bumped to8.1.31(Psalm 6's minimum).8.1matrix job (was8.0), since Psalm 6 can't run below that.psalm.xml: explicitphpVersion="8.0"(analysis target, unaffected by the runtime bump);ensureOverrideAttribute="false"(project floor is PHP 8.0,#[Override]only exists from 8.3); suppressedClassMustBeFinal(making library classes final is an API decision for consumers, not a static-analysis one).Json::encode()helper (src/Json.php) centralizingJSON_THROW_ON_ERROR/JSON_INVALID_UTF8_SUBSTITUTE, used byGitlabPrinter,JsonPrinter,Baseline— also pretty-prints all JSON output now.Violationmarked@psalm-immutable(it already has no setters) instead of working around repeated-getter narrowing.Baseline::loadFromFilenow guards againstfile_get_contents()returningfalse.FullyQualifiedClassName::fromStringdocuments (with a targeted suppress) whyarray_pop()can't returnnullthere.ArchRuleCheckerConstraintAdapter: nativemixedparameter types instead of docblocks, for PHPUnit 9/10/11 cross-version compatibility.DebugExpression/TargetPhpVersion/GitlabPrinter: minor null/false-return handling.ImplicitToStringCastsuppressed project-wide inpsalm.xmlrather than forcing->__toString()at every call site — implicit conversion via concatenation is idiomatic PHP.CLAUDE.md: documents the convention followed throughout — prefer a targeted/global@psalm-suppresswith an explanatory comment over runtime code whose only purpose is to satisfy the analyzer.