From e5ed08dbef5eee25912e35d5e35e2bec3b80286b Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 12:01:09 +0900 Subject: [PATCH 1/6] Execute each embedded donut child once and tag executed children Wraps every embedded #[Embed] request in a donut page with OnceRequest so onGet() reading the body and DonutRequest rendering it as a hole share one execution instead of running the child twice (previous MISS/HIT divergence for non-idempotent children). A child that only ran into the body - never substituted as a hole - now has its tag added to the stored template's own storage tags, so purging it invalidates the page instead of leaving a stale shell (#226). Also fixes ResourceBodyEvaluator::materialize() crashing on an embedded BEAR\Async\AsyncRequest (or any AbstractRequest subclass whose jsonSerialize() does not answer the memoized ResourceObject): it is now left unmaterialized instead of asserting. CHANGELOG: documents the fix and the one-time ETag churn it causes for a #[CacheableResponse] class/method that also declares #[Embed] (Ray.Aop weaves a new interceptor into such classes); verified by execution that a page with no #[Embed] keeps its stock ETag and that #[DonutCache] pages never carry an ETag at all. --- CHANGELOG.md | 3 + src/DonutCacheModule.php | 69 +++++- src/DonutChildExecutions.php | 54 +++++ src/DonutChildExecutionsInterface.php | 41 ++++ src/DonutRepository.php | 64 +++++- src/DonutRequest.php | 21 +- src/EmbedOnceInterceptor.php | 127 +++++++++++ src/OnceRequest.php | 201 ++++++++++++++++++ src/ResourceBodyEvaluator.php | 14 +- .../EmbedOnceInterceptorInstallOrderTest.php | 104 +++++++++ tests/EmbedOnceInterceptorWarningTest.php | 154 ++++++++++++++ tests/EmbeddedChildMaterializationTest.php | 20 ++ tests/Fake/FakeAsyncLikeRequest.php | 95 +++++++++ .../fake-app/src/Resource/App/Counter.php | 29 +++ .../Resource/Page/Html/BodyOnlyChildDonut.php | 31 +++ .../Page/Html/CounterCacheMethodLevel.php | 29 +++ .../src/Resource/Page/Html/CounterDonut.php | 32 +++ .../src/Resource/Page/Html/RequeryDonut.php | 37 ++++ .../Resource/Page/Html/SelfLinkOnlyDonut.php | 24 +++ .../var/templates/App/Counter.html.twig | 1 + .../Page/Html/BodyOnlyChildDonut.html.twig | 1 + .../Html/CounterCacheMethodLevel.html.twig | 1 + .../Page/Html/CounterDonut.html.twig | 1 + .../Page/Html/RequeryDonut.html.twig | 1 + tests/OnceRequestBehaviorTest.php | 179 ++++++++++++++++ tests/OnceRequestUnitTest.php | 107 ++++++++++ 26 files changed, 1417 insertions(+), 23 deletions(-) create mode 100644 src/DonutChildExecutions.php create mode 100644 src/DonutChildExecutionsInterface.php create mode 100644 src/EmbedOnceInterceptor.php create mode 100644 src/OnceRequest.php create mode 100644 tests/EmbedOnceInterceptorInstallOrderTest.php create mode 100644 tests/EmbedOnceInterceptorWarningTest.php create mode 100644 tests/Fake/FakeAsyncLikeRequest.php create mode 100644 tests/Fake/fake-app/src/Resource/App/Counter.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildDonut.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/CounterCacheMethodLevel.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/CounterDonut.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/RequeryDonut.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/SelfLinkOnlyDonut.php create mode 100644 tests/Fake/fake-app/var/templates/App/Counter.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildDonut.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/CounterCacheMethodLevel.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/CounterDonut.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/RequeryDonut.html.twig create mode 100644 tests/OnceRequestBehaviorTest.php create mode 100644 tests/OnceRequestUnitTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 40fa8da5..efd76b3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Deploy this version with a new `CacheVersionModule` version (an app that binds its own cache adapter rather than this package's `Storage*Module`s must clear its pools instead): entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache shows raw `[le2:...]` placeholders during a rolling deploy. - A `#[Cacheable]`/`#[CacheableResponse]` response whose own code sets `Cache-Control` to `private` or `no-store` is no longer stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. +- Upgrading changes the Ray.Aop-woven class name - and so the ETag (`crc32($ro::class . ...)`) - for a `#[CacheableResponse]` class or method that also declares an `#[Embed]`, because it is now woven with a new interceptor (#226); a page with no `#[Embed]` keeps its stock woven class and ETag, and a `#[DonutCache]` page never carries an ETag at all, so neither is affected. - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. `ResourceDonut::refresh()` takes an additional `UncacheableMarkInterface` argument. - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`. - `KeepMutationsAndFailures` also keeps a session cut at the record limit (`semantic_logger_truncated`): a `command` past the limit leaves no other trace. @@ -22,6 +23,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - `LogFileWriter`: two sessions flushed in the same microsecond no longer overwrite each other. The timestamped filename now carries a random suffix, so both survive (#221). - A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carried two or more query parameters leaked a raw `[le:...]` placeholder to the client and could throw `ParameterException` on the next request - fixed by moving new placeholders to a `[le2:...]` marker (#232). +- An embedded child of a `#[DonutCache]`/`#[CacheableResponse]` page now runs at most once per request: it previously ran again when rendered as a hole after already being read in `onGet()`. A child read into the body but never rendered as a hole now tags the stored template, so purging that child's own tag refreshes the page instead of leaving it stale (#226). +- `ResourceBodyEvaluator` no longer crashes materializing an embedded `AbstractRequest` whose `jsonSerialize()` does not answer the memoized `ResourceObject` (`BEAR\Async\AsyncRequest`'s own shape): such a request is now left unmaterialized instead of asserting. Full async support remains out of scope. ## [1.17.0] - 2026-09-17 diff --git a/src/DonutCacheModule.php b/src/DonutCacheModule.php index b615af2e..3d54e38e 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -8,6 +8,7 @@ use BEAR\RepositoryModule\Annotation\CacheableResponse; use BEAR\RepositoryModule\Annotation\DonutCache; use BEAR\RepositoryModule\Annotation\RefreshCache; +use BEAR\Resource\Annotation\Embed; use Override; use Ray\Aop\AbstractMatcher; use Ray\Aop\MatcherInterface; @@ -52,6 +53,8 @@ protected function configure(): void $this->bind(RepositoryLoggerInterface::class)->to(RepositoryLogger::class)->in(Scope::SINGLETON); $this->bind(PurgerInterface::class)->to(NullPurger::class); $this->bind(UriTagInterface::class)->to(UriTag::class); + // One registry per request (see DonutChildExecutions); issue #226 + $this->bind(DonutChildExecutionsInterface::class)->to(DonutChildExecutions::class)->in(Scope::SINGLETON); $this->installAopClassModule(); $this->installAopMethodModule(); } @@ -76,6 +79,28 @@ private function installAopClassModule(): void self::commandMethods($this->matcher), [DonutCommandInterceptor::class], ); + + // Plain bindInterceptor, not bindPriorityInterceptor: this one must run INSIDE + // EmbedInterceptor (see EmbedOnceInterceptor's own docblock for why Ray.Aop's + // onion ordering guarantees that regardless of install order), where a priority + // interceptor never would. Also requires #[Embed]: a class with none (Page\Index + // is this shape) has nothing for EmbedOnceInterceptor to do, so it is left out of + // the pointcut set entirely rather than woven in as a no-op - keeping both the + // Ray.Aop-generated class name (and so EtagSetter's crc32 input) and the per-call + // reflection cost unaffected for every page that embeds nothing. Still a + // LogicalAndMatcher, not a bare annotatedWith(), so it stays in onionOrderMatch()'s + // leftover (install-order-independent) bucket. + $this->bindInterceptor( + $this->matcher->logicalOr( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->annotatedWith(DonutCache::class), + ), + $this->matcher->logicalAnd( + $this->matcher->startsWith('onGet'), + $this->matcher->annotatedWith(Embed::class), + ), + [EmbedOnceInterceptor::class], + ); } /** @@ -103,15 +128,7 @@ private function installAopMethodModule(): void // declaration already governs the method is excluded. The two sets differ because // #[DonutCache] governs onGet only: excluding it from the write bindings too would // leave a #[RefreshCache] write on such a class with no interceptor at all. - $readNotDeclared = $this->matcher->logicalNot( - $this->matcher->logicalOr( - $this->matcher->annotatedWith(CacheableResponse::class), - $this->matcher->logicalOr( - $this->matcher->annotatedWith(Cacheable::class), - $this->matcher->annotatedWith(DonutCache::class), - ), - ), - ); + $readNotDeclared = $this->readNotDeclaredMatcher(); $writeNotDeclared = $this->matcher->logicalNot( $this->matcher->logicalOr( $this->matcher->annotatedWith(CacheableResponse::class), @@ -127,6 +144,21 @@ private function installAopMethodModule(): void [DonutCacheInterceptor::class], ); + // Same plain-bindInterceptor reasoning as installAopClassModule()'s own + // EmbedOnceInterceptor binding (#[Embed] required too), for a class whose + // #[CacheableResponse] sits on onGet() itself rather than on the class + // (BlogPostingCache is this fixture). + $this->bindInterceptor( + $readNotDeclared, + $this->matcher->logicalAnd( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->logicalAnd( + $this->matcher->startsWith('onGet'), + $this->matcher->annotatedWith(Embed::class), + ), + ), + [EmbedOnceInterceptor::class], + ); $this->bindInterceptor( $writeNotDeclared, $this->matcher->logicalAnd( @@ -141,4 +173,23 @@ private function installAopMethodModule(): void [DonutCommandInterceptor::class], ); } + + /** + * Classes governed by neither #[CacheableResponse], #[Cacheable] nor #[DonutCache] at + * the class level - shared by installAopMethodModule()'s read bindings so the two stay + * in lockstep: a method-level #[CacheableResponse] read is exactly where + * EmbedOnceInterceptor must run too. + */ + private function readNotDeclaredMatcher(): AbstractMatcher + { + return $this->matcher->logicalNot( + $this->matcher->logicalOr( + $this->matcher->annotatedWith(CacheableResponse::class), + $this->matcher->logicalOr( + $this->matcher->annotatedWith(Cacheable::class), + $this->matcher->annotatedWith(DonutCache::class), + ), + ), + ); + } } diff --git a/src/DonutChildExecutions.php b/src/DonutChildExecutions.php new file mode 100644 index 00000000..b769a0d5 --- /dev/null +++ b/src/DonutChildExecutions.php @@ -0,0 +1,54 @@ +> */ + private WeakMap $byPage; + + public function __construct() + { + // A fresh WeakMap is empty, so its template parameters are whatever the property + // declares them as - never actually checked against `object`/`mixed`. + /** @psalm-suppress PropertyTypeCoercion */ + $this->byPage = new WeakMap(); + } + + #[Override] + public function register(ResourceObject $page, OnceRequest $once): void + { + $existing = $this->registered($page); + $existing[] = $once; + $this->byPage[$page] = $existing; + } + + #[Override] + public function take(ResourceObject $page): array + { + $registered = $this->registered($page); + unset($this->byPage[$page]); + + return $registered; + } + + /** @return list */ + private function registered(ResourceObject $page): array + { + if (! isset($this->byPage[$page])) { + return []; + } + + /** @var list $registered */ + $registered = $this->byPage[$page]; + + return $registered; + } +} diff --git a/src/DonutChildExecutionsInterface.php b/src/DonutChildExecutionsInterface.php new file mode 100644 index 00000000..4a8818d7 --- /dev/null +++ b/src/DonutChildExecutionsInterface.php @@ -0,0 +1,41 @@ +body` (or overwrites the slot with a + * plain value pulled out of it) and `ResourceDonut::create()`'s body walk can no longer find + * it. `DonutRepository` reads the list back once the page's donut template has been built - + * late enough that `OnceRequest::wasRendered()` is final for every wrapper that did become a + * hole - to tag the template with whichever children executed but were not. + * + * Bound `Scope::SINGLETON`, one instance per DI container (per request in the usual + * BEAR.Sunday bootstrap) - the same lifetime `UncacheableMark` already relies on. Keyed by + * object identity in a `WeakMap`, not by URI: two sibling pages embedding the same child URI + * must not share a slot, and an entry a `#[DonutCache]` write never reaches (a non-200 + * response, a cache-store failure) is simply never read back, not leaked - `WeakMap` drops it + * with the page object itself. + */ +interface DonutChildExecutionsInterface +{ + /** Record that `$once` was created for one of `$page`'s own `#[Embed]`s */ + public function register(ResourceObject $page, OnceRequest $once): void; + + /** + * Return and forget every wrapper registered for `$page` + * + * A page read exactly once: the second call for the same `$page` returns an empty list, + * so a write path that (for whatever reason) reads twice cannot double-tag. + * + * @return list + */ + public function take(ResourceObject $page): array; +} diff --git a/src/DonutRepository.php b/src/DonutRepository.php index b48002e0..580222ca 100644 --- a/src/DonutRepository.php +++ b/src/DonutRepository.php @@ -24,6 +24,8 @@ use Koriym\SemanticLogger\SemanticLoggerInterface; use Override; +use function array_key_exists; +use function array_merge; use function array_unique; use function array_values; use function explode; @@ -31,6 +33,11 @@ final readonly class DonutRepository implements DonutRepositoryInterface { + /** + * One dependency per collaborator, all constructor-injected. + * + * @SuppressWarnings("PHPMD.ExcessiveParameterList") + */ public function __construct( private QueryRepositoryInterface $queryRepository, private DonutHeaderSetter $donutHeaderSetter, @@ -41,6 +48,7 @@ public function __construct( private DonutRendererInterface $renderer, private UnstorableGateInterface $gate, private UncacheableMarkInterface $mark, + private DonutChildExecutionsInterface $childExecutions, private UriTagInterface $uriTag = new UriTag(), ) { } @@ -146,8 +154,12 @@ private function doPutStatic(ResourceObject $ro, int|null $ttl, int|null $sMaxAg $keys = new SurrogateKeys($ro->uri); $keys->addTag($ro); $declaredKeys = $this->getHeaderKeys($ro); - $templateKeys = $this->templateKeys($ro, $declaredKeys); - $donut = ResourceDonut::create($ro, $this->renderer, $keys, $sMaxAge, true, $this->mark)->withStorageState($ttl, $templateKeys); + // create() must run before templateKeys(): it is what actually runs onGet()'s + // embeds and the page's own (string) $ro template render, so an OnceRequest's + // wasRendered() (issue #226) is not final until it returns. + $donut = ResourceDonut::create($ro, $this->renderer, $keys, $sMaxAge, true, $this->mark); + $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->executedChildTags($ro)); + $donut = $donut->withStorageState($ttl, $templateKeys); $donut->render($ro, $this->renderer); // Checked against the still-unmutated header, before setHeaders() below runs // headerSetter/setCdnCacheControl - the same point QueryRepository checks at (issue @@ -195,8 +207,10 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void $this->logger->event(new PutDonutContext((string) $ro->uri, $donutTtl, null)); $keys = new SurrogateKeys($ro->uri); $declaredKeys = $this->getHeaderKeys($ro); - $templateKeys = $this->templateKeys($ro, $declaredKeys); - $donut = ResourceDonut::create($ro, $this->renderer, $keys, $donutTtl, false, $this->mark)->withStorageState($donutTtl, $templateKeys); + // create() must run before templateKeys(): see doPutStatic()'s own comment above. + $donut = ResourceDonut::create($ro, $this->renderer, $keys, $donutTtl, false, $this->mark); + $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->executedChildTags($ro)); + $donut = $donut->withStorageState($donutTtl, $templateKeys); $donut->render($ro, $this->renderer); $templateUnstorable = $this->gate->reasonExcludingHoleMarks($ro); $responseUnstorable = $this->gate->reason($ro); @@ -221,17 +235,49 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void /** * The tags a donut template is stored under: its own URI tag (what `purge($uri)` invalidates - - * an untagged template is reachable by no `invalidateTags()` call, issue #185), plus whatever - * the resource declared. The children's tags are deliberately absent: a child's write must drop - * the content and leave the shell to be recomposed. + * an untagged template is reachable by no `invalidateTags()` call, issue #185), whatever the + * resource declared, plus (issue #226) whatever executedChildTags() found. The latter already + * excludes a child rendered as a hole - deliberately kept out so that child's own write drops + * only the rendered content and leaves the shell to be recomposed, not the whole template. * * @param list $declaredKeys + * @param list $extraChildTags + * + * @return list + */ + private function templateKeys(ResourceObject $ro, array $declaredKeys, array $extraChildTags = []): array + { + return array_values(array_unique([($this->uriTag)($ro->uri), ...$declaredKeys, ...$extraChildTags])); + } + + /** + * The tags of this page's own embedded children that ran during its onGet()/template + * render but were never substituted into the template as a hole (issue #226) - read + * from whatever EmbedOnceInterceptor registered for $ro, regardless of whether the + * child is still reachable through $ro->body by now (onGet() may have unset() it, or + * overwritten the slot with a plain value pulled out of it). + * + * Never runs a child to find this out: hasExecuted() answers from the wrapper's own + * memo, set only if something already read it. * * @return list */ - private function templateKeys(ResourceObject $ro, array $declaredKeys): array + private function executedChildTags(ResourceObject $ro): array { - return array_values(array_unique([($this->uriTag)($ro->uri), ...$declaredKeys])); + $tags = []; + foreach ($this->childExecutions->take($ro) as $once) { + if (! $once->hasExecuted() || $once->wasRendered()) { + continue; + } + + $childRo = $once->executedResourceObject(); + $tags[] = ($this->uriTag)($childRo->uri); + if (array_key_exists(Header::SURROGATE_KEY, $childRo->headers)) { + $tags = array_merge($tags, explode(' ', $childRo->headers[Header::SURROGATE_KEY])); + } + } + + return array_values(array_unique($tags)); } /** diff --git a/src/DonutRequest.php b/src/DonutRequest.php index 104b4cc2..be838867 100644 --- a/src/DonutRequest.php +++ b/src/DonutRequest.php @@ -33,17 +33,32 @@ public function __toString(): string // string before the hole runs, usable both as enterHole()'s key and, unchanged, // for the placeholder/view mapping that ResourceDonut::refresh() later re-fetches // by (issue #222: a query-less key here would also let two sibling holes on the - // same path but different query args collide on one owner slot). + // same path but different query args collide on one owner slot). Verified equal to + // the executed object's own URI below: __invoke()'s first statement does the exact + // same sync from the exact same $this->query, so the two can never disagree. $uri = $this->request->toUri(); $this->mark->enterHole($uri); try { - $view = (string) ($this->request)(); + // The single execution this hole's content and tags both come from - not a + // second, fresh call through $this->request (which for a plain, unwrapped + // AbstractRequest would double-execute a non-idempotent child, and for an + // OnceRequest would still be correct, but there is no reason to prefer a + // second call over the result already in hand). + $executed = ($this->request)(); + $view = (string) $executed; } finally { $this->mark->leaveHole($uri); } - $this->etags->addTag($this->request->resourceObject); + $this->etags->addTag($executed); $this->donutStorage->setView($uri, $view); + if ($this->request instanceof OnceRequest) { + // Excludes this child from DonutRepository's issue #226 "executed but not + // rendered" sweep: a rendered hole's tag already landed on $this->etags above, + // and (by design, see DonutRepository::templateKeys()) a rendered hole's tag + // stays out of the template's own storage tags either way. + $this->request->markRendered(); + } return ResourceDonut::placeholder($uri); } diff --git a/src/EmbedOnceInterceptor.php b/src/EmbedOnceInterceptor.php new file mode 100644 index 00000000..b5695b2b --- /dev/null +++ b/src/EmbedOnceInterceptor.php @@ -0,0 +1,127 @@ +body` - this one needs to run after. Ray.Aop's own onion ordering gives + * that for free rather than by binding order: `EmbedInterceptor`'s method matcher is a bare + * `annotatedWith(Embed::class)`, which `Ray\Aop\MethodMatch::onionOrderMatch()` binds + * synchronously while walking the method's attributes in declared order - strictly before it + * returns the leftover (including every matcher built with `logicalAnd()`/`logicalOr()`, this + * one's method matcher among them) to be bound in whatever order they were registered. A + * method carrying `#[Embed(...)]` therefore always gets `EmbedInterceptor` ahead of this one in + * its interceptor chain, regardless of whether `DonutCacheModule` or `EmbedResourceModule` is + * installed first - verified for both orders in `EmbedOnceInterceptorInstallOrderTest`. + * + * The `declaredEmbedRels()` check is the fallback for everything that guarantee does not + * cover - a third-party AOP container with different ordering rules, or a future Ray.Aop + * release that changes them: if a declared `#[Embed]`'s `rel` is still missing from the body, + * swapping would either skip it (leaving the double-execution/#226 gap open for that embed) + * or, if run before `EmbedInterceptor` instead of after, race it. Either way nothing here can + * safely proceed, so it warns and leaves the body exactly as it found it - `onGet()` and the + * eventual donut render still work, just without once-only semantics for that one request. + */ +final class EmbedOnceInterceptor implements MethodInterceptor +{ + public function __construct( + private readonly DonutChildExecutionsInterface $childExecutions, + ) { + } + + #[Override] + public function invoke(MethodInvocation $invocation) + { + $ro = $invocation->getThis(); + assert($ro instanceof ResourceObject); + $this->swap($ro, $invocation); + + return $invocation->proceed(); + } + + /** @param MethodInvocation $invocation */ + private function swap(ResourceObject $ro, MethodInvocation $invocation): void + { + $body = $ro->body; + foreach ($this->declaredEmbedRels($invocation) as $rel) { + if (is_array($body) && array_key_exists($rel, $body)) { + continue; + } + + // The real too-early state: EmbedInterceptor::prepareBody() is what turns a + // fresh ResourceObject's null body into [] - if this runs first, $body is + // still null here, never an array missing just one key. + trigger_error(sprintf( + '%s: #[Embed(rel: "%s")] on %s::onGet() was not in the body yet - ' . + 'EmbedOnceInterceptor ran before EmbedInterceptor (check module install order); skipping the swap for this request', + self::class, + $rel, + $ro::class, + ), E_USER_WARNING); + + return; + } + + if (! is_array($body)) { + return; + } + + /** @psalm-suppress MixedAssignment $child */ + foreach ($body as $rel => $child) { + if (! $child instanceof Request) { + continue; + } + + $once = new OnceRequest($child); + $body[$rel] = $once; + $this->childExecutions->register($ro, $once); + } + + $ro->body = $body; + } + + /** + * The `#[Embed]` relations declared on the invoked method, `_self` excluded - a self-link + * embed (`EmbedInterceptor::linkSelf()`) never reaches `$ro->body`, so its absence there + * is not a sign anything ran out of order. + * + * @param MethodInvocation $invocation + * + * @return list + */ + private function declaredEmbedRels(MethodInvocation $invocation): array + { + $rels = []; + foreach ($invocation->getMethod()->getAnnotations() as $annotation) { + if (! $annotation instanceof Embed || $annotation->rel === '_self') { + continue; + } + + $rels[] = $annotation->rel; + } + + return $rels; + } +} diff --git a/src/OnceRequest.php b/src/OnceRequest.php new file mode 100644 index 00000000..36979d56 --- /dev/null +++ b/src/OnceRequest.php @@ -0,0 +1,201 @@ +body[$rel]` for every + * `#[Embed]`. Left as-is, that `Request` can be executed twice for one page render: once + * when the page's own `onGet()` reads a value out of it (`offsetGet()`/`__get()`, which goes + * through `AbstractRequest`'s private memo), and once more when `DonutRequest::__toString()` + * renders it as a hole (`($this->request)()`, which calls `__invoke()` directly and so + * bypasses that memo - see `DonutRequest`). For a non-idempotent child (a counter, a random + * value) the two runs disagree, and the donut template ends up holding a different value + * than whatever the page's own body already used - the MISS render and the stored render + * diverge. + * + * `EmbedOnceInterceptor` swaps every embedded `Request` for one of these before `onGet()` + * runs, so every access path - `onGet()` reading the body, and the later donut hole render - + * shares the one execution. `__invoke()` is the only path that actually calls the inner + * `Request`; every other `AbstractRequest` accessor (`__toString()`, `offsetGet()`, `__get()`, + * `getIterator()`, `jsonSerialize()`) already funnels through `__invoke()` via + * `AbstractRequest`'s own private memo, and callers that invoke `($wrapper)()` directly - + * `DonutRequest` among them - hit this class's own memo instead of the inner `Request`'s, + * closing exactly the gap described above. + * + * `withQuery()`/`addQuery()`/`link*()` discard the memo: a child whose query or links the + * page explicitly changes mid-`onGet()` is expected to run again under the new request, not + * silently answer the previous one. + * + * `AbstractRequest` itself never gets this treatment and is never asked to: a plain + * `$resource->get()`/`newRequest()` call in application code, or a `#[Cacheable]` resource + * read outside a donut page, keeps running every time it is invoked, exactly as before. + * + * @psalm-suppress PropertyNotSetInConstructor `$invoker`/`$linker` are AbstractRequest's own + * construction-only properties, used only by its default `__invoke()` - overridden here + * to always delegate to the inner `Request` instead, so neither is ever read. + */ +final class OnceRequest extends AbstractRequest +{ + private ResourceObject|null $executed = null; + private bool $rendered = false; + + public function __construct(private readonly Request $request) + { + $this->resourceObject = $request->resourceObject; + $this->uri = $request->uri; + $this->method = $request->method; + $this->query = $request->query; + $this->links = $request->links; + $this->in = $request->in; + } + + /** + * {@inheritDoc} + * + * The single execution point: runs the inner `Request` the first time this is called + * (directly, or through `AbstractRequest`'s own memo via `__toString()`/`offsetGet()`/ + * etc.) and returns that same result on every later call, until a query or link change + * discards it. + */ + #[Override] + public function __invoke(array|null $query = null): ResourceObject + { + if ($query !== null) { + $this->request->addQuery($query); + $this->resync(); + } + + if ($this->executed === null) { + $this->executed = ($this->request)(); + } + + return $this->executed; + } + + /** {@inheritDoc} */ + #[Override] + public function withQuery(array $query): RequestInterface + { + $this->request->withQuery($query); + $this->resync(); + + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function addQuery(array $query): RequestInterface + { + $this->request->addQuery($query); + $this->resync(); + + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function toUri(): string + { + return $this->request->toUri(); + } + + /** {@inheritDoc} */ + #[Override] + public function toUriWithMethod(): string + { + return $this->request->toUriWithMethod(); + } + + /** {@inheritDoc} */ + #[Override] + public function linkSelf(string $linkKey): RequestInterface + { + $this->request->linkSelf($linkKey); + $this->resync(); + + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function linkNew(string $linkKey): RequestInterface + { + $this->request->linkNew($linkKey); + $this->resync(); + + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function linkCrawl(string $linkKey): RequestInterface + { + $this->request->linkCrawl($linkKey); + $this->resync(); + + return $this; + } + + /** Whether the inner `Request` has run, under the query/links currently set */ + public function hasExecuted(): bool + { + return $this->executed !== null; + } + + /** Whether `DonutRequest` has rendered this execution as a hole (see `markRendered()`) */ + public function wasRendered(): bool + { + return $this->rendered; + } + + /** + * Record that this execution was substituted into the donut template as a hole + * + * Called by `DonutRequest::__toString()` once it has rendered this child - so that + * `DonutRepository` (issue #226) does not also tag it as a body-only, non-hole read: + * the hole already carries its own tag (`DonutRequest` adds it directly), and a + * rendered hole's tag is deliberately kept out of the donut template's own storage + * tags (`DonutRepository::templateKeys()`) so that the child's own write drops the + * rendered content and leaves the shell to be recomposed, not the whole template. + */ + public function markRendered(): void + { + $this->rendered = true; + } + + /** + * Return the single execution's result, running it now if nothing has read it yet + * + * Never executes a second time: used by `DonutRepository` (issue #226) to read the + * tags of a child that already ran, not to decide whether it should run. + */ + public function executedResourceObject(): ResourceObject + { + return ($this)(); + } + + /** Discard the memo and mirror the inner `Request`'s query/links after they changed */ + private function resync(): void + { + $this->query = $this->request->query; + $this->links = $this->request->links; + $this->executed = null; + $this->rendered = false; + // AbstractRequest's own private invoke() short-circuits on $result !== null, + // bypassing our overridden __invoke() entirely for every accessor that goes + // through it (offsetGet(), __get(), getIterator(), __toString(), jsonSerialize()) - + // left stale here, a withQuery()/addQuery()/link*() call after such a read would + // keep answering the pre-change ResourceObject forever. + $this->result = null; + } +} diff --git a/src/ResourceBodyEvaluator.php b/src/ResourceBodyEvaluator.php index a961de23..4bc9ebac 100644 --- a/src/ResourceBodyEvaluator.php +++ b/src/ResourceBodyEvaluator.php @@ -74,11 +74,21 @@ private function copyForStore(mixed $item): mixed * The copy is the storage boundary: the memoized object stays reachable through the * live request and the pool may hold it by reference, while the recursion above * rewrites body in place. + * + * AbstractRequest's own contract is that jsonSerialize() answers that memoized + * ResourceObject - but a subclass may override it to mean something else (BEAR\Async\ + * AsyncRequest decodes an already-flushed JSON view into an array instead, to support + * PendingRequests' own parallel-execution protocol). Materializing such a request would + * need that package's own flush semantics, which is out of scope here, so it is left as + * the live request rather than forced through asResourceObject() and crashed on. */ - private function materialize(RequestInterface $request): ResourceObject + private function materialize(RequestInterface $request): mixed { if ($request instanceof AbstractRequest) { - return clone $this->asResourceObject($request->jsonSerialize()); + /** @psalm-suppress MixedAssignment $memo - mixed when jsonSerialize() answers something other than the memo */ + $memo = $request->jsonSerialize(); + + return $memo instanceof ResourceObject ? clone $memo : $request; } return $this->asResourceObject($request()); diff --git a/tests/EmbedOnceInterceptorInstallOrderTest.php b/tests/EmbedOnceInterceptorInstallOrderTest.php new file mode 100644 index 00000000..3d7fa85f --- /dev/null +++ b/tests/EmbedOnceInterceptorInstallOrderTest.php @@ -0,0 +1,104 @@ +assertOnceSemanticsHold($module); + } + + public function testReversedOrderResourceModuleInstalledBeforeQueryRepositoryModule(): void + { + $module = new class extends AbstractModule { + protected function configure(): void + { + $this->install(new ResourceModule('FakeVendor\HelloWorld')); + $this->install(new QueryRepositoryModule()); + } + }; + $this->assertOnceSemanticsHold($module); + } + + private function assertOnceSemanticsHold(AbstractModule $module): void + { + $module->override(new class extends AbstractModule { + protected function configure(): void + { + $this->bind(AdapterInterface::class)->annotatedWith(ResourceObjectPool::class)->to(ArrayAdapter::class); + $this->bind(AdapterInterface::class)->annotatedWith(EtagPool::class)->to(ArrayAdapter::class); + $this->bind(SemanticLoggerInterface::class)->annotatedWith(CacheLog::class)->to(NullSemanticLogger::class); + } + }); + $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); + + $warnings = []; + set_error_handler(static function (int $errno, string $message) use (&$warnings): bool { + if ($errno === E_USER_WARNING) { + $warnings[] = $message; + } + + return true; + }); + try { + $injector = new Injector($module, __DIR__ . '/tmp'); + $resource = $injector->getInstance(ResourceInterface::class); + $ro = $resource->get(self::COUNTER_DONUT); + } finally { + restore_error_handler(); + } + + $this->assertSame([], $warnings, 'EmbedOnceInterceptor must never warn that it ran before EmbedInterceptor, in either install order'); + $this->assertSame(1, Counter::$calls, 'the embed must still execute exactly once, in either install order'); + $view = (string) $ro->view; + $this->assertStringContainsString('read:1', $view); + $this->assertStringContainsString('call:1', $view); + } +} diff --git a/tests/EmbedOnceInterceptorWarningTest.php b/tests/EmbedOnceInterceptorWarningTest.php new file mode 100644 index 00000000..41b527cd --- /dev/null +++ b/tests/EmbedOnceInterceptorWarningTest.php @@ -0,0 +1,154 @@ +body = null; + + $invocation = new class ($ro) implements MethodInvocation { + public function __construct(private readonly ResourceObject $ro) + { + } + + public function proceed(): mixed + { + return $this->ro; + } + + public function getThis(): ResourceObject + { + return $this->ro; + } + + public function getArguments(): ArrayObject + { + return new ArrayObject(); + } + + public function getNamedArguments(): ArrayObject + { + return new ArrayObject(); + } + + public function getMethod(): ReflectionMethod + { + return new ReflectionMethod(CounterDonut::class, 'onGet'); + } + }; + + $warnings = []; + set_error_handler(static function (int $errno, string $message) use (&$warnings): bool { + if ($errno === E_USER_WARNING) { + $warnings[] = $message; + } + + return true; + }); + try { + $interceptor = new EmbedOnceInterceptor(new DonutChildExecutions()); + $result = $interceptor->invoke($invocation); + } finally { + restore_error_handler(); + } + + $this->assertSame($ro, $result, 'proceed() still runs - the request is served, just without once-only semantics'); + $this->assertNull($ro->body, 'nothing is swapped (or even initialized) when this runs before EmbedInterceptor'); + $this->assertCount(1, $warnings); + $this->assertTrue( + str_contains($warnings[0], 'counter') && str_contains($warnings[0], 'EmbedOnceInterceptor ran before EmbedInterceptor'), + sprintf('unexpected warning message: %s', $warnings[0]), + ); + } + + public function testSelfLinkOnlyEmbedLeavesANullBodyAlone(): void + { + // _self never reaches $ro->body (EmbedInterceptor::linkSelf()), so + // declaredEmbedRels() is empty for this class and the declared-rel loop never + // runs - the one real shape that reaches swap()'s is_array($body) fallback + // instead, with body still null if nothing else has touched it yet. + $ro = new SelfLinkOnlyDonut(); + $ro->body = null; + + $invocation = new class ($ro) implements MethodInvocation { + public function __construct(private readonly ResourceObject $ro) + { + } + + public function proceed(): mixed + { + return $this->ro; + } + + public function getThis(): ResourceObject + { + return $this->ro; + } + + public function getArguments(): ArrayObject + { + return new ArrayObject(); + } + + public function getNamedArguments(): ArrayObject + { + return new ArrayObject(); + } + + public function getMethod(): ReflectionMethod + { + return new ReflectionMethod(SelfLinkOnlyDonut::class, 'onGet'); + } + }; + + $warnings = []; + set_error_handler(static function (int $errno, string $message) use (&$warnings): bool { + if ($errno === E_USER_WARNING) { + $warnings[] = $message; + } + + return true; + }); + try { + $interceptor = new EmbedOnceInterceptor(new DonutChildExecutions()); + $result = $interceptor->invoke($invocation); + } finally { + restore_error_handler(); + } + + $this->assertSame($ro, $result); + $this->assertNull($ro->body, 'nothing to swap - a self-link embed has no body entry to begin with'); + $this->assertSame([], $warnings, 'no declared rel is ever missing, since _self never counts as one'); + } +} diff --git a/tests/EmbeddedChildMaterializationTest.php b/tests/EmbeddedChildMaterializationTest.php index 0aca8267..2f21e6d0 100644 --- a/tests/EmbeddedChildMaterializationTest.php +++ b/tests/EmbeddedChildMaterializationTest.php @@ -151,6 +151,26 @@ public function testRequestWithoutMemoIsMaterializedByRunningIt(): void $this->assertSame(1, $request->invoked); } + /** + * An AbstractRequest whose jsonSerialize() does not answer a memoized ResourceObject + * (BEAR\Async\AsyncRequest's own shape, see ResourceBodyEvaluator::materialize()) is + * left as the live request instead of crashing: a blind assert($memo instanceof + * ResourceObject) would raise AssertionError here (zend.assertions=1 in this suite), + * and even with assertions off, `clone` on the decoded array would be a fatal error. + */ + public function testAsyncLikeRequestIsLeftUnmaterializedInsteadOfCrashing(): void + { + $ro = new class extends ResourceObject { + }; + $ro->uri = new Uri('app://self/async-like'); + $request = new FakeAsyncLikeRequest($ro); + + $body = (new ResourceBodyEvaluator())(['child' => $request]); + + assert(is_array($body)); + $this->assertSame($request, $body['child'], 'not materialized - the live request is kept, not wrapped into a ResourceObject'); + } + /** * A ResourceObject sitting in the body is copied before its own body is rewritten * diff --git a/tests/Fake/FakeAsyncLikeRequest.php b/tests/Fake/FakeAsyncLikeRequest.php new file mode 100644 index 00000000..ead90abc --- /dev/null +++ b/tests/Fake/FakeAsyncLikeRequest.php @@ -0,0 +1,95 @@ + 'from-pending-requests']; + } + + /** + * Never exercised by ResourceBodyEvaluator (it reads jsonSerialize() only) - stubbed + * out just to satisfy AbstractRequest's abstract contract for a concrete subclass. + */ + #[Override] + public function withQuery(array $query): RequestInterface + { + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function addQuery(array $query): RequestInterface + { + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function toUri(): string + { + return (string) $this->uri; + } + + /** {@inheritDoc} */ + #[Override] + public function toUriWithMethod(): string + { + return $this->toUri(); + } + + /** {@inheritDoc} */ + #[Override] + public function linkSelf(string $linkKey): RequestInterface + { + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function linkNew(string $linkKey): RequestInterface + { + return $this; + } + + /** {@inheritDoc} */ + #[Override] + public function linkCrawl(string $linkKey): RequestInterface + { + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/App/Counter.php b/tests/Fake/fake-app/src/Resource/App/Counter.php new file mode 100644 index 00000000..a0b8229e --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/App/Counter.php @@ -0,0 +1,29 @@ +get()` and + * every embedded read runs this, the same as any other call. Used to prove that + * `OnceRequest`'s once-only semantics apply only when `EmbedOnceInterceptor` actually wraps + * an embed, never to a direct call. + */ +class Counter extends ResourceObject +{ + public static int $calls = 0; + + public function onGet(string|null $tag = null): static + { + self::$calls++; + $this->body = [ + 'call' => self::$calls, + 'tag' => $tag, + ]; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildDonut.php b/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildDonut.php new file mode 100644 index 00000000..1a1f2502 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildDonut.php @@ -0,0 +1,31 @@ +body` no longer holds any trace it ever existed. + * - `unused` is declared but never touched at all - onGet() never reads it, the template + * never renders it - proving it contributes no tag and, more importantly, never runs + * ("never execute a child just to tag it"). + */ +#[DonutCache] +class BodyOnlyChildDonut extends ResourceObject +{ + #[Embed(rel: 'category', src: 'page://self/html/comment')] + #[Embed(rel: 'unused', src: 'app://self/counter')] + public function onGet(): static + { + $this->body['description'] = $this->body['category']['comment']; + unset($this->body['category']); + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/CounterCacheMethodLevel.php b/tests/Fake/fake-app/src/Resource/Page/Html/CounterCacheMethodLevel.php new file mode 100644 index 00000000..95c58530 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/CounterCacheMethodLevel.php @@ -0,0 +1,29 @@ + */ + public $body = ['label' => 'counter-cache-method-level']; + + #[CacheableResponse] + #[Embed(rel: 'counter', src: 'app://self/counter')] + public function onGet(): static + { + $this->body['read'] = $this->body['counter']['call']; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/CounterDonut.php b/tests/Fake/fake-app/src/Resource/Page/Html/CounterDonut.php new file mode 100644 index 00000000..24dd4095 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/CounterDonut.php @@ -0,0 +1,32 @@ +|null */ + public $body = ['label' => 'counter-donut']; + + #[Embed(rel: 'counter', src: 'app://self/counter')] + public function onGet(): static + { + $this->body['read'] = $this->body['counter']['call']; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/RequeryDonut.php b/tests/Fake/fake-app/src/Resource/Page/Html/RequeryDonut.php new file mode 100644 index 00000000..6da53096 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/RequeryDonut.php @@ -0,0 +1,37 @@ +body['item']['call']; + $this->body['item']->withQuery(['tag' => 'requeried']); + $second = $this->body['item']['call']; + $secondTag = $this->body['item']['tag']; + unset($this->body['item']); + + $this->body['first'] = $first; + $this->body['second'] = $second; + $this->body['secondTag'] = $secondTag; + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/SelfLinkOnlyDonut.php b/tests/Fake/fake-app/src/Resource/Page/Html/SelfLinkOnlyDonut.php new file mode 100644 index 00000000..cdcc2111 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/SelfLinkOnlyDonut.php @@ -0,0 +1,24 @@ +body` at all. declaredEmbedRels() filters + * `_self` out, so this is the one real shape where EmbedOnceInterceptor::swap() reaches + * its is_array($body) fallback (as opposed to the declared-rel loop) with a genuinely + * null body if nothing else has run yet. + */ +#[DonutCache] +class SelfLinkOnlyDonut extends ResourceObject +{ + #[Embed(rel: '_self', src: 'page://self/html/self-link-only-donut')] + public function onGet(): static + { + return $this; + } +} diff --git a/tests/Fake/fake-app/var/templates/App/Counter.html.twig b/tests/Fake/fake-app/var/templates/App/Counter.html.twig new file mode 100644 index 00000000..5208b858 --- /dev/null +++ b/tests/Fake/fake-app/var/templates/App/Counter.html.twig @@ -0,0 +1 @@ +call:{{ call }} tag:{{ tag }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildDonut.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildDonut.html.twig new file mode 100644 index 00000000..8342b6ba --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildDonut.html.twig @@ -0,0 +1 @@ +desc:{{ description }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/CounterCacheMethodLevel.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/CounterCacheMethodLevel.html.twig new file mode 100644 index 00000000..af007d9a --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/CounterCacheMethodLevel.html.twig @@ -0,0 +1 @@ +read:{{ read }}{{ counter }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/CounterDonut.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/CounterDonut.html.twig new file mode 100644 index 00000000..af007d9a --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/CounterDonut.html.twig @@ -0,0 +1 @@ +read:{{ read }}{{ counter }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/RequeryDonut.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/RequeryDonut.html.twig new file mode 100644 index 00000000..d16cb3a1 --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/RequeryDonut.html.twig @@ -0,0 +1 @@ +first:{{ first }} second:{{ second }} secondTag:{{ secondTag }} \ No newline at end of file diff --git a/tests/OnceRequestBehaviorTest.php b/tests/OnceRequestBehaviorTest.php new file mode 100644 index 00000000..70ab9170 --- /dev/null +++ b/tests/OnceRequestBehaviorTest.php @@ -0,0 +1,179 @@ +override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); + $injector = new Injector($module, __DIR__ . '/tmp'); + $this->resource = $injector->getInstance(ResourceInterface::class); + $this->queryRepository = $injector->getInstance(QueryRepositoryInterface::class); + $this->logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + } + + public function testEmbeddedChildExecutesOnceAcrossBodyReadAndHoleRender(): void + { + $ro = $this->resource->get(self::COUNTER_DONUT); + $this->flushAndValidate($this->logger); + + $this->assertSame(1, Counter::$calls, 'read in onGet() and rendered as a hole: one execution total'); + $view = (string) $ro->view; + $this->assertStringContainsString('read:1', $view); + $this->assertStringContainsString('call:1', $view, 'the hole was substituted from the very same execution onGet() already read'); + } + + public function testWithQueryAfterAReadReExecutesThroughOffsetGet(): void + { + $ro = $this->resource->get(self::REQUERY_DONUT); + $this->flushAndValidate($this->logger); + + // Both reads went through offsetGet(), never a direct ($request)() call - if + // OnceRequest::resync() left AbstractRequest's own private $result memo stale, + // both would read 1 and $calls would stay 1. + $this->assertSame(2, Counter::$calls, 'withQuery() after a read must force a second, real execution'); + $view = (string) $ro->view; + $this->assertStringContainsString('first:1', $view); + $this->assertStringContainsString('second:2', $view); + $this->assertStringContainsString('secondTag:requeried', $view, 'the second read reflects the new query, not a stale memo of the first'); + } + + public function testExplicitResourceCallAlwaysExecutes(): void + { + $this->resource->get(self::COUNTER); + $this->resource->get(self::COUNTER); + $this->resource->get(self::COUNTER); + + $this->assertSame(3, Counter::$calls, 'a direct, non-embedded call is never wrapped - every call is a real roll of the dice'); + } + + public function testCacheableResourceOutsideDonutPageIsUnaffected(): void + { + $this->resource->get(self::CHILD); + $miss = $this->flushAndValidate($this->logger); + $missClose = self::scopeCloseOf($miss, 'get', self::CHILD); + $this->assertNotNull($missClose); + $this->assertSame('cache_miss', $missClose[0]); + + $this->resource->get(self::CHILD); + $hit = $this->flushAndValidate($this->logger); + $hitClose = self::scopeCloseOf($hit, 'get', self::CHILD); + $this->assertNotNull($hitClose); + $this->assertSame('cache_hit', $hitClose[0], '#[Cacheable] outside any donut page caches exactly as before - untouched by EmbedOnceInterceptor'); + } + + public function testBodyOnlyChildTagsTheStoredTemplateAndNeverTouchedEmbedDoesNot(): void + { + $this->resource->get(self::BODY_ONLY_CHILD_DONUT); + $tree = $this->flushAndValidate($this->logger); + + $this->assertSame(0, Counter::$calls, "the 'unused' embed is never read and never rendered - it must never run just to be tagged"); + + $saveDonut = self::eventContextsJsonOf($tree, 'save_donut', self::BODY_ONLY_CHILD_DONUT); + $this->assertCount(1, $saveDonut); + $this->assertStringContainsString('"' . self::CHILD_URI_TAG . '"', $saveDonut[0], "the body-only child's own URI tag lands on the stored template (issue #226)"); + $this->assertStringContainsString('"' . self::CHILD_SURROGATE_KEY . '"', $saveDonut[0], "the body-only child's own declared Surrogate-Key lands on the stored template too"); + $this->assertStringNotContainsString('"' . self::COUNTER_URI_TAG . '"', $saveDonut[0], 'the never-touched embed contributes no tag'); + } + + public function testPurgingTheBodyOnlyChildInvalidatesThePage(): void + { + $this->resource->get(self::BODY_ONLY_CHILD_DONUT); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::BODY_ONLY_CHILD_DONUT); + $warm = $this->flushAndValidate($this->logger); + $warmClose = self::scopeCloseOf($warm, 'get', self::BODY_ONLY_CHILD_DONUT); + $this->assertNotNull($warmClose); + $this->assertSame('cache_hit', $warmClose[0], 'the stored template is warm before the purge'); + + $this->queryRepository->purge(new Uri(self::CHILD)); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::BODY_ONLY_CHILD_DONUT); + $tree = $this->flushAndValidate($this->logger); + $close = self::scopeCloseOf($tree, 'get', self::BODY_ONLY_CHILD_DONUT); + $this->assertNotNull($close); + $this->assertSame('cache_miss', $close[0], "purging the body-only child's tag must invalidate the page's own stored template, not just leave it to a silent refresh"); + } + + public function testMissAndHitServeByteIdenticalViewForAnIdempotentChild(): void + { + $missRo = $this->resource->get(self::BLOG_POSTING_DONUT); + $this->flushAndValidate($this->logger); + $missView = (string) $missRo->view; + $this->assertStringContainsString('comment01', $missView); + + $hitRo = $this->resource->get(self::BLOG_POSTING_DONUT); + $hit = $this->flushAndValidate($this->logger); + $hitClose = self::scopeCloseOf($hit, 'get', self::BLOG_POSTING_DONUT); + $this->assertNotNull($hitClose); + $this->assertSame('cache_hit', $hitClose[0], 'this read must come from the stored donut template, not a fresh render'); + + $this->assertSame( + $missView, + (string) $hitRo->view, + 'the MISS render and the HIT-refresh of an idempotent child must be byte-identical - the whole point of "MISS equals the render without #[DonutCache], HIT re-fetches only holes"', + ); + } + + public function testMethodLevelCacheableResponseEmbedExecutesOnceToo(): void + { + // DonutCacheModule::installAopMethodModule() binds EmbedOnceInterceptor a second + // time, for a class whose #[CacheableResponse] sits on onGet() itself (like + // BlogPostingCache) rather than on the class - this is the fixture proving THAT + // binding is actually reached, not merely registered at configure() time: a dead + // binding here would leave 'counter' as a plain Request, double-executing it the + // same way CounterDonut (class-level) would without EmbedOnceInterceptor at all. + $ro = $this->resource->get(self::COUNTER_CACHE_METHOD_LEVEL); + $this->flushAndValidate($this->logger); + + $this->assertSame(1, Counter::$calls); + $view = (string) $ro->view; + $this->assertStringContainsString('read:1', $view); + $this->assertStringContainsString('call:1', $view); + } +} diff --git a/tests/OnceRequestUnitTest.php b/tests/OnceRequestUnitTest.php new file mode 100644 index 00000000..263929c5 --- /dev/null +++ b/tests/OnceRequestUnitTest.php @@ -0,0 +1,107 @@ +resource = $injector->getInstance(ResourceInterface::class); + } + + private function newCounterRequest(): Request + { + $request = $this->resource->newRequest(Method::GET, 'app://self/counter'); + self::assertInstanceOf(Request::class, $request); + + return $request; + } + + public function testAddQueryDiscardsTheMemoAndReExecutes(): void + { + $once = new OnceRequest($this->newCounterRequest()); + $first = $once(); + $this->assertTrue($once->hasExecuted()); + + $once->addQuery(['tag' => 'added']); + $this->assertFalse($once->hasExecuted(), 'addQuery() must discard the memo, same as withQuery()'); + + $second = $once(); + $this->assertSame(2, Counter::$calls); + $this->assertNotSame($first, $second); + assert(is_array($second->body)); + $this->assertSame('added', $second->body['tag']); + } + + public function testInvokeWithAQueryArgumentMergesAndReExecutes(): void + { + $once = new OnceRequest($this->newCounterRequest()); + $once(); + $this->assertSame(1, Counter::$calls); + + $second = $once(['tag' => 'merged']); + $this->assertSame(2, Counter::$calls, '__invoke($query) must force a second execution under the merged query'); + assert(is_array($second->body)); + $this->assertSame('merged', $second->body['tag']); + } + + public function testToUriWithMethodDelegatesToTheInnerRequest(): void + { + $inner = $this->newCounterRequest(); + $once = new OnceRequest($inner); + + $this->assertSame($inner->toUriWithMethod(), $once->toUriWithMethod()); + } + + public function testLinkSelfDelegatesAndDiscardsTheMemo(): void + { + $once = new OnceRequest($this->newCounterRequest()); + $once(); + $this->assertTrue($once->hasExecuted()); + + $once->linkSelf('someLink'); + $this->assertFalse($once->hasExecuted(), 'a link change must discard the memo too - the next invocation is a linked one'); + $this->assertNotSame([], $once->links); + } + + public function testLinkNewDelegatesAndDiscardsTheMemo(): void + { + $once = new OnceRequest($this->newCounterRequest()); + $once(); + + $once->linkNew('someLink'); + $this->assertFalse($once->hasExecuted()); + $this->assertNotSame([], $once->links); + } + + public function testLinkCrawlDelegatesAndDiscardsTheMemo(): void + { + $once = new OnceRequest($this->newCounterRequest()); + $once(); + + $once->linkCrawl('someLink'); + $this->assertFalse($once->hasExecuted()); + $this->assertNotSame([], $once->links); + } +} From c4b8dca451ffb288e6df050f17157757bdf745bd Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 12:08:46 +0900 Subject: [PATCH 2/6] Guard version-dependent test fixtures for bear/resource's lowest-supported release bear/resource's lowest allowed release (per this package's own ^1.16.1 floor) declares AbstractRequest::jsonSerialize(): ResourceObject (not mixed) and ResourceInterface has no newRequest(): both are PHP class-declaration-time incompatibilities, not runtime ones, so the affected tests now detect the gap via Reflection/method_exists and skip with a stated reason instead of fatal-erroring the whole PHPUnit process on class autoload. --- tests/EmbeddedChildMaterializationTest.php | 12 ++++++++++++ tests/OnceRequestUnitTest.php | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/tests/EmbeddedChildMaterializationTest.php b/tests/EmbeddedChildMaterializationTest.php index 2f21e6d0..29a1e0d1 100644 --- a/tests/EmbeddedChildMaterializationTest.php +++ b/tests/EmbeddedChildMaterializationTest.php @@ -12,6 +12,7 @@ use FakeVendor\HelloWorld\Resource\Page\Dep\CountingChild; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; +use ReflectionMethod; use function assert; use function is_array; @@ -157,9 +158,20 @@ public function testRequestWithoutMemoIsMaterializedByRunningIt(): void * left as the live request instead of crashing: a blind assert($memo instanceof * ResourceObject) would raise AssertionError here (zend.assertions=1 in this suite), * and even with assertions off, `clone` on the decoded array would be a fatal error. + * + * `AbstractRequest::jsonSerialize()` is declared `: ResourceObject` in older bear/resource + * releases (this project's own lowest-supported one among them) and `: mixed` in newer + * ones - a fixture overriding it to answer an array is only loadable under the latter, so + * this skips rather than fatal-erroring PHP's own class-declaration check under the former + * (`FakeAsyncLikeRequest` is only autoloaded past this guard). */ public function testAsyncLikeRequestIsLeftUnmaterializedInsteadOfCrashing(): void { + $returnType = (new ReflectionMethod(AbstractRequest::class, 'jsonSerialize'))->getReturnType(); + if ((string) $returnType !== 'mixed') { + self::markTestSkipped('this bear/resource release declares AbstractRequest::jsonSerialize(): ' . $returnType . ' - a request answering non-ResourceObject data from it cannot be constructed here'); + } + $ro = new class extends ResourceObject { }; $ro->uri = new Uri('app://self/async-like'); diff --git a/tests/OnceRequestUnitTest.php b/tests/OnceRequestUnitTest.php index 263929c5..0d5f1257 100644 --- a/tests/OnceRequestUnitTest.php +++ b/tests/OnceRequestUnitTest.php @@ -25,6 +25,10 @@ class OnceRequestUnitTest extends TestCase protected function setUp(): void { + if (! method_exists(ResourceInterface::class, 'newRequest')) { + self::markTestSkipped('this bear/resource release has no ResourceInterface::newRequest() - the delegate/discard methods this suite covers are exercised end to end instead, in OnceRequestBehaviorTest'); + } + Counter::$calls = 0; $injector = new Injector(ModuleFactory::getInstance('FakeVendor\HelloWorld'), __DIR__ . '/tmp'); $this->resource = $injector->getInstance(ResourceInterface::class); From d2696967e2e285c2e59348bdd484783f5cce3bfc Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 12:10:55 +0900 Subject: [PATCH 3/6] Fix cs/phpstan on the bear/resource version guard method_exists() needs a use-function import (cs), and phpstan's own bear/resource stub always sees newRequest() declared, so the version check is annotated accordingly. --- tests/OnceRequestUnitTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/OnceRequestUnitTest.php b/tests/OnceRequestUnitTest.php index 0d5f1257..89579da1 100644 --- a/tests/OnceRequestUnitTest.php +++ b/tests/OnceRequestUnitTest.php @@ -13,6 +13,7 @@ use function assert; use function is_array; +use function method_exists; /** * Unit-level coverage for the OnceRequest delegate/discard methods that the end-to-end @@ -25,7 +26,7 @@ class OnceRequestUnitTest extends TestCase protected function setUp(): void { - if (! method_exists(ResourceInterface::class, 'newRequest')) { + if (! method_exists(ResourceInterface::class, 'newRequest')) { // @phpstan-ignore-line self::markTestSkipped('this bear/resource release has no ResourceInterface::newRequest() - the delegate/discard methods this suite covers are exercised end to end instead, in OnceRequestBehaviorTest'); } From be7450a6b588ba2c1cabedb0abdf381062f9d9cd Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 13:03:22 +0900 Subject: [PATCH 4/6] Keep every OnceRequest execution generation, tag each one, and add Request-matching __get() Review findings on #226: 1. A withQuery()/addQuery()/link*() call discarded OnceRequest's execution history, so only the LAST generation's tag ever reached the stored template and the live response's SurrogateKeys - purging an earlier generation's own tag left the page serving a stale cache_hit. OnceRequest now keeps every generation (ResourceObject + rendered flag), and DonutRepository::unrenderedChildren() reads all of them instead of just the current one. 2. On the doPutStatic()/#[CacheableResponse] path, an unrendered child's tag reached templateKeys() but never SurrogateKeys, so the stored view/ETag carried only the page's own tag - purging the child left a stale cache_hit even though the #[DonutCache] template was correctly tagged. Fixed in both doPutStatic() and doPutDonut() (the latter never stores a server-side ETag/view at all, but its live response's CDN Surrogate-Key header had the same gap). 3. OnceRequest::__get() now mirrors Request::__get()'s own DSL (eager/lazy sets on both wrapper and inner request and returns ; code/headers/body read from this class's own memo; anything else throws OutOfRangeException) - AbstractRequest's default __get() would otherwise answer through its own separate private memo. --- src/DonutChildExecutionsInterface.php | 4 +- src/DonutRepository.php | 87 ++++++++++---- src/OnceRequest.php | 111 +++++++++++++----- .../Html/BodyOnlyChildCacheableResponse.php | 27 +++++ .../BodyOnlyChildCacheableResponse.html.twig | 1 + tests/OnceRequestBehaviorTest.php | 69 +++++++++++ tests/OnceRequestUnitTest.php | 65 ++++++++++ 7 files changed, 313 insertions(+), 51 deletions(-) create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildCacheableResponse.php create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildCacheableResponse.html.twig diff --git a/src/DonutChildExecutionsInterface.php b/src/DonutChildExecutionsInterface.php index 4a8818d7..eb624aba 100644 --- a/src/DonutChildExecutionsInterface.php +++ b/src/DonutChildExecutionsInterface.php @@ -14,8 +14,8 @@ * even if `onGet()` later `unset()`s it from `$ro->body` (or overwrites the slot with a * plain value pulled out of it) and `ResourceDonut::create()`'s body walk can no longer find * it. `DonutRepository` reads the list back once the page's donut template has been built - - * late enough that `OnceRequest::wasRendered()` is final for every wrapper that did become a - * hole - to tag the template with whichever children executed but were not. + * late enough that `OnceRequest::generations()` is final for every generation of every + * wrapper - to tag the template with whichever generations executed but were never rendered. * * Bound `Scope::SINGLETON`, one instance per DI container (per request in the usual * BEAR.Sunday bootstrap) - the same lifetime `UncacheableMark` already relies on. Keyed by diff --git a/src/DonutRepository.php b/src/DonutRepository.php index 580222ca..35ace451 100644 --- a/src/DonutRepository.php +++ b/src/DonutRepository.php @@ -154,11 +154,20 @@ private function doPutStatic(ResourceObject $ro, int|null $ttl, int|null $sMaxAg $keys = new SurrogateKeys($ro->uri); $keys->addTag($ro); $declaredKeys = $this->getHeaderKeys($ro); - // create() must run before templateKeys(): it is what actually runs onGet()'s - // embeds and the page's own (string) $ro template render, so an OnceRequest's - // wasRendered() (issue #226) is not final until it returns. + // create() must run before unrenderedChildren(): it is what actually runs onGet()'s + // embeds and the page's own (string) $ro template render, so an OnceRequest's own + // generations() (issue #226) is not final until it returns. $donut = ResourceDonut::create($ro, $this->renderer, $keys, $sMaxAge, true, $this->mark); - $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->executedChildTags($ro)); + $unrenderedChildren = $this->unrenderedChildren($ro); + // The live response's own Surrogate-Key carries every unrendered child's tag too + // (issue #226 review): without this, purging such a child's tag invalidates the + // stored template but leaves the saved ETag/view entry - tagged from $keys alone - + // reachable, so a purge after this write still serves a cache_hit. + foreach ($unrenderedChildren as $childRo) { + $keys->addTag($childRo); + } + + $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->childTags($unrenderedChildren)); $donut = $donut->withStorageState($ttl, $templateKeys); $donut->render($ro, $this->renderer); // Checked against the still-unmutated header, before setHeaders() below runs @@ -207,9 +216,19 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void $this->logger->event(new PutDonutContext((string) $ro->uri, $donutTtl, null)); $keys = new SurrogateKeys($ro->uri); $declaredKeys = $this->getHeaderKeys($ro); - // create() must run before templateKeys(): see doPutStatic()'s own comment above. + // create() must run before unrenderedChildren(): see doPutStatic()'s own comment above. $donut = ResourceDonut::create($ro, $this->renderer, $keys, $donutTtl, false, $this->mark); - $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->executedChildTags($ro)); + $unrenderedChildren = $this->unrenderedChildren($ro); + // Same reasoning as doPutStatic()'s own loop: this page's live CDN Surrogate-Key + // header (set below via $keys->setSurrogateHeader()) must carry an unrendered + // child's tag too, or a CDN-level purge by that tag cannot reach this response - + // even though a #[DonutCache] page never gets a server-side ETag/view entry of its + // own (only the template, tagged via $templateKeys below, is ever saved here). + foreach ($unrenderedChildren as $childRo) { + $keys->addTag($childRo); + } + + $templateKeys = $this->templateKeys($ro, $declaredKeys, $this->childTags($unrenderedChildren)); $donut = $donut->withStorageState($donutTtl, $templateKeys); $donut->render($ro, $this->renderer); $templateUnstorable = $this->gate->reasonExcludingHoleMarks($ro); @@ -236,7 +255,7 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void /** * The tags a donut template is stored under: its own URI tag (what `purge($uri)` invalidates - * an untagged template is reachable by no `invalidateTags()` call, issue #185), whatever the - * resource declared, plus (issue #226) whatever executedChildTags() found. The latter already + * resource declared, plus (issue #226) whatever extra tags the caller found. The latter already * excludes a child rendered as a hole - deliberately kept out so that child's own write drops * only the rendered content and leaves the shell to be recomposed, not the whole template. * @@ -251,26 +270,54 @@ private function templateKeys(ResourceObject $ro, array $declaredKeys, array $ex } /** - * The tags of this page's own embedded children that ran during its onGet()/template - * render but were never substituted into the template as a hole (issue #226) - read - * from whatever EmbedOnceInterceptor registered for $ro, regardless of whether the - * child is still reachable through $ro->body by now (onGet() may have unset() it, or - * overwritten the slot with a plain value pulled out of it). + * This page's own embedded children that executed (any generation - see + * `OnceRequest::generations()`) but were never substituted into the template as a hole + * (issue #226) - read once from whatever `EmbedOnceInterceptor` registered for $ro, + * regardless of whether a child is still reachable through $ro->body by now (onGet() + * may have unset() it, or overwritten the slot with a plain value pulled out of it). * - * Never runs a child to find this out: hasExecuted() answers from the wrapper's own - * memo, set only if something already read it. + * A child re-executed mid-`onGet()` (a `withQuery()`/`addQuery()`/`link*()` call) + * answers more than one generation here: every one of them ran and none but possibly + * the last could ever have become a hole, so every one but a rendered last needs its own + * tag - purging an EARLIER generation's tag must invalidate the page precisely as much + * as purging the latest's. * - * @return list + * Never runs a child to find this out: `OnceRequest::generations()` answers purely from + * each wrapper's own memo, populated only by whatever already read it. + * + * `take()` is destructive (the registry answers empty on a second call for the same + * $ro), so this reads it exactly once and the caller must reuse the result for every + * purpose - both the live response's `SurrogateKeys` and the template's own tags. + * + * @return list */ - private function executedChildTags(ResourceObject $ro): array + private function unrenderedChildren(ResourceObject $ro): array { - $tags = []; + $children = []; foreach ($this->childExecutions->take($ro) as $once) { - if (! $once->hasExecuted() || $once->wasRendered()) { - continue; + foreach ($once->generations() as $generation) { + if ($generation['rendered']) { + continue; + } + + $children[] = $generation['resourceObject']; } + } - $childRo = $once->executedResourceObject(); + return $children; + } + + /** + * Flatten a list of children into their own URI tags and declared Surrogate-Keys + * + * @param list $children + * + * @return list + */ + private function childTags(array $children): array + { + $tags = []; + foreach ($children as $childRo) { $tags[] = ($this->uriTag)($childRo->uri); if (array_key_exists(Header::SURROGATE_KEY, $childRo->headers)) { $tags = array_merge($tags, explode(' ', $childRo->headers[Header::SURROGATE_KEY])); diff --git a/src/OnceRequest.php b/src/OnceRequest.php index 36979d56..dac16f02 100644 --- a/src/OnceRequest.php +++ b/src/OnceRequest.php @@ -8,10 +8,14 @@ use BEAR\Resource\Request; use BEAR\Resource\RequestInterface; use BEAR\Resource\ResourceObject; +use OutOfRangeException; use Override; +use function count; +use function in_array; + /** - * Runs the wrapped embedded `Request` at most once + * Runs the wrapped embedded `Request` at most once per query/links generation * * `EmbedInterceptor` places a fresh `Request` clone into `$ro->body[$rel]` for every * `#[Embed]`. Left as-is, that `Request` can be executed twice for one page render: once @@ -34,7 +38,12 @@ * * `withQuery()`/`addQuery()`/`link*()` discard the memo: a child whose query or links the * page explicitly changes mid-`onGet()` is expected to run again under the new request, not - * silently answer the previous one. + * silently answer the previous one. Each such re-execution is a new *generation* - and + * `$executions` keeps every one of them, not only the current one: `onGet()` reading a + * child, re-querying it, and reading it again (never leaving either result as a hole) must + * tag both generations' own URIs/surrogate keys for `DonutRepository` (issue #226), not just + * the last - purging the FIRST generation's tag must invalidate the page just as much as + * purging the second's. * * `AbstractRequest` itself never gets this treatment and is never asked to: a plain * `$resource->get()`/`newRequest()` call in application code, or a `#[Cacheable]` resource @@ -43,11 +52,17 @@ * @psalm-suppress PropertyNotSetInConstructor `$invoker`/`$linker` are AbstractRequest's own * construction-only properties, used only by its default `__invoke()` - overridden here * to always delegate to the inner `Request` instead, so neither is ever read. + * @psalm-type Execution = array{resourceObject: ResourceObject, rendered: bool} */ final class OnceRequest extends AbstractRequest { private ResourceObject|null $executed = null; - private bool $rendered = false; + + /** @var list every generation's result, oldest first */ + private array $generations = []; + + /** @var array parallel to $generations - whether DonutRequest rendered that one as a hole */ + private array $rendered = []; public function __construct(private readonly Request $request) { @@ -62,10 +77,10 @@ public function __construct(private readonly Request $request) /** * {@inheritDoc} * - * The single execution point: runs the inner `Request` the first time this is called - * (directly, or through `AbstractRequest`'s own memo via `__toString()`/`offsetGet()`/ - * etc.) and returns that same result on every later call, until a query or link change - * discards it. + * The single execution point for the current generation: runs the inner `Request` the + * first time this is called (directly, or through `AbstractRequest`'s own memo via + * `__toString()`/`offsetGet()`/etc.) and returns that same result on every later call, + * until a query or link change starts a new generation. */ #[Override] public function __invoke(array|null $query = null): ResourceObject @@ -77,11 +92,38 @@ public function __invoke(array|null $query = null): ResourceObject if ($this->executed === null) { $this->executed = ($this->request)(); + $this->generations[] = $this->executed; + $this->rendered[] = false; } return $this->executed; } + /** + * {@inheritDoc} + * + * Mirrors `Request::__get()`'s own DSL exactly - `AbstractRequest`'s default `__get()` + * answers through its own private memo, not this class's `$executed`, which would read + * as unset (and so re-execute) even after a prior `offsetGet()`/`__toString()` call + * already ran this generation through `__invoke()` above. + */ + #[Override] + public function __get(string $name): mixed + { + if ($name === 'eager' || $name === 'lazy') { + $this->in = $name; + $this->request->in = $name; + + return $this; + } + + if (in_array($name, ['code', 'headers', 'body'], true)) { + return ($this)()->{$name}; + } + + throw new OutOfRangeException($name); + } + /** {@inheritDoc} */ #[Override] public function withQuery(array $query): RequestInterface @@ -146,51 +188,62 @@ public function linkCrawl(string $linkKey): RequestInterface return $this; } - /** Whether the inner `Request` has run, under the query/links currently set */ + /** Whether the current generation's inner `Request` has run yet */ public function hasExecuted(): bool { return $this->executed !== null; } - /** Whether `DonutRequest` has rendered this execution as a hole (see `markRendered()`) */ - public function wasRendered(): bool - { - return $this->rendered; - } - /** - * Record that this execution was substituted into the donut template as a hole + * Record that the current generation's execution was substituted into the donut + * template as a hole + * + * Called by `DonutRequest::__toString()` once it has rendered this generation - so that + * `DonutRepository` (issue #226) does not also tag it as a body-only, non-hole read: the + * hole already carries its own tag (`DonutRequest` adds it directly), and a rendered + * hole's tag is deliberately kept out of the donut template's own storage tags + * (`DonutRepository::templateKeys()`) so that the child's own write drops the rendered + * content and leaves the shell to be recomposed, not the whole template. Only ever the + * LATEST generation can be a hole - the template at render time holds whatever is + * currently in `$ro->body[$rel]`, which a superseded earlier generation no longer is. * - * Called by `DonutRequest::__toString()` once it has rendered this child - so that - * `DonutRepository` (issue #226) does not also tag it as a body-only, non-hole read: - * the hole already carries its own tag (`DonutRequest` adds it directly), and a - * rendered hole's tag is deliberately kept out of the donut template's own storage - * tags (`DonutRepository::templateKeys()`) so that the child's own write drops the - * rendered content and leaves the shell to be recomposed, not the whole template. + * `DonutRequest` only ever calls this right after invoking this request, so there is + * always at least one generation to mark by the time this runs. */ public function markRendered(): void { - $this->rendered = true; + $this->rendered[count($this->rendered) - 1] = true; } /** - * Return the single execution's result, running it now if nothing has read it yet + * Every generation this request has run across every withQuery()/addQuery()/link*() + * call, oldest first, paired with whether `DonutRequest` rendered it as a hole + * + * Read once by `DonutRepository` (issue #226) to tag every generation that executed but + * was never rendered as a hole - not only the current one: `onGet()` re-querying an + * embed mid-request (see class docblock) leaves an earlier generation's own URI/ + * surrogate key just as reachable through `purge()` as the latest's. * - * Never executes a second time: used by `DonutRepository` (issue #226) to read the - * tags of a child that already ran, not to decide whether it should run. + * @return list */ - public function executedResourceObject(): ResourceObject + public function generations(): array { - return ($this)(); + $result = []; + foreach ($this->generations as $i => $resourceObject) { + $result[] = ['resourceObject' => $resourceObject, 'rendered' => $this->rendered[$i]]; + } + + return $result; } - /** Discard the memo and mirror the inner `Request`'s query/links after they changed */ + /** Discard the current generation's memo and mirror the inner `Request`'s query/links after they changed */ private function resync(): void { $this->query = $this->request->query; $this->links = $this->request->links; $this->executed = null; - $this->rendered = false; + // $this->generations/$this->rendered are NOT cleared here - issue #226 needs every + // generation's own tag, not only the one still current after this change. // AbstractRequest's own private invoke() short-circuits on $result !== null, // bypassing our overridden __invoke() entirely for every accessor that goes // through it (offsetGet(), __get(), getIterator(), __toString(), jsonSerialize()) - diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildCacheableResponse.php b/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildCacheableResponse.php new file mode 100644 index 00000000..7b11964a --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/BodyOnlyChildCacheableResponse.php @@ -0,0 +1,27 @@ +body['description'] = $this->body['category']['comment']; + unset($this->body['category']); + + return $this; + } +} diff --git a/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildCacheableResponse.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildCacheableResponse.html.twig new file mode 100644 index 00000000..8342b6ba --- /dev/null +++ b/tests/Fake/fake-app/var/templates/Page/Html/BodyOnlyChildCacheableResponse.html.twig @@ -0,0 +1 @@ +desc:{{ description }} \ No newline at end of file diff --git a/tests/OnceRequestBehaviorTest.php b/tests/OnceRequestBehaviorTest.php index 70ab9170..52efd27e 100644 --- a/tests/OnceRequestBehaviorTest.php +++ b/tests/OnceRequestBehaviorTest.php @@ -39,6 +39,7 @@ class OnceRequestBehaviorTest extends TestCase private const CHILD_SURROGATE_KEY = 'comment01'; private const COUNTER_URI_TAG = '_app_counter_'; private const BLOG_POSTING_DONUT = 'page://self/html/blog-posting-donut'; + private const BODY_ONLY_CHILD_CACHEABLE_RESPONSE = 'page://self/html/body-only-child-cacheable-response'; private ResourceInterface $resource; private QueryRepositoryInterface $queryRepository; @@ -81,6 +82,32 @@ public function testWithQueryAfterAReadReExecutesThroughOffsetGet(): void $this->assertStringContainsString('secondTag:requeried', $view, 'the second read reflects the new query, not a stale memo of the first'); } + public function testEveryGenerationTagsTheStoredTemplateAndPurgingTheFirstOneInvalidatesThePage(): void + { + $this->resource->get(self::REQUERY_DONUT); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::REQUERY_DONUT); + $warm = $this->flushAndValidate($this->logger); + $warmClose = self::scopeCloseOf($warm, 'get', self::REQUERY_DONUT); + $this->assertNotNull($warmClose); + $this->assertSame('cache_hit', $warmClose[0], 'the stored template is warm before the purge'); + + // Only the FIRST generation's tag (the pre-withQuery() counter, no query) - never + // the second's (tag=requeried). Issue #226 review: OnceRequest discarded its own + // execution history on resync(), so only the LAST generation's tag ever reached + // the stored template - purging an earlier generation's tag left the page serving + // a stale cache_hit. + $this->queryRepository->purge(new Uri(self::COUNTER)); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::REQUERY_DONUT); + $afterPurge = $this->flushAndValidate($this->logger); + $closeAfterPurge = self::scopeCloseOf($afterPurge, 'get', self::REQUERY_DONUT); + $this->assertNotNull($closeAfterPurge); + $this->assertSame('cache_miss', $closeAfterPurge[0], "purging the FIRST generation's tag alone must invalidate the page - every generation is tagged, not only the last"); + } + public function testExplicitResourceCallAlwaysExecutes(): void { $this->resource->get(self::COUNTER); @@ -105,6 +132,33 @@ public function testCacheableResourceOutsideDonutPageIsUnaffected(): void $this->assertSame('cache_hit', $hitClose[0], '#[Cacheable] outside any donut page caches exactly as before - untouched by EmbedOnceInterceptor'); } + /** + * The doPutStatic()/#[CacheableResponse] counterpart of + * testPurgingTheBodyOnlyChildInvalidatesThePage(): that test proves the #[DonutCache] + * TEMPLATE is tagged; this one proves the live response's own stored VIEW/ETag - saved + * through a separate SurrogateKeys instance on this path (issue #226 review) - is too. + */ + public function testPurgingTheBodyOnlyChildInvalidatesTheCacheableResponseViewToo(): void + { + $this->resource->get(self::BODY_ONLY_CHILD_CACHEABLE_RESPONSE); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::BODY_ONLY_CHILD_CACHEABLE_RESPONSE); + $warm = $this->flushAndValidate($this->logger); + $warmClose = self::scopeCloseOf($warm, 'get', self::BODY_ONLY_CHILD_CACHEABLE_RESPONSE); + $this->assertNotNull($warmClose); + $this->assertSame('cache_hit', $warmClose[0], 'the stored view/ETag is warm before the purge'); + + $this->queryRepository->purge(new Uri(self::CHILD)); + $this->flushAndValidate($this->logger); + + $this->resource->get(self::BODY_ONLY_CHILD_CACHEABLE_RESPONSE); + $tree = $this->flushAndValidate($this->logger); + $close = self::scopeCloseOf($tree, 'get', self::BODY_ONLY_CHILD_CACHEABLE_RESPONSE); + $this->assertNotNull($close); + $this->assertSame('cache_miss', $close[0], "purging the body-only child's tag must invalidate the live response's own stored view/ETag too, not only a #[DonutCache] template"); + } + public function testBodyOnlyChildTagsTheStoredTemplateAndNeverTouchedEmbedDoesNot(): void { $this->resource->get(self::BODY_ONLY_CHILD_DONUT); @@ -119,6 +173,21 @@ public function testBodyOnlyChildTagsTheStoredTemplateAndNeverTouchedEmbedDoesNo $this->assertStringNotContainsString('"' . self::COUNTER_URI_TAG . '"', $saveDonut[0], 'the never-touched embed contributes no tag'); } + /** + * The #[DonutCache] path's own live-response counterpart: no server-side ETag/view is + * ever saved for this path (doPutDonut() never calls saveEtag()/saveView()), but the + * CDN-facing Surrogate-Key header on the response itself (issue #226 review) must still + * carry the body-only child's tag, or a CDN-level purge by that tag cannot reach it. + */ + public function testDonutCachePathAlsoPutsTheBodyOnlyChildTagOnTheLiveSurrogateKeyHeader(): void + { + $ro = $this->resource->get(self::BODY_ONLY_CHILD_DONUT); + $this->flushAndValidate($this->logger); + + $surrogateKey = $ro->headers[Header::SURROGATE_KEY] ?? ''; + $this->assertStringContainsString(self::CHILD_URI_TAG, $surrogateKey, "the live #[DonutCache] response's own Surrogate-Key header must carry the body-only child's tag too"); + } + public function testPurgingTheBodyOnlyChildInvalidatesThePage(): void { $this->resource->get(self::BODY_ONLY_CHILD_DONUT); diff --git a/tests/OnceRequestUnitTest.php b/tests/OnceRequestUnitTest.php index 89579da1..fc565793 100644 --- a/tests/OnceRequestUnitTest.php +++ b/tests/OnceRequestUnitTest.php @@ -8,6 +8,7 @@ use BEAR\Resource\Request; use BEAR\Resource\ResourceInterface; use FakeVendor\HelloWorld\Resource\App\Counter; +use OutOfRangeException; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; @@ -109,4 +110,68 @@ public function testLinkCrawlDelegatesAndDiscardsTheMemo(): void $this->assertFalse($once->hasExecuted()); $this->assertNotSame([], $once->links); } + + public function testGetCodeReadsFromTheMemoizedExecutionWithoutReExecuting(): void + { + $once = new OnceRequest($this->newCounterRequest()); + + $code = $once->code; + $this->assertSame(1, Counter::$calls, '__get() must run through this class\'s own memo, not AbstractRequest\'s separate one'); + + $this->assertSame($code, $once->code); + $this->assertSame(1, Counter::$calls, 'a second __get() for the same generation must not execute again'); + } + + public function testGetBodyMatchesTheSameExecutionCodeCameFrom(): void + { + $once = new OnceRequest($this->newCounterRequest()); + + $body = $once->body; + assert(is_array($body)); + $this->assertSame(1, $body['call'], 'the body must come from the one execution __get(\'code\') above would also have read'); + } + + public function testGetHeadersAlsoReadsFromTheSameMemoizedExecution(): void + { + $once = new OnceRequest($this->newCounterRequest()); + + $executed = $once(); + $this->assertSame(1, Counter::$calls); + + $this->assertSame($executed->headers, $once->headers, '__get(\'headers\') must answer the same execution __invoke() already memoized, not a fresh one'); + $this->assertSame(1, Counter::$calls, 'reading headers after an already-memoized execution must not run a second one'); + } + + public function testGetEagerSetsInOnBothWrapperAndInnerRequestAndReturnsItself(): void + { + $inner = $this->newCounterRequest(); + $once = new OnceRequest($inner); + + $result = $once->eager; + + $this->assertSame($once, $result, 'the eager/lazy DSL returns $this for chaining, same as Request::__get()'); + $this->assertSame('eager', $once->in); + $this->assertSame('eager', $inner->in, 'must forward to the inner request too, not only set the wrapper\'s own $in'); + } + + public function testGetLazySetsInOnBothWrapperAndInnerRequest(): void + { + $inner = $this->newCounterRequest(); + $once = new OnceRequest($inner); + + $flippedToEager = $once->eager; // flip away from the default 'lazy' first, so this assertion is not a no-op + $flippedToLazy = $flippedToEager->lazy; + + $this->assertSame($once, $flippedToLazy, 'the DSL keeps returning $this across the chain'); + $this->assertSame('lazy', $once->in); + $this->assertSame('lazy', $inner->in); + } + + public function testGetUnknownPropertyThrowsOutOfRangeException(): void + { + $once = new OnceRequest($this->newCounterRequest()); + + $this->expectException(OutOfRangeException::class); + $once->notAKnownProperty; // @phpstan-ignore-line + } } From d663b90fbeb277c8b64c9204b1eac5535f09b12d Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 13:03:31 +0900 Subject: [PATCH 5/6] Raise bear/resource floor to ^1.22.4 and clarify CHANGELOG wording Fable validated ^1.22.4 as the effective minimum that works on the production path (including hole rendering) on PHP 8.2; the old ^1.16.1 floor is replaced. CHANGELOG wording: 'reused until its query or links change' rather than 'at most once per request' (the memo IS discarded and the child DOES run again after a mid-onGet() withQuery()/addQuery()/link*() call - see the OnceRequest generation-tracking commit); and toned the async wording down to what the fix actually does (the evaluator no longer crashes on an AsyncRequest-shaped child; async support itself stays out of scope). --- CHANGELOG.md | 5 +++-- composer.json | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efd76b3e..7d3c7a73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,13 +18,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Upgrading changes the Ray.Aop-woven class name - and so the ETag (`crc32($ro::class . ...)`) - for a `#[CacheableResponse]` class or method that also declares an `#[Embed]`, because it is now woven with a new interceptor (#226); a page with no `#[Embed]` keeps its stock woven class and ETag, and a `#[DonutCache]` page never carries an ETag at all, so neither is affected. - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. `ResourceDonut::refresh()` takes an additional `UncacheableMarkInterface` argument. - Requires `koriym/semantic-logger` `^0.11`. Written logs use its `log` document shape, and diagnostic `kind` is now `code`. +- Requires `bear/resource` `^1.22.4`. - `KeepMutationsAndFailures` also keeps a session cut at the record limit (`semantic_logger_truncated`): a `command` past the limit leaves no other trace. ### Fixed - `LogFileWriter`: two sessions flushed in the same microsecond no longer overwrite each other. The timestamped filename now carries a random suffix, so both survive (#221). - A `#[DonutCache]` page embedding an `#[Embed]` hole whose URI carried two or more query parameters leaked a raw `[le:...]` placeholder to the client and could throw `ParameterException` on the next request - fixed by moving new placeholders to a `[le2:...]` marker (#232). -- An embedded child of a `#[DonutCache]`/`#[CacheableResponse]` page now runs at most once per request: it previously ran again when rendered as a hole after already being read in `onGet()`. A child read into the body but never rendered as a hole now tags the stored template, so purging that child's own tag refreshes the page instead of leaving it stale (#226). -- `ResourceBodyEvaluator` no longer crashes materializing an embedded `AbstractRequest` whose `jsonSerialize()` does not answer the memoized `ResourceObject` (`BEAR\Async\AsyncRequest`'s own shape): such a request is now left unmaterialized instead of asserting. Full async support remains out of scope. +- An embedded child of a `#[DonutCache]`/`#[CacheableResponse]` page is now reused until its query or links change, instead of running again when rendered as a hole after already being read in `onGet()`. Every such execution that was read but never rendered as a hole now tags the stored template and the live response's own Surrogate-Key/ETag, so purging any one of them refreshes the page instead of leaving it stale (#226). +- `ResourceBodyEvaluator` no longer crashes materializing an embedded child whose `jsonSerialize()` does not answer the memoized `ResourceObject` (the shape `BEAR\Async\AsyncRequest` overrides it to): such a child is now left unmaterialized instead of asserting. Async support itself remains out of scope. ## [1.17.0] - 2026-09-17 diff --git a/composer.json b/composer.json index 85fdd57b..4b03019a 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,7 @@ ], "require": { "php": "^8.2", - "bear/resource": "^1.16.1", + "bear/resource": "^1.22.4", "bear/sunday": "^1.5", "koriym/semantic-logger": "^0.11", "psr/cache": "^1.0 || ^2.0 || ^3.0", From 14f8c9406b05c549047200927e66714d31aca077 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Sun, 25 Oct 2026 13:07:12 +0900 Subject: [PATCH 6/6] Fix docblock accuracy and prose after deeper mutation testing Mutation-tested __get() by removing the whole override: code/headers/body still passed, since AbstractRequest's own default __get() already funnels through this class's overridden __invoke() for any unknown property - only eager/lazy and the OutOfRangeException are genuinely new behavior. Corrected the docblock to say so, rather than implying the whole override fixes a bug. Also fixed a run-on sentence in DonutChildExecutionsInterface's own docblock. --- src/DonutChildExecutionsInterface.php | 4 ++-- src/OnceRequest.php | 11 +++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/DonutChildExecutionsInterface.php b/src/DonutChildExecutionsInterface.php index eb624aba..187bee05 100644 --- a/src/DonutChildExecutionsInterface.php +++ b/src/DonutChildExecutionsInterface.php @@ -14,8 +14,8 @@ * even if `onGet()` later `unset()`s it from `$ro->body` (or overwrites the slot with a * plain value pulled out of it) and `ResourceDonut::create()`'s body walk can no longer find * it. `DonutRepository` reads the list back once the page's donut template has been built - - * late enough that `OnceRequest::generations()` is final for every generation of every - * wrapper - to tag the template with whichever generations executed but were never rendered. + * late enough that every wrapper's `OnceRequest::generations()` is final - to tag the + * template with whichever generations executed but were never rendered as a hole. * * Bound `Scope::SINGLETON`, one instance per DI container (per request in the usual * BEAR.Sunday bootstrap) - the same lifetime `UncacheableMark` already relies on. Keyed by diff --git a/src/OnceRequest.php b/src/OnceRequest.php index dac16f02..a3137948 100644 --- a/src/OnceRequest.php +++ b/src/OnceRequest.php @@ -102,10 +102,13 @@ public function __invoke(array|null $query = null): ResourceObject /** * {@inheritDoc} * - * Mirrors `Request::__get()`'s own DSL exactly - `AbstractRequest`'s default `__get()` - * answers through its own private memo, not this class's `$executed`, which would read - * as unset (and so re-execute) even after a prior `offsetGet()`/`__toString()` call - * already ran this generation through `__invoke()` above. + * Mirrors `Request::__get()`'s own DSL exactly. `eager`/`lazy` is genuinely new here - + * `AbstractRequest`'s default `__get()` has no such case and would instead try to read + * a non-existent `eager`/`lazy` property off the memoized `ResourceObject`. The `code`/ + * `headers`/`body` branch is a deliberate, explicit match for `Request`'s own contract + * rather than a correctness fix: `AbstractRequest`'s default already funnels every + * unknown property through its own private memo's `invoke()`, which calls this class's + * overridden `__invoke()` just the same, so it already answers from the one execution. */ #[Override] public function __get(string $name): mixed