fa/breadcrumb-the-shire
1
0
Fork 1
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor(action-context): remove unused fragment aggregator + building block

Browse Source 
actionFragmentContext was built spec-driven in step 1 to handle the
anticipated drawer-fragment pattern: authorize + finder → status DTO.
The action-context rollout (steps 2-11) found that none of the three
real drawer fragments (users/view-fragment, addressbook/view-fragment,
helpdesk/ticket-fragment) match that pattern — all three delegate to
domain services that own their own status models. Step 11 declared
them structural exceptions; the helper code remained unused.

This commit removes the dead spec:
* core/Support/helpers/action_context.php drops actionFragmentContext
  (~9 LOC) and its building block actionFragmentResolveOrStatus
  (~30 LOC) plus their docblocks. The MUST-call-actionRequireCsrf
  warning, present in three aggregator docblocks before, now appears
  twice (one per remaining aggregator).
* tests/Support/Helpers/ActionContextHelperTest.php drops the six
  unit tests that exercised these functions (~83 LOC).
* tests/Architecture/ActionContextHelperContractTest.php drops the
  fragment building block from the buildingBlocks() data provider
  (5 entries instead of 6) and removes testFragmentResolveReturnDocblockIsFrozen.
  The CSRF-warning expectation is updated from 3 to 2 with a code
  comment explaining the rollback.

Verified:
* No production caller exists in pages/ or modules/.
* All 9 aggregator callers (5 actionEditContext + 4 actionCreateContext)
  remain unchanged.
* ActionContextCsrfPairingContractTest and DetailDrawerFragmentContractTest
  are deliberately left untouched: their allowlists/recognizer regexes
  still mention actionFragmentContext, but the patterns now match an
  empty set — harmless dead text. Documented as open items in the run
  report; future cleanup is optional and orthogonal to this removal.
* QGs all green (PHPUnit 2088 tests, PHPStan level 5, CS-fixer 0 diffs).

Net: 3 files, +31/-198 LOC, behavior unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Felix Schneider
2026-04-26 17:34:53 +02:00
parent 95723cebd9
commit 9ec10f5c02
3 changed files with 31 additions and 198 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
core/Support/helpers/action_context.php
View File

