Skip to content

Commit 9f7e5b2

Browse files
committed
fix: replace inert canSee() stub override with a working storage rewrite
External review (post-merge) found that the merged PR's fix 3 was dead code: canSee() is declared only on the AuthorizedToSee trait, which FieldElement merely inherits (via Element), and a Psalm plugin stub cannot override a method the stubbed class only inherits from a used trait. Confirmed empirically against real Nova 5.10.1 — the original false positive was still reproducible on canSee() after the "fix" shipped, and MethodParamsProviderInterface (the review's suggested alternative) is also a dead end for the same open-hierarchy reason already established for the resource-model-generic problem. The programmatic mechanism this repo already uses elsewhere for "narrow because we know more than the vendor signature admits" cases does work: NovaFieldAuthorizationHandler walks every FieldElement descendant post-populate and points its own declaring_method_ids at a narrowed clone of canSee(), the same storage fields Methods::getDeclaringMethodId() actually reads. Tool/Dashboard/ Filters\Filter/Menu\* share the same trait but are never touched, so their canSee() stays wide. AuthorizedToSee.phpstub is deleted — it redeclared canSee() with the exact type Nova already has, a no-op that happened to survive review because it changed nothing to break. Also fixes the two masking factors that let this ship in the first place: the test fixture registered fake Nova via psalm.xml's <stubs> config, where config stubs are scanned before a plugin's own addStubFile() stubs and always "win" regardless of whether the override mechanism actually works against normally-scanned code. Fake Nova now reaches Psalm only through the composer autoload-dev classmap, exactly as a real Nova app's vendor/ would, so the suite exercises the plugin's actual registration path. AcceptanceTest.php's issue assertions are now keyed on selected_text instead of line number (a leading comment shifting every later line number was a standing footgun), stderr is captured to a temp file instead of a second pipe (sequential pipe draining can deadlock), and the exit code is asserted against Psalm's real {0 = clean, 2 = issues found} rather than being silently discarded. Two smaller findings from the same review, addressed in place: Filterable's Builder|Relation template accepts a closure typed against only one shape even for a field reachable via both request kinds — documented as an explicit trade-off in the stub and README rather than reverted, since the shared contract type remains available for a field that needs both-safe. FieldElement's bounded template is also stricter than Nova's own docblocks for setters with no upstream @phpstan-param (hideFromIndex et al.) — documented, with a fixture regression case. Claude-Session: https://claude.ai/code/session_01FzaFiRNezfiiLYieQifR6R
1 parent 173fa03 commit 9f7e5b2

11 files changed

Lines changed: 203 additions & 66 deletions

File tree

.github/workflows/tests.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ on:
55
paths:
66
- '**.php'
77
- '**.phpstub'
8+
- 'tests/fixtures/**'
89
- 'phpunit.xml.dist'
910
- 'composer.json'
1011
- '.github/workflows/tests.yml'

