diff --git a/tests/Architecture/FormPartialDefensiveReadsContractTest.php b/tests/Architecture/FormPartialDefensiveReadsContractTest.php new file mode 100644 index 0000000..299010d --- /dev/null +++ b/tests/Architecture/FormPartialDefensiveReadsContractTest.php @@ -0,0 +1,784 @@ +/_form.phtml + * modules/ * /pages//_form.phtml + * MUST satisfy at least one of: + * 1. The variable is initialized/assigned earlier in the same partial. + * 2. The read is itself defensively guarded (`$x ?? …`, `isset($x)`, + * `empty($x)`, `!empty($x)`, `array_key_exists(…, $x)`). + * 3. Every consuming view (`/(default).phtml`, + * `/(none).phtml`, …) defines the variable BEFORE the + * `require __DIR__ . '/_form.phtml'` (or `require __DIR__.'/_form.phtml'`). + * + * --- Allowlist --- + * Documented exceptions where a partial reads a variable that no consumer + * provides, but the partial is not actually entered on that consumer's path + * (e.g. behind an `if ($showSsoTab)` whose flag is false on that consumer). + * Today: empty. + * + * --- Static-analysis caveat --- + * The scanner is conservative. When it cannot statically classify a + * construct (e.g. `extract($vars)`, dynamic include paths, eval), the test + * fails explicitly with "cannot statically verify, please review manually" + * rather than passing silently. This mirrors the fail-loud behaviour of + * DetailDrawerFragmentContractTest and ActionContextCsrfPairingContractTest. + * + * --- Coverage guarantee --- + * A second test asserts that the scanner sees exactly the seven partials + * known at authoring time. If a new partial is added, that test fails until + * the count is bumped, ensuring the contract is reviewed for new call sites. + */ +class FormPartialDefensiveReadsContractTest extends TestCase +{ + use ProjectFileAssertionSupport; + + /** + * Per-partial allowlist of variable names that are read without + * defensive guard but are deliberately not provided by any consumer. + * Example entry would be: + * 'pages/admin/tenants/_form.phtml' => ['someVar' => 'reason / proof'], + * Today: empty (all 7 partials are clean). + * + * @var array> + */ + private const ALLOWLIST = []; + + /** + * Authoring-time count of shared form partials. Bumping this is a + * deliberate review checkpoint: when adding a new partial, audit + * its read patterns against the convention before raising the count. + */ + private const EXPECTED_PARTIAL_COUNT = 7; + + public function testSharedFormPartialsHaveDefensiveReadsOrCallerProvidesVariables(): void + { + $partials = $this->locateFormPartials(); + $this->assertNotEmpty($partials, 'No shared form partials found — scanner is broken or layout changed.'); + + $violations = []; + + foreach ($partials as $partialRel) { + $partialAbs = $this->projectRootPath() . '/' . $partialRel; + $partialContent = (string) file_get_contents($partialAbs); + + try { + $analysis = $this->analysePartial($partialContent); + } catch (\RuntimeException $e) { + $violations[] = sprintf( + "Partial %s: cannot statically verify reads — %s. Please review manually.", + $partialRel, + $e->getMessage() + ); + continue; + } + + if ($analysis['unverifiable']) { + $violations[] = sprintf( + "Partial %s: contains construct(s) the scanner cannot classify (%s). Cannot statically verify, please review manually.", + $partialRel, + implode(', ', $analysis['unverifiable']) + ); + continue; + } + + if ($analysis['unguardedReads'] === []) { + continue; + } + + $consumers = $this->locateConsumingTemplates($partialRel); + // Collect each consumer's pre-require variable definitions once. + $consumerDefinitions = []; + foreach ($consumers as $consumerRel) { + $consumerAbs = $this->projectRootPath() . '/' . $consumerRel; + $consumerContent = (string) file_get_contents($consumerAbs); + try { + $consumerDefinitions[$consumerRel] = $this->collectConsumerDefinitions( + $consumerContent, + $partialRel + ); + } catch (\RuntimeException $e) { + $violations[] = sprintf( + "Consumer %s of partial %s: cannot statically verify pre-require definitions — %s. Please review manually.", + $consumerRel, + $partialRel, + $e->getMessage() + ); + $consumerDefinitions[$consumerRel] = null; + } + } + + // @phpstan-ignore-next-line nullCoalesce.offset (ALLOWLIST is intentionally empty today) + $allowlist = self::ALLOWLIST[$partialRel] ?? []; + + foreach ($analysis['unguardedReads'] as $varName => $line) { + // @phpstan-ignore-next-line function.impossibleType (allowlist is empty today; check guards future entries) + if (array_key_exists($varName, $allowlist)) { + continue; + } + + $missingFrom = []; + foreach ($consumerDefinitions as $consumerRel => $definitions) { + if ($definitions === null) { + // Already recorded as "cannot verify". + continue; + } + if (!in_array($varName, $definitions, true)) { + $missingFrom[] = $consumerRel; + } + } + + if ($missingFrom === []) { + continue; + } + + $violations[] = sprintf( + "Form partial '%s' line %d reads \$%s without defensive guard, but consuming template(s) do not define it before the require:\n %s\nEither:\n - Define the variable in each consuming template before `require __DIR__ . '/_form.phtml';`, OR\n - Add a defensive read in the partial (e.g. `\$%s ?? false` / `isset(\$%s)`).", + $partialRel, + $line, + $varName, + implode("\n ", $missingFrom), + $varName, + $varName + ); + } + } + + $this->assertSame( + [], + $violations, + "Form-partial defensive-reads contract violations:\n\n" . implode("\n\n", $violations) + ); + } + + public function testScannerSeesExpectedNumberOfPartials(): void + { + $partials = $this->locateFormPartials(); + $this->assertCount( + self::EXPECTED_PARTIAL_COUNT, + $partials, + sprintf( + "Expected exactly %d shared form partials at authoring time but found %d:\n %s\n" + . "If a new partial was added intentionally, audit it against the convention then bump EXPECTED_PARTIAL_COUNT.", + self::EXPECTED_PARTIAL_COUNT, + count($partials), + implode("\n ", $partials) + ) + ); + } + + /** + * @return list list of project-relative paths + */ + private function locateFormPartials(): array + { + $root = $this->projectRootPath(); + $partials = []; + + // Core admin form partials: pages/admin//_form.phtml + $adminBase = $root . '/pages/admin'; + if (is_dir($adminBase)) { + foreach (scandir($adminBase) ?: [] as $entry) { + if ($entry === '.' || $entry === '..') { + continue; + } + $entryPath = $adminBase . '/' . $entry; + if (!is_dir($entryPath)) { + continue; + } + $candidate = $entryPath . '/_form.phtml'; + if (is_file($candidate)) { + $partials[] = 'pages/admin/' . $entry . '/_form.phtml'; + } + } + } + + // Module form partials: modules//pages/**/_form.phtml + $modulesBase = $root . '/modules'; + if (is_dir($modulesBase)) { + foreach (scandir($modulesBase) ?: [] as $module) { + if ($module === '.' || $module === '..') { + continue; + } + $modulePages = $modulesBase . '/' . $module . '/pages'; + if (!is_dir($modulePages)) { + continue; + } + $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($modulePages, \FilesystemIterator::SKIP_DOTS)); + /** @var \SplFileInfo $file */ + foreach ($iterator as $file) { + if ($file->isFile() && $file->getFilename() === '_form.phtml') { + $partials[] = str_replace($root . '/', '', $file->getPathname()); + } + } + } + } + + sort($partials); + return $partials; + } + + /** + * Analyse a partial's PHP source via token_get_all. + * + * @return array{unguardedReads: array, unverifiable: list} + * unguardedReads — name => first-line where the unguarded read appears + * unverifiable — list of construct names the scanner refused to classify + */ + private function analysePartial(string $source): array + { + $tokens = token_get_all($source); + + // PHP superglobals — always defined. + $defined = [ + 'GLOBALS' => true, + '_GET' => true, + '_POST' => true, + '_REQUEST' => true, + '_SESSION' => true, + '_SERVER' => true, + '_FILES' => true, + '_COOKIE' => true, + '_ENV' => true, + 'this' => true, + ]; + $unguarded = []; // name => line + $unverifiable = []; + + $count = count($tokens); + + // Sticky context flags. + // A simple scope stack to detect inside-function-parameter-list etc. + // For form partials this is rarely needed but we keep it robust. + $parenDepth = 0; + + // Tracks whether the *current* T_VARIABLE token sits inside the args + // of a guard call like isset(/empty(/array_key_exists(/!empty( . When + // the call closes we mark all variables read inside as defined. + // Implementation: when we see the guard-opening token + its `(`, + // remember the paren-depth of that open paren. Until we close back + // through it, any T_VARIABLE we read is treated as guarded. + $guardOpenDepths = []; // list + + for ($i = 0; $i < $count; $i++) { + $tok = $tokens[$i]; + + if (is_array($tok)) { + [$id, $text, $line] = $tok; + + // Detect extract(...) — we cannot statically know what becomes defined. + if ($id === T_STRING && strtolower($text) === 'extract') { + $next = $this->nextSignificant($tokens, $i); + if ($next !== null && is_string($tokens[$next]) && $tokens[$next] === '(') { + $unverifiable[] = sprintf("extract() at line %d", $line); + } + } + + // catch (\Exception $e) — define $e. + if ($id === T_CATCH) { + // Find the `(` then collect T_VARIABLE up to the matching `)`. + $parenPos = null; + for ($j = $i + 1; $j < $count; $j++) { + if (is_string($tokens[$j]) && $tokens[$j] === '(') { + $parenPos = $j; + break; + } + } + if ($parenPos !== null) { + $depth = 1; + for ($j = $parenPos + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + // foreach (... as $key => $value) — define $key and $value. + if ($id === T_FOREACH) { + $asPos = $this->findTokenAfter($tokens, $i, T_AS); + if ($asPos !== null) { + // Collect T_VARIABLE tokens between T_AS and the matching ')' + // (the `)` that closes this foreach header). + $depth = 1; + for ($j = $asPos + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + // function (...) { ... } — define parameter variables. + // Anonymous + named functions handled the same way. + if ($id === T_FUNCTION || $id === T_FN) { + // Find the `(` that opens the parameter list. + $parenPos = null; + for ($j = $i + 1; $j < $count; $j++) { + if (is_string($tokens[$j]) && $tokens[$j] === '(') { + $parenPos = $j; + break; + } + // If we hit `{` first, it's not a parameter list. + if (is_string($tokens[$j]) && $tokens[$j] === '{') { + break; + } + } + if ($parenPos !== null) { + $depth = 1; + for ($j = $parenPos + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + // isset(...) / empty(...) — variables read inside count as guarded. + if ($id === T_ISSET || $id === T_EMPTY) { + // The next significant token is the `(`. + $next = $this->nextSignificant($tokens, $i); + if ($next !== null && is_string($tokens[$next]) && $tokens[$next] === '(') { + // Mark all T_VARIABLE inside as defined (and skip them + // in main read-detection by continuing past them). + $depth = 1; + for ($j = $next + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + $i = $j; // skip ahead + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + // array_key_exists(KEY, $var) — second arg is guarded. + if ($id === T_STRING && strtolower($text) === 'array_key_exists') { + $next = $this->nextSignificant($tokens, $i); + if ($next !== null && is_string($tokens[$next]) && $tokens[$next] === '(') { + $depth = 1; + for ($j = $next + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + $i = $j; + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + if ($id === T_VARIABLE) { + $name = ltrim($text, '$'); + + // Skip static class property access: ClassName::$prop. + // The previous significant token would be T_DOUBLE_COLON. + $prev = $this->prevSignificant($tokens, $i); + if ($prev !== null) { + $pt = $tokens[$prev]; + if (is_array($pt) && $pt[0] === T_DOUBLE_COLON) { + continue; + } + // Object property: $obj->$dynamicProp (T_OBJECT_OPERATOR). + // Treat as a property read, not a variable read. + if (is_array($pt) && $pt[0] === T_OBJECT_OPERATOR) { + continue; + } + } + + // Look at next significant token to detect assignment. + $next = $this->nextSignificant($tokens, $i); + $isAssignment = false; + if ($next !== null) { + $nt = $tokens[$next]; + if (is_string($nt) && $nt === '=') { + $isAssignment = true; + } + // T_DOUBLE_ARROW (=>) is array key, not assignment. + // T_IS_EQUAL / T_IS_IDENTICAL are tokens, so `==`/`===` + // do NOT match the literal '=' branch above. + } + + // Look at following token chain for `??` (null-coalesce) + // operator on this variable, possibly through array + // subscripts and property accesses, e.g. + // $form['key'] ?? null + // $obj->prop ?? null + // are all defensive reads of the BASE variable. + $isCoalescedRead = false; + if ($next !== null) { + $afterChain = $this->skipMemberChain($tokens, $next); + if ($afterChain !== null) { + $ct = $tokens[$afterChain]; + if (is_array($ct) && $ct[0] === T_COALESCE) { + $isCoalescedRead = true; + } + } + } + + // Look at preceding significant token for `&` (by-ref) + // pattern in foreach/function — already handled, skip. + + // If this is a list/array destructuring on LHS like + // `[$a, $b] = …` or `list($a) = …` we'd need broader + // analysis; fall through to "read" — partials don't use + // these patterns. The existence test guards against drift. + + if ($isAssignment) { + $defined[$name] = true; + // Continue — the RHS may still contain reads, and the + // tokenizer will visit them on subsequent iterations. + continue; + } + + if ($isCoalescedRead) { + // Defensive read; do not require pre-definition. + // The variable does NOT become "defined" though, + // because subsequent unguarded reads must still be + // verified. (Common pattern: `$x = $x ?? default;` + // would already match the assignment branch.) + continue; + } + + if (isset($defined[$name])) { + continue; + } + + // Unguarded read of a not-yet-defined variable. + if (!isset($unguarded[$name])) { + $unguarded[$name] = $line; + } + continue; + } + } + // String tokens like '(' / ')' — depth tracking not strictly + // needed for the patterns above; they each scan their own + // matching `)`. Keep the loop simple. + } + + return [ + 'unguardedReads' => $unguarded, + 'unverifiable' => $unverifiable, + ]; + } + + /** + * Walk a consumer template and collect every variable name assigned + * BEFORE the first `require … _form.phtml` statement. Reads are not + * collected — only assignments (LHS). + * + * Throws RuntimeException when no require-of-_form is found (the + * caller used a non-static include the test cannot follow). + * + * @return list + */ + private function collectConsumerDefinitions(string $source, string $partialRel): array + { + $tokens = token_get_all($source); + $count = count($tokens); + + // Find the offset of `require __DIR__ . '/_form.phtml'` (or similar + // relative require to the same directory's _form.phtml). + $requireOffset = null; + for ($i = 0; $i < $count; $i++) { + $t = $tokens[$i]; + if (!is_array($t)) { + continue; + } + if ($t[0] !== T_REQUIRE && $t[0] !== T_REQUIRE_ONCE + && $t[0] !== T_INCLUDE && $t[0] !== T_INCLUDE_ONCE) { + continue; + } + // Look ahead until the statement-terminating `;` and check if + // the path string contains `_form.phtml`. + $stmt = ''; + for ($j = $i + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt) && $jt === ';') { + break; + } + $stmt .= is_array($jt) ? $jt[1] : $jt; + } + if (str_contains($stmt, '_form.phtml')) { + $requireOffset = $i; + break; + } + } + + if ($requireOffset === null) { + throw new \RuntimeException(sprintf( + "no `require … _form.phtml` statement found while scanning consumer for partial %s", + $partialRel + )); + } + + // Collect $x in `$x = …` patterns up to (but excluding) requireOffset. + // Also recognise foreach-binding variables defined before the require + // (rare in practice for consumers, but cheap to support). + $defined = []; + for ($i = 0; $i < $requireOffset; $i++) { + $t = $tokens[$i]; + + // foreach (...) — collect bound variables. + if (is_array($t) && $t[0] === T_FOREACH) { + $asPos = $this->findTokenAfter($tokens, $i, T_AS); + if ($asPos !== null) { + $depth = 1; + for ($j = $asPos + 1; $j < $count; $j++) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '(') { + $depth++; + } elseif ($jt === ')') { + $depth--; + if ($depth === 0) { + break; + } + } + } elseif ($jt[0] === T_VARIABLE) { + $defined[ltrim($jt[1], '$')] = true; + } + } + } + continue; + } + + if (!is_array($t) || $t[0] !== T_VARIABLE) { + continue; + } + + $next = $this->nextSignificant($tokens, $i); + if ($next === null) { + continue; + } + $nt = $tokens[$next]; + if (is_string($nt) && $nt === '=') { + $defined[ltrim($t[1], '$')] = true; + } + } + + return array_keys($defined); + } + + /** + * @return list list of project-relative paths to consuming templates + */ + private function locateConsumingTemplates(string $partialRel): array + { + $root = $this->projectRootPath(); + $partialDir = dirname($partialRel); + $absDir = $root . '/' . $partialDir; + $consumers = []; + + if (!is_dir($absDir)) { + return []; + } + + // Direct directory scan: all .phtml files in the same directory that + // require _form.phtml. We rely on the convention that consumers live + // alongside the partial. + foreach (scandir($absDir) ?: [] as $entry) { + if ($entry === '.' || $entry === '..' || $entry === '_form.phtml') { + continue; + } + $abs = $absDir . '/' . $entry; + if (!is_file($abs) || !str_ends_with($entry, '.phtml')) { + continue; + } + $content = (string) file_get_contents($abs); + // Match either `require __DIR__ . '/_form.phtml'` or an explicit + // relative path that contains `_form.phtml`. + if (preg_match('/\b(?:require|require_once|include|include_once)\b[^;]*_form\.phtml/', $content) === 1) { + $consumers[] = $partialDir . '/' . $entry; + } + } + + sort($consumers); + return $consumers; + } + + /** + * @param array $tokens + */ + private function nextSignificant(array $tokens, int $from): ?int + { + $count = count($tokens); + for ($i = $from + 1; $i < $count; $i++) { + $t = $tokens[$i]; + if (is_array($t)) { + if (in_array($t[0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + continue; + } + } + return $i; + } + return null; + } + + /** + * @param array $tokens + */ + private function prevSignificant(array $tokens, int $from): ?int + { + for ($i = $from - 1; $i >= 0; $i--) { + $t = $tokens[$i]; + if (is_array($t)) { + if (in_array($t[0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + continue; + } + } + return $i; + } + return null; + } + + /** + * Starting from $from (an offset already pointing at a `[` or `->` or + * `?->` token following a variable), walk past the entire member-access + * chain and return the offset of the next significant token *after* the + * chain. Returns the original $from when no chain follows. + * + * Example token stream for `$form['key'] ?? null`: + * $form [ 'key' ] ?? null + * ^from points at `[`; this returns the offset of `??`. + * + * @param array $tokens + */ + private function skipMemberChain(array $tokens, int $from): ?int + { + $count = count($tokens); + $i = $from; + while ($i < $count) { + $t = $tokens[$i]; + // Bracket: skip the matching `]`. + if (is_string($t) && $t === '[') { + $depth = 1; + $j = $i + 1; + while ($j < $count && $depth > 0) { + $jt = $tokens[$j]; + if (is_string($jt)) { + if ($jt === '[') { + $depth++; + } elseif ($jt === ']') { + $depth--; + } + } + $j++; + } + $i = $j; + $next = $this->nextSignificant($tokens, $i - 1); + if ($next === null) { + return null; + } + $i = $next; + continue; + } + // Object operator (->) or nullsafe (?->) — skip the operator + // plus its identifier (T_STRING) or expression token, then loop. + if (is_array($t) && ($t[0] === T_OBJECT_OPERATOR || (defined('T_NULLSAFE_OBJECT_OPERATOR') && $t[0] === T_NULLSAFE_OBJECT_OPERATOR))) { + $next = $this->nextSignificant($tokens, $i); + if ($next === null) { + return null; + } + $i = $this->nextSignificant($tokens, $next) ?? $count; + continue; + } + // Anything else terminates the chain — return current offset. + return $i; + } + return null; + } + + /** + * Return offset of the next token with the given type, after $from. + * + * @param array $tokens + */ + private function findTokenAfter(array $tokens, int $from, int $type): ?int + { + $count = count($tokens); + for ($i = $from + 1; $i < $count; $i++) { + $t = $tokens[$i]; + if (is_array($t) && $t[0] === $type) { + return $i; + } + // Stop at the closing ')' of the immediate construct so we do not + // wander into an unrelated `as` token elsewhere. For T_FOREACH the + // T_AS we want lives inside the very first parenthesis group; if we + // hit a `;` we have left the foreach header entirely. + if (is_string($t) && $t === ';') { + return null; + } + } + return null; + } +}