From 9d3bb72a42ddd9457e65301d71c8838ac63cc7f6 Mon Sep 17 00:00:00 2001 From: Gerard Date: Wed, 10 Jun 2026 10:19:50 +0200 Subject: [PATCH 1/3] engineer: add EnforceFormRequestToDtoRule (queue #55 instance 2, Phase-2 promotion) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flags concrete FormRequest subclasses that neither declare nor inherit a toDto() method. Doctrine: ADR-0012 (FormRequest -> DTO Flow). Identifier: enforceFormRequestToDto.missingToDtoMethod. Promoted from entreezuil's reflection-based Pest arch test (tests/Arch/FormRequestsTest.php) — the second instance of the 'arch test detects misuse but not omission' shape under war-room enforcement queue #55 (WR-0066). Sister of EnforceResourceDataValidatorOptInRule (instance 3, PR #20): same parameterized-base shape via the formRequestBaseClass PHPStan parameter (default Illuminate\Foundation\Http\FormRequest). Abstract classes exempt (BaseFormRequest intermediates); inherited and trait-provided toDto() satisfy the contract (method_exists parity with the source-of-truth Pest matcher); short-name collisions do not match (FQCN ancestor traversal). Per-territory legitimate exemptions (LoginRequest) migrate to consumer phpstan.neon ignoreErrors per package convention. Implementation note: constructor default uses FormRequest::class instead of an FQCN string literal — Pint's class_keyword fixer class_exists()'s class-shaped string literals and the Pint phar bundles a real FormRequest with an unbundled trait dependency, fataling composer format. Co-Authored-By: Claude Fable 5 --- extension.neon | 12 ++ src/Rules/EnforceFormRequestToDtoRule.php | 122 ++++++++++++++++++ .../FormRequestToDto/AbstractBaseRequest.php | 18 +++ .../CompliantInheritedToDtoRequest.php | 36 ++++++ .../CompliantToDtoRequest.php | 29 +++++ .../UnrelatedShortNameCollision.php | 23 ++++ .../FormRequestToDto/ViolatorRequest.php | 20 +++ tests/Fixtures/FormRequestToDto/_stubs.php | 35 +++++ .../Rules/EnforceFormRequestToDtoRuleTest.php | 111 ++++++++++++++++ 9 files changed, 406 insertions(+) create mode 100644 src/Rules/EnforceFormRequestToDtoRule.php create mode 100644 tests/Fixtures/FormRequestToDto/AbstractBaseRequest.php create mode 100644 tests/Fixtures/FormRequestToDto/CompliantInheritedToDtoRequest.php create mode 100644 tests/Fixtures/FormRequestToDto/CompliantToDtoRequest.php create mode 100644 tests/Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php create mode 100644 tests/Fixtures/FormRequestToDto/ViolatorRequest.php create mode 100644 tests/Fixtures/FormRequestToDto/_stubs.php create mode 100644 tests/Rules/EnforceFormRequestToDtoRuleTest.php diff --git a/extension.neon b/extension.neon index 0034fb9..15f7cbd 100644 --- a/extension.neon +++ b/extension.neon @@ -5,8 +5,15 @@ parameters: # `phpstan.neon` if a territory ships the base under a different FQCN. resourceDataBaseClass: 'App\\Http\\Resources\\ResourceData' + # `EnforceFormRequestToDtoRule`: FQCN of the FormRequest base class. + # Default matches the Laravel framework base every territory's requests + # ultimately extend. Override per consumer `phpstan.neon` to narrow the + # contract to a territory-local base FQCN. + formRequestBaseClass: 'Illuminate\\Foundation\\Http\\FormRequest' + parametersSchema: resourceDataBaseClass: string() + formRequestBaseClass: string() services: - @@ -38,6 +45,11 @@ services: arguments: resourceDataBaseClass: %resourceDataBaseClass% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\EnforceFormRequestToDtoRule + arguments: + formRequestBaseClass: %formRequestBaseClass% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Type\ConnectionTransactionReturnTypeExtension tags: [phpstan.broker.dynamicMethodReturnTypeExtension] diff --git a/src/Rules/EnforceFormRequestToDtoRule.php b/src/Rules/EnforceFormRequestToDtoRule.php new file mode 100644 index 0000000..a19fcc7 --- /dev/null +++ b/src/Rules/EnforceFormRequestToDtoRule.php @@ -0,0 +1,122 @@ +validated()` arrays to + * Actions — untyped, key-renameable, and invisible to static analysis. + * + * Doctrine source: ADR-0012 (FormRequest → DTO Flow). Promoted from + * entreezuil's reflection-based Pest arch test + * (`tests/Arch/FormRequestsTest.php`, "form requests with mutation actions + * define toDto method") — the second instance of the "arch test detects + * misuse but not omission" enforcement shape under war-room enforcement + * queue #55, dispositioned for Phase-2 promotion by the Commander on + * 2026-05-07. Sister of `EnforceResourceDataValidatorOptInRule` (instance 3, + * PR #20). + * + * Scope: classes whose ancestor chain includes the configured base FQCN + * (default: `Illuminate\Foundation\Http\FormRequest`). Inheritance is + * matched via PHPStan reflection — short-name collisions in unrelated + * namespaces do not fire. Abstract classes are skipped (the per-territory + * `BaseFormRequest` shape is an intermediate layer, not a mutation request). + * + * Detection (all three must hold): + * 1. Class transitively extends the configured base class. + * 2. Class is concrete (abstract intermediates are exempt). + * 3. Class neither declares nor inherits a `toDto()` method — own + * declarations, parent-class declarations, and trait-provided methods + * all satisfy the contract (mirroring the source-of-truth Pest test's + * `method_exists()` matcher). + * + * Legitimately DTO-less requests (entreezuil precedent: `LoginRequest`, + * whose auth flow calls `AuthManager::attempt()` directly) are suppressed + * per consumer `phpstan.neon` `ignoreErrors` keyed on the identifier — + * never by name inside the rule, per the package convention. + * + * Implementation note: the constructor default uses `FormRequest::class` + * (compile-time constant, never autoloads) instead of an FQCN string + * literal. Pint's class_keyword fixer calls class_exists() on string + * literals that look like class names, and the Pint phar bundles a real + * `Illuminate\Foundation\Http\FormRequest` whose ValidatesWhenResolvedTrait + * dependency is NOT bundled — a bare FQCN string literal anywhere in this + * package's PHP source makes `composer format` fatal with "Trait not + * found". The `use` import is alias-only; consumers analysing non-Laravel + * trees are unaffected because `::class` resolution requires no autoload. + * + * @implements Rule + */ +final class EnforceFormRequestToDtoRule implements Rule +{ + private const string DTO_METHOD_NAME = 'toDto'; + + public function __construct( + private string $formRequestBaseClass = FormRequest::class, + ) {} + + public function getNodeType(): string + { + return InClassNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $classNode = $node->getOriginalNode(); + + if (!$classNode instanceof Class_) { + return []; + } + + if ($classNode->isAbstract()) { + return []; + } + + $classReflection = $node->getClassReflection(); + + if (!$this->extendsFormRequestBase($classReflection)) { + return []; + } + + if ($classReflection->hasNativeMethod(self::DTO_METHOD_NAME)) { + return []; + } + + return [ + RuleErrorBuilder::message(sprintf( + '%s extends FormRequest but does not define a toDto() method — raw validated-array handoff risk (ADR-0012 / war-room queue #55 / entreezuil FormRequestsTest opt-in invariant).', + $classReflection->getName(), + )) + ->identifier('enforceFormRequestToDto.missingToDtoMethod') + ->line($classNode->getStartLine()) + ->build(), + ]; + } + + /** + * Inheritance gate: the class must be a (transitive) subclass of the + * configured base FQCN. Uses PHPStan reflection — handles intermediate + * abstract layers and namespace-relative `extends` clauses. Short-name + * collisions in unrelated namespaces do not match, and the base class + * itself is not a subclass of itself. + */ + private function extendsFormRequestBase(ClassReflection $classReflection): bool + { + return $classReflection->isSubclassOf($this->formRequestBaseClass); + } +} diff --git a/tests/Fixtures/FormRequestToDto/AbstractBaseRequest.php b/tests/Fixtures/FormRequestToDto/AbstractBaseRequest.php new file mode 100644 index 0000000..38bd683 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/AbstractBaseRequest.php @@ -0,0 +1,18 @@ +validated()['name']; + + return new StoreUserData($name); + } +} + +final class CompliantInheritedToDtoRequest extends RequestWithSharedDto +{ + /** + * @return array + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } +} diff --git a/tests/Fixtures/FormRequestToDto/CompliantToDtoRequest.php b/tests/Fixtures/FormRequestToDto/CompliantToDtoRequest.php new file mode 100644 index 0000000..8546d44 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/CompliantToDtoRequest.php @@ -0,0 +1,29 @@ + + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } + + public function toDto(): StoreUserData + { + /** @var string $name */ + $name = $this->validated()['name']; + + return new StoreUserData($name); + } +} diff --git a/tests/Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php b/tests/Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php new file mode 100644 index 0000000..7cf88d0 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php @@ -0,0 +1,23 @@ + + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } +} diff --git a/tests/Fixtures/FormRequestToDto/ViolatorRequest.php b/tests/Fixtures/FormRequestToDto/ViolatorRequest.php new file mode 100644 index 0000000..bcc5ee9 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/ViolatorRequest.php @@ -0,0 +1,20 @@ + + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } +} diff --git a/tests/Fixtures/FormRequestToDto/_stubs.php b/tests/Fixtures/FormRequestToDto/_stubs.php new file mode 100644 index 0000000..6aa2a66 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/_stubs.php @@ -0,0 +1,35 @@ + + */ + public function validated(): array + { + // No-op for fixtures — the production base in consuming territories + // returns the validator's validated payload. + return []; + } +} + +namespace App\DataTransferObjects; + +final readonly class StoreUserData +{ + public function __construct( + public string $name = '', + ) {} +} diff --git a/tests/Rules/EnforceFormRequestToDtoRuleTest.php b/tests/Rules/EnforceFormRequestToDtoRuleTest.php new file mode 100644 index 0000000..5fee0be --- /dev/null +++ b/tests/Rules/EnforceFormRequestToDtoRuleTest.php @@ -0,0 +1,111 @@ + + */ +final class EnforceFormRequestToDtoRuleTest extends RuleTestCase +{ + /** + * Override hook: when set, `getRule()` returns this instance instead of + * the default. Lets a single test reconfigure the base FQCN parameter. + */ + private ?Rule $ruleOverride = null; + + public function testViolatorWithoutToDtoIsFlagged(): void + { + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/ViolatorRequest.php', + ], + [ + [ + 'App\Http\Requests\ViolatorRequest extends FormRequest but does not define a toDto() method — raw validated-array handoff risk (ADR-0012 / war-room queue #55 / entreezuil FormRequestsTest opt-in invariant).', + 9, + ], + ], + ); + } + + public function testCompliantOwnToDtoIsNotFlagged(): void + { + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/CompliantToDtoRequest.php', + ], + [], + ); + } + + public function testCompliantInheritedToDtoIsNotFlagged(): void + { + // The abstract intermediate declares toDto(); the concrete leaf + // inherits it. Neither fires — abstract classes are exempt, and + // inherited declarations satisfy the contract (mirroring the + // source-of-truth Pest test's method_exists() matcher). + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/CompliantInheritedToDtoRequest.php', + ], + [], + ); + } + + public function testAbstractBaseWithoutToDtoIsNotFlagged(): void + { + // The per-territory `BaseFormRequest` shape: abstract, extends the + // framework base, declares no toDto(). Abstract classes are exempt. + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/AbstractBaseRequest.php', + ], + [], + ); + } + + public function testUnrelatedShortNameCollisionIsNotFlaggedUnderDefaultBase(): void + { + // The class extends `App\Unrelated\FormRequest`, not the configured + // default base `Illuminate\Foundation\Http\FormRequest`. Short-name + // collision must NOT match — verifying the rule's FQCN-based + // inheritance gate (vs short-name string match). + $this->analyse( + [__DIR__ . '/../Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php'], + [], + ); + } + + public function testCustomBaseClassParameterMatchesAlternativeFqcn(): void + { + // Re-run the same fixture with the parameter overridden to point at + // the alternative base FQCN — must now fire. Proves the + // `formRequestBaseClass` parameter is honored end-to-end. + $this->ruleOverride = new EnforceFormRequestToDtoRule('App\Unrelated\FormRequest'); + + $this->analyse( + [__DIR__ . '/../Fixtures/FormRequestToDto/UnrelatedShortNameCollision.php'], + [ + [ + 'App\Unrelated\UnrelatedShortNameCollision extends FormRequest but does not define a toDto() method — raw validated-array handoff risk (ADR-0012 / war-room queue #55 / entreezuil FormRequestsTest opt-in invariant).', + 12, + ], + ], + ); + } + + protected function getRule(): Rule + { + return $this->ruleOverride ?? new EnforceFormRequestToDtoRule; + } +} From 6d8685078ab38cad8725b67dc97cc03822af137b Mon Sep 17 00:00:00 2001 From: Gerard Date: Wed, 10 Jun 2026 10:19:50 +0200 Subject: [PATCH 2/3] docs: CHANGELOG + CLAUDE.md + README for EnforceFormRequestToDtoRule CHANGELOG [Unreleased] Added entry with candidate-Major versioning flag and pre-cascade audit demand; CLAUDE.md rules-shipped table row + Phase 2 narrative (third Phase 2 addition); README rules table row + configurable base class and exemptions section. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + CLAUDE.md | 3 ++- README.md | 24 ++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b61a0d5..c3dbe9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and - **Tests:** direct type-inference coverage for `ConnectionTransactionReturnTypeExtension` via a `PHPStan\Testing\TypeInferenceTestCase` (`tests/Type/ConnectionTransactionReturnTypeExtensionTest.php` + fixture `tests/Fixtures/ConnectionTransactionReturnType/transaction-return-type.php`). The extension previously had no direct test — it was only exercised implicitly by audit-snapshot rule fixtures, none of which asserted the resolved return type. The new fixture loads `extension.neon` (same config consumers register) and `assertType()`s the inferred type of `$connection->transaction(...)` for closures returning a constant scalar, an object/DTO, a nullable, an array shape, and a widened (non-constant) scalar — pinning that the extension forwards the closure acceptor's return type rather than `mixed`. Test-only; no consumer-facing surface. Closes Quartermaster F-2. **Versioning:** none (internal test coverage). - `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.** +- `EnforceFormRequestToDtoRule` — flags concrete classes extending `Illuminate\Foundation\Http\FormRequest` that neither declare nor inherit a `toDto()` method. Without the method, controllers hand `$request->validated()` arrays to Actions — untyped, key-renameable, and invisible to static analysis; the typed-DTO handoff is the ADR-0012 contract. Doctrine: ADR-0012 (FormRequest → DTO Flow). Identifier: `enforceFormRequestToDto.missingToDtoMethod`. Promoted from entreezuil's reflection-based Pest arch test (`tests/Arch/FormRequestsTest.php`, "form requests with mutation actions define toDto method") — the second instance of the "arch test detects misuse but not omission" enforcement shape under war-room enforcement queue #55 (Commander dispositioned the Phase-2 promotion 2026-05-07; war-room board WR-0066). Sister of `EnforceResourceDataValidatorOptInRule` (queue #55 instance 3, PR #20) — same opt-in-omission pedagogy, same parameterized-base shape. kendo carries only the weaker misuse-only form; the stronger entreezuil omission semantic is what ships here. Inheritance is matched via PHPStan reflection (FQCN ancestor traversal) — short-name collisions in unrelated namespaces do NOT match. The base FQCN is parameterizable via the `formRequestBaseClass` PHPStan parameter (default: `Illuminate\Foundation\Http\FormRequest`); territories can narrow the contract to a territory-local base per consumer `phpstan.neon`. Abstract classes are exempt (the per-territory `BaseFormRequest` intermediate is not a mutation request); inherited and trait-provided `toDto()` declarations satisfy the contract (mirroring the source-of-truth Pest test's `method_exists()` matcher). Legitimately DTO-less requests (entreezuil precedent: `LoginRequest`, whose auth flow calls `AuthManager::attempt()` directly) are suppressed per consumer `phpstan.neon` `ignoreErrors` keyed on the identifier — never by name inside the rule, per the package convention. **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has a concrete FormRequest without a `toDto()` method — read-only/query requests included until suppressed). Pre-cascade audit required across ISMS, kendo, emmie, entreezuil, ublgenie, brick-inventory, codebook before tagging** — entreezuil's Pest arch test already closed its own violators, but every other consumer territory enforces at most the weaker misuse-only shape and may carry undetected omissions. Sister extraction for the routes `->can()` middleware omission shape (queue #55 instance #1, WR-0067) remains deferred to a separate dispatch. ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index d133930..4daf860 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,9 +29,10 @@ Composer package distributing war-room-doctrine PHPStan rules across `script-dev | `EnforceAuditTransactionScopeRule` | ADR-0029 | `enforceAuditTransactionScope.nonTransactionalMutationInClosure` | | `ForbidEloquentMutationInControllersRule` | ADR-0011 + ADR-0019 | `forbidEloquentMutationInControllers.eloquentMutationInController` | | `EnforceResourceDataValidatorOptInRule` | ADR-0009 §EAGER_LOAD validator opt-in | `enforceResourceDataValidatorOptIn.missingValidatorCall` | +| `EnforceFormRequestToDtoRule` | ADR-0012 §FormRequest → DTO Flow | `enforceFormRequestToDto.missingToDtoMethod` | | `ConnectionTransactionReturnTypeExtension` | (type extension, no rule) | — | -Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. +Phase 2 expands the rule set: `EnforceAuditSnapshotOnRetryRule` (ADR-0001 §Snapshot-on-Retry Safety) was the first Phase 2 addition, promoted from cross-territory Pest arch tests (emmie PR #187, entreezuil PR #139, ublgenie PR #166, kendo PR #1029). `EnforceResourceDataValidatorOptInRule` (ADR-0009 §EAGER_LOAD validator opt-in) is the second Phase 2 addition, promoted from kendo PR #1084 under war-room enforcement queue #55. `EnforceFormRequestToDtoRule` (ADR-0012) is the third Phase 2 addition, promoted from entreezuil's `tests/Arch/FormRequestsTest.php` under the same queue #55 (instance 2). `EnforceExplicitHydrationRule` (ADR-0019) is the next Phase 2 candidate. ## Conventions diff --git a/README.md b/README.md index 78af118..57af490 100644 --- a/README.md +++ b/README.md @@ -46,6 +46,7 @@ includes: | `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. | +| `EnforceFormRequestToDtoRule` | `enforceFormRequestToDto.missingToDtoMethod` | Concrete classes extending `Illuminate\Foundation\Http\FormRequest` | If the class neither declares nor inherits a `toDto()` method, error. Abstract intermediates (`BaseFormRequest`) are exempt. Hand Actions a typed DTO, not `$request->validated()` arrays. Doctrine: ADR-0012 (FormRequest → DTO Flow). | ### `EnforceActionTransactionsRule` — write-method list @@ -82,6 +83,29 @@ parameters: Inheritance is matched via PHPStan reflection (FQCN ancestor traversal), not short-name matching — a class named `ResourceData` in an unrelated namespace will not be matched. Compliant call shapes are `self::validateRelationsLoaded($model)`, `static::validateRelationsLoaded($model)`, and `$this->validateRelationsLoaded($model)` — the production base method is `protected static`, but the instance form is also accepted for compatibility with the source-of-truth Pest arch test's permissive matcher. Empty-array constants (`EAGER_LOAD_COUNT = []`) do not fire — they are no-ops. +### `EnforceFormRequestToDtoRule` — configurable base class + exemptions + +The rule scopes to concrete classes extending `Illuminate\Foundation\Http\FormRequest` by default. To narrow the contract to a territory-local base FQCN, override the `formRequestBaseClass` parameter in `phpstan.neon`: + +```neon +parameters: + formRequestBaseClass: 'App\Http\Requests\BaseFormRequest' +``` + +Inheritance is matched via PHPStan reflection (FQCN ancestor traversal), not short-name matching. Abstract classes never fire — a per-territory abstract `BaseFormRequest` intermediate is exempt by shape, not by name. A `toDto()` declared on a parent class or provided by a trait satisfies the contract (mirroring the source-of-truth entreezuil Pest arch test's `method_exists()` matcher). + +Legitimately DTO-less requests (e.g. a `LoginRequest` whose auth flow calls `AuthManager::attempt()` directly, or read-only filter/query requests) are suppressed per territory via `phpstan.neon` — never by name inside the rule: + +```neon +parameters: + ignoreErrors: + - + identifier: enforceFormRequestToDto.missingToDtoMethod + path: app/Http/Requests/LoginRequest.php +``` + +Each ignore should carry a comment with rationale. + ### Action namespace assumption `EnforceActionTransactionsRule` and `ForbidDatabaseManagerInActionsRule` only fire on classes whose namespace starts with `App\Actions`. This matches the Laravel convention used in every `script-development` territory. Territories using a different actions namespace should open a PR to make this configurable. From ef0c5b1d9b3b0f59173f2ddf2a1a2db323cc7f5a Mon Sep 17 00:00:00 2001 From: Gerard Date: Fri, 12 Jun 2026 11:28:10 +0200 Subject: [PATCH 3/3] fix: NEON double-backslash no-op in formRequestBaseClass + resourceDataBaseClass defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shipped extension.neon parameter defaults used double-backslash single-quoted FQCNs. NEON only unescapes \\ inside double-quoted strings; in single quotes the backslashes stay literal, so the value decoded to a class name matching nothing and ClassReflection::isSubclassOf() returned false for every analysed class — both EnforceFormRequestToDtoRule and (pre-existing, since PR #20) EnforceResourceDataValidatorOptInRule were silent no-ops for any consumer using the default. CI stayed green because every test constructs the rule directly via the PHP ::class constructor default, never exercising the NEON registration path. Fixes (PR #33 review — jasperboerhof BLOCKER + MAJOR + MINOR, General-review concerns): - extension.neon: single-backslash defaults + inline NEON-quoting warning. - container-resolved regression test per rule (getAdditionalConfigFiles + getByType) pinning the NEON default and %parameter% wiring; both confirmed to fail when the double-backslash defect is reintroduced. - TraitProvidedToDtoRequest fixture + test (trait-provided toDto() satisfies the contract via hasNativeMethod trait flattening). - TransitiveViolatorRequest fixture + test (concrete leaf extends abstract base without toDto() — transitive omission must fire at the leaf). Gates: composer test (107), phpstan, format:check, coverage (88.06% >= 83) green. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 6 ++ extension.neon | 10 ++- .../TraitProvidedToDtoRequest.php | 39 +++++++++ .../TransitiveViolatorRequest.php | 24 ++++++ .../Rules/EnforceFormRequestToDtoRuleTest.php | 79 +++++++++++++++++++ ...orceResourceDataValidatorOptInRuleTest.php | 39 +++++++++ 6 files changed, 194 insertions(+), 3 deletions(-) create mode 100644 tests/Fixtures/FormRequestToDto/TraitProvidedToDtoRequest.php create mode 100644 tests/Fixtures/FormRequestToDto/TransitiveViolatorRequest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 2285840..0aad2e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and - `EnforceCurrentUserAttributeRule` — corrected the controller-detection gate from an ancestry check (`ClassReflection::isSubclassOf(Illuminate\Routing\Controller)`) to a namespace prefix check (`$scope->getNamespace()` starts with `App\Http\Controllers`). The ancestry gate was a **silent no-op**: every consumer territory (kendo, ublgenie, entreezuil) ships base-less `final` controllers with no `extends Controller`, so the ancestry walk matched **zero** controllers and the rule enforced nothing. The namespace gate mirrors the sibling `ForbidEloquentMutationInControllersRule` and the canonical "controllers are identified by the `App\Http\Controllers` namespace" convention. Caught by the agent-review sweep on PR #26 (review `pullrequestreview-4401182606`). Regression-proofed by a base-less `final` controller fixture (`RequestUserInBaselessController` — flagged; `CurrentUserAttributeInBaselessController` — clean) that reproduces the exact real-world shape the rule missed. **Versioning:** false-negative closure — the rule now actually fires where it always intended to; this is part of the same `[Unreleased]` Major bump the rule's addition already carries (consumers with un-migrated controllers will now see the errors). No new identifier; no consumer `ignoreErrors` migration shape change. +- `EnforceFormRequestToDtoRule` / `EnforceResourceDataValidatorOptInRule` — corrected the shipped `extension.neon` parameter defaults from double-backslash single-quoted FQCNs (`'Illuminate\\Foundation\\Http\\FormRequest'`, `'App\\Http\\Resources\\ResourceData'`) to single-backslash (`'Illuminate\Foundation\Http\FormRequest'`, `'App\Http\Resources\ResourceData'`). NEON only unescapes `\\` inside **double**-quoted strings; in single quotes (and unquoted) `\\` stays two literal characters, so the parameter decoded to a 4-segment double-backslash class name that matches no real class. The effect: `ClassReflection::isSubclassOf()` returned false for every analysed class and **both rules were silent no-ops for any consumer registering `extension.neon` without an explicit parameter override** — CI stayed green because every PHPUnit test, the coverage gate, and Infection construct the rule directly via the PHP `::class` constructor default (PHP single-quoted strings *do* collapse `\\`), never exercising the NEON registration path. Verified empirically end-to-end: shipped default → `[OK] No errors` on the `ViolatorRequest` / `ViolatorResource` fixtures; single-backslash → fires with the exact expected message/line. The `resourceDataBaseClass` defect is pre-existing (shipped in PR #20, v0.3.0) — `EnforceResourceDataValidatorOptInRule` has been a no-op for default-configured consumers since release; fixed here in the same edit rather than deferred since `EnforceFormRequestToDtoRule` introduced the identical defect on the line above. Caught by the agent-review sweep on PR #33 (jasperboerhof BLOCKER). Regression-proofed by a container-resolved test per rule (`testRuleResolvesFromExtensionNeonAndFires` — resolves the rule from the PHPStan container via `getAdditionalConfigFiles()` + `getByType()`, exercising the NEON default and `%parameter%` wiring, then asserts the violator fires; both tests confirmed to fail when the double-backslash defect is reintroduced). An inline NEON-quoting warning comment now guards both defaults in `extension.neon`. **Versioning:** for `EnforceFormRequestToDtoRule`, this is part of the same `[Unreleased]` candidate-Major the rule's addition carries (the rule now actually fires in consumers). For `EnforceResourceDataValidatorOptInRule`, the v0.3.0 release that shipped the rule was a no-op for default consumers; restoring enforcement surfaces previously-undetected violations on `^0.3` consumers without a parameter override — **the pre-cascade audit demanded for both rules must now treat the resource-data rule as effectively un-enforced until this fix ships.** + +### Added + +- **Tests:** `EnforceFormRequestToDtoRule` gains two fixture-backed coverage additions closing documented-but-unpinned semantics: `TraitProvidedToDtoRequest` (a concrete request whose `toDto()` arrives via a trait — pins the trait leg of the `method_exists()` parity promise, which routes through PHPStan's `hasNativeMethod()` trait flattening) and `TransitiveViolatorRequest` (a concrete leaf extending the abstract `AbstractBaseRequest` with no `toDto()` anywhere in the chain — pins transitive-violation detection through an intermediate abstract layer, the inverse of the existing inherited-compliant case). Both raised in the PR #33 review (jasperboerhof MINOR + General-review concern). Test-only; no consumer-facing surface. **Versioning:** none (test coverage). + ### Changed - **CI:** pinned `symfony/console` to `^7.2` in `require-dev`. `symfony/console` 8.x (v8.1.0, released 2026-05-29) breaks Infection 0.33.x's mutation runner — its DI container references `Symfony\Component\Console\Helper\QuestionHelper` as a service Symfony Console 8 no longer registers that way, so `composer mutation:ci` aborts with `Unknown service` and exits 1. Because the package's `composer.lock` is gitignored, CI resolves dependencies fresh on every run; `illuminate/*` v13 permits Symfony 8, so the resolver began pulling v8.1.0 and the mutation gate went red fleet-wide (PRs green on 2026-05-28 turned red on 2026-05-29 with no source change). The pin holds the dev toolchain at `symfony/console` v7.4.x — verified mutation gate green (Covered Code MSI 81% ≥ 75) — until Infection ships Symfony Console 8 support, at which point this constraint should be widened or removed. **Versioning:** none (dev-only test-infra; no consumer-facing surface). diff --git a/extension.neon b/extension.neon index cfb29e2..32bcfd0 100644 --- a/extension.neon +++ b/extension.neon @@ -3,13 +3,17 @@ parameters: # base class. Default matches the kendo / emmie / ublgenie / entreezuil # convention `App\Http\Resources\ResourceData`. Override per consumer # `phpstan.neon` if a territory ships the base under a different FQCN. - resourceDataBaseClass: 'App\\Http\\Resources\\ResourceData' + # NOTE: single backslashes — NEON only unescapes `\\` inside DOUBLE quotes; + # in single quotes (and unquoted) `\\` stays two literal characters, which + # decodes to a class name that matches nothing and silently no-ops the rule. + resourceDataBaseClass: 'App\Http\Resources\ResourceData' # `EnforceFormRequestToDtoRule`: FQCN of the FormRequest base class. # Default matches the Laravel framework base every territory's requests # ultimately extend. Override per consumer `phpstan.neon` to narrow the - # contract to a territory-local base FQCN. - formRequestBaseClass: 'Illuminate\\Foundation\\Http\\FormRequest' + # contract to a territory-local base FQCN. Single backslashes — see the + # NEON-quoting note above. + formRequestBaseClass: 'Illuminate\Foundation\Http\FormRequest' parametersSchema: resourceDataBaseClass: string() diff --git a/tests/Fixtures/FormRequestToDto/TraitProvidedToDtoRequest.php b/tests/Fixtures/FormRequestToDto/TraitProvidedToDtoRequest.php new file mode 100644 index 0000000..79db3da --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/TraitProvidedToDtoRequest.php @@ -0,0 +1,39 @@ +validated()['name']; + + return new StoreUserData($name); + } +} + +final class TraitProvidedToDtoRequest extends FormRequest +{ + use ProvidesToDto; + + /** + * @return array + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } +} diff --git a/tests/Fixtures/FormRequestToDto/TransitiveViolatorRequest.php b/tests/Fixtures/FormRequestToDto/TransitiveViolatorRequest.php new file mode 100644 index 0000000..b25c852 --- /dev/null +++ b/tests/Fixtures/FormRequestToDto/TransitiveViolatorRequest.php @@ -0,0 +1,24 @@ + + */ + public function rules(): array + { + return [ + 'name' => ['required', 'string'], + ]; + } +} diff --git a/tests/Rules/EnforceFormRequestToDtoRuleTest.php b/tests/Rules/EnforceFormRequestToDtoRuleTest.php index 5fee0be..0a20b04 100644 --- a/tests/Rules/EnforceFormRequestToDtoRuleTest.php +++ b/tests/Rules/EnforceFormRequestToDtoRuleTest.php @@ -61,6 +61,71 @@ public function testCompliantInheritedToDtoIsNotFlagged(): void ); } + public function testTraitProvidedToDtoIsNotFlagged(): void + { + // A concrete request whose toDto() arrives via a trait. The rule + // routes through PHPStan's hasNativeMethod(), which flattens + // trait-composed methods — pins the trait leg of the contract that + // the docblock, README, and CHANGELOG all promise but no fixture + // previously exercised. + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/TraitProvidedToDtoRequest.php', + ], + [], + ); + } + + public function testConcreteLeafExtendingAbstractBaseWithoutToDtoIsFlagged(): void + { + // The inverse of testCompliantInheritedToDtoIsNotFlagged: a concrete + // leaf extends the abstract intermediate `AbstractBaseRequest`, which + // declares no toDto() anywhere in the chain. Transitive-violation + // detection must fire at the leaf — proving the ancestor traversal + // catches omission through an intermediate abstract layer, not only + // direct framework-base extension. + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/AbstractBaseRequest.php', + __DIR__ . '/../Fixtures/FormRequestToDto/TransitiveViolatorRequest.php', + ], + [ + [ + 'App\Http\Requests\TransitiveViolatorRequest extends FormRequest but does not define a toDto() method — raw validated-array handoff risk (ADR-0012 / war-room queue #55 / entreezuil FormRequestsTest opt-in invariant).', + 13, + ], + ], + ); + } + + public function testRuleResolvesFromExtensionNeonAndFires(): void + { + // End-to-end pin on the extension.neon registration path consumers + // actually use: resolve the rule from the PHPStan container so the + // shipped `formRequestBaseClass` default and the `%formRequestBaseClass%` + // argument wiring are exercised — NOT the PHP constructor default. + // A NEON quoting regression in the shipped default (e.g. the + // double-backslash single-quoted form) silently no-ops the rule while + // every direct-instantiation test stays green; this is the only gate + // that catches it. + $this->ruleOverride = self::getContainer()->getByType(EnforceFormRequestToDtoRule::class); + + $this->analyse( + [ + __DIR__ . '/../Fixtures/FormRequestToDto/_stubs.php', + __DIR__ . '/../Fixtures/FormRequestToDto/ViolatorRequest.php', + ], + [ + [ + 'App\Http\Requests\ViolatorRequest extends FormRequest but does not define a toDto() method — raw validated-array handoff risk (ADR-0012 / war-room queue #55 / entreezuil FormRequestsTest opt-in invariant).', + 9, + ], + ], + ); + } + public function testAbstractBaseWithoutToDtoIsNotFlagged(): void { // The per-territory `BaseFormRequest` shape: abstract, extends the @@ -104,6 +169,20 @@ public function testCustomBaseClassParameterMatchesAlternativeFqcn(): void ); } + /** + * Load the shipped extension.neon so testRuleResolvesFromExtensionNeonAndFires + * can pull the rule out of the container with its NEON-configured + * `formRequestBaseClass` parameter applied. + * + * @return array + */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + protected function getRule(): Rule { return $this->ruleOverride ?? new EnforceFormRequestToDtoRule; diff --git a/tests/Rules/EnforceResourceDataValidatorOptInRuleTest.php b/tests/Rules/EnforceResourceDataValidatorOptInRuleTest.php index e0f9dfb..0ab72d8 100644 --- a/tests/Rules/EnforceResourceDataValidatorOptInRuleTest.php +++ b/tests/Rules/EnforceResourceDataValidatorOptInRuleTest.php @@ -136,6 +136,45 @@ public function testCustomBaseClassParameterMatchesAlternativeFqcn(): void ); } + public function testRuleResolvesFromExtensionNeonAndFires(): void + { + // End-to-end pin on the extension.neon registration path consumers + // actually use: resolve the rule from the PHPStan container so the + // shipped `resourceDataBaseClass` default and the + // `%resourceDataBaseClass%` argument wiring are exercised — NOT the PHP + // constructor default. The shipped default carried a NEON + // double-backslash quoting defect that silently no-op'd this rule for + // every default consumer since PR #20; this gate catches it. + $this->ruleOverride = self::getContainer()->getByType(EnforceResourceDataValidatorOptInRule::class); + + $this->analyse( + [ + __DIR__ . '/../Fixtures/ResourceDataValidatorOptIn/_stubs.php', + __DIR__ . '/../Fixtures/ResourceDataValidatorOptIn/ViolatorResource.php', + ], + [ + [ + 'App\Http\Resources\ViolatorResource declares EAGER_LOAD_COUNT but does not call validateRelationsLoaded() — silent-zero bug risk (ADR-0009 / war-room queue #55 / kendo PR #1084 opt-in invariant).', + 9, + ], + ], + ); + } + + /** + * Load the shipped extension.neon so testRuleResolvesFromExtensionNeonAndFires + * can pull the rule out of the container with its NEON-configured + * `resourceDataBaseClass` parameter applied. + * + * @return array + */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/../../extension.neon', + ]; + } + protected function getRule(): Rule { return $this->ruleOverride ?? new EnforceResourceDataValidatorOptInRule;