Files
breadcrumb-the-shire/tests/Architecture/FormPartialDefensiveReadsContractTest.php
fs 95723cebd9 test(architecture): enforce defensive reads in shared form partials
Codifies the convention that shared form partials (pages/admin/*/_form.phtml,
modules/**/_form.phtml) either guard variable reads with ?? / isset() / array
access checks, or have every consuming view template define the variable
before requiring the partial.

The convention was already followed by 6 of 7 partials in the codebase. The
seventh (tenants/_form.phtml) regressed silently when a per-theme logo block
introduced bare $canUpdateTenant reads in commit 6e3fc63c — fixed in commit
e29e6c3. This test catches that exact bug shape and any future variant.

Implementation uses token_get_all (no full PHP parser, no expression
evaluation). For each partial it identifies variable references that are not
locally defined or guarded, then walks the consuming templates discovered via
require statements and verifies each variable is present before the require.
On detection of statically unrecognizable constructs (extract(),
dynamic require paths) the test fails loudly with a "review manually" hint
rather than silently passing.

Verified by reverting commit e29e6c3 in the working tree and running the test
— it produces the exact $canUpdateTenant / line 223 finding that prompted the
original fix. Restored after the dry-run.

ALLOWLIST stays empty today. Future legitimate exceptions go in the test
header with per-entry justification, mirroring DetailDrawerFragmentContractTest.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-26 17:19:14 +02:00

785 lines
32 KiB
PHP

<?php
namespace MintyPHP\Tests\Architecture;
use PHPUnit\Framework\TestCase;
/**
* Codifies the "defensive variable reads in shared form partials" convention.
*
* --- Why this exists ---
* On 2026-04-25, `pages/admin/tenants/_form.phtml` referenced `$canUpdateTenant`
* without a `?? false` fallback while `create(default).phtml` did NOT define it
* before the `require __DIR__ . '/_form.phtml'`. Result: undefined-variable
* warnings and a 500 on every `admin/tenants/create` GET. The bug existed for
* one day and was fixed in commit `e29e6c3`. A subsequent sweep confirmed all
* other six shared form partials follow the convention. This test prevents the
* single-patch regression class going forward.
*
* --- The convention ---
* Every variable READ inside a shared form partial under
* pages/admin/<dir>/_form.phtml
* modules/ * /pages/<dir>/_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 (`<dir>/<name>(default).phtml`,
* `<dir>/<name>(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<string, array<string, string>>
*/
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<string> list of project-relative paths
*/
private function locateFormPartials(): array
{
$root = $this->projectRootPath();
$partials = [];
// Core admin form partials: pages/admin/<dir>/_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/<id>/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<string, int>, unverifiable: list<string>}
* 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<int>
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<string>
*/
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<string> 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<int, mixed> $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<int, mixed> $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<int, mixed> $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<int, mixed> $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;
}
}