Drop loose-comparison rule; add PHPStan + Psalm fixture guardrail#42
Merged
Conversation
The loose-comparison fixture exercised a rule both PHPStan (strict-rules) and Psalm already catch. Removing the duplication: SlevomatCodingStandard .Operators.DisallowEqualOperators is pinned to severity=0 and strict_comparison is set to false, so a future preset switch can't silently bring it back. PHPStan (level max + strict-rules + phpunit) and Psalm (errorLevel 1 + strictBinaryOperands) now run on tests/fixtures/ in CI. If a fixture demonstrates a pattern either tool already flags, the build fails — which is the signal that the rule belongs in static analysis, not in the coding standard. Configs live in tests/ because today only fixtures are analyzed; if sniff/fixer sources are ever analyzed, separate root configs can be added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the two separate PHPStan and Psalm CI steps (which gave OR semantics — a fixture failed CI when either tool flagged it) with a single gate that fails only when both tools flag the same file. The previous setup would have wrongly dropped fixtures that only one SA catches. AND is the conservative policy: keep a CS rule unless both major analyzers already catch the pattern. A canary fixture (tests/sa-canary/loose-comparison.php) must always be in the intersection. If it isn't, the SA configs drifted, a tool crashed, or the JSON schema changed; CI fails loudly rather than silently passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move tests/sa-canary/loose-comparison.php into tests/fixtures/valid/ so RulesTest::valid asserts the CS side: phpcs and php-cs-fixer must not flag the file. If a preset bump silently re-enables strict_comparison or DisallowEqualOperators, that test fails. Previously we only checked the SA side (both lint tools flag it). Drop the now-unneeded tests/sa-canary/ directory from the SA configs and generalize the gate script's hardcoded canary path into an EXPECTED_REL array, so future rule drops follow the same one-fixture-per-rule pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the dedicated “loose comparison” code-style enforcement (PHPCS + PHP CS Fixer) and replaces that duplication with a CI guardrail that ensures fixtures don’t creep into “lint territory” already covered by both PHPStan and Psalm, using a canary-based intersection check.
Changes:
- Disable PHPCS
DisallowEqualOperatorsand PHP CS Fixerstrict_comparison, and add avalid/loose-comparison.phpcanary fixture. - Add fixture-scoped PHPStan/Psalm configs and a CI script (
tests/check-sa-overlap.sh) that assertsPHPStan ∩ Psalmequals an explicit canary list. - Add PHPStan + Psalm dev dependencies and wire the overlap gate into GitHub Actions.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/psalm.xml |
Adds Psalm config scoped to fixture analysis with selected suppressions. |
tests/phpstan.neon |
Adds PHPStan config scoped to fixtures (max + strict rules) with a targeted ignore. |
tests/check-sa-overlap.sh |
Adds CI guardrail script to enforce the PHPStan/Psalm overlap canary set. |
tests/fixtures/valid/psr-12-import-order.php |
Tweaks fixture to avoid unused-expression SA findings. |
tests/fixtures/valid/no-array-typehint.php |
Adds param phpDoc to satisfy SA type expectations while preserving the fixture’s intent. |
tests/fixtures/valid/mixed-assignment.php |
Adjusts mixed handling to avoid SA warning while keeping the fixture’s purpose. |
tests/fixtures/valid/loose-comparison.php |
Adds canary fixture proving == is no longer a CS violation but is SA-caught. |
tests/fixtures/invalid/loose-comparison.php |
Removes invalid loose-comparison fixture in favor of the new canary approach. |
php-cs-fixer-rules.php |
Explicitly disables strict_comparison to prevent silent re-enablement via presets. |
Eventjet/ruleset.xml |
Explicitly disables DisallowEqualOperators via severity=0 for future-proofing. |
composer.json |
Adds PHPStan + Psalm dev dependencies required for the new overlap gate. |
.github/workflows/checks.yml |
Runs the new SA-overlap gate in CI after PHPUnit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CI failed on PHP 8.1 / --prefer-lowest with Psalm flagging nothing while PHPStan flagged the canary correctly. Stderr was suppressed, so we had no signal on why Psalm produced no output. Capture both tools' stderr and include it in the canary-missing error message so the next run tells us. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The SA-overlap gate asks whether PHPStan ∩ Psalm flags the canary fixture. The answer is matrix-independent, so running it on all 8 (php × composer) combinations adds no signal — and on PHP 8.4 + --prefer-lowest the oldest Psalm transitive deps (Symfony Console, Amp DNS, …) emit a flood of "Implicitly marking parameter as nullable is deprecated" warnings that corrupt the JSON output, breaking the gate. Run it once on the canonical config instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If a path in EXPECTED_REL doesn't exist, realpath exits non-zero with no stdout. The script lacked set -e, so `expected` could become empty and both missing/extra checks would silently succeed — a renamed or deleted canary would be "passed" by an effectively no-op gate. Validate every EXPECTED_REL entry with -f before building `expected`, and exit 1 with a clear message if any is missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bare `mktemp` works on GNU coreutils (Linux/CI) but fails on BSD mktemp
(macOS) which requires a template argument. Pass an explicit template
under ${TMPDIR:-/tmp} so the gate is runnable locally on either OS.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes so a jq problem doesn't masquerade as a missing canary: 1. Check up front that jq is in PATH and fail with an explicit message if it isn't, instead of letting empty parse output trigger the "missing canary" error. 2. Append jq's stderr to the per-tool stderr capture files so any JSON-parse errors get printed alongside PHPStan/Psalm stderr in the missing-canary error path — making the real cause visible there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wolfgangschaefer
approved these changes
May 13, 2026
rieschl
approved these changes
May 13, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
rieschl
pushed a commit
that referenced
this pull request
May 18, 2026
- Adds invalid test fixtures for rules that didn't have one yet, plus a per-tool skip list (`RulesTest::SKIPPED_INVALID`) for cases where only one of the two formatters has an equivalent rule. - Enables 14 previously-unconfigured PHP CS Fixer rules and 3 Slevomat sniffs that cover behavior the other tool was already enforcing, dropping 18 skips along the way. - Drops two CS rules (`Squiz.Scope.StaticThisUsage`, `SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration.InvalidFormat`) per the SA-overlap policy from #42 and rewrites 8 other new fixtures so they're no longer in the PHPStan ∩ Psalm intersection enforced by `tests/check-sa-overlap.sh`. - The PHP 4 constructor fixture had to drop its namespace because `no_php4_constructor` is a no-op on namespaced classes.
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.
Summary
loose-comparisonCS rule (slevomatDisallowEqualOperators+ php-cs-fixerstrict_comparison). Both PHPStan (with strict-rules) and Psalm flag==at the strictness levels we now enforce — the CS rule was duplicating lint.tests/fixtures/valid/loose-comparison.php) acts as the proof-of-life for both directions:RulesTest::validasserts phpcs and php-cs-fixer leave it alone (the CS rule really is disabled), andtests/check-sa-overlap.shasserts PHPStan ∩ Psalm contains exactly the registered canary set (the dropping decision is justified).Motivation
#41 introduces a large batch of
invalid/fixtures, and several exercise patterns PHPStan/Psalm already flag at strict settings (unused-use, dead-catch, this-in-static, php4-constructor, etc.). Landing this guardrail first means that batch — and anything similar later — gets caught at review time, and the CS standard doesn't accrete lint-territory rules.How the guardrail works
tests/fixtures/only.tests/phpstan.neonandtests/psalm.xmlwith comments explaining why they're scoped to fixtures. If sniff/fixer sources ever get analyzed, root-level configs can be added without conflict.tests/check-sa-overlap.shruns both tools with JSON output, intersects the flagged-file sets, and asserts the intersection equals exactly theEXPECTED_RELarray (currently just the loose-comparison canary). Missing canary = SA config drift or tool crash; extra entries = lint-territory fixture sneaking in. Future rule drops follow the same one-fixture-in-valid/+ one-line-in-EXPECTED_RELpattern.Why explicitly disable instead of removing?
SlevomatCodingStandard.Operators.DisallowEqualOperatorsis pinned toseverity=0, andstrict_comparisonis set tofalse. Today no enabled preset pulls these in, but if someone later imports@PhpCsFixer/@Symfonyfor php-cs-fixer or a broader Slevomat ruleset for phpcs, those would silently re-enable the rules. Explicit-off prevents that — and the dual-purpose canary fixture would failRulesTest::validif it happened, providing a second layer of defense.Surviving-fixture edits
Three
valid/fixtures triggered real lint issues PHPStan/Psalm correctly identified:psr-12-import-order.php—new stdClass();had no consumer; now assigned and passed totrigger_error.mixed-assignment.php—echo $fooon mixed; switched tovar_export($foo)(accepts mixed). The@var mixedannotation is preserved, which was the actual point of the fixture.no-array-typehint.php— added@param array<int, string>so the iterable value type is known. The fixture still demonstrates anarrayparameter withoutstring[]syntax (its pair,wrong-array-typehint-syntax.php, is still rejected).Test plan
vendor/bin/phpunit— all valid/invalid CS assertions passtests/check-sa-overlap.sh— happy path passes; verified both failure modes (canary removed → "canary missing"; extra both-flagged fixture → "lint-territory" error)