diff --git a/CHANGELOG.md b/CHANGELOG.md index 289efa2..c4eefb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ### Changed +- `ForbidEloquentMutationInControllersRule` — registration moved off `Class_` onto `CallLike` (per-node), closing a receiver-type blind spot. The rule previously registered on `Class_` and manually walked every method body, resolving each receiver via `$scope->getType($node->var)` against the **class-entry** scope — a scope carrying no flow-derived knowledge of method-local variables, so any receiver born inside the method body (`$m = new Model; $m->save();`; `$m = Model::query()->…->firstOrFail(); $m->delete();`; a `Builder` held in a local var) resolved to `mixed` and **silently never fired** — only receivers typed from a method signature (typed parameters) matched. Empirically proven on tc-api PR #133 (PHPStan-baseline entries for exactly these shapes came back "unmatched" — the rule never emitted them). Now registers on `CallLike` and branches `MethodCall` / `StaticCall` (mirrors `EnforceCurrentUserAttributeRule` / `LogRule`), so PHPStan supplies a method-level flow scope and local-variable receivers resolve to their real Model / Builder types. The namespace gate (`controllerNamespacePrefixes`, unchanged), the `checkInstanceCall` / `checkStaticCall` type-matching, the blocklist, the error identifier `forbidEloquentMutationInControllers.eloquentMutationInController`, and the message format are all preserved byte-for-byte; the containing-controller FQCN for the message now comes from `$scope->getClassReflection()?->getName()` at the call site (the manual `Class_`-scope `resolveClassFqcn` + `walkNodes` helpers are deleted). The static-call path (`Model::destroy($id)`) is unaffected — it never depended on flow scope. Every pre-existing fixture still fires at the same line; new positive fixtures cover the three local-receiver shapes and a new compliant fixture pins that a local var of a NON-Model class stays clean (the type gate still discriminates under flow scope). Nullsafe `$m?->delete()` calls are covered without a dedicated branch: PHPStan's `NodeScopeResolver` emits a synthetic non-null-narrowed `MethodCall` node (attribute `virtualNullsafeMethodCall`) for every `?->` call, so the plain `MethodCall` branch already fires once — an explicit `NullsafeMethodCall` branch double-reports (the real node plus its synthetic twin). `TypeCombinator::removeNull()` is applied to the receiver before the type gate for the DISTINCT plain-call-on-nullable shape (a plain `->delete()` on a `?Post` from `->first()`: `Post|null` is only a `maybe()` Model supertype, never `yes()`, so without the strip it never fires; a nullable Model receiver carries the same audit-bypass risk). Per-node registration additionally reaches trait bodies analysed through a using class: a mutation in a trait declared under a controllers namespace (e.g. `App\Http\Controllers\Concerns\*`) now fires, message naming the using class — intended coverage the old `Class_` walk structurally never had (a trait file has no `Class_` node). These are pinned by new fixtures (`ViolationNullsafeDelete` — single-fire, no double-report; `ViolationPlainNullableDelete` — the `removeNull` path; `ViolationInTraitFile`). **Versioning: candidate MAJOR per ADR-0021 §Versioning** — surfaces new errors in previously-clean consumer code (known: tc-api `EducationController::store` / `destroy`; the nullsafe + trait paths are additional new-error surface under the same candidacy); per the pre-1.0 caret convention `^0.7` excludes the next minor, so tagging auto-adopts nobody — each consumer runs a pre-cascade audit and remediates on its own pin-bump PR. Seed: tc-api PR #133 baseline-unmatched finding. - `EnforceResourceDataValidatorOptInRule` + `EnforceFormRequestToDtoRule` + `EnforceAuditModelProtectionsRule` — migrated the internal inheritance gate off the deprecated `ClassReflection::isSubclassOf(string)` API onto `isSubclassOfClass(ClassReflection)` (war-room enforcement queue #112). `isSubclassOf(string)` is `@deprecated Use isSubclassOfClass instead.` in PHPStan 2.2+ and **removed in PHPStan 3.x** — a latent break for every consumer's static analysis the day this package (the fleet's canonical static-analysis backbone) targets PHPStan 3. Each of the three rules now injects `PHPStan\Reflection\ReflectionProvider` (constructor DI, autowired by the PHPStan/Nette container — no `extension.neon` argument wiring needed, proven by each rule's container-resolution test) and resolves its configured/known base FQCN via `reflectionProvider->hasClass()/getClass()` before calling `isSubclassOfClass()`. **Behaviour is byte-for-byte preserved:** the migration inlines the deprecated method's own body (`if (!hasClass($fqcn)) return false; return isSubclassOfClass(getClass($fqcn));`), so the load-bearing unknown-base-class no-op — a tree lacking the configured base class (or `Illuminate\Database\Eloquent\Model` for the audit rule) silently does not fire, the "consumers analysing non-Laravel trees are unaffected" guarantee — is reproduced exactly. Pinned by a new base-class-absent no-op test per rule (a configured base FQCN absent from the analysed tree ⇒ zero errors); all existing positive tests stay green. **Versioning: PATCH per ADR-0021 §Versioning** (internal API migration / future-proofing — adds no errors, removes none, no consumer-visible behaviour change). Seed: war-room enforcement queue #112. ## [0.8.0] — 2026-07-13 diff --git a/src/Rules/ForbidEloquentMutationInControllersRule.php b/src/Rules/ForbidEloquentMutationInControllersRule.php index 4020a66..0fa6abb 100644 --- a/src/Rules/ForbidEloquentMutationInControllersRule.php +++ b/src/Rules/ForbidEloquentMutationInControllersRule.php @@ -7,22 +7,20 @@ use Illuminate\Database\Eloquent\Builder as EloquentBuilder; use Illuminate\Database\Eloquent\Model; use PhpParser\Node; +use PhpParser\Node\Expr\CallLike; use PhpParser\Node\Expr\MethodCall; use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Identifier; use PhpParser\Node\Name; -use PhpParser\Node\Stmt\Class_; -use PhpParser\Node\Stmt\ClassMethod; use PHPStan\Analyser\Scope; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\ObjectType; use PHPStan\Type\Type; +use PHPStan\Type\TypeCombinator; -use function array_filter; use function in_array; -use function is_array; use function mb_strrpos; use function mb_substr; use function sprintf; @@ -66,22 +64,62 @@ * opts them in by adding the prefix to `controllerNamespacePrefixes` in * its `phpstan.neon` — mirrors the `formRequestBaseClass` / * `resourceDataBaseClass` parameter precedent. - * 2. For every `ClassMethod` in the class node, recursively walk the method - * body collecting `MethodCall` and `StaticCall` nodes. - * 3. **MethodCall:** resolve the receiver expression's type via - * `$scope->getType($node->var)`. Fire if the type is a subtype of + * 2. **MethodCall:** resolve the receiver expression's type via + * `$scope->getType($node->var)`, then strip null via + * `TypeCombinator::removeNull()`. Fire if the result is a subtype of * `Illuminate\Database\Eloquent\Model` OR a subtype of * `Illuminate\Database\Eloquent\Builder` (the generic parameter is not * unwrapped — `ObjectType::isSuperTypeOf()` handles `Builder` as a * subtype of the unparameterized `Builder` cleanly without brittle - * generic introspection). Method name must be in the blocklist. - * 4. **StaticCall:** resolve the class name via `$scope->resolveName()`. Fire + * generic introspection). Method name must be in the blocklist. The + * null-strip is load-bearing for a plain `->delete()` on a nullable + * `?Model` receiver: `Post|null` is only a `maybe()` supertype of `Model`, + * never `yes()`, so without it that shape never fires. Nullsafe `?->` + * calls are covered here too — see the implementation note below. + * 3. **StaticCall:** resolve the class name via `$scope->resolveName()`. Fire * if the FQCN is a Model subclass and the method name is in the blocklist. * The class-name resolution path covers `User::create([...])`, * `User::destroy($id)`, `User::updateOrCreate(...)`. Class-name expressions * that are not a literal `Name` node (`$class::create(...)`) are out of * scope. * + * Implementation note: `getNodeType()` returns `CallLike::class` so PHPStan + * hands each call node a method-level flow scope — mirrors `LogRule` / + * `EnforceCurrentUserAttributeRule`. An earlier revision registered on `Class_` + * and walked each method body manually, resolving receiver types against the + * CLASS-entry scope; that scope carries no flow-derived knowledge of + * method-local variables, so receivers born inside the body + * (`$m = new Model; $m->save();`, `$m = Model::where(...)->firstOrFail(); $m->delete();`, + * a `Builder` held in a local var) resolved to `mixed` and NEVER fired — only + * receivers typed from a method signature (typed parameters) matched. Per-node + * registration closes that blind spot: PHPStan supplies the flow scope, so + * local-variable receivers resolve to their real Model / Builder types. The + * `CallLike` registration hands the rule `MethodCall` and `StaticCall` nodes + * (plus `New_` / `FuncCall`, which fall through to no-op). The containing- + * controller FQCN for the message comes from + * `$scope->getClassReflection()?->getName()` at the call site (non-null once the + * namespace gate has passed, since the gate implies an in-class scope). + * + * Nullsafe calls: `$m?->delete()` is NOT matched via a `NullsafeMethodCall` + * branch. PHPStan's `NodeScopeResolver` emits, for every `?->` call, a synthetic + * `MethodCall` node (attribute `virtualNullsafeMethodCall === true`) analysed + * under the non-null assumption, so the plain `MethodCall` branch already fires + * once on nullsafe calls with the receiver already null-narrowed. Registering a + * SEPARATE `NullsafeMethodCall` branch double-reports (CI-proven: the real + * `NullsafeMethodCall` node AND its synthetic `MethodCall` twin both fire) — so + * there is deliberately no such branch. `removeNull` therefore earns its keep on + * the DISTINCT plain-call-on-nullable shape (`$m = ...->first(); $m->delete();` + * where `$m` is `?Model`), not the nullsafe form. + * + * Trait coverage: per-node registration also reaches trait bodies — a mutation + * inside a trait declared in a controllers namespace (e.g. + * `App\Http\Controllers\Concerns\*`) fires, and the message names the USING + * class, because PHPStan analyses a trait through each using class and the + * call-site `$scope->getClassReflection()` resolves to that using class (the + * namespace gate reads the file's namespace). The old `Class_`-scope walk + * structurally never ran on a trait file (a trait file has no `Class_` node), so + * this is new-but-intended coverage. + * * Method-name blocklist — full ADR-0011 + ADR-0019 mutation surface (24 entries): * * save, saveOrFail, saveQuietly, @@ -109,7 +147,7 @@ * - Dynamic method names (`$method = 'save'; $model->{$method}()`) — would * need value-flow analysis. Acceptable miss; rely on reviewer. * - * @implements Rule + * @implements Rule */ final class ForbidEloquentMutationInControllersRule implements Rule { @@ -151,7 +189,7 @@ public function __construct( public function getNodeType(): string { - return Class_::class; + return CallLike::class; } public function processNode(Node $node, Scope $scope): array @@ -162,19 +200,31 @@ public function processNode(Node $node, Scope $scope): array return []; } - $classFqcn = $this->resolveClassFqcn($node, $namespace); $modelType = new ObjectType(Model::class); $builderType = new ObjectType(EloquentBuilder::class); - $errors = []; + if ($node instanceof MethodCall) { + $violation = $this->checkInstanceCall($node, $scope, $modelType, $builderType); + } elseif ($node instanceof StaticCall) { + $violation = $this->checkStaticCall($node, $scope, $modelType); + } else { + return []; + } - foreach ($node->getMethods() as $method) { - foreach ($this->collectViolations($method, $scope, $modelType, $builderType) as $violation) { - $errors[] = $this->buildError($classFqcn, $violation); - } + if ($violation === null) { + return []; + } + + // At a call-site scope the class reflection resolves (the namespace gate + // already implies an in-class scope). Guard defensively — a call outside + // any class yields no controller FQCN, so there is nothing to report. + $classReflection = $scope->getClassReflection(); + + if ($classReflection === null) { + return []; } - return $errors; + return [$this->buildError($classReflection->getName(), $violation)]; } /** @@ -193,53 +243,21 @@ private function namespaceIsController(string $namespace): bool return false; } - /** - * @return list - */ - private function collectViolations( - ClassMethod $method, - Scope $scope, - ObjectType $modelType, - ObjectType $builderType, - ): array { - $violations = []; - - if ($method->stmts === null) { - return $violations; - } - - $this->walkNodes( - $method->stmts, - function(Node $node) use (&$violations, $scope, $modelType, $builderType): void { - if ($node instanceof MethodCall) { - $violation = $this->checkInstanceCall($node, $scope, $modelType, $builderType); - - if ($violation !== null) { - $violations[] = $violation; - } - - return; - } - - if ($node instanceof StaticCall) { - $violation = $this->checkStaticCall($node, $scope, $modelType); - - if ($violation !== null) { - $violations[] = $violation; - } - } - }, - ); - - return $violations; - } - /** * Match `$model->mutation(...)` or `$builder->mutation(...)`. Receiver type * must be a subtype of `Illuminate\Database\Eloquent\Model` OR * `Illuminate\Database\Eloquent\Builder`; method name must be in the * blocklist. * + * Null is stripped from the receiver type before the gate: + * `ObjectType::isSuperTypeOf(Post|null)` is `maybe()`, not `yes()`, so a + * plain `->delete()` on a nullable `?Post` receiver would otherwise slip + * through. A nullable Model receiver carries the same audit-bypass risk. + * (The nullsafe `$m?->delete()` form is handled separately — PHPStan feeds a + * synthetic non-null-narrowed `MethodCall` node for it, see the class + * docblock — so `removeNull`'s live surface is the plain-call-on-nullable + * shape.) + * * @return array{type: string, method: string, node: MethodCall}|null */ private function checkInstanceCall( @@ -258,7 +276,7 @@ private function checkInstanceCall( return null; } - $receiverType = $scope->getType($node->var); + $receiverType = TypeCombinator::removeNull($scope->getType($node->var)); $receiverFqcn = $this->matchedReceiverFqcn($receiverType, $modelType, $builderType); @@ -343,20 +361,6 @@ private function buildError(string $classFqcn, array $violation): IdentifierRule ->build(); } - /** - * Resolve the fully-qualified class name from the AST node + namespace. - * Avoids depending on `$scope->getClassReflection()`, which can return - * null during fixture-mode analysis where the class isn't autoloadable. - */ - private function resolveClassFqcn(Class_ $node, string $namespace): string - { - if ($node->name === null) { - return $namespace; - } - - return $namespace . '\\' . $node->name->toString(); - } - private function shortName(string $fqcn): string { $pos = mb_strrpos($fqcn, '\\'); @@ -367,39 +371,4 @@ private function shortName(string $fqcn): string return mb_substr($fqcn, $pos + 1); } - - /** - * Recursively walk a list of nodes, invoking `$callback` on each one. - * Mirrors `EnforceActionTransactionsRule::walkNodes()` / - * `EnforceAuditSnapshotOnRetryRule::walkNodes()` / - * `EnforceAuditTransactionScopeRule::walkNodes()` / - * `EnforceResourceDataValidatorOptInRule::walkNodes()` for parity — - * re-evaluate at v1.0 once the four-way duplication trigger is acted - * on (Standing Concern #29). - * - * @param array $nodes - */ - private function walkNodes(array $nodes, callable $callback): void - { - foreach ($nodes as $node) { - if (!$node instanceof Node) { - continue; - } - - $callback($node); - - foreach ($node->getSubNodeNames() as $name) { - $subNode = $node->{$name}; - - if ($subNode instanceof Node) { - $this->walkNodes([$subNode], $callback); - } elseif (is_array($subNode)) { - $this->walkNodes( - array_filter($subNode, static fn(mixed $item): bool => $item instanceof Node), - $callback, - ); - } - } - } - } } diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantLocalVarNonModel.php b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantLocalVarNonModel.php new file mode 100644 index 0000000..00ea423 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantLocalVarNonModel.php @@ -0,0 +1,20 @@ +save(); + $service->delete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInTraitFile.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInTraitFile.php new file mode 100644 index 0000000..3b298d8 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInTraitFile.php @@ -0,0 +1,29 @@ +save(); + } +} + +namespace App\Http\Controllers; + +use App\Http\Controllers\Concerns\MutatesUsers; + +final class ViolationInTraitFile +{ + use MutatesUsers; +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarBuilderUpdate.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarBuilderUpdate.php new file mode 100644 index 0000000..320345b --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarBuilderUpdate.php @@ -0,0 +1,21 @@ +...->update()` chain. The old `Class_`-scope walk + // resolved `$query` to `mixed`; the flow scope resolves it to a + // `Illuminate\Database\Eloquent\Builder` subtype. + $query = User::query()->where('email', 'inactive@example.test'); + + return $query->update(['name' => 'INACTIVE']); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarFirstOrFailDelete.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarFirstOrFailDelete.php new file mode 100644 index 0000000..f7c5b47 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarFirstOrFailDelete.php @@ -0,0 +1,22 @@ +where(...)->firstOrFail()` returns a hydrated `Post`, + // held in a method-local variable. The old `Class_`-scope walk could not + // resolve this receiver; the flow scope now does. (`query()` is used + // rather than the static `Post::where()` magic forward so the Builder + // generic resolves under vanilla PHPStan — larastan is not loaded in the + // fixture environment.) + $post = Post::query()->where('id', $id)->firstOrFail(); + $post->delete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarNewSave.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarNewSave.php new file mode 100644 index 0000000..ede9ecf --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarNewSave.php @@ -0,0 +1,21 @@ +save(); + + return $user; + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationNullsafeDelete.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationNullsafeDelete.php new file mode 100644 index 0000000..1d32938 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationNullsafeDelete.php @@ -0,0 +1,20 @@ +delete()` is a + // `NullsafeMethodCall` (a sibling of `MethodCall` under `CallLike`). + // `TypeCombinator::removeNull()` strips the `|null` so the Model gate + // fires — without it, `Post|null` is only a `maybe()` Model supertype. + $post = Post::query()->where('id', $id)->first(); + $post?->delete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationPlainNullableDelete.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationPlainNullableDelete.php new file mode 100644 index 0000000..43acbcd --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationPlainNullableDelete.php @@ -0,0 +1,19 @@ +delete()` (NO `?->`) on a nullable `?Post` receiver — pins the + // `TypeCombinator::removeNull()` path: `Post|null` is only a `maybe()` + // Model supertype, so without the null-strip the gate never fires. + $post = Post::query()->where('id', $id)->first(); + $post->delete(); + } +} diff --git a/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php b/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php index beaf338..39e4360 100644 --- a/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php +++ b/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php @@ -192,6 +192,110 @@ public function testViolationBuilderUpdate(): void ); } + public function testViolationLocalVarNewSave(): void + { + // Blind-spot regression: receiver born inside the method body + // (`$user = new User; $user->save();`). The prior `Class_`-scope walk + // resolved `$user` to `mixed` and never fired. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarNewSave.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationLocalVarNewSave', 'save', 'User'), + 17, + ], + ], + ); + } + + public function testViolationLocalVarFirstOrFailDelete(): void + { + // Blind-spot regression: `Post::where(...)->firstOrFail()` hydrates a + // Post into a method-local var, then `$post->delete()`. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarFirstOrFailDelete.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationLocalVarFirstOrFailDelete', 'delete', 'Post'), + 20, + ], + ], + ); + } + + public function testViolationLocalVarBuilderUpdate(): void + { + // Blind-spot regression: a Builder held in a method-local var, distinct + // from the inline `User::query()->...->update()` chain. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarBuilderUpdate.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationLocalVarBuilderUpdate', 'update', 'Builder'), + 19, + ], + ], + ); + } + + public function testCompliantLocalVarNonModel(): void + { + // Type gate still discriminates under flow scope: a local var of a + // NON-Model class calling `save()` / `delete()` stays clean. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/CompliantLocalVarNonModel.php'], + [], + ); + } + + public function testViolationNullsafeDelete(): void + { + // `NullsafeMethodCall` gap: `$post?->delete()` on a `?Post` receiver. + // The nullsafe branch + `removeNull` gate fires where a MethodCall-only + // match (or a naive branch without null-strip) would miss it. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationNullsafeDelete.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationNullsafeDelete', 'delete', 'Post'), + 18, + ], + ], + ); + } + + public function testViolationPlainNullableDelete(): void + { + // Pins the `removeNull` path: a plain `->delete()` (no `?->`) on a + // nullable `?Post` receiver. `Post|null` is only a `maybe()` Model + // supertype, so without the null-strip the gate never fires. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationPlainNullableDelete.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationPlainNullableDelete', 'delete', 'Post'), + 17, + ], + ], + ); + } + + public function testViolationInTraitFile(): void + { + // Trait coverage: a mutation inside a trait declared in a controllers + // namespace fires (per-node registration reaches trait bodies), and the + // message names the USING class. + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationInTraitFile.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationInTraitFile', 'save', 'User'), + 18, + ], + ], + ); + } + public function testSubNamespacedControllerIsCleanUnderDefaultConfig(): void { // emmie's `App\Http\Client\Controllers` namespace does NOT start with