Hardening grid and resource rules, grid-bundle 1.15/1.16 compatibility - #113
Merged
shochdoerfer merged 18 commits intoSep 27, 2026
Merged
Conversation
…n config All three ignoreErrors entries are unmatched and therefore silently suppressed by reportUnmatchedIgnoredErrors: false: - AbstractGridBuilderRule.php does not exist in the codebase at all. - The two 'Cannot call' entries no longer match, since the collectors now hand rules a ClassReflection plus an already-resolved property name instead of a mixed value that gets called. Removing the flag restores the default reportUnmatchedIgnoredErrors: true, so any future ignoreErrors entry that stops matching is reported instead of silently rotting.
The declared ^1.13 floor was never true and had never been exercised, because CI installs the committed lockfile (which resolves 1.15.0) and never runs a lowest-dependency job. Verified by installing each version and running the real test suite: - 1.13.0: fatal error. Sylius\Component\Grid\Attribute\AsGrid does not exist, and AbstractGrid in 1.13 has no default getName(), so the #[AsGrid] test fixtures are abstract-incomplete. - 1.14.0: 1 failure. AsGrid exists, but EnumFilter was only added in 1.15, so the filter fixture's EnumFilter case collects nothing. - 1.15.0 and 1.16.1: 12 tests / 14 assertions pass. Only the lock content-hash is updated; no resolved version changed.
The previous workflow ran a single 'composer install', so CI only ever
exercised whatever the committed lockfile happened to pin. That hid both
the broken ^1.13 floor and any regression in the 1.16 code path.
CI now installs one explicit constraint per lane instead of reusing the
lockfile. Pinning per lane matters: if the lock were kept, bumping it to
1.16 would silently turn the low lane into a second high lane and the
matrix would stop testing anything.
- '^1.15 <1.16' -> resolves 1.15.1, legacy Sylius\Bundle\GridBundle
builder interfaces plus the #[AsGrid] attribute
- '^1.16' -> resolves 1.16.1, Sylius\Component\Grid interfaces
Both lanes verified locally: 12 tests / 14 assertions and a clean
'level: max' run. Added a 'show resolved versions' step so a future
lane failure is diagnosable without re-running composer by hand, and
fail-fast: false so one red lane does not hide the other's result.
Note: 'composer update' resolves the full declared ranges, so the
PHPStan and PHPUnit patch versions now float in CI. That is intended:
this package is an extension whose job is to support a range, and the
committed lockfile remains the local-development pin.
Both rules called Type::getValue() on an attribute argument behind a '@var array<string, ConstantStringType>' annotation. The annotation was an unchecked assumption, not a guarantee, so any argument that is not a constant string aborted the entire run: Internal error: Call to undefined method PHPStan\Type\ObjectType::getValue() Because a PHPStan internal error invalidates the whole result ('Result is incomplete because of severe errors'), one bad attribute suppressed every other finding in the project, including the unrelated collector-based grid rules. Reproduced pre-fix, both arguments crash the analysis: #[AsResource(formType: new stdClass())] ObjectType #[Index(grid: [self::class])] ConstantArrayType #[AsResource(formType: self::class)] fine The rules now accept only a single constant string, via getConstantStrings(), and skip everything else. A non-string argument is not resolvable to a class, so skipping is the honest answer. Behaviour change: #[AsResource(formType: 123)] previously reported 'Form Type "123" not found!'; it is now skipped, because an int is not a class-string and reporting it as a missing class was misleading. IndexOperationNeedsGridClassRule had no test at all, so this fix also adds the first coverage for it. New fixtures are registered in autoload-dev.files, as the other fixtures are. 15 tests / 17 assertions, level: max clean. Reverting the fix makes the new tests error out, so they genuinely cover the regression.
The rule listened on MethodReturnStatementsNode, which is emitted once per method, but #[AsGrid(resourceClass: ...)] is a class-level attribute. Three consequences: 1. A grid declaring no method was never visited, so its resource class was never validated at all. 2. The error was anchored to whichever method happened to be visited first. The existing attribute test expected line 24, which is the buildGrid() method, rather than line 21 where #[AsGrid] actually is. 3. AbstractGrid was required before the attribute was read, so a grid that does not extend the legacy base class had its resource class skipped entirely. Since grid-bundle 1.16 a grid may implement Sylius\Component\Grid\GridInterface instead, and Sylius deprecated Sylius\Bundle\GridBundle\Grid\GridInterface with 'will be removed in 2.0'. This is the direction the framework is moving, not an exotic case. The rule now listens on InClassNode, so it runs exactly once per class, and the attribute is checked without any hierarchy gate: the attribute is what makes the class a grid. The legacy getResourceClass() path keeps its gate on AbstractGrid or ResourceAwareGridInterface, so an unrelated class with a similarly named method is still ignored. The error is now reported on the #[AsGrid(...)] attribute itself, or on the getResourceClass() declaration for the legacy path. Also guards the attribute argument with getConstantStrings(), matching the fix for the resource rules. The old code indexed getConstantStrings()[0] directly, so a non-constant resourceClass raised 'Undefined array key 0' and aborted the analysis. The legacy method body is read from the AST rather than from the resolved method return type: a 'getResourceClass(): string' returning Supplier::class has the plain type 'string' while treatPhpDocTypesAsCertain is false, which would lose the class name. Verified: 17 tests pass on both lanes, 1.15 skips the >= 1.16 only test and 1.16 runs it, level: max clean on both. Reverting the rule makes the new tests fail, so they cover the regression.
GridBuilderFieldIsPartOfResourceClass resolved the resource class once per grid and then reused that variable for every field. The recursive branch replaces it with the type of the segment it just resolved, so once a dotted field such as "address.city" had been walked, every following field was validated against App\Entity\Address instead of App\Entity\Supplier. The recursive walk also kept the resolved PHPStan Type around and called ClassReflection methods on it. Type::hasProperty() is declared on Type and answers with a TrinaryLogic, which casts to bool as true, so !$resourceClass->hasProperty($name) was always false. Every segment past the first one of a recursive field therefore passed silently, and so did every field that followed a recursive field. Walking a field path now resolves each segment back to a ClassReflection, and the resource class is re-resolved for each field. The type of a segment is only narrowed down when exactly one class name can be determined, so an unresolvable path reports the existing "Unable to identify the type" error instead of passing. Adds fixtures for a field declared after a recursive field and for a missing third segment of a recursive field. Neither was reported before this change.
composer show accepts a single package plus an optional version, so passing three package names aborted with "Too many arguments" and exited 1. The step failed every CI run on both lanes.
Filter::supports() asked whether the class the static call was made on was a supertype of FilterInterface, which asks the opposite of what was intended. The result was never yes, so the node matched nothing and no filter without a dedicated node was ever collected. On grid-bundle 1.15 that silently skipped Filter, BooleanFilter, DateFilter and MoneyFilter. The node also only knew the old bundle interface. From 1.16 on the concrete filter factories return Sylius\Component\Grid\Builder\Filter\FilterInterface, so both interfaces are now checked, the same way the collectors accept both. The registry asks every node until one supports the filter, so once the generic node started matching it had to be registered after the concrete ones. Its getFilterFields() only reads the first argument, so leaving it in front of StringFilter and SelectFilter would have hidden the field array those nodes read from their later arguments. extension.neon now registers it last and says why. Adds a Filter::create() case to the grid fixture; it was not reported before this change.
The field collector only accepted a static call named create(). CallableField::createForService() builds the same kind of field for a service-backed callable and takes the field name as its first argument, exactly like create(), so it was dropped before any field node saw it and the field was never validated against the resource class. Both factory names are now accepted. This is the only alternative factory among the grid-bundle field and filter classes; every other one uses create(), so the filter collector is left as it is. Adds a createForService() case to the grid fixture; it was not reported before this change.
getCollectors() builds its own registry, so it did not follow the reordering of the catch-all node. Leaving the two out of step would let the tests pass against an order the extension does not ship. Note that the built-in outcome does not depend on the order: only Filter implements FilterInterface, so the catch-all node and the name-based nodes match disjoint sets. The order matters for custom nodes registered for custom filter classes, which the catch-all would otherwise shadow. That case has no automated coverage yet.
These three built-in filters were never validated. They are plain final classes with a static create() that returns FilterInterface, but they do not themselves implement FilterInterface, so the catch-all Filter node could not match them and they had no dedicated node. A grid using them was silently unchecked. All three take the field name as the first create() argument, so each node is a plain name match plus the shared snake_case conversion. MoneyFilter's second argument is the currency code and is deliberately ignored, like EnumFilter's option arguments. Verified the new nodes are load-bearing: pointing one FILTER_TYPE at a wrong class name makes the corresponding expectation disappear.
Most of the drift was in the grid layer, and it was actively misleading rather than merely incomplete: - Both grid rules were documented as Rule<CollectedDataNode>, which is right, while their own @implements Rule<StaticCall> docblocks are wrong. The file now flags that getNodeType() is authoritative and logs the stale docblock as Known Issue 2. - The new-namespace "limitation" was wrong. grid-bundle 1.16 moved the interfaces, not the concrete classes, so matching the bundle namespace is correct on both lanes. Added a Grid Bundle 1.16 section explaining why there is nothing in the Component namespace to match. - The Filter node inversion was still listed as a live bug and as a "Bug?" in the node table. It is fixed; replaced with the registry ordering constraint, and the table now carries the real create() signatures instead of "likely" guesses. - ResourceAwareGridNeedsResourceClass was listed as MethodReturnStatementsNode. It is InClassNode and checks the #AsGrid attribute before any method. - Known Issue 6 pointed at the wrong fixture for the lowercase class. - BooleanFilter, DateFilter and MoneyFilter are missing from the node lists, and the field collector now accepts createForService. Added the C4 per-field re-resolution invariant, the fixture-sharing and line-number traps, and the requirement to prove a new node is actually load-bearing before trusting a green suite.
Both rules declared @implements Rule<StaticCall> while getNodeType() returns CollectedDataNode::class. The redundant `if (!$node instanceof CollectedDataNode)` guard narrowed $node back to the correct type inside the method body, which is precisely why the wrong annotation never produced a PHPStan error: the guard was hiding it. Removing the guard without fixing the docblock makes $node->get() fail as an undefined method on PhpParser\Node. The parameter stays typed as Node because Rule::processNode() declares it that way and narrowing a parameter is a contravariance violation. Verified on grid-bundle ^1.15 (17 tests/18 assertions/1 skipped) and ^1.16 (17 tests/19 assertions), PHPStan level max clean on both.
Known Issue 2 described a defect that d73d56a fixed, so it is replaced by the underlying trap, which is still live for anyone adding a rule: a defensive instanceof guard re-narrows the parameter and silently suppresses a wrong @implements generic from PHPStan entirely.
PHPStan's Rule interface declares `@return class-string<TNodeType>`. Three rules overrode that with a bare `class-string`, which widens the type and throws the generic away, and the two Resource rules had no annotation at all. All five now carry `class-string<CollectedDataNode>` or `class-string<InClassNode>`. This is a safety net, not just tidying: with the generic preserved, PHPStan now rejects a getNodeType() that disagrees with the @implements annotation. Verified by pointing one rule's getNodeType() at a different node class, which produces "should return class-string<...> but returns string". That is the exact class of bug fixed in d73d56a, previously invisible until a runtime failure. Also drops an unused catch variable in the field rule.
The catch-all Filter node matches anything implementing FilterInterface, so being registered before a more specific node silently shadows it. Nothing tested that, because the rule tests hand-build their own registry and never read extension.neon. Adds a user-style filter that implements FilterInterface, plus a grid whose single CustomFilter::create() call yields a different field depending on which node resolves it. Three cases: * catch-all alone resolves the class, proving the fixture is wired up * dedicated node before catch-all -> dedicated node wins * catch-all before dedicated node -> catch-all shadows it The dedicated node reports 'customNodeField' while the catch-all reports the create() argument, so a passing assertion can only come from the intended node. A fourth test parses extension.neon and asserts the catch-all is registered last, which is the invariant that actually ships. CustomFilter.php implements every interface method rather than stubbing them, and is intentionally left in the PHPStan scope: on grid-bundle 1.16 the legacy FilterInterface extends the new Component one, so analysing it is what proves the implementation still holds on that lane.
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.
Hardens the grid and resource rules, closes validation gaps that left several Sylius filter types unchecked, and gives the grid-bundle 1.15 → 1.16 interface move explicit test coverage on both sides.
Raised the supported grid-bundle floor to ^1.15 and split CI into explicit ^1.15 <1.16 and ^1.16 lanes, each pinned to its own constraint so the low lane can't silently drift to the high one.
The generic catch-all filter node now recognises both the legacy and the 1.16 FilterInterface, which otherwise stopped matching on 1.16.