Add Psalm static analysis (composer analyse + CI job)#3
Merged
Conversation
Wires Psalm in as a static-analysis safety net for the production code and the test suite. Runs locally via `composer analyse` and in CI as a new parallel job alongside the unit-test matrix. Configuration: - errorLevel="6", a middle-ground bar that catches real bugs without drowning in MixedReturnStatement noise from Roundcube interop. - projectFiles: imapsync.php + lib/ + tests/. extraFiles: the rcube_imap_generic / rcube_plugin / rcube_utils / rcmail / html-* classes under dist/roundcubemail-1.7.0/program/lib/Roundcube/ so Psalm can resolve symbols without analysing them. - A small baseline (psalm-baseline.xml) covers three brownfield type-shape complaints (NullArgument in imapsync.php, the bool-vs-int return mismatch where we hand back rcube_imap_generic's append() result, and the same kind of issue in FakeImapClient's nextUid helper). All three are Roundcube-interop quirks, not real bugs — they get baselined now and can be tightened later. With the baseline in place: "No errors found!", 97% inferred type coverage. CI: - New static-analysis job runs in parallel to unit-tests (no needs:) so type feedback arrives without waiting for the PHP matrix. - composer dist:fetch runs first so Roundcube symbols resolve. Docs: - AGENTS.md layout block + Testing subsection + freshness paragraph reference the new check. - README dev-commands list adds composer analyse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI revealed that Psalm 5.x caps PHP support at 8.3 (require: "^7.4 || ~8.0.0 || ~8.1.0 || ~8.2.0 || ~8.3.0"), which made composer install fail on the PHP 8.4 unit-test matrix job before any test could run. Psalm 6 supports PHP 8.1-8.4 cleanly. Bump the dev dep to ^6.0 and drop the Docker-fallback hack from the analyse script (it was working around exactly this incompatibility but only at run time, not at install time, so it didn't help the matrix anyway). Regenerate psalm-baseline.xml against Psalm 6 — the new strict checks (MissingOverrideAttribute, ClassMustBeFinal, additional InvalidReturnType shapes) add ~25 brownfield entries to the baseline. With the rebuild in place: "No errors found!", 96% type inference. Unit tests still green locally (24/24). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the unit-test matrix (8.1/8.2/8.3/8.4 → 8.4 only) and bumps the integration-tests and static-analysis jobs from 8.3 to 8.4. Roundcube's current standard target is 8.4. Testing the older versions in CI gave us coverage we never used in production. The composer.json still declares ">=8.1" so installs on older PHP versions remain unblocked, but CI no longer validates them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 67-line psalm-baseline.xml from the Psalm 6 bump was a known-debt file: it covered real issues that we were just suppressing. This commit fixes them all and removes the baseline so future regressions fail the build cleanly. Fixes: - `#[\Override]` attribute added to every method that overrides a parent or implements an interface method (init, the 12 methods on RoundcubeImapSyncGenericClient and FakeImapClient that implement the RoundcubeImapSyncClient interface, the four PHPUnit lifecycle hooks setUpBeforeClass / tearDownAfterClass on the two integration test classes). Auto-applied via `psalm --alter`. - `imapsync.php`: addFormRow passed `null` as the first arg to `html_table::add`, which Psalm reads as "no column attributes" but the signature types as `array|string`. Use `''` to mean the same thing without the null-into-non-nullable shape mismatch. - `lib/RoundcubeImapSyncClient.php::appendMessage`: cast the return to `(bool)`. `rcube_imap_generic::append()` returns mixed: false on failure, true on plain success, or the appended UID (string) when the server supports UIDPLUS. Our interface only contracts a bool, so collapse the success cases. - `tests/Fakes/FakeImapClient.php::nextUid`: narrow the inner message-array type via a local `@var non-empty-array<int, mixed>` annotation so `max(array_keys(...))` infers as `int` instead of `float|int`. `psalm-baseline.xml` and the `errorBaseline` attribute in `psalm.xml` are gone. `composer analyse` now returns "No errors found!" against the raw codebase. 24 unit + 7 integration tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Followup to the baseline removal. The layout block still listed it as a tracked file. Updated and made the no-baseline policy visible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Wires Psalm in as a static-analysis safety net for the production code and the test suite. Runs locally via `composer analyse` and in CI as a new parallel job alongside the unit-test matrix.
Configuration
All three are Roundcube-interop quirks, not real bugs. They get baselined now and can be tightened later. With the baseline in place: "No errors found!", 97% inferred type coverage.
CI
Docs
Breaking changes
None. Pure dev-tooling addition.
How to verify
```bash
composer install
composer dist:fetch
composer analyse
```
Expected: `No errors found!` after applying the baseline.
🤖 Generated with Claude Code