@@ -1,13 +1,15 @@
<?php <?php
/** /**
* Step 1: helpers introduced without production callers; produced by run * Action-context helpers introduced by run 2026-04-25-action-context-helper
* 2026-04-25-action-context-helper. The 6 orthogonal building blocks plus * (Step 1) and pruned by run 2026-04-25-action-fragment-context-removal after
* the 3 cluster aggregators centralize the ~40-80 line pre-render preamble * Cluster 10 (Drawer-Fragments) was declared a structural exception in run
* that every Edit/Create/View/Drawer-Fragment action repeats today. * 2026-04-25-action-context-rollout-step11-fragments-exception. The remaining
* * 5 orthogonal building blocks plus the 2 cluster aggregators
* No page action or module page is allowed to call these yet — Step 2 is the * (actionEditContext, actionCreateContext) centralize the ~40-80 line
* Departments-Edit pilot and Step 3 is the cluster-wide rollout. * pre-render preamble that Edit/Create actions repeat. Drawer-fragment actions
* delegate to domain-services with their own status models and are NOT served
* by these helpers — see the Step 11 decision record for rationale.
*/ */
use MintyPHP\Router; use MintyPHP\Router;
@@ -180,55 +182,6 @@ function actionBuildViewAuth(array $capabilities, array $whitelistedFlags): arra
]; ];
} }
/**
* Drawer-fragment variant of actionResolveModelOrFail + Authorize.
*
* Drawer fragments must NOT redirect (CLAUDE.md). They return HTTP status
* codes 400/403/404 instead. This helper produces a status DTO that the
* caller translates into http_response_code() + early return.
*
* Usage:
* $r = actionFragmentResolveOrStatus($finder, $rawId, ABILITY_VIEW, [...]);
* if ($r['status'] !== 'ok') { http_response_code($r['http_code']); return; }
*
* @param callable(string): mixed $finder Returns the resolved model or null.
* @param string $rawId Raw id from the URL.
* @param string $abilityKey Ability constant for authorize().
* @param array<string, mixed> $context Authorize context payload.
*
* @return array{status: 'ok'|'forbidden'|'not_found'|'invalid_id', model?: mixed, capabilities?: array<string, mixed>, http_code?: int}
*/
function actionFragmentResolveOrStatus(
callable $finder,
string $rawId,
string $abilityKey,
array $context
): array {
Guard::requireLogin();
$trimmedId = trim($rawId);
if ($trimmedId === '') {
return ['status' => 'invalid_id', 'http_code' => 400];
}
$decision = app(AuthorizationService::class)->authorize($abilityKey, $context);
if (!$decision->isAllowed()) {
return ['status' => 'forbidden', 'http_code' => 403];
}
$model = $finder($trimmedId);
if ($model === null) {
return ['status' => 'not_found', 'http_code' => 404];
}
$capabilities = $decision->attribute('capabilities', []);
return [
'status' => 'ok',
'model' => $model,
'capabilities' => is_array($capabilities) ? $capabilities : [],
];
}
/** /**
* CALLERS MUST call actionRequireCsrf() BEFORE this aggregator for POST requests. This helper does NOT verify CSRF (GR-SEC-001). For GET-only edit-render flows, CSRF is not required. * CALLERS MUST call actionRequireCsrf() BEFORE this aggregator for POST requests. This helper does NOT verify CSRF (GR-SEC-001). For GET-only edit-render flows, CSRF is not required.
* *
@@ -356,29 +309,3 @@ function actionCreateContext(array $args): array
'viewAuth' => $viewAuth, 'viewAuth' => $viewAuth,
]; ];
} }
/**
* CALLERS MUST call actionRequireCsrf() BEFORE this aggregator for POST requests. This helper does NOT verify CSRF (GR-SEC-001). Drawer fragments are typically GET, but the warning is intentionally defensive and consistent across aggregators.
*
* Cluster-10 Drawer-Fragment aggregator: thin wrapper around
* actionFragmentResolveOrStatus. Returns the status DTO unchanged so the
* caller can translate it to http_response_code(...) + early return.
*
* Required keys in $args:
* - finder: callable(string): mixed
* - rawId: string
* - abilityKey: string
* - context: array<string, mixed>
*
* @param array<string, mixed> $args
* @return array{status: 'ok'|'forbidden'|'not_found'|'invalid_id', model?: mixed, capabilities?: array<string, mixed>, http_code?: int}
*/
function actionFragmentContext(array $args): array
{
$finder = $args['finder'];
$rawId = (string) $args['rawId'];
$abilityKey = (string) $args['abilityKey'];
$context = is_array($args['context'] ?? null) ? $args['context'] : [];
return actionFragmentResolveOrStatus($finder, $rawId, $abilityKey, $context);
}

51
tests/Architecture/ActionContextHelperContractTest.php
View File