README.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ Hook methods that override a base declaration (`fields()`, `apply()`, `calculate
6060

6161
### Nova stubs
6262

63-
The plugin ships stubs for `Action`, `Field`, `FieldElement`, `Element`, `PartitionResult`, `Panel`, `Resource`, `Filterable`, `AuthorizedToSee` and `Stack` that fix vendor signatures Psalm cannot resolve. `Resource` is templated, so a resource can declare its model with `@extends`:
63+
The plugin ships stubs for `Action`, `Field`, `FieldElement`, `Element`, `PartitionResult`, `Panel`, `Resource`, `Filterable` and `Stack` that fix vendor signatures Psalm cannot resolve. `Resource` is templated, so a resource can declare its model with `@extends`:
6464

6565
```php
6666
/** @extends \Laravel\Nova\Resource<\App\Models\User> */
@@ -72,7 +72,11 @@ final class User extends Resource
7272

7373
The stubs are registered by the plugin itself; no `<stubs>` entry is needed in `psalm.xml`.
7474

75-
`FieldElement`'s visibility callbacks (`showOnIndex()`, `showOnDetail()`, `hideFromIndex()`, …) are narrowed through a bounded template rather than a fixed union, so a closure typed against the resource's own model (`fn(NovaRequest $request, Post $post): bool`) is accepted instead of being rejected as too narrow. This is a deliberate trade-off, and it is wider than just wrong-model confusion: any type consistent with the bound (`Model|Fluent|array<array-key, mixed>|object`) is accepted for the resource parameter, so a closure typed against the *wrong* model (`fn(NovaRequest $request, Comment $comment)` on a field that only ever appears on `Post`) still type-checks, and so does one typed `stdClass`, `DateTimeImmutable`, or an unrelated array shape. Wrong request classes, wrong return types and wrong arity are still reported.
75+
`FieldElement`'s visibility callbacks (`showOnIndex()`, `showOnDetail()`, `hideFromIndex()`, …) are narrowed through a bounded template rather than a fixed union, so a closure typed against the resource's own model (`fn(NovaRequest $request, Post $post): bool`) is accepted instead of being rejected as too narrow. This is a deliberate trade-off, and it is wider than just wrong-model confusion: any type consistent with the bound (`Model|Fluent|array<array-key, mixed>|object`) is accepted for the resource parameter, so a closure typed against the *wrong* model (`fn(NovaRequest $request, Comment $comment)` on a field that only ever appears on `Post`) still type-checks, and so does one typed `stdClass`, `DateTimeImmutable`, or an unrelated array shape. Wrong request classes, wrong return types and wrong arity are still reported. This also makes the plugin *stricter* than Nova's own docblocks for the setters Nova types with a bare `mixed` second parameter (Nova only narrows some of them itself, via `@phpstan-param`): the bound now catches a resource parameter typed as something unrelated (e.g. `int`) that Nova's own bare `mixed` wouldn't have flagged.
76+
77+
`canSee()` is narrowed to `NovaRequest` for every `Field`-derived class, but not through a stub: it's declared only on the `AuthorizedToSee` trait, which `Field`/`FieldElement` merely inherit, and a stub cannot override a method the stubbed class only inherits from a used trait. The plugin rewrites it programmatically post-populate instead, scoped to the `Field` hierarchy only — `Tool`, `Dashboard`, `Filters\Filter` and `Menu\*` share the same trait but can receive a plain `Illuminate\Http\Request` at runtime, so their `canSee()` stays wide.
78+
79+
`Filterable::filterable()`'s query parameter accepts either a concrete `Illuminate\Database\Eloquent\Builder` or a `Relation`, since Nova passes a `Relation` for relationship-index requests. Typing a closure against only one of the two type-checks even for a field that's reachable through both request kinds — type against the shared `Illuminate\Contracts\Database\Eloquent\Builder` contract instead if a field needs to be safe against both.
7680

7781
## Requirements
7882

composer.json

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@
2828
"autoload-dev": {
2929
"psr-4": {
3030
"InteractionDesignFoundation\\PsalmLaravelNova\\Tests\\": "tests/"
31-
}
31+
},
32+
"classmap": [
33+
"tests/fixtures/fake-nova/"
34+
]
3235
},
3336
"config": {
3437
"allow-plugins": {
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace InteractionDesignFoundation\PsalmLaravelNova;
4+
5+
use Psalm\Codebase;
6+
use Psalm\Internal\MethodIdentifier;
7+
use Psalm\Plugin\EventHandler\AfterCodebasePopulatedInterface;
8+
use Psalm\Plugin\EventHandler\Event\AfterCodebasePopulatedEvent;
9+
use Psalm\Storage\ClassLikeStorage;
10+
use Psalm\Storage\MethodStorage;
11+
use Psalm\Type\Atomic\TClosure;
12+
use Psalm\Type\Atomic\TNamedObject;
13+
use Psalm\Type\Union;
14+
15+
/**
16+
* Narrows `canSee()`'s callback parameter to `NovaRequest` for every `Field`-derived class, without
17+
* touching `Tool`/`Dashboard`/`Filters\Filter`/`Menu\*`, which share the same `AuthorizedToSee` trait
18+
* but can receive a plain `Illuminate\Http\Request` at runtime (`BootTools` middleware).
19+
*
20+
* A stub file cannot do this: `canSee()` is declared only on the `AuthorizedToSee` trait, and
21+
* `FieldElement`/`Field`/every concrete field class only *inherit* it (no class in that chain
22+
* redeclares it). A plugin stub can override a method the stubbed class itself declares, and can
23+
* add a genuinely new one, but — confirmed empirically against real Nova 5.10.1, redeclaring
24+
* `canSee()` on `Element.phpstub` (which actually `use`s the trait) or on `FieldElement.phpstub`
25+
* (which merely inherits it) — it cannot override a method the class only inherits: Psalm's
26+
* `Methods::getMethodParams()` resolves the call through `getDeclaringMethodId()`, which reads
27+
* `declaring_method_ids['cansee']` off the *called* class's own storage; that entry still points at
28+
* `AuthorizedToSee`/`Element` regardless of what the stub adds, so the stub's declaration is simply
29+
* never consulted. `MethodParamsProviderInterface` cannot fill the gap either: Psalm keys it by the
30+
* exact called class (`Methods::getMethodParams()`, `AtomicMethodCallAnalyzer::$fq_class_name`), with
31+
* no hierarchy walk, so it would need to enumerate every concrete Field subclass — impossible for an
32+
* open, user-extensible hierarchy (the same reason `MethodParamsProviderInterface` was already ruled
33+
* out for resolving a resource's model, see `NovaResourceQueryMethodHandler`).
34+
*
35+
* What does work, because it operates on the same storage fields `getDeclaringMethodId()` actually
36+
* reads: post-populate, for every class extending `FieldElement`, point that class's own
37+
* `declaring_method_ids['cansee']` at itself and give it its own `methods['cansee']` entry — a
38+
* narrowed clone of whatever `AuthorizedToSee::canSee()` currently declares. This is exactly what a
39+
* real `public function canSee(...)` override on that class would produce in storage, just built
40+
* programmatically instead of textually. Classes outside the `FieldElement` hierarchy are never
41+
* touched, so `Tool::canSee(fn(Request $request): bool => true)` keeps type-checking and
42+
* `Tool::canSee(fn(NovaRequest $request): bool => true)` keeps being rejected.
43+
* @internal
44+
*/
45+
final class NovaFieldAuthorizationHandler implements AfterCodebasePopulatedInterface
46+
{
47+
private const FIELD_ELEMENT = 'laravel\nova\fields\fieldelement';
48+
49+
private const CAN_SEE = 'cansee';
50+
51+
private const NOVA_REQUEST = 'Laravel\Nova\Http\Requests\NovaRequest';
52+
53+
#[\Override]
54+
public static function afterCodebasePopulated(AfterCodebasePopulatedEvent $event): void
55+
{
56+
$codebase = $event->getCodebase();
57+
58+
foreach ($codebase->classlike_storage_provider::getAll() as $storage) {
59+
$isFieldElement = mb_strtolower($storage->name) === self::FIELD_ELEMENT
60+
|| isset($storage->parent_classes[self::FIELD_ELEMENT]);
61+
if (!$isFieldElement) {
62+
continue;
63+
}
64+
65+
self::narrowCanSee($codebase, $storage);
66+
}
67+
}
68+
69+
private static function narrowCanSee(Codebase $codebase, ClassLikeStorage $storage): void
70+
{
71+
$declaringId = $storage->declaring_method_ids[self::CAN_SEE] ?? null;
72+
if ($declaringId === null
73+
|| mb_strtolower($declaringId->fq_class_name) === mb_strtolower($storage->name)
74+
|| !$codebase->classlike_storage_provider->has($declaringId->fq_class_name)
75+
) {
76+
// No canSee() to narrow, or the class already declares its own (leave user intent alone).
77+
return;
78+
}
79+
80+
$declaringStorage = $codebase->methods->getStorage($declaringId);
81+
$narrowedCallback = self::narrowCallbackParam($declaringStorage);
82+
if ($narrowedCallback === null) {
83+
// Nova's canSee() shape changed in a way we don't recognise: silence over false positives.
84+
return;
85+
}
86+
87+
$narrowed = clone $declaringStorage;
88+
$narrowed->params = [$narrowedCallback];
89+
90+
$selfId = new MethodIdentifier($storage->name, self::CAN_SEE);
91+
$storage->methods[self::CAN_SEE] = $narrowed;
92+
$storage->declaring_method_ids[self::CAN_SEE] = $selfId;
93+
$storage->appearing_method_ids[self::CAN_SEE] = $selfId;
94+
}
95+
96+
/**
97+
* `canSee(Closure $callback)`: rewrite the closure's own param type, not `$callback`'s.
98+
* @psalm-mutation-free
99+
*/
100+
private static function narrowCallbackParam(MethodStorage $canSee): ?\Psalm\Storage\FunctionLikeParameter
101+
{
102+
$callbackParam = $canSee->params[0] ?? null;
103+
if ($callbackParam === null) {
104+
return null;
105+
}
106+
107+
$callbackType = $callbackParam->type;
108+
if ($callbackType === null) {
109+
return null;
110+
}
111+
112+
$closure = $callbackType->getSingleAtomic();
113+
if (!$closure instanceof TClosure || $closure->params === null || !isset($closure->params[0])) {
114+
return null;
115+
}
116+
117+
$narrowedRequestParam = $closure->params[0]->setType(new Union([new TNamedObject(self::NOVA_REQUEST)]));
118+
$narrowedClosure = $closure->replace([$narrowedRequestParam], $closure->return_type);
119+
120+
return $callbackParam->setType(new Union([$narrowedClosure]));
121+
}
122+
}

src/Plugin.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,19 @@ public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement
2020
require_once __DIR__.'/NovaMakeSignatureHandler.php';
2121
require_once __DIR__.'/NovaWhenReturnTypeHandler.php';
2222
require_once __DIR__.'/NovaSuppressHandler.php';
23+
require_once __DIR__.'/NovaFieldAuthorizationHandler.php';
2324

2425
$registration->registerHooksFromClass(NovaResourceQueryMethodHandler::class);
2526
$registration->registerHooksFromClass(NovaMakeSignatureHandler::class);
2627
$registration->registerHooksFromClass(NovaWhenReturnTypeHandler::class);
2728
$registration->registerHooksFromClass(NovaSuppressHandler::class);
29+
$registration->registerHooksFromClass(NovaFieldAuthorizationHandler::class);
2830

2931
// Nova stubs that fix vendor signatures Psalm cannot resolve (and template Resource so a
3032
// resource can declare its model via @extends). Shipped with the package so it stays
3133
// self-contained.
3234
$stubsDir = __DIR__.'/../stubs/Nova';
3335
$registration->addStubFile($stubsDir.'/Actions/Action.phpstub');
34-
$registration->addStubFile($stubsDir.'/AuthorizedToSee.phpstub');
3536
$registration->addStubFile($stubsDir.'/Fields/Field.phpstub');
3637
$registration->addStubFile($stubsDir.'/Fields/FieldElement.phpstub');
3738
$registration->addStubFile($stubsDir.'/Fields/Filterable.phpstub');

stubs/Nova/AuthorizedToSee.phpstub

Lines changed: 0 additions & 24 deletions
This file was deleted.

stubs/Nova/Fields/FieldElement.phpstub

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,14 @@ namespace Laravel\Nova\Fields;
2121
*
2222
* `showOnCreating()`/`hideWhenCreating()` take request-only callbacks and need no change.
2323
*
24-
* `canSee()` is redeclared here (narrowed to `NovaRequest`) rather than left on the shared
25-
* `AuthorizedToSee` trait: field resolution (`ResolvesFields`) always goes through `NovaRequest`,
26-
* unlike `Tool`/`Dashboard`/`Filters\Filter`/`Menu\*`, which can receive a plain `Request` — see
27-
* `AuthorizedToSee.phpstub` for why the trait itself stays wide.
24+
* `canSee()` is narrowed to `NovaRequest` too (field resolution always goes through `NovaRequest`,
25+
* unlike `Tool`/`Dashboard`/`Filters\Filter`/`Menu\*`, which can receive a plain `Request`), but not
26+
* here: it's declared only on the `AuthorizedToSee` trait, which `FieldElement` merely inherits
27+
* (via `Element`), and a stub can't override a method the stubbed class only inherits — confirmed
28+
* empirically against real Nova. See `NovaFieldAuthorizationHandler` for the programmatic fix.
2829
*/
2930
abstract class FieldElement extends \Laravel\Nova\Element
3031
{
31-
/**
32-
* @param \Closure(\Laravel\Nova\Http\Requests\NovaRequest):bool $callback
33-
* @return $this
34-
*/
35-
public function canSee(\Closure $callback) {}
36-
3732
/**
3833
* @template TResource of \Illuminate\Database\Eloquent\Model|\Laravel\Nova\Support\Fluent|array<array-key, mixed>|object
3934
* @param (callable(\Laravel\Nova\Http\Requests\NovaRequest, TResource):bool)|bool $callback

stubs/Nova/Fields/Filterable.phpstub

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ namespace Laravel\Nova\Fields;
1515
* the pattern used in `FieldElement.phpstub`: Psalm infers `TBuilder` from the closure's own param type
1616
* instead of checking against a fixed union, so a closure typed against either concrete class still
1717
* narrows correctly, while a genuinely wrong type (e.g. `stdClass`) is still rejected.
18+
*
19+
* Accepted trade-off: typing the closure against ONE concrete shape (just `Builder`, or just
20+
* `Relation`) type-checks even though the field may be reachable via both request kinds — nothing in
21+
* Nova's own signature says whether a given resource is ever browsed as a relation, so Psalm cannot
22+
* warn about it. A closure that only calls methods common to both (or that type-checks its own
23+
* `$query` parameter internally) is safe either way; one written against only `Builder`-specific or
24+
* only `Relation`-specific methods can still throw at runtime on the other request kind. Type against
25+
* the shared `Illuminate\Contracts\Database\Eloquent\Builder` contract instead if that matters for a
26+
* given field.
1827
*/
1928
trait Filterable
2029
{

0 commit comments

Comments
 (0)