diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d6147c..0aad2e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,11 +12,18 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and - `EnforceCurrentUserAttributeRule` — flags calls to `Request::user()` / `Auth::user()` / `auth()->user()` inside classes in the `App\Http\Controllers` namespace. The canonical fix is Laravel's `#[\Illuminate\Container\Attributes\CurrentUser]` container attribute on the method parameter — eliminates the implicit-nullable-then-assert dance (`$user = $request->user(); assert($user instanceof User);`) introduced by emmie PR #263 (EMMIE-0197) and recurring across the war-room fleet. Doctrine: war-room §Architectural Principles — Explicit over implicit. Identifier: `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser`. Detection branches on three call shapes via `CallLike` registration (mirrors `LogRule` v0.3.0 shape): `MethodCall` on `Illuminate\Http\Request` subtype receiver (type-based via `ObjectType::isSuperTypeOf()`); `MethodCall` whose receiver is a `FuncCall('auth')` (AST-shape match — the helper's return type is unloaded in stub-only environments); `StaticCall` resolving to `Illuminate\Support\Facades\Auth` (FQCN comparison via `$scope->resolveName()`). Scoped to controllers via the `App\Http\Controllers` namespace prefix (`$scope->getNamespace()` + `str_starts_with`) — mirrors `ForbidEloquentMutationInControllersRule` and the canonical "controllers are identified by the `App\Http\Controllers` namespace" convention. FormRequest (`App\Http\Requests`, where `$this->user()` is canonical because container-attribute injection does not apply to `FormRequest::rules()` / `toDto()` / `authorize()` invocations), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix. Origin: war-room cross-territory recon 2026-05-22 (50+ violations across codebook, ublgenie, entreezuil, emmie; kendo already clean with 30 adopted sites). **Versioning: per ADR-0021 §Versioning, candidate Major bump (the rule surfaces new errors in already-clean code wherever a consumer territory has un-migrated controllers). The release PR will determine whether this collapses into the existing v0.3.0 [Unreleased] block (already a Major) or cuts as a separate Major (v0.4.0) after v0.3.0 ships.** **Pre-cascade audit required before tagging** — consuming territories will need either Medic dispatches to migrate (ublgenie 6 sites, entreezuil 3 sites, emmie 2 sites) or PHPStan-baseline staging (codebook ~40+ sites — safer than mass-edit on lightly-staffed AVG/NEN-7510-downstream territory). Kendo gets a constraint bump only (zero violations). Multi-guard ergonomics (`#[CurrentUser] Client $client` resolves via client guard, `#[CurrentUser] User $user` via user guard — verified in emmie's `ClientController::me`) work as expected: Laravel dispatches by typed parameter. Out of scope v1: `Auth::guard('name')->user()` and other guard-specific resolution paths — rare, substitution is more nuanced (`#[CurrentUser(guard: 'name')]`), do not appear in the recon yield. - `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. ### Fixed - `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/CLAUDE.md b/CLAUDE.md index 4103457..e0a85f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,10 +29,11 @@ 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` | | `EnforceCurrentUserAttributeRule` | War-room §Explicit over implicit | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | | `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 8dddada..5944fa7 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). | | `EnforceCurrentUserAttributeRule` | `enforceCurrentUserAttribute.useAttributeInsteadOfRequestUser` | `Request::user()` / `Auth::user()` / `auth()->user()` calls inside `App\Http\Controllers\*` classes (namespace prefix, incl. sub-namespaces) | Use `#[\Illuminate\Container\Attributes\CurrentUser] User $user` on the method parameter. Scope is decided by namespace, not class ancestry — a base-less `final` controller in `App\Http\Controllers` fires; FormRequests (`App\Http\Requests`), middleware (`App\Http\Middleware`), services, Actions (`App\Actions`), jobs, and console commands are silent because their namespaces do not start with the controller prefix (container-attribute injection does not apply to FormRequest methods regardless). | ### `EnforceActionTransactionsRule` — write-method list @@ -83,6 +84,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. diff --git a/extension.neon b/extension.neon index 4c936b5..32bcfd0 100644 --- a/extension.neon +++ b/extension.neon @@ -3,10 +3,21 @@ 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. Single backslashes — see the + # NEON-quoting note above. + formRequestBaseClass: 'Illuminate\Foundation\Http\FormRequest' parametersSchema: resourceDataBaseClass: string() + formRequestBaseClass: string() services: - @@ -38,6 +49,11 @@ services: arguments: resourceDataBaseClass: %resourceDataBaseClass% tags: [phpstan.rules.rule] + - + class: ScriptDevelopment\PhpstanWarroomRules\Rules\EnforceFormRequestToDtoRule + arguments: + formRequestBaseClass: %formRequestBaseClass% + tags: [phpstan.rules.rule] - class: ScriptDevelopment\PhpstanWarroomRules\Rules\EnforceCurrentUserAttributeRule tags: [phpstan.rules.rule] 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/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/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..0a20b04 --- /dev/null +++ b/tests/Rules/EnforceFormRequestToDtoRuleTest.php @@ -0,0 +1,190 @@ + + */ +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 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 + // 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, + ], + ], + ); + } + + /** + * 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;