Add invalid fixtures and close per-tool rule gaps#41
Merged
Conversation
Adds the snake-case test fixtures that were already covered as PascalCase files plus many new ones, and wires up RulesTest::SKIPPED_INVALID so each fixture can be skipped per tool when one of the two formatters has no equivalent rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds twelve previously-unconfigured PHP CS Fixer rules and three Slevomat sniffs that cover behavior we were already testing for via PHPCS or PHP CS Fixer respectively, removing 16 entries from RulesTest::SKIPPED_INVALID. self_accessor moves from the strict ruleset to the basic one because the test harness runs both tools against the basic config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fixer rule is a no-op on namespaced classes (PHP 4 constructors are meaningless there), so the fixture also has to drop its namespace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
rieschl
approved these changes
May 13, 2026
# Conflicts: # Eventjet/ruleset.xml
PHPStan and Psalm both flag `$this` in static methods and malformed inline `@var` annotations, so CS rules for the same patterns duplicate lint work. Disable them on both sides: - `Squiz.Scope.StaticThisUsage` set to severity 0 - `SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration` keeps its other codes (InvalidCommentType, MissingVariable, NoAssignment — genuine format/structure checks) but excludes InvalidFormat - `phpdoc_var_annotation_correct_order` set to false so the fixer doesn't silently canonicalize the same shape away Move both fixtures from invalid/ to valid/ and register them in EXPECTED_REL so the SA-overlap gate asserts both SAs keep flagging them. Rename this-in-static.php to ThisInStatic.php to satisfy Squiz.Classes.ClassFileName. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These fixtures previously had constant or trivially-reducible operands
(e.g. `if(true)`, `'bar' === 'bar'`, `true AND false`) which both PHPStan
and Psalm flag — putting them in the PHPStan ∩ Psalm intersection that
tests/check-sa-overlap.sh enforces as the "drop the CS rule" signal.
The CS rules these fixtures exercise are NOT being dropped — only the
fixture content is reshaped so the SAs lose the ability to fold the
expression statically, while the CS-side violation (missing space,
`else if`, `AND`, yoda, PHP4 constructor, unsorted uses, etc.) stays
intact.
- control-signature.php: `if(true)` → `if($cond)` from bool param
- else-if.php: $foo from int param, no longer constant
- logical-operator-and.php: `true AND false` → `$a AND $b` inside if()
to also dodge "unused result of and"
- logical-operator-spacing.php: `true&&false` → `$a&&$b` returned from
a function
- missing-space-after-construct.php: added `@return iterable<int, int>`
so the yielded value is no longer mixed
- php4-constructor.php: added `: void` return type
- unsorted-uses.php: dropped the unreachable second throw
- yoda-comparison.php: $foo from string param, no longer constant
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens parity between PHPCS and PHP CS Fixer by (1) adding missing invalid fixtures and (2) enabling rules/sniffs that were already effectively enforced by the other tool, while introducing a per-tool skip mechanism for invalid fixtures where no equivalent rule exists.
Changes:
- Extend
RulesTestwith a per-tool invalid-fixture skip list (SKIPPED_INVALID) to handle rule gaps between PHPCS and PHP CS Fixer. - Enable additional PHP CS Fixer rules and Slevomat sniffs to reduce tool drift and remove unnecessary skips.
- Add many new invalid fixtures plus two “canary” valid fixtures used to ensure dropped CS rules are still caught by static analysis (PHPStan + Psalm).
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/RulesTest.php | Adds/extends per-tool invalid fixture skip list to cover formatter rule gaps. |
| tests/check-sa-overlap.sh | Registers additional dropped-rule canary fixtures expected in PHPStan ∩ Psalm. |
| php-cs-fixer-rules.php | Enables additional fixer rules (and explicitly disables one phpdoc canonicalization case). |
| php-cs-fixer-strict-rules.php | Moves self_accessor out of strict-only overrides (now defined in base rules). |
| Eventjet/ruleset.xml | Explicitly disables $this-in-static sniff and excludes a specific InlineDocComment code; enables additional Slevomat sniffs. |
| tests/fixtures/valid/ThisInStatic.php | Adds canary fixture ensuring CS ignores $this in static methods while SA flags it. |
| tests/fixtures/valid/inline-doc-comment.php | Adds canary fixture ensuring CS doesn’t auto-canonicalize malformed inline @var, while SA flags it. |
| tests/fixtures/invalid/array-bracket-spacing.php | Adds invalid fixture for bracket spacing inside array access. |
| tests/fixtures/invalid/array-not-trimmed.php | Adds invalid fixture for single-line array whitespace trimming. |
| tests/fixtures/invalid/assignment-in-condition.php | Adds invalid fixture for assignment in condition (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/blank-line-after-phpdoc.php | Adds invalid fixture for blank line after phpdoc (fixer-only; skipped for PHPCS). |
| tests/fixtures/invalid/bracketed-namespace.php | Adds invalid fixture for bracketed namespace syntax (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/catch-exception.php | Adds invalid fixture for catching Exception instead of Throwable (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/control-signature.php | Adds invalid fixture for missing space in control structure signature. |
| tests/fixtures/invalid/dead-catch.php | Adds invalid fixture for unreachable catch ordering (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/deprecated-function.php | Adds invalid fixture for deprecated function usage (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/else-if.php | Adds invalid fixture for else if instead of elseif. |
| tests/fixtures/invalid/empty-comment.php | Adds invalid fixture for empty comment. |
| tests/fixtures/invalid/empty-phpdoc.php | Adds invalid fixture for empty phpdoc block. |
| tests/fixtures/invalid/extra-blank-lines.php | Adds invalid fixture for extra blank lines. |
| tests/fixtures/invalid/forbidden-author-annotation.php | Adds invalid fixture for forbidden phpdoc annotations. |
| tests/fixtures/invalid/forbidden-class-comment.php | Adds invalid fixture for “Class X” style class doc comment (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/forbidden-function-sizeof.php | Adds invalid fixture for forbidden alias functions (e.g., sizeof). |
| tests/fixtures/invalid/group-use.php | Adds invalid fixture for disallowed group use declaration. |
| tests/fixtures/invalid/logical-operator-and.php | Adds invalid fixture for AND logical operator. |
| tests/fixtures/invalid/logical-operator-spacing.php | Adds invalid fixture for missing spaces around &&. |
| tests/fixtures/invalid/long-array-syntax.php | Adds invalid fixture for long array syntax (array(...)). |
| tests/fixtures/invalid/long-cast.php | Adds invalid fixture for long cast syntax. |
| tests/fixtures/invalid/long-type-hint.php | Adds invalid fixture for long scalar type names in phpdoc (e.g., integer). |
| tests/fixtures/invalid/missing-const-visibility.php | Adds invalid fixture for missing class constant visibility. |
| tests/fixtures/invalid/missing-declare-strict-types.php | Adds invalid fixture for missing declare(strict_types=1). |
| tests/fixtures/invalid/missing-space-after-construct.php | Adds invalid fixture for missing space after language construct (e.g., yield(1)). |
| tests/fixtures/invalid/multiline-double-arrow.php | Adds invalid fixture for multiline => whitespace (fixer-only; skipped for PHPCS). |
| tests/fixtures/invalid/multiple-namespaces.php | Adds invalid fixture for multiple namespaces per file (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/multiple-uses-per-line.php | Adds invalid fixture for multiple use statements per line. |
| tests/fixtures/invalid/new-without-parentheses.php | Adds invalid fixture for new without parentheses. |
| tests/fixtures/invalid/no-early-exit.php | Adds invalid fixture for lack of early exit. |
| tests/fixtures/invalid/no-space-around-equals.php | Adds invalid fixture for missing spaces around =. |
| tests/fixtures/invalid/non-static-data-provider.php | Adds invalid fixture for non-static PHPUnit data provider. |
| tests/fixtures/invalid/nullable-type-for-null-default.php | Adds invalid fixture requiring nullable type when default is null. |
| tests/fixtures/invalid/parameter-type-hint-spacing.php | Adds invalid fixture for parameter typehint spacing. |
| tests/fixtures/invalid/php4-constructor.php | Adds invalid fixture for PHP 4 constructor. |
| tests/fixtures/invalid/phpdoc-not-trimmed.php | Adds invalid fixture for leading/trailing blank lines in doc comment. |
| tests/fixtures/invalid/property-declaration.php | Adds invalid fixture for property declaration modifier order. |
| tests/fixtures/invalid/return-type-hint-spacing.php | Adds invalid fixture for return typehint spacing. |
| tests/fixtures/invalid/self-member-reference.php | Adds invalid fixture for non-self:: member reference within class. |
| tests/fixtures/invalid/snake-case-variable.php | Adds invalid fixture for snake_case variable naming (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/space-around-object-operator.php | Adds invalid fixture for spaces around ->. |
| tests/fixtures/invalid/space-before-semicolon.php | Adds invalid fixture for space before semicolon. |
| tests/fixtures/invalid/superfluous-elseif.php | Adds invalid fixture for superfluous elseif. |
| tests/fixtures/invalid/text-before-open-tag.php | Adds invalid fixture for content before <?php (PHPCS-only; skipped for fixer). |
| tests/fixtures/invalid/trailing-comma-singleline.php | Adds invalid fixture for trailing comma in single-line array (fixer-only; skipped for PHPCS). |
| tests/fixtures/invalid/trailing-whitespace.php | Adds invalid fixture for trailing whitespace. |
| tests/fixtures/invalid/union-type-with-spaces.php | Adds invalid fixture for spaces around union type operators. |
| tests/fixtures/invalid/unnecessary-final-modifier.php | Adds invalid fixture for final method inside final class. |
| tests/fixtures/invalid/unsorted-uses.php | Adds invalid fixture for unsorted imports. |
| tests/fixtures/invalid/unused-closure-use.php | Adds invalid fixture for unused variables in closure use (...). |
| tests/fixtures/invalid/unused-use.php | Adds invalid fixture for unused import. |
| tests/fixtures/invalid/use-from-same-namespace.php | Adds invalid fixture for importing from same namespace. |
| tests/fixtures/invalid/use-leading-backslash.php | Adds invalid fixture for leading backslash in use statement. |
| tests/fixtures/invalid/useless-else.php | Adds invalid fixture for useless else. |
| tests/fixtures/invalid/useless-if-with-return.php | Adds invalid fixture for if-return pattern that can be simplified. |
| tests/fixtures/invalid/useless-parentheses.php | Adds invalid fixture for useless parentheses. |
| tests/fixtures/invalid/uppercase-php-function.php | Adds invalid fixture for uppercase native PHP function calls. |
| tests/fixtures/invalid/uppercase-type-hint.php | Adds invalid fixture for uppercase scalar type hints. |
| tests/fixtures/invalid/whitespace-after-comma.php | Adds invalid fixture for missing whitespace after commas in arrays. |
| tests/fixtures/invalid/yoda-comparison.php | Adds invalid fixture for Yoda comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rieschl
approved these changes
May 13, 2026
The --prefer-lowest CI job was failing on PHP 8.3 because: - slevomat/coding-standard 8.4 lacks the DNFTypeHintFormat sniff referenced in Eventjet/ruleset.xml. The sniff was added in 8.16. - friendsofphp/php-cs-fixer 3.32 caps supported PHP at 8.2 and trips the tokenizer on PHP 8.3+ even with PHP_CS_FIXER_IGNORE_ENV=1. 3.65 raises the cap to 8.3 and runs cleanly on 8.4 under the env override. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rieschl
approved these changes
May 18, 2026
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
RulesTest::SKIPPED_INVALID) for cases where only one of the two formatters has an equivalent rule.Squiz.Scope.StaticThisUsage,SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration.InvalidFormat) per the SA-overlap policy from Drop loose-comparison rule; add PHPStan + Psalm fixture guardrail #42 and rewrites 8 other new fixtures so they're no longer in the PHPStan ∩ Psalm intersection enforced bytests/check-sa-overlap.sh.no_php4_constructoris a no-op on namespaced classes.Why now
This came out of working out exactly which rules each tool already covers and which gaps are real vs. just unset. That mapping is the input we need for the
magobranch — the closer our PHPCS + PHP CS Fixer configs are to enforcing the same set of rules, the more confidently we can shape Mago's rule set to match what's already in place, and the less drift we'll have to clean up afterwards. The new test scaffolding (per-tool skip list) also extends cleanly to a third tool column.Drops driven by the SA-overlap gate
After merging master (which brought in #42's
tests/check-sa-overlap.sh), two new fixtures in this branch hit the gate — both PHPStan and Psalm already flagged the pattern, so a CS rule for it was lint-duplicate:Squiz.Scope.StaticThisUsage—$thisin a static method. Dropped, fixture moved tovalid/ThisInStatic.phpas a dual-purpose canary (asserts the CS rule is off AND that both SAs still catch it).SlevomatCodingStandard.Commenting.InlineDocCommentDeclaration.InvalidFormat— malformed@var $name Typeorder. Narrowly excluded (the sniff's three other sub-codes —InvalidCommentType,MissingVariable,NoAssignment— are genuine structural checks and stay enabled). PHP CS Fixer'sphpdoc_var_annotation_correct_orderis set tofalseto match, so the wrong-order form isn't silently canonicalized away before the SAs see it. Fixture moved tovalid/inline-doc-comment.phpas a canary.Eight other new fixtures (
control-signature,else-if,logical-operator-and,logical-operator-spacing,missing-space-after-construct,php4-constructor,unsorted-uses,yoda-comparison) tripped the gate because their bodies were statically reducible (if(true),'bar' === 'bar',true AND false, an unreachable secondthrow, etc.). The CS rules they exercise are kept; the fixtures are rewritten so the SAs can no longer fold the expression while the CS-relevant violation stays intact.No new valid fixtures except the two canaries
The existing valid fixtures (
single-quotes.php,cast-no-space.php,psr-12-import-order.php,mixed-assignment.php, etc.) collectively pin down "a well-formed file under our standard," and they're run through both tools on every test invocation. That baseline already proves the canonical positive form for the rules added here — none of the 14 fixer rules or 3 sniffs introduce a non-trivial interaction that the existing valid set doesn't cover. The convention in this repo is that valid fixtures exist where the correct form is non-obvious or configuration-dependent (e.g., proving that omitting a@return arraytag is acceptable); pinning down the trivial positive form for each new rule would invert that convention. The two newvalid/entries (ThisInStatic.php,inline-doc-comment.php) are canaries for dropped rules, not canonical-form fixtures, and they're registered inEXPECTED_RELintests/check-sa-overlap.sh. Positive fixtures for Mago will be added in the Mago PR for the cases Mago actually disagrees on, gated by the existingSKIPPED_VALIDmechanism on that branch.Test plan
vendor/bin/phpunit— 206 tests, 18 skipped, all passingbash tests/check-sa-overlap.sh— green🤖 Generated with Claude Code