diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f6e4e4..7c240e7 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 ### Added +- `ForbidEloquentMutationInControllersRule` — bans Eloquent persistence APIs (`save`, `update`, `delete`, `create`, `destroy`, `forceDelete`, `forceFill`, `push`, `restore`, `touch`, and their `*OrFail` / `*Quietly` / `*OrCreate` variants — 24-method blocklist) on `Illuminate\Database\Eloquent\Model` subclasses and `Illuminate\Database\Eloquent\Builder` chains when the call site is inside an `App\Http\Controllers\*` class (including sub-namespaces like kendo's `App\Http\Controllers\Central\*`, matched via `str_starts_with`). Reads (`find`, `where`, `get`, `first`, `paginate`, `pluck`, `count`, `exists`, `query`) are deliberately permitted — route-model binding, ResourceData hydration, and policy checks need controller-level Model access; the doctrine line is "Controllers may READ Models, but MUST NOT mutate them." Identifier: `forbidEloquentMutationInControllers.eloquentMutationInController`. Doctrine: ADR-0011 (Action Class Architecture) — Actions are the chokepoint for mutations — combined with ADR-0019 (Explicit Model Hydration) — `Model::create()` / `fill()` / `forceFill()` / `update()` banned application-wide; this rule enforces the controller surface where the violations have been historically common. Algorithm: namespace gate (`App\Http\Controllers`) → recursively walk every `ClassMethod` body collecting `MethodCall` + `StaticCall` nodes → for `MethodCall`, fire if `ObjectType::isSuperTypeOf()` against `Model` OR `Builder` matches the receiver type and the method name is in the blocklist; for `StaticCall`, fire if `Scope::resolveName()` resolves to a Model subclass FQCN and the method name is in the blocklist. Builder coverage is type-aware (`User::query()->where(...)->update([...])` fires) — the generic parameter is not unwrapped because `ObjectType` matches `Builder` as a subtype of the unparameterized `Builder` cleanly, no brittle generic introspection needed. Supersedes the consumer-side string-match Pest arch tests in kendo (`backend/tests/Arch/ControllersTest.php` `controllers must not call Eloquent write methods directly`), ublgenie + entreezuil (`tests/Arch/ControllersTest.php` of the same shape), and the bridge subset in ISMS (`backend/tests/Architecture/ControllerCurrentUserTest.php` from PR #10, 2026-05-28). The string-match shape catches `->save(`, `->update([`, `->delete(`, `->forceDelete(` but cannot discriminate `Model::create()` from `Response::create()`, `Collection::push()` from `Model::push()`, or `->update($vars)` without an inline array literal — the type-aware AST inspection here closes those gaps. Cross-territory cascade post-merge: consumer Pest tests deleted; emmie + brick-inventory-orchestrator pick up coverage automatically on next composer update. Out of scope: non-`App\Http\Controllers\*` namespaces (Actions/Services/Jobs/Middleware are allowed to call persistence APIs), non-Eloquent receivers, dynamic method names (`$model->{$var}()` — value-flow analysis), variable class names in static calls (`$class::create(...)`). Closes war-room enforcement queue #87. **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has a controller calling Eloquent persistence APIs directly — the three territories currently running the string-match Pest test caught the bulk of these, but the type-aware shape will surface additional violations the string-match shape missed: `Model::create()`, `Model::destroy()`, chained Builder mutations, `*Quietly` variants, etc.). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory before tagging — three territories' Pest tests already closed the string-match-visible violators; the type-aware additional surface (Builder chains, `Model::create()`, `*Quietly` variants) may carry undetected violators.** - `EnforceAuditTransactionScopeRule` — enforces ADR-0029 (Audit Row Durability Contract) §Decision rule 3. Flags non-transactional state mutations (`StatefulGuard` / `Session` / `Cache` / `Bus` / `Queue` / `Mailer` / `Notification` / `Broadcaster` / `Filesystem` and their `Illuminate\Support\Facades\*` counterparts, mutation methods only) inside `transaction(...)` closures in `App\Actions\*` classes. Identifier: `enforceAuditTransactionScope.nonTransactionalMutationInClosure`. Doctrine: ADR-0029 §Decision rule 3. Seed: ISMS-0003 PR #7 commit `f1d357b` (2026-05-28) — three Auth Actions (`AuthenticateWorkerAction`, `VerifyTwoFactorChallengeAction`, `LogoutWorkerAction`) mutated `StatefulGuard` + `Session` state inside the transaction closure before the audit row write; an audit-write failure would have rolled back the audit row while leaving the session/guard mutation intact (A.8.15 / A.5.33 violation). Reads (`Auth::user()`, `Session::get()`, `Cache::get()`, etc.) are deliberately permitted — only mutations carry the rollback-vs-side-effect asymmetry. Instance-call detection matches the constructor-property's declared FQCN against the blocklist keys; static-call detection resolves the facade name via `Scope::resolveName()`. Nested `transaction(...)` calls inside an outer closure are walked transitively — a nested mutation is still inside the outermost transaction's rollback scope; top-level transaction discovery deduplicates so each violation reports exactly once. Out of scope: manual transaction management (`DB::beginTransaction()` / `commit()`); non-`App\Actions\*` namespaces; the failure-side discipline (sentinel-return; throw-inside-closure detection) which lives as per-territory Pest arch tests under enforcement queue #85. **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has an `App\Actions\*` class mutating non-transactional state inside a `transaction(...)` closure). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory before tagging — ISMS-0003 PR #7 commit `f1d357b` already closed ISMS's known violators; other consumer territories may carry undetected violators.** ## [0.3.0] — 2026-05-13 diff --git a/CLAUDE.md b/CLAUDE.md index 1226086..d133930 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `LogBuilderTruncateRule` | ADR-0001 §Append-only | `logRule.logModification` (shared with `LogRule`; covers `Builder->truncate()` on Log-named tables — ships with v0.3.0 per `[Unreleased]`) | | `EnforceAuditSnapshotOnRetryRule` | ADR-0001 §Snapshot-on-Retry Safety | `enforceAuditSnapshotOnRetry.firstStatementMustResetState` | | `EnforceAuditTransactionScopeRule` | ADR-0029 | `enforceAuditTransactionScope.nonTransactionalMutationInClosure` | +| `ForbidEloquentMutationInControllersRule` | ADR-0011 + ADR-0019 | `forbidEloquentMutationInControllers.eloquentMutationInController` | | `EnforceResourceDataValidatorOptInRule` | ADR-0009 §EAGER_LOAD validator opt-in | `enforceResourceDataValidatorOptIn.missingValidatorCall` | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — | diff --git a/README.md b/README.md index fe30957..ac86fb0 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,7 @@ includes: | `LogBuilderTruncateRule` | `logRule.logModification` | `Builder->truncate()` calls | If the fluent chain's most recent `table()` call targets a Log-named table (string-literal argument matching `"log"` / `"logs"`, case-insensitive), error. Sibling rule to `LogRule`; shares the `logRule.logModification` identifier so a single `ignoreErrors` entry covers both. Eloquent `from()` chains and Model-`$table`-property-driven tables are acceptable misses. Doctrine: ADR-0001 §Append-only. | | `EnforceAuditSnapshotOnRetryRule` | `enforceAuditSnapshotOnRetry.firstStatementMustResetState` | `App\Actions\*` whose constructor injects an entity audit logger | The first statement inside `$connection->transaction(...)` must reset the model's in-memory state (`$model->refresh()`, fresh fetch, or fresh instantiation). Doctrine: ADR-0001 §Snapshot-on-Retry Safety. | | `EnforceAuditTransactionScopeRule` | `enforceAuditTransactionScope.nonTransactionalMutationInClosure` | `App\Actions\*` whose `execute()` calls `transaction(...)` with a literal closure | Mutating `StatefulGuard` / `Session` / `Cache` / `Bus` / `Queue` / `Mailer` / `Notification` / `Broadcaster` / `Filesystem` state (or their `Illuminate\Support\Facades\*` counterparts) inside the closure is an error. Reads (`Auth::user()`, `Session::get()`, `Cache::get()`) are permitted. Doctrine: ADR-0029 (Audit Row Durability Contract) §Decision rule 3. | +| `ForbidEloquentMutationInControllersRule` | `forbidEloquentMutationInControllers.eloquentMutationInController` | `App\Http\Controllers\*` (including sub-namespaces) | Calling Eloquent persistence APIs (`save`, `update`, `delete`, `create`, `destroy`, `forceDelete`, `forceFill`, `push`, `restore`, `touch`, and their `*OrFail` / `*Quietly` / `*OrCreate` variants — 24-method blocklist) on `Illuminate\Database\Eloquent\Model` subclasses or `Illuminate\Database\Eloquent\Builder` chains is an error. Reads (`find`, `where`, `get`, `first`, `paginate`, `pluck`, `count`, `exists`, `query`) are permitted. Delegate mutations to an Action. Doctrine: ADR-0011 (Action Class Architecture) + ADR-0019 (Explicit Model Hydration). | | `EnforceResourceDataValidatorOptInRule` | `enforceResourceDataValidatorOptIn.missingValidatorCall` | Classes extending `App\Http\Resources\ResourceData` | If the class declares a non-empty `EAGER_LOAD_COUNT` / `EAGER_LOAD_SUM` constant but never calls `validateRelationsLoaded()` in any method, error. | ### `EnforceActionTransactionsRule` — write-method list diff --git a/extension.neon b/extension.neon index 97921e2..0034fb9 100644 --- a/extension.neon +++ b/extension.neon @@ -30,6 +30,9 @@ services: - class: ScriptDevelopment\PhpstanWarroomRules\Rules\EnforceAuditTransactionScopeRule tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\ForbidEloquentMutationInControllersRule + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Rules\EnforceResourceDataValidatorOptInRule arguments: diff --git a/src/Rules/ForbidEloquentMutationInControllersRule.php b/src/Rules/ForbidEloquentMutationInControllersRule.php new file mode 100644 index 0000000..6512463 --- /dev/null +++ b/src/Rules/ForbidEloquentMutationInControllersRule.php @@ -0,0 +1,370 @@ +save(`, `->update([`, + * `->delete(`, `->forceDelete(` but cannot discriminate `Model::create()` from + * `Response::create()`, `Collection::push()` from `Model::push()`, or + * `->update($vars)` without an inline array literal — the type-aware AST + * inspection here closes those gaps. + * + * Algorithm: + * + * 1. Namespace gate — class FQCN must start with `App\Http\Controllers`. + * Sub-namespaces (kendo's `App\Http\Controllers\Central\*`) pass naturally + * via `str_starts_with`. If a future consumer ships controllers under a + * divergent namespace, lift this into a `controllerNamespacePrefixes` + * parameter per the `EnforceResourceDataValidatorOptInRule` 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 + * `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 + * 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. + * + * Method-name blocklist — full ADR-0011 + ADR-0019 mutation surface (24 entries): + * + * save, saveOrFail, saveQuietly, + * update, updateOrFail, updateQuietly, + * delete, deleteOrFail, + * forceDelete, forceDeleteQuietly, + * destroy, create, createOrFirst, firstOrCreate, updateOrCreate, + * fill, forceFill, + * push, pushQuietly, + * restore, restoreQuietly, restoreOrCreate, + * touch, touchQuietly + * + * Suppression: standard PHPStan inline-ignore mechanism on the rule's + * identifier `forbidEloquentMutationInControllers.eloquentMutationInController`. + * + * Out of scope: + * + * - Non-`App\Http\Controllers\*` namespaces — Actions, Services, Jobs, + * console commands, middleware all freely call Eloquent persistence APIs. + * - Non-Eloquent receivers — `$service->save()`, `$collection->push($item)`, + * `Response::create(...)` all pass the type gate. + * - Read methods on Models — `User::find($id)`, `$user->where(...)->get()` + * are deliberately allowed; controllers reading Models is necessary for + * route-model binding, ResourceData hydration, and policy checks. + * - Dynamic method names (`$method = 'save'; $model->{$method}()`) — would + * need value-flow analysis. Acceptable miss; rely on reviewer. + * + * @implements Rule + */ +final class ForbidEloquentMutationInControllersRule implements Rule +{ + private const string CONTROLLER_NAMESPACE_PREFIX = 'App\Http\Controllers'; + + /** + * Eloquent persistence-API method names that mutate model or table state. + * Reads are deliberately omitted — controllers reading Models is necessary + * for route-model binding, ResourceData hydration, and policy checks. + * + * @var list + */ + private const array MUTATION_METHODS = [ + 'save', 'saveOrFail', 'saveQuietly', + 'update', 'updateOrFail', 'updateQuietly', + 'delete', 'deleteOrFail', + 'forceDelete', 'forceDeleteQuietly', + 'destroy', 'create', 'createOrFirst', 'firstOrCreate', 'updateOrCreate', + 'fill', 'forceFill', + 'push', 'pushQuietly', + 'restore', 'restoreQuietly', 'restoreOrCreate', + 'touch', 'touchQuietly', + ]; + + public function getNodeType(): string + { + return Class_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $namespace = $scope->getNamespace(); + + if ($namespace === null || !str_starts_with($namespace, self::CONTROLLER_NAMESPACE_PREFIX)) { + return []; + } + + $classFqcn = $this->resolveClassFqcn($node, $namespace); + $modelType = new ObjectType(Model::class); + $builderType = new ObjectType(EloquentBuilder::class); + + $errors = []; + + foreach ($node->getMethods() as $method) { + foreach ($this->collectViolations($method, $scope, $modelType, $builderType) as $violation) { + $errors[] = $this->buildError($classFqcn, $violation); + } + } + + return $errors; + } + + /** + * @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. + * + * @return array{type: string, method: string, node: MethodCall}|null + */ + private function checkInstanceCall( + MethodCall $node, + Scope $scope, + ObjectType $modelType, + ObjectType $builderType, + ): ?array { + if (!$node->name instanceof Identifier) { + return null; + } + + $methodName = $node->name->toString(); + + if (!in_array($methodName, self::MUTATION_METHODS, true)) { + return null; + } + + $receiverType = $scope->getType($node->var); + + $receiverFqcn = $this->matchedReceiverFqcn($receiverType, $modelType, $builderType); + + if ($receiverFqcn === null) { + return null; + } + + return ['type' => $receiverFqcn, 'method' => $methodName, 'node' => $node]; + } + + /** + * Match `User::create([...])` / `User::destroy($id)` etc. Resolves the + * static-call class via the file's use-import map; only literal `Name` + * class expressions are inspected. Variable class names + * (`$class::create(...)`) and dynamic method names are out of scope. + * + * @return array{type: string, method: string, node: StaticCall}|null + */ + private function checkStaticCall(StaticCall $node, Scope $scope, ObjectType $modelType): ?array + { + if (!$node->name instanceof Identifier) { + return null; + } + + $methodName = $node->name->toString(); + + if (!in_array($methodName, self::MUTATION_METHODS, true)) { + return null; + } + + if (!$node->class instanceof Name) { + return null; + } + + $resolvedFqcn = $scope->resolveName($node->class); + $resolvedType = new ObjectType($resolvedFqcn); + + if (!$modelType->isSuperTypeOf($resolvedType)->yes()) { + return null; + } + + return ['type' => $resolvedFqcn, 'method' => $methodName, 'node' => $node]; + } + + /** + * Returns the matched receiver's representative FQCN (the short-name used + * in the error message), or null if the receiver type is not a Model / + * Builder subtype. + */ + private function matchedReceiverFqcn(Type $receiverType, ObjectType $modelType, ObjectType $builderType): ?string + { + if ($modelType->isSuperTypeOf($receiverType)->yes()) { + $referenced = $receiverType->getReferencedClasses(); + + return $referenced[0] ?? Model::class; + } + + if ($builderType->isSuperTypeOf($receiverType)->yes()) { + $referenced = $receiverType->getReferencedClasses(); + + return $referenced[0] ?? EloquentBuilder::class; + } + + return null; + } + + /** + * @param array{type: string, method: string, node: MethodCall|StaticCall} $violation + */ + private function buildError(string $classFqcn, array $violation): IdentifierRuleError + { + $message = sprintf( + 'Controller %s must not call Eloquent persistence method %s() on %s — delegate to an Action (ADR-0011 + ADR-0019).', + $classFqcn, + $violation['method'], + $this->shortName($violation['type']), + ); + + return RuleErrorBuilder::message($message) + ->identifier('forbidEloquentMutationInControllers.eloquentMutationInController') + ->line($violation['node']->getStartLine()) + ->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, '\\'); + + if ($pos === false) { + return $fqcn; + } + + 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/CompliantActionDelegation.php b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantActionDelegation.php new file mode 100644 index 0000000..5fd4bf4 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantActionDelegation.php @@ -0,0 +1,27 @@ +action->execute($input); + + return $result->user; + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonControllerMutation.php b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonControllerMutation.php new file mode 100644 index 0000000..b4f8d19 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonControllerMutation.php @@ -0,0 +1,26 @@ +name = 'Bar'; + $user->save(); + $user->update(['email' => 'bar@example.test']); + User::create(['name' => 'Baz', 'email' => 'baz@example.test']); + $user->delete(); + + return $user; + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonModelReceiver.php b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonModelReceiver.php new file mode 100644 index 0000000..57b22f8 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantNonModelReceiver.php @@ -0,0 +1,24 @@ +service->save(); + $this->service->delete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantReadOnlyController.php b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantReadOnlyController.php new file mode 100644 index 0000000..911fbcc --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/CompliantReadOnlyController.php @@ -0,0 +1,52 @@ +where('id', $id) + ->first(); + } + + public function index(): mixed + { + return User::query() + ->where('email', 'foo@bar') + ->paginate(25); + } + + public function count(): int + { + return User::query()->count(); + } + + public function exists(int $id): bool + { + return User::query()->where('id', $id)->exists(); + } + + public function pluck(): mixed + { + return User::query()->pluck('email'); + } + + public function find(int $id): ?User + { + return User::find($id); + } + + public function get(): mixed + { + return User::query()->get(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationBuilderUpdate.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationBuilderUpdate.php new file mode 100644 index 0000000..81a9b7b --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationBuilderUpdate.php @@ -0,0 +1,23 @@ +`. + // The receiver type-check accepts any subtype of + // `Illuminate\Database\Eloquent\Builder` — the generic parameter is + // not unwrapped. `$builder->update([...])` returns int (rows affected) + // and bypasses model events, audit observers, and the explicit- + // hydration contract from ADR-0019. + return User::query() + ->where('email', 'inactive@example.test') + ->update(['name' => 'INACTIVE']); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceDelete.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceDelete.php new file mode 100644 index 0000000..5ffffbc --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceDelete.php @@ -0,0 +1,15 @@ +delete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceForceDelete.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceForceDelete.php new file mode 100644 index 0000000..fd20c00 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceForceDelete.php @@ -0,0 +1,15 @@ +forceDelete(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceSave.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceSave.php new file mode 100644 index 0000000..6deacd3 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceSave.php @@ -0,0 +1,18 @@ +name = 'Updated'; + $user->save(); + + return $user; + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceUpdateArray.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceUpdateArray.php new file mode 100644 index 0000000..89a095c --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceUpdateArray.php @@ -0,0 +1,17 @@ +update(['name' => $name]); + + return $user; + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationKendoCentralSubnamespace.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationKendoCentralSubnamespace.php new file mode 100644 index 0000000..716c109 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationKendoCentralSubnamespace.php @@ -0,0 +1,23 @@ +save(); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationMultipleInOneMethod.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationMultipleInOneMethod.php new file mode 100644 index 0000000..a062908 --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationMultipleInOneMethod.php @@ -0,0 +1,18 @@ +save(); + $post->delete(); + User::create(['name' => 'Whoops', 'email' => 'whoops@example.test']); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticCreate.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticCreate.php new file mode 100644 index 0000000..2f705fc --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticCreate.php @@ -0,0 +1,15 @@ + $name, 'email' => $email]); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticDestroy.php b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticDestroy.php new file mode 100644 index 0000000..c418fbb --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/ViolationStaticDestroy.php @@ -0,0 +1,15 @@ + $email], + ['name' => $name], + ); + } +} diff --git a/tests/Fixtures/ForbidEloquentMutationInControllers/_stubs.php b/tests/Fixtures/ForbidEloquentMutationInControllers/_stubs.php new file mode 100644 index 0000000..0131ade --- /dev/null +++ b/tests/Fixtures/ForbidEloquentMutationInControllers/_stubs.php @@ -0,0 +1,109 @@ +save()` + `$post->delete()` in the same + * controller method, two separate violations at distinct lines on distinct + * receivers). + */ +final class Post extends Model +{ + public int $id = 1; + + public string $title = ''; +} + +namespace App\Services; + +/** + * Non-Eloquent service receiver — exercises the rule's receiver-type gate. + * A controller calling `$service->save()` on this class MUST NOT fire because + * `MyService` is not a subtype of `Illuminate\Database\Eloquent\Model`. + */ +final class MyService +{ + public function save(): bool + { + return true; + } + + public function delete(): bool + { + return true; + } +} + +namespace App\DataTransferObjects\Input\User; + +/** + * Action-input DTO. Used by the action-delegation compliant fixture to exercise + * the legitimate Controller → DTO → Action → Model pipeline. + */ +final readonly class CreateUserInput +{ + public function __construct( + public string $name, + public string $email, + ) {} +} + +namespace App\DataTransferObjects\Result\User; + +use App\Models\User; + +/** + * Action-return shape. Used by the action-delegation compliant fixture. + */ +final readonly class CreateUserResult +{ + public function __construct( + public User $user, + ) {} +} + +namespace App\Actions\User; + +use App\DataTransferObjects\Input\User\CreateUserInput; +use App\DataTransferObjects\Result\User\CreateUserResult; +use App\Models\User; + +/** + * Stub Action — invoked from the action-delegation compliant fixture to show + * the canonical shape (`$this->action->execute($dto)`). + */ +final readonly class CreateUserAction +{ + public function execute(CreateUserInput $input): CreateUserResult + { + return new CreateUserResult(new User); + } +} diff --git a/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php b/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php new file mode 100644 index 0000000..4b3bad9 --- /dev/null +++ b/tests/Rules/ForbidEloquentMutationInControllersRuleTest.php @@ -0,0 +1,202 @@ + + */ +final class ForbidEloquentMutationInControllersRuleTest extends RuleTestCase +{ + public function testCompliantReadOnlyController(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/CompliantReadOnlyController.php'], + [], + ); + } + + public function testCompliantActionDelegation(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/CompliantActionDelegation.php'], + [], + ); + } + + public function testCompliantNonControllerMutation(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/CompliantNonControllerMutation.php'], + [], + ); + } + + public function testCompliantNonModelReceiver(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/CompliantNonModelReceiver.php'], + [], + ); + } + + public function testViolationInstanceSave(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceSave.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationInstanceSave', 'save', 'User'), + 14, + ], + ], + ); + } + + public function testViolationInstanceUpdateArray(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceUpdateArray.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationInstanceUpdateArray', 'update', 'User'), + 13, + ], + ], + ); + } + + public function testViolationInstanceDelete(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceDelete.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationInstanceDelete', 'delete', 'Post'), + 13, + ], + ], + ); + } + + public function testViolationInstanceForceDelete(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationInstanceForceDelete.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationInstanceForceDelete', 'forceDelete', 'Post'), + 13, + ], + ], + ); + } + + public function testViolationStaticCreate(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationStaticCreate.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationStaticCreate', 'create', 'User'), + 13, + ], + ], + ); + } + + public function testViolationStaticDestroy(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationStaticDestroy.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationStaticDestroy', 'destroy', 'User'), + 13, + ], + ], + ); + } + + public function testViolationStaticUpdateOrCreate(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationStaticUpdateOrCreate.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationStaticUpdateOrCreate', 'updateOrCreate', 'User'), + 13, + ], + ], + ); + } + + public function testViolationMultipleInOneMethod(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationMultipleInOneMethod.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationMultipleInOneMethod', 'save', 'User'), + 14, + ], + [ + $this->message('App\Http\Controllers\ViolationMultipleInOneMethod', 'delete', 'Post'), + 15, + ], + [ + $this->message('App\Http\Controllers\ViolationMultipleInOneMethod', 'create', 'User'), + 16, + ], + ], + ); + } + + public function testViolationKendoCentralSubnamespace(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationKendoCentralSubnamespace.php'], + [ + [ + $this->message('App\Http\Controllers\Central\IssueController', 'save', 'Post'), + 21, + ], + ], + ); + } + + public function testViolationBuilderUpdate(): void + { + $this->analyse( + [__DIR__ . '/../Fixtures/ForbidEloquentMutationInControllers/ViolationBuilderUpdate.php'], + [ + [ + $this->message('App\Http\Controllers\ViolationBuilderUpdate', 'update', 'Builder'), + 19, + ], + ], + ); + } + + protected function getRule(): Rule + { + return new ForbidEloquentMutationInControllersRule; + } + + private function message(string $classFqcn, string $method, string $receiverShortName): string + { + return sprintf( + 'Controller %s must not call Eloquent persistence method %s() on %s — delegate to an Action (ADR-0011 + ADR-0019).', + $classFqcn, + $method, + $receiverShortName, + ); + } +}