Fix: false-positive InvalidArgument on Nova field callbacks - #12
Merged
alies-dev merged 11 commits intoSep 2, 2026
Merged
Conversation
Nova types four callback APIs wider than what it actually passes at runtime, so the idiomatic narrow closure is rejected as InvalidArgument/ArgumentTypeCoercion: - FieldElement's six visibility setters take the resource as Model|Fluent|object|array; a bounded method-level template lets Psalm infer it from the closure instead. Wrong request class, return type and arity are still reported; cross-model confusion is the accepted trade-off, since a Field cannot know its owning resource. - Filterable::filterable() declares the query as the Eloquent Builder *contract* while always passing the concrete Builder. - AuthorizedToSee::canSee() declares NovaRequest|Request but is only ever given a NovaRequest. - Stack's lines omit Field itself, though the documented usage passes instantiated fields. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
Shells out to Psalm against tests/fixtures and asserts on its JSON report: the idiomatic scenario must be issue-free, the broken one must still raise all three genuine errors. Removing the four stub registrations makes the first test fail with five issues, so the harness measures the fix rather than the fakes. The fake Laravel/Nova declarations are registered as fixture <stubs>, not project files: Psalm refuses stub files for class-likes inside <projectFiles>, and config stubs are scanned before plugin stubs, which is what lets the plugin's stubs win the override. They are also excluded from php-cs-fixer, which otherwise marks their methods final and reformats the vendor docblocks under test. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
List Filterable, AuthorizedToSee and Stack alongside the existing stub set, and fold the cross-model false-negative trade-off (a visibility callback typed against the wrong model still type-checks) into the README so a consumer of the plugin can find it without reading the commit history or the FieldElement stub's docblock. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
still_errors.php only exercised Fix 1 (wrong request class/return type) and Fix 3 (canSee). Fix 2 (Filterable) and Fix 4 (Stack) had no negative coverage, so either could silently widen back to mixed without CI noticing. Add one case per fix: a filter callback typed with the wrong builder, and two Stack lines that are neither class-string<Field>, callable, nor Field. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
Nova's QueriesResources::newQuery() returns an Eloquent Relation (not a concrete Builder) for relationship-index requests and passes it straight into filterable()'s callback. Relation implements the query builder contract but does not extend the concrete Builder class, so a closure narrowed to Builder alone type-checks yet can receive a Relation at runtime and throw a TypeError. Widen the callback's query parameter to a bounded template over Builder|Relation, mirroring FieldElement's pattern, so either concrete shape narrows correctly while a genuinely wrong type is still rejected. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
…d regression #6 AuthorizedToSee is shared by Tool, Dashboard, Filters\Filter and Menu\*, not just Field/Resource classes. Nova's BootTools middleware resolves Tool::canSee() with a plain Illuminate\Http\Request, not a NovaRequest, so narrowing the trait-level canSee() to NovaRequest made Tool::canSee(fn(NovaRequest $r) => ...) type-check even though it can receive a plain Request at runtime. Revert the trait to Nova's original wide NovaRequest|Request union, and redeclare a narrow canSee(): Closure(NovaRequest):bool on FieldElement instead, since field resolution (ResolvesFields) provably always goes through NovaRequest. Every Field subclass inherits the narrow override through FieldElement without affecting Tool/Dashboard/Filter/Menu. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
The caveat understated what the bounded @template TResource trade-off gives up: since the bound spans Model|Fluent|array<array-key,mixed>|object, Psalm accepts any closure whose resource parameter is consistent with that bound, not just one typed against the wrong model. A closure typed stdClass, DateTimeImmutable, or an unrelated array shape type-checks too. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
- clean.php/still_errors.php gain a Relation-typed filterable() case, proving the bounded template accepts it alongside Builder. - A new fake Tool class plus CleanTool/BrokenTool scenarios prove AuthorizedToSee stays sound: a plain-Request closure on Tool still type-checks, and a NovaRequest-only closure on Tool is now correctly rejected (ArgumentTypeCoercion) since the trait is no longer narrowed globally. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
php-cs-fixer's finder only matched *.php, phpcs.xml only scanned src, and format_php.yml's push trigger didn't include **.phpstub, so the "clean" cs-fixer/phpcs results from prior rounds never actually inspected any stub file's formatting. Bring .phpstub under php-cs-fixer's finder (they're plain PHP under a different extension) and disable final_public_method_for_abstract_class project-wide first: it only sees this repo's files, so it can't know laravel/nova itself overrides several of the abstract methods our stubs describe (e.g. Textarea overrides FieldElement::showOnIndex()) - marking a stubbed vendor method final would reintroduce the exact false-positive class this plugin exists to remove. No abstract classes exist in src/ today, so the rule change has no effect there. Applying the fixer then only touches formatting (declare(strict_types=1), union-type spacing, implicit-nullable-parameter deprecation cleanup in Action.phpstub). phpcs.xml intentionally leaves stubs/ unscanned and documents why: several Slevomat/IxDF sniffs (RequireAbstractOrFinal, DisallowMixedTypeHint, DisallowEmptyFunction, InvalidNoReturn) would force changes that make the stubs wrong rather than just reformatted (e.g. RequireAbstractOrFinal would force `final class Panel`, but the real Laravel\Nova\Panel is neither final nor abstract and is legitimately extended). format_php.yml now triggers on **.phpstub regardless, matching psalm.yml, so a future stub-only PR still runs php-cs-fixer. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
alies-dev
marked this pull request as ready for review
September 2, 2026 13:13
…uded #6 Only the tests/fixtures exclusion in .php-cs-fixer.php is load-bearing: without it, cs-fixer wants to reformat the fake Nova fixture files, which would shift the line numbers AcceptanceTest.php asserts on. Scanning .phpstub required disabling final_public_method_for_abstract_class project-wide for no functional gain, since phpcs already excludes stubs/ for the same category of rule conflicts. Reverts that scope instead of carrying the extra config surface. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
CleanTool asserted Tool::canSee(fn(Request)) is accepted, but that holds by closure-param contravariance regardless of whether AuthorizedToSee is narrowed, so it added no regression signal; BrokenTool in still_errors.php already guards the real case. Also collapsed 6 near-identical visibility-setter calls in clean.php (all sharing the same @template bound) down to 2, plus the existing bool-branch case. Dropped declare(strict_types=1) from the 4 new stub files to match the existing stub convention (bare <?php) — meaningless in a stub anyway, since it's never executed. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
alies-dev
deleted the
alies-dev/false-positive-invalidargument-argumenttypecoerc
branch
September 2, 2026 13:40
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.
Context
Field visibility/display callbacks (
showOnDetail,hideFromIndex, etc.),Filterable::filterable(),canSee(), andStack::make()lines all typed their callback/array parameters at the widest possible type Nova's own loose signatures allow (array<array-key,mixed>|object, the contractBuilder,NovaRequest|Request,class-string<Field>|callable). Since callback parameters are contravariant, Psalm rejected any narrower, runtime-correct closure signature a developer wrote by hand, e.g.fn(NovaRequest $request, Post $resource): boolfor aResource<Post>, even thoughPostis exactly what Nova passes at runtime. The only workarounds were widening every closure param back tomixed/objector reaching for@psalm-suppress.Threading the resource's model generic through the callback via a method-hierarchy resolver (Psalm's
MethodParamsProviderInterface) turned out to be a dead end: that provider only matches the concrete called class, and the Field hierarchy is open (user-defined fields), so it can't be enumerated ahead of time.Fixes #6.
Solution
Four new stub files widen the accepted parameter shape without losing detection of genuinely wrong callback signatures:
stubs/Nova/Fields/FieldElement.phpstub— the six visibility setters (showOnIndex,showOnDetail,hideFromIndex,hideFromDetail,hideWhenUpdating,showOnUpdating) get a bounded@template TResource of Model|Fluent|array<array-key,mixed>|objecton the callback's resource parameter, so bothfn(NovaRequest, Post)andfn(NovaRequest, mixed)type-check, while a wrong request class, wrong return type, or extra required parameter still errors. Accepted trade-off, documented in the stub and in the README: since the bound admitsstdClass,DateTimeImmutable, or an unrelated array shape too, a closure typed against the wrong model (or any type consistent with the bound) will also silently pass. This class also gets a narrowcanSee(): Closure(NovaRequest):booloverride, since Field resolution always goes throughNovaRequest.stubs/Nova/Fields/Filterable.phpstub—filterable()'s builder parameter uses a bounded@template TBuilder of Builder|Relation, since Nova passes aRelation(not a concreteBuilder) for relationship-index requests, so both shapes type-check whilestdClassand friends still error.stubs/Nova/AuthorizedToSee.phpstub— left at its original wideNovaRequest|Requestunion. This trait is shared byTool,Dashboard,Filters\Filter, andMenu\*, and Nova'sBootToolsmiddleware handsTool::canSee()a plainRequest, not aNovaRequest— narrowing it here would have reintroduced the same false-positive-turned-runtime-risk this PR is fixing, just relocated onto Tool/Dashboard authors. The narrowing that's actually safe lives onFieldElementinstead (see above).stubs/Nova/Fields/Stack.phpstub—__construct(andmake(), mirrored automatically by the existingNovaMakeSignatureHandler) acceptsFieldinstances directly in the lines array, alongsideclass-string<Field>|callable.Each stub's class/trait header replicates the real declaration's
extendsexactly, since Psalm resets a stubbed class's interface/trait data if the header doesn't match.A new Psalm acceptance test (
tests/AcceptanceTest.php+tests/fixtures/**) runs a minimal fixture project against a small hand-written fake Nova/Illuminate class set:scenarios/clean.phpexercises the idiomatic shapes (including aTool::canSee(fn(Request): bool)case and aRelation-typedfilterable()case) and expects zero issues,scenarios/still_errors.phpcovers the cases that must still be rejected, including aTool::canSee(fn(NovaRequest): bool)case proving the trait-level type wasn't narrowed. The fake Nova classes live in the fixture's own<stubs>config section rather than an autoloader, since Psalm scans config stubs before plugin stubs, letting the plugin's narrower stubs win as they would in a real project; an autoloader-based approach let the wide fakes load first and get overwritten backwards..github/workflows/tests.ymlruns the new suite in CI, andpsalm.ymlscans**.phpstubpaths so a bad stub trips the plugin's own self-check. Neitherphp-cs-fixernorphpcsscansstubs/: several of their rules (forcingfinal class, disallowingmixed/empty method bodies) actively conflict with how a faithful Nova stub has to look, and getting them to pass would mean changing what the stubs assert rather than how they're formatted..php-cs-fixer.phpdoes gain one line excludingtests/fixturesfrom formatting, since the fake Nova fixture classes must keep mirroring the vendor source they stand in for and the scenario files' line numbers are asserted on inAcceptanceTest.php.An earlier version of this PR narrowed
filterable()to concreteBuilderonly andcanSee()toNovaRequesttrait-wide; external review caught that both were unsound for real Nova call paths (relationship-index filtering and Tool/Dashboard respectively), which is why the current shapes look the way they do.https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R