@@ -7,22 +7,27 @@ use ReflectionFunction;
use ReflectionNamedType; use ReflectionNamedType;
/** /**
* Freezes the API of the 6 orthogonal building blocks in * Freezes the API of the 5 orthogonal building blocks in
* core/Support/helpers/action_context.php. * core/Support/helpers/action_context.php.
* *
* Aggregators (actionEditContext, actionCreateContext, actionFragmentContext) * Aggregators (actionEditContext, actionCreateContext) are intentionally NOT
* are intentionally NOT frozen — they may evolve additively in Step 2 of the * frozen — they may evolve additively across the rollout.
* rollout.
* *
* This test also asserts that the Tenant-Scope and Fragment-Resolver helpers * This test also asserts that the Tenant-Scope helper carries its PHPStan
* carry their PHPStan array-shape return docblocks. Acceptance check SC-004 * array-shape return docblock. Acceptance check SC-004 relies on the shape
* relies on the shape staying stable. * staying stable.
* *
* Step 2 (Pilot-Migration Departments-Edit, Run * Step 2 (Pilot-Migration Departments-Edit, Run
* 2026-04-25-action-context-rollout-step2-departments) is the first * 2026-04-25-action-context-rollout-step2-departments) is the first
* production caller. The previous testNoProductionCallSitesYet assertion * production caller. The previous testNoProductionCallSitesYet assertion
* has been removed; the CSRF↔Aggregator pairing is enforced by * has been removed; the CSRF↔Aggregator pairing is enforced by
* tests/Architecture/ActionContextCsrfPairingContractTest.php instead. * tests/Architecture/ActionContextCsrfPairingContractTest.php instead.
*
* Run 2026-04-25-action-fragment-context-removal pruned the dead
* Cluster-10 Drawer-Fragment helpers (actionFragmentResolveOrStatus building
* block + actionFragmentContext aggregator) after Step 11 declared all three
* fragment actions structural exceptions that delegate to domain-services
* with their own status models.
*/ */
class ActionContextHelperContractTest extends TestCase class ActionContextHelperContractTest extends TestCase
{ {
@@ -60,12 +65,6 @@ class ActionContextHelperContractTest extends TestCase
['capabilities', null, false], ['capabilities', null, false],
['whitelistedFlags', null, false], ['whitelistedFlags', null, false],
]], ]],
['actionFragmentResolveOrStatus', [
['finder', null, false],
['rawId', null, false],
['abilityKey', null, false],
['context', null, false],
]],
]; ];
} }
@@ -132,18 +131,6 @@ class ActionContextHelperContractTest extends TestCase
); );
} }
public function testFragmentResolveReturnDocblockIsFrozen(): void
{
$reflection = new ReflectionFunction('actionFragmentResolveOrStatus');
$doc = (string) $reflection->getDocComment();
$this->assertMatchesRegularExpression(
"/@return array\{status:[^}]*'ok'[^}]*'forbidden'[^}]*'not_found'[^}]*'invalid_id'/",
$doc,
"actionFragmentResolveOrStatus must declare PHPStan array-shape return covering all 4 status values."
);
}
public function testEnforceCanViewPageDocblockHasStrictComparisonNote(): void public function testEnforceCanViewPageDocblockHasStrictComparisonNote(): void
{ {
$reflection = new ReflectionFunction('actionEnforceCanViewPage'); $reflection = new ReflectionFunction('actionEnforceCanViewPage');
@@ -181,19 +168,25 @@ class ActionContextHelperContractTest extends TestCase
'/MUST call actionRequireCsrf\(\) BEFORE this aggregator/', '/MUST call actionRequireCsrf\(\) BEFORE this aggregator/',
$contents $contents
); );
// Expected count is 2: actionEditContext + actionCreateContext.
// Run 2026-04-25-action-fragment-context-removal removed the
// actionFragmentContext aggregator (Cluster 10 has no aggregator —
// drawer-fragments use domain-service status models instead), reducing
// the warning count from 3 to 2.
$this->assertSame( $this->assertSame(
3, 2,
$count, $count,
'Each of the 3 aggregators (actionEditContext, actionCreateContext, actionFragmentContext) must carry the explicit CSRF-warning in its docblock (GR-SEC-001).' 'Each of the 2 aggregators (actionEditContext, actionCreateContext) must carry the explicit CSRF-warning in its docblock (GR-SEC-001).'
); );
} }
public function testAggregatorsExistButAreNotFrozen(): void public function testAggregatorsExistButAreNotFrozen(): void
{ {
// We assert existence only — signatures may evolve in Step 2. // We assert existence only — signatures may evolve.
// Run 2026-04-25-action-fragment-context-removal removed
// actionFragmentContext; only EditContext and CreateContext remain.
$this->assertTrue(function_exists('actionEditContext')); $this->assertTrue(function_exists('actionEditContext'));
$this->assertTrue(function_exists('actionCreateContext')); $this->assertTrue(function_exists('actionCreateContext'));
$this->assertTrue(function_exists('actionFragmentContext'));
} }
/** /**

87
tests/Support/Helpers/ActionContextHelperTest.php
View File

@@ -234,57 +234,6 @@ class ActionContextHelperTest extends TestCase
$this->assertSame(['page' => ['can_view_page' => true]], $result); $this->assertSame(['page' => ['can_view_page' => true]], $result);
} }
/* ----------------------------------------------------------------
* actionFragmentResolveOrStatus
* ---------------------------------------------------------------- */
public function testFragmentResolveReturnsOkWithModelAndCapabilities(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([
'capabilities' => ['can_view' => true],
]));
$finder = static fn (string $id): array => ['id' => $id];
$r = actionFragmentResolveOrStatus($finder, 'abc-123', 'ABILITY_VIEW', []);
$this->assertSame('ok', $r['status']);
$this->assertSame(['id' => 'abc-123'], $r['model'] ?? null);
$this->assertSame(['can_view' => true], $r['capabilities'] ?? null);
}
public function testFragmentResolveReturnsForbidden(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::deny());
$finder = static fn (string $id): array => ['id' => $id];
$r = actionFragmentResolveOrStatus($finder, 'abc', 'ABILITY_VIEW', []);
$this->assertSame('forbidden', $r['status']);
$this->assertSame(403, $r['http_code'] ?? null);
}
public function testFragmentResolveReturnsNotFound(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow());
$finder = static fn (): mixed => null;
$r = actionFragmentResolveOrStatus($finder, 'abc', 'ABILITY_VIEW', []);
$this->assertSame('not_found', $r['status']);
$this->assertSame(404, $r['http_code'] ?? null);
}
public function testFragmentResolveReturnsInvalidIdForBlankRawId(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow());
$finder = static fn (): mixed => null;
$r = actionFragmentResolveOrStatus($finder, ' ', 'ABILITY_VIEW', []);
$this->assertSame('invalid_id', $r['status']);
$this->assertSame(400, $r['http_code'] ?? null);
}
/* ---------------------------------------------------------------- /* ----------------------------------------------------------------
* Aggregator: actionEditContext * Aggregator: actionEditContext
* ---------------------------------------------------------------- */ * ---------------------------------------------------------------- */
@@ -449,42 +398,6 @@ class ActionContextHelperTest extends TestCase
$this->assertStringContainsString('error/forbidden', $this->capturedRedirect()); $this->assertStringContainsString('error/forbidden', $this->capturedRedirect());
} }
/* ----------------------------------------------------------------
* Aggregator: actionFragmentContext
* ---------------------------------------------------------------- */
public function testFragmentContextOk(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::allow([
'capabilities' => ['can_view' => true],
]));
$result = actionFragmentContext([
'finder' => static fn (string $id): array => ['uuid' => $id],
'rawId' => 'u-1',
'abilityKey' => 'ABILITY_VIEW',
'context' => [],
]);
$this->assertSame('ok', $result['status']);
$this->assertSame(['uuid' => 'u-1'], $result['model'] ?? null);
}
public function testFragmentContextForbidden(): void
{
$this->stubAuthorizationService(static fn (): AuthorizationDecision => AuthorizationDecision::deny());
$result = actionFragmentContext([
'finder' => static fn (string $id): array => ['uuid' => $id],
'rawId' => 'u-1',
'abilityKey' => 'ABILITY_VIEW',
'context' => [],
]);
$this->assertSame('forbidden', $result['status']);
$this->assertSame(403, $result['http_code'] ?? null);
}
/* ---------------------------------------------------------------- /* ----------------------------------------------------------------
* helpers * helpers
* ---------------------------------------------------------------- */ * ---------------------------------------------------------------- */