diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 1021fc6..4e361ba 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -5,6 +5,7 @@ on: paths: - '**.php' - '**.phpstub' + - 'tests/fixtures/**' - 'phpunit.xml.dist' - 'composer.json' - '.github/workflows/tests.yml' diff --git a/README.md b/README.md index c6eca7b..4fbf847 100644 --- a/README.md +++ b/README.md @@ -60,7 +60,7 @@ Hook methods that override a base declaration (`fields()`, `apply()`, `calculate ### Nova stubs -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`: +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`: ```php /** @extends \Laravel\Nova\Resource<\App\Models\User> */ @@ -72,7 +72,11 @@ final class User extends Resource The stubs are registered by the plugin itself; no `` entry is needed in `psalm.xml`. -`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|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. +`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|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. + +`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. + +`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. ## Requirements diff --git a/composer.json b/composer.json index 650bdd0..75c9a57 100644 --- a/composer.json +++ b/composer.json @@ -28,7 +28,10 @@ "autoload-dev": { "psr-4": { "InteractionDesignFoundation\\PsalmLaravelNova\\Tests\\": "tests/" - } + }, + "classmap": [ + "tests/fixtures/fake-nova/" + ] }, "config": { "allow-plugins": { diff --git a/src/NovaFieldAuthorizationHandler.php b/src/NovaFieldAuthorizationHandler.php new file mode 100644 index 0000000..36fafdb --- /dev/null +++ b/src/NovaFieldAuthorizationHandler.php @@ -0,0 +1,101 @@ +getCodebase(); + + foreach ($codebase->classlike_storage_provider::getAll() as $storage) { + $isFieldElement = mb_strtolower($storage->name) === self::FIELD_ELEMENT + || isset($storage->parent_classes[self::FIELD_ELEMENT]); + if (!$isFieldElement) { + continue; + } + + self::narrowCanSee($codebase, $storage); + } + } + + private static function narrowCanSee(Codebase $codebase, ClassLikeStorage $storage): void + { + $declaringId = $storage->declaring_method_ids[self::CAN_SEE] ?? null; + if ($declaringId === null || mb_strtolower($declaringId->fq_class_name) !== self::AUTHORIZED_TO_SEE) { + // canSee() is missing, or a user class overrode it somewhere in the chain — only ever + // narrow Nova's own trait method, never second-guess a user's own override. + return; + } + + $declaringStorage = $codebase->methods->getStorage($declaringId); + $narrowedCallback = self::narrowCallbackParam($declaringStorage); + if ($narrowedCallback === null) { + // Nova's canSee() shape changed in a way we don't recognise: silence over false positives. + return; + } + + $narrowed = clone $declaringStorage; + $narrowed->params = [$narrowedCallback]; + + $selfId = new MethodIdentifier($storage->name, self::CAN_SEE); + $storage->methods[self::CAN_SEE] = $narrowed; + $storage->declaring_method_ids[self::CAN_SEE] = $selfId; + $storage->appearing_method_ids[self::CAN_SEE] = $selfId; + } + + /** + * `canSee(Closure $callback)`: rewrite the closure's own param type, not `$callback`'s. + * @psalm-mutation-free + */ + private static function narrowCallbackParam(MethodStorage $canSee): ?\Psalm\Storage\FunctionLikeParameter + { + $callbackParam = $canSee->params[0] ?? null; + if ($callbackParam === null) { + return null; + } + + $callbackType = $callbackParam->type; + if ($callbackType === null) { + return null; + } + + $closure = $callbackType->getSingleAtomic(); + if (!$closure instanceof TClosure || $closure->params === null || !isset($closure->params[0])) { + return null; + } + + $narrowedRequestParam = $closure->params[0]->setType(new Union([new TNamedObject(self::NOVA_REQUEST)])); + $narrowedClosure = $closure->replace([$narrowedRequestParam], $closure->return_type); + + return $callbackParam->setType(new Union([$narrowedClosure])); + } +} diff --git a/src/Plugin.php b/src/Plugin.php index a228f70..3b07118 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -20,18 +20,19 @@ public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement require_once __DIR__.'/NovaMakeSignatureHandler.php'; require_once __DIR__.'/NovaWhenReturnTypeHandler.php'; require_once __DIR__.'/NovaSuppressHandler.php'; + require_once __DIR__.'/NovaFieldAuthorizationHandler.php'; $registration->registerHooksFromClass(NovaResourceQueryMethodHandler::class); $registration->registerHooksFromClass(NovaMakeSignatureHandler::class); $registration->registerHooksFromClass(NovaWhenReturnTypeHandler::class); $registration->registerHooksFromClass(NovaSuppressHandler::class); + $registration->registerHooksFromClass(NovaFieldAuthorizationHandler::class); // Nova stubs that fix vendor signatures Psalm cannot resolve (and template Resource so a // resource can declare its model via @extends). Shipped with the package so it stays // self-contained. $stubsDir = __DIR__.'/../stubs/Nova'; $registration->addStubFile($stubsDir.'/Actions/Action.phpstub'); - $registration->addStubFile($stubsDir.'/AuthorizedToSee.phpstub'); $registration->addStubFile($stubsDir.'/Fields/Field.phpstub'); $registration->addStubFile($stubsDir.'/Fields/FieldElement.phpstub'); $registration->addStubFile($stubsDir.'/Fields/Filterable.phpstub'); diff --git a/stubs/Nova/AuthorizedToSee.phpstub b/stubs/Nova/AuthorizedToSee.phpstub deleted file mode 100644 index 05f20ae..0000000 --- a/stubs/Nova/AuthorizedToSee.phpstub +++ /dev/null @@ -1,24 +0,0 @@ - ...)` type-check even though it can - * receive a plain `Request` at runtime — a real regression, not an acceptable trade-off — so the trait - * keeps Nova's original wide union. - * - * Classes provably reachable only through `NovaRequest` redeclare a narrower `canSee()` at the class - * level instead — see `Fields/FieldElement.phpstub`, which every `Field` inherits from. - */ -trait AuthorizedToSee -{ - /** - * @param \Closure(\Laravel\Nova\Http\Requests\NovaRequest|\Illuminate\Http\Request):bool $callback - * @return $this - */ - public function canSee(\Closure $callback) {} -} diff --git a/stubs/Nova/Fields/FieldElement.phpstub b/stubs/Nova/Fields/FieldElement.phpstub index 56603a9..faf7f98 100644 --- a/stubs/Nova/Fields/FieldElement.phpstub +++ b/stubs/Nova/Fields/FieldElement.phpstub @@ -21,19 +21,11 @@ namespace Laravel\Nova\Fields; * * `showOnCreating()`/`hideWhenCreating()` take request-only callbacks and need no change. * - * `canSee()` is redeclared here (narrowed to `NovaRequest`) rather than left on the shared - * `AuthorizedToSee` trait: field resolution (`ResolvesFields`) always goes through `NovaRequest`, - * unlike `Tool`/`Dashboard`/`Filters\Filter`/`Menu\*`, which can receive a plain `Request` — see - * `AuthorizedToSee.phpstub` for why the trait itself stays wide. + * `canSee()` is narrowed too, but not here — it's only inherited (via `AuthorizedToSee`), and a stub + * can't override an inherited method. See `NovaFieldAuthorizationHandler`. */ abstract class FieldElement extends \Laravel\Nova\Element { - /** - * @param \Closure(\Laravel\Nova\Http\Requests\NovaRequest):bool $callback - * @return $this - */ - public function canSee(\Closure $callback) {} - /** * @template TResource of \Illuminate\Database\Eloquent\Model|\Laravel\Nova\Support\Fluent|array|object * @param (callable(\Laravel\Nova\Http\Requests\NovaRequest, TResource):bool)|bool $callback diff --git a/stubs/Nova/Fields/Filterable.phpstub b/stubs/Nova/Fields/Filterable.phpstub index 9d1cb5e..732d7ec 100644 --- a/stubs/Nova/Fields/Filterable.phpstub +++ b/stubs/Nova/Fields/Filterable.phpstub @@ -15,6 +15,9 @@ namespace Laravel\Nova\Fields; * the pattern used in `FieldElement.phpstub`: Psalm infers `TBuilder` from the closure's own param type * instead of checking against a fixed union, so a closure typed against either concrete class still * narrows correctly, while a genuinely wrong type (e.g. `stdClass`) is still rejected. + * + * Trade-off: a closure typed against only ONE of the two shapes still type-checks, even if the field + * is reachable via both request kinds — Psalm can't know. Use the shared contract type if that matters. */ trait Filterable { diff --git a/tests/AcceptanceTest.php b/tests/AcceptanceTest.php index 9378cde..0aea72a 100644 --- a/tests/AcceptanceTest.php +++ b/tests/AcceptanceTest.php @@ -13,12 +13,12 @@ * `fake-nova/` holds minimal Laravel/Nova declarations that reproduce the vendor docblocks the * plugin's stubs override. Both scenario files are analysed in a single Psalm run. * - * @see tests/fixtures/psalm.xml for why the fakes are stub files rather than project files. + * @see tests/fixtures/psalm.xml for why the fakes are reached through the composer classmap. */ #[CoversNothing] final class AcceptanceTest extends TestCase { - /** @var list|null */ + /** @var list|null */ private static ?array $issues = null; #[Test] @@ -37,29 +37,35 @@ public function genuinely_wrong_callbacks_are_still_reported(): void self::assertSame( [ // Wrong request class. - ['line' => 22, 'type' => 'InvalidArgument'], + ['text' => 'static fn(\stdClass $request, Post $post): bool => true', 'type' => 'InvalidArgument'], // Wrong return type. - ['line' => 25, 'type' => 'InvalidArgument'], + ['text' => "static fn(NovaRequest \$request, Post \$post): string => 'nope'", 'type' => 'InvalidArgument'], + // Wrong resource param type on a setter with no upstream @phpstan-param (stricter + // than pre-plugin Nova, which left this bare `mixed`). + ['text' => 'static fn(NovaRequest $request, int $post): bool => true', 'type' => 'InvalidArgument'], // Wrong param type on the authorisation callback. - ['line' => 28, 'type' => 'InvalidArgument'], + ['text' => 'static fn(int $request): bool => true', 'type' => 'InvalidArgument'], // Wrong builder type on the filter callback. - ['line' => 31, 'type' => 'InvalidArgument'], + [ + 'text' => 'static function (NovaRequest $request, \stdClass $wrongBuilder, mixed $value, string $attribute): void {}', + 'type' => 'InvalidArgument', + ], // Stack line is not a valid class-string|callable|Field. - ['line' => 34, 'type' => 'InvalidArgument'], + ['text' => '[42]', 'type' => 'InvalidArgument'], // Stack line via $lines is not a valid class-string|callable|Field either. - ['line' => 37, 'type' => 'InvalidArgument'], + ['text' => '[new \stdClass()]', 'type' => 'InvalidArgument'], // Tool::canSee() narrowed to NovaRequest would be unsound: Tool can receive a plain Request. - ['line' => 51, 'type' => 'ArgumentTypeCoercion'], + ['text' => 'static fn(NovaRequest $request): bool => true', 'type' => 'ArgumentTypeCoercion'], ], array_map( - static fn(array $issue): array => ['line' => $issue['line_from'], 'type' => $issue['type']], + static fn(array $issue): array => ['text' => $issue['selected_text'], 'type' => $issue['type']], $issues, ), "Unexpected Psalm issues in still_errors.php:\n".self::describe($issues), ); } - /** @return list */ + /** @return list */ private function issuesIn(string $fixtureRelativePath): array { return array_values(array_filter( @@ -68,7 +74,7 @@ private function issuesIn(string $fixtureRelativePath): array )); } - /** @return list */ + /** @return list */ private static function psalmIssues(): array { if (self::$issues !== null) { @@ -76,37 +82,54 @@ private static function psalmIssues(): array } $projectRoot = \dirname(__DIR__); - $descriptors = [1 => ['pipe', 'w'], 2 => ['pipe', 'w']]; - $process = proc_open( - [ - \PHP_BINARY, - $projectRoot.'/vendor/bin/psalm', - '--no-cache', - '--no-progress', - '--output-format=json', - '-c', - 'tests/fixtures/psalm.xml', - ], - $descriptors, - $pipes, - $projectRoot, - ); - self::assertIsResource($process, 'Could not start Psalm.'); + // stderr goes to a temp file, not a second pipe: draining stdout and stderr from two live + // pipes sequentially can deadlock if Psalm fills the undrained one while blocked writing to + // the other. A file has no such buffer limit. + $stderrFile = tempnam(sys_get_temp_dir(), 'psalm-plugin-nova-stderr-'); + self::assertIsString($stderrFile, 'Could not create a temp file for stderr.'); + + try { + $process = proc_open( + [ + \PHP_BINARY, + $projectRoot.'/vendor/bin/psalm', + '--no-cache', + '--no-progress', + '--output-format=json', + '-c', + 'tests/fixtures/psalm.xml', + ], + [1 => ['pipe', 'w'], 2 => ['file', $stderrFile, 'w']], + $pipes, + $projectRoot, + ); + self::assertIsResource($process, 'Could not start Psalm.'); - $stdout = (string) stream_get_contents($pipes[1]); - $stderr = (string) stream_get_contents($pipes[2]); - array_map(fclose(...), $pipes); - proc_close($process); + $stdout = (string) stream_get_contents($pipes[1]); + fclose($pipes[1]); + $exitCode = proc_close($process); + $stderr = (string) file_get_contents($stderrFile); + } finally { + unlink($stderrFile); + } + + // 0 = no issues, 2 = issues were found (expected — still_errors.php is meant to raise some; + // see IssueBuffer::finish()). Anything else is Psalm itself failing to run, not an issue. + self::assertContains( + $exitCode, + [0, 2], + "Psalm exited with code {$exitCode}.\nstdout: {$stdout}\nstderr: {$stderr}", + ); - /** @var list|null $issues */ + /** @var list|null $issues */ $issues = json_decode($stdout, associative: true); self::assertIsArray($issues, "Psalm did not return JSON.\nstdout: {$stdout}\nstderr: {$stderr}"); return self::$issues = $issues; } - /** @param list $issues */ + /** @param list $issues */ private static function describe(array $issues): string { return implode("\n", array_map( diff --git a/tests/fixtures/psalm.xml b/tests/fixtures/psalm.xml index 03b7b92..68b7fc2 100644 --- a/tests/fixtures/psalm.xml +++ b/tests/fixtures/psalm.xml @@ -11,15 +11,9 @@ - - - - - - + diff --git a/tests/fixtures/scenarios/clean.php b/tests/fixtures/scenarios/clean.php index 8e43fd0..04c712f 100644 --- a/tests/fixtures/scenarios/clean.php +++ b/tests/fixtures/scenarios/clean.php @@ -5,6 +5,8 @@ use App\Models\Post; use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Relations\Relation; +use Illuminate\Http\Request; +use Laravel\Nova\Fields\Field; use Laravel\Nova\Fields\Line; use Laravel\Nova\Fields\Stack; use Laravel\Nova\Fields\Text; @@ -50,3 +52,28 @@ static function (NovaRequest $request, Relation $query, mixed $value, string $at ]; } } + +/** A user's own canSee() override must never be narrowed — only Nova's own trait method is. */ +final class FieldWithOwnAuthorization extends Field +{ + /** + * @param \Closure(Request): bool $callback + * @psalm-suppress MissingPureAnnotation, UnusedParam — irrelevant to what this fixture tests + */ + #[\Override] + public function canSee(\Closure $callback) + { + return $this; + } +} + +final class UsesFieldWithOwnAuthorization +{ + /** @return list<\Laravel\Nova\Fields\Field> */ + public function fields(): array + { + return [ + (new FieldWithOwnAuthorization('Name'))->canSee(static fn(Request $request): bool => true), + ]; + } +} diff --git a/tests/fixtures/scenarios/still_errors.php b/tests/fixtures/scenarios/still_errors.php index c58f37e..e7fb9c6 100644 --- a/tests/fixtures/scenarios/still_errors.php +++ b/tests/fixtures/scenarios/still_errors.php @@ -24,6 +24,11 @@ public function fields(): array // Wrong return type. Text::make('B')->showOnDetail(static fn(NovaRequest $request, Post $post): string => 'nope'), + // Wrong resource param type — a stricter check than Nova's own docblock: hideFromIndex() + // (unlike showOnIndex()/showOnDetail()) has no @phpstan-param upstream, so it was bare + // `mixed` and accepted anything before this plugin's bounded template. + Text::make('G')->hideFromIndex(static fn(NovaRequest $request, int $post): bool => true), + // Wrong param type on the authorisation callback. Text::make('C')->canSee(static fn(int $request): bool => true),