From c787aaa705fd4db9443a64dea83da2e1a8ced2b7 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 02:46:38 +0900 Subject: [PATCH 01/11] Remove the unreleased mark/dev-gate machinery, restore 1.17.0 constructor shapes, keep the runtime-private Cache-Control guard and every other 1.x fix Deletes UncacheableMark(Interface), UnstorableGate(Interface), ThrowingUnstorableGate, DevUnstorableGateModule and Exception\UnstorableResponseDetected along with every mark/ isMarked/enterHole/leaveHole path. DonutRepository drops DonutHeaderSetter and restores HeaderSetter + CdnCacheControlHeaderSetterInterface at their 1.17.0 constructor positions; QueryRepository, DonutRequest and ResourceDonut::refresh() restore their 1.17.0 signatures. DeclaredCacheControl gains reason(), ported from UnstorableGate minus the mark check: a response whose own code sets Cache-Control to private/no-store at runtime is still refused the server-side store, keeping the developer's header and stripping CDN headers; a value #[HttpCache]/#[NoHttpCache] declares is exempt. Removes the Header::PRAGMA constant, only ever used by the removed mark-based header override. Test/fixture cleanup: deletes every fixture and test whose only purpose was the mark system's cross-request/hole-scope semantics (no equivalent once mark is gone); adapts the rest to set Cache-Control directly instead of calling mark(); adds DeclaredCacheControlTest for reason(); strengthens the five declared-vs-runtime Cache-Control combinations (runtime private/no-store -> not stored, #[HttpCache(isPrivate)]/#[HttpCache(noStore)]/#[NoHttpCache] -> still stored) with call-counter proof of actual server-side storage, not just header values. --- CHANGELOG.md | 13 +- README.md | 4 +- src/DeclaredCacheControl.php | 39 ++- src/DevUnstorableGateModule.php | 43 --- src/DonutCacheModule.php | 2 - src/DonutHeaderSetter.php | 47 --- src/DonutRepository.php | 87 +++--- src/DonutRequest.php | 24 +- src/Exception/UnstorableResponseDetected.php | 25 -- src/Header.php | 1 - src/HeaderSetter.php | 27 +- src/Log/Context/ManualStoreResultContext.php | 2 +- src/QueryRepository.php | 11 +- src/QueryRepositoryModule.php | 5 - src/ResourceDonut.php | 39 +-- src/ThrowingUnstorableGate.php | 52 ---- src/UncacheableMark.php | 88 ------ src/UncacheableMarkInterface.php | 70 ----- src/UnstorableGate.php | 69 ----- src/UnstorableGateInterface.php | 48 --- tests/CdnCacheControlHeaderTest.php | 20 +- tests/DeclaredCacheControlTest.php | 80 +++++ tests/DevUnstorableGateModuleTest.php | 55 ---- tests/DonutCacheTest.php | 8 +- tests/DonutMaterialDependencyTest.php | 7 +- tests/DonutQueryInterceptorPurgeTest.php | 48 --- tests/DonutRepositoryTest.php | 225 ++------------ tests/DonutRequestTest.php | 111 +------ tests/Fake/fake-app/src/RenderTimeToken.php | 27 -- .../App/HandSetPrivateCacheControl.php | 17 +- .../App/HttpCacheControlWithCacheable.php | 4 + .../src/Resource/App/QueryDonutHole.php | 34 --- .../fake-app/src/Resource/App/QueryHole.php | 18 +- .../src/Resource/App/ThrowingHole.php | 15 +- .../src/Resource/App/UnstorableValue.php | 14 +- .../Page/Html/BlogPostingCacheControl.php | 3 + ...acheWithMarkingAndNestedDonutCacheHole.php | 28 -- .../DonutCacheWithNestedDonutCacheHole.php | 27 -- .../Html/DonutCacheWithUnstorableHole.php | 25 -- .../src/Resource/Page/Html/MutableComment.php | 12 - .../src/Resource/Page/Html/RenderTimeMark.php | 34 --- .../Resource/Page/Html/UnstorableDonut.php | 14 +- .../Html/UnstorableDonutCacheTemplate.php | 15 +- .../Html/UnstorableWithManualCdnHeader.php | 13 +- .../templates/App/QueryDonutHole.html.twig | 1 - ...thMarkingAndNestedDonutCacheHole.html.twig | 1 - ...nutCacheWithNestedDonutCacheHole.html.twig | 1 - .../DonutCacheWithUnstorableHole.html.twig | 1 - .../Page/Html/RenderTimeMark.html.twig | 1 - tests/GetInterceptorTest.php | 7 + tests/HandSetPrivateCacheControlTest.php | 26 +- tests/HeaderSetterTest.php | 22 +- tests/QueryRepositoryTest.php | 24 +- tests/ResourceDonutTest.php | 109 ++----- tests/ResourceRepositoryTest.php | 8 +- tests/ResourceStorageTest.php | 6 +- tests/UnstorableGateTest.php | 274 ------------------ 57 files changed, 333 insertions(+), 1698 deletions(-) delete mode 100644 src/DevUnstorableGateModule.php delete mode 100644 src/DonutHeaderSetter.php delete mode 100644 src/Exception/UnstorableResponseDetected.php delete mode 100644 src/ThrowingUnstorableGate.php delete mode 100644 src/UncacheableMark.php delete mode 100644 src/UncacheableMarkInterface.php delete mode 100644 src/UnstorableGate.php delete mode 100644 src/UnstorableGateInterface.php create mode 100644 tests/DeclaredCacheControlTest.php delete mode 100644 tests/DevUnstorableGateModuleTest.php delete mode 100644 tests/Fake/fake-app/src/RenderTimeToken.php delete mode 100644 tests/Fake/fake-app/src/Resource/App/QueryDonutHole.php delete mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php delete mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithNestedDonutCacheHole.php delete mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithUnstorableHole.php delete mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/RenderTimeMark.php delete mode 100644 tests/Fake/fake-app/var/templates/App/QueryDonutHole.html.twig delete mode 100644 tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig delete mode 100644 tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithNestedDonutCacheHole.html.twig delete mode 100644 tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithUnstorableHole.html.twig delete mode 100644 tests/Fake/fake-app/var/templates/Page/Html/RenderTimeMark.html.twig delete mode 100644 tests/UnstorableGateTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e3eea71..a6e0cf37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,25 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added -- `UncacheableMarkInterface::mark($reason)`: code that produces a per-user value (a CSRF token, etc.) marks the current response unstorable; it is then not cached (`put_skipped{reason: unstorable}`) and is sent with `Cache-Control: private, no-store` and `Pragma: no-cache` and no CDN headers (#222). A mark also skips every `#[Cacheable]`/`#[CacheableResponse]` write that happens later in the same request, including an unrelated sibling's (#234). -- `UncacheableMark` is request-scoped: a long-running worker (Swoole, RoadRunner) that reuses one container across requests must rebind it per request, as it already must for `ServerContextInterface`. -- `DevUnstorableGateModule`: `$module->override(new DevUnstorableGateModule())` turns such a skip into a thrown `Exception\UnstorableResponseDetected` during development (`install()` has no effect here). - ### 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. -- `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. `ResourceDonut::refresh()` takes an additional `UncacheableMarkInterface` argument. +- A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response whose own code sets `Cache-Control` to `private` or `no-store` is not stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. This check reads only the response's own header, not a descendant's - put a per-user value in a non-cacheable `#[DonutCache]` hole instead. +- `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - 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. -- `ResourceDonut::refresh()`: a hole that throws while an already-cached `#[DonutCache]` page is recomposed (its own `onGet()` or its render) no longer lets that exception reach the caller directly from the refresh. It now degrades the same way a cache-store failure does and falls back to rendering the whole page fresh. A transient failure (the hole recovers on that fresh render) now answers 200 with the full body instead of an error - for example a corrupted legacy placeholder's `ParameterException`, previously an uncaught 500 straight from the refresh, now answers 200 on both this request and the next, since the fresh render re-stores the template with a correct `[le2:...]` placeholder. A hole that keeps throwing still fails the request - from the fresh render instead of from the refresh - exactly as the page would without the cache attribute at all. +- `ResourceDonut::refresh()`: a hole that throws while an already-cached `#[DonutCache]` page is recomposed now degrades to a fresh render instead of failing the request directly from the refresh, the same way a cache-store failure already does. A hole that keeps throwing still fails the request, now from the fresh render instead of the refresh. - `CacheDependency::depends()` no longer unsets the child's own `Surrogate-Key` header after copying it into the parent: a child that is both read by the parent and later rendered as a hole keeps its own key for its own independent cache entry. ### 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 exception while rendering a `#[DonutCache]`/`#[CacheableResponse]` page - a hole's own resource or the page's own template - no longer stores an empty template that was served to every later visitor (since 1.17.0). The page's own template failure now surfaces exactly as it would for the same page without the cache attribute, instead of being swallowed into an empty page; a hole's failure now surfaces as a clean, catchable exception too - actually a better outcome than an `#[Embed]` of the same resource gets outside a `#[DonutCache]` page, where BEAR.Resource's own `AbstractRequest::__toString()` swallows it into a fatal `E_USER_ERROR` instead. +- An exception while rendering a `#[DonutCache]`/`#[CacheableResponse]` page - a hole's own resource or the page's own template - no longer stores an empty template that is served to every later visitor; the failure now surfaces to the caller instead of being swallowed into an empty page. - A degraded cache read (a down cache pool, or - see above - a hole failing while a cached page is refreshed) now actually renders its fresh-render fallback instead of returning the un-rendered result of running `onGet()` alone: previously such a request answered 200 with an unresolved `#[Embed]` hole still in its body and no view, rather than the composed page. - An embedded child of a `#[DonutCache]`/`#[CacheableResponse]` page that `onGet()` already read into the body is no longer re-executed when the page renders it as a hole; both now come from the same single execution (#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. diff --git a/README.md b/README.md index 0dc05432..17af8e89 100644 --- a/README.md +++ b/README.md @@ -17,12 +17,14 @@ - **CDN Integration**: Seamlessly integrates with modern CDNs (e.g., Fastly, Akamai) for shared cache management. - **Conditional Requests with ETag Support**: Reduces network overhead by leveraging `ETag` and `304 Not Modified` responses. - **Distributed Cache Support**: Works with server-side caches (e.g., Redis, APC), shared caches (e.g., CDNs), and client-side caches. -- **Unstorable-Response Guard**: A producer of a per-user value (a CSRF token, etc.) can mark the current response unstorable via `UncacheableMarkInterface`, so it is never written to the shared cache ([#222](https://github.com/bearsunday/BEAR.QueryRepository/issues/222)); the mark is per-request state and must be rebound each request under a long-running worker (Swoole, RoadRunner). This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. `CacheVersionModule` only namespaces pools built through this package's own `Storage*Module`s or a `#[CacheNamespace]`-aware adapter; an app binding its own cache adapter directly must clear those pools instead. +- **Runtime-Private Response Guard**: A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` resource whose own code sets `Cache-Control` to `private` or `no-store` at runtime is not written to the shared cache; its own header is kept exactly and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is exempt - that is the attribute speaking, not a per-request value. This guard reads only the resource's own header, never a descendant's: a per-user value an `#[Embed]`'d child returns is not detected this way, so put it in a non-cacheable `#[DonutCache]` hole instead - the hole's own output is never baked into the stored template and is re-rendered on every hit regardless. This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. `CacheVersionModule` only namespaces pools built through this package's own `Storage*Module`s or a `#[CacheNamespace]`-aware adapter; an app binding its own cache adapter directly must clear those pools instead. ## Cache dependencies Declare a dependency with `#[Embed]` and never `unset()` or overwrite the slot - a cached `onGet()` must not fetch dependent data with `$resource->get()`. What a child becomes depends on how the page uses it: +A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response is shared with every visitor, so a per-user value belongs in a non-cacheable `#[DonutCache]` hole, not in the cached response itself. + | Need | Use | Effect of a child purge | |---|---|---| | Per-user content (no shared cache) | `#[DonutCache]` hole (non-cacheable child) | that hole alone re-runs on every HIT | diff --git a/src/DeclaredCacheControl.php b/src/DeclaredCacheControl.php index cae949a3..bc61c765 100644 --- a/src/DeclaredCacheControl.php +++ b/src/DeclaredCacheControl.php @@ -9,15 +9,25 @@ use BEAR\Resource\ResourceObject; use ReflectionClass; +use function str_contains; +use function strtolower; + /** - * The Cache-Control value `#[HttpCache]` or `#[NoHttpCache]` declares for a resource + * The Cache-Control value `#[HttpCache]` or `#[NoHttpCache]` declares for a resource, and + * whether a response's own code overrode it to something unstorable * - * Mirrors `HttpCacheInterceptor`'s precedence, so this always matches what was written to - * the header. `UnstorableGateInterface::reason()` exempts a header that still reads exactly - * this value, since it's the attribute speaking, not a per-request value (#222). + * Mirrors `HttpCacheInterceptor`'s precedence, so `__invoke()` always matches what was written + * to the header. This package is a shared cache in the sense of RFC 9111: whatever it stores + * is served to every later visitor, so reason() refuses a write whose Cache-Control reads + * private or no-store (matched case-insensitively, RFC 7230 §3.2/RFC 9111 §5.2) and was not + * set by that same declaration - the attribute's own client-facing directive is exempt, + * since `#[Cacheable]` combined with `#[HttpCache(noStore: true)]` still stores server-side + * while telling the client not to. */ final readonly class DeclaredCacheControl { + private const REASON = 'unstorable'; + public function __invoke(ResourceObject $ro): string|null { $class = new ReflectionClass($ro); @@ -34,4 +44,25 @@ public function __invoke(ResourceObject $ro): string|null return (string) $cacheControl; } + + /** + * The reason this response must not be written to the shared cache, or null when storable + * + * @return 'unstorable'|null + */ + public function reason(ResourceObject $ro): string|null + { + $name = Header::findKey($ro->headers, Header::CACHE_CONTROL); + $cacheControl = $name === null ? '' : $ro->headers[$name]; + if ($cacheControl === '' || $cacheControl === $this($ro)) { + return null; + } + + $lower = strtolower($cacheControl); + if (str_contains($lower, 'no-store') || str_contains($lower, 'private')) { + return self::REASON; + } + + return null; + } } diff --git a/src/DevUnstorableGateModule.php b/src/DevUnstorableGateModule.php deleted file mode 100644 index 7f5604ec..00000000 --- a/src/DevUnstorableGateModule.php +++ /dev/null @@ -1,43 +0,0 @@ -install(new DevUnstorableGateModule())` silently keeps the original, non-throwing - * binding. Use `$module->override(new DevUnstorableGateModule())` instead, the same way a - * test overrides TwigModule's template path. - * - * ThrowingUnstorableGate decorates the concrete UnstorableGate, not the interface, so this - * module binds the concrete class too: once UnstorableGateInterface points elsewhere, - * UnstorableGate stops being auto-wireable via the interface binding alone. - * - * Turns a response marked unstorable (issue #222) into a thrown UnstorableResponseDetected - * instead of the package default of a silent skip - useful alongside other development-time - * tooling such as DevEtagModule. - */ -final class DevUnstorableGateModule extends AbstractModule -{ - /** - * {@inheritDoc} - */ - #[Override] - protected function configure(): void - { - $this->bind(UnstorableGate::class); - $this->bind(UnstorableGateInterface::class)->to(ThrowingUnstorableGate::class); - } -} diff --git a/src/DonutCacheModule.php b/src/DonutCacheModule.php index f26b275f..c70adb19 100644 --- a/src/DonutCacheModule.php +++ b/src/DonutCacheModule.php @@ -20,7 +20,6 @@ * The following bindings are provided: * * HeaderSetter - * DonutHeaderSetter * CdnCacheControlHeaderSetterInterface * DonutRepositoryInterface * RepositoryLoggerInterface @@ -43,7 +42,6 @@ final class DonutCacheModule extends AbstractModule protected function configure(): void { $this->bind(HeaderSetter::class); - $this->bind(DonutHeaderSetter::class); $this->bind(CdnCacheControlHeaderSetterInterface::class)->to(CdnCacheControlHeaderSetter::class); $this->bind(DonutRepositoryInterface::class)->to(DonutRepository::class)->in(Scope::SINGLETON); // Kept bound for BC; internal cache code logs through SemanticLoggerInterface instead, diff --git a/src/DonutHeaderSetter.php b/src/DonutHeaderSetter.php deleted file mode 100644 index bf218d0a..00000000 --- a/src/DonutHeaderSetter.php +++ /dev/null @@ -1,47 +0,0 @@ -headerSetter)($ro, $cacheControlMaxAge, $httpCache, $time); - } - - /** - * Hand the CDN a lifetime for 200 that is also storable - * - * The setters do not read the status code and the Fastly and Akamai defaults are - * 31_536_000 seconds - a year - so an unguarded call would tell every shared cache to - * hold a 301 or a 204 for that long. A response the gate marks unstorable (#222) - * is refused the same way: the gate sits here rather than in the setters so both checks - * also cover an application's own CdnCacheControlHeaderSetterInterface. - */ - public function setCdnCacheControl(ResourceObject $ro, int|null $sMaxAge): void - { - if ($ro->code !== 200 || $this->gate->reason($ro) !== null) { - return; - } - - ($this->cdnCacheControlHeaderSetter)($ro, $sMaxAge); - } -} diff --git a/src/DonutRepository.php b/src/DonutRepository.php index 0eaba7d5..51f3f2ea 100644 --- a/src/DonutRepository.php +++ b/src/DonutRepository.php @@ -41,16 +41,16 @@ */ public function __construct( private QueryRepositoryInterface $queryRepository, - private DonutHeaderSetter $donutHeaderSetter, + private HeaderSetter $headerSetter, private ResourceStorageInterface $resourceStorage, private ResourceInterface $resource, + private CdnCacheControlHeaderSetterInterface $cdnCacheControlHeaderSetter, #[CacheLog] private SemanticLoggerInterface $logger, private DonutRendererInterface $renderer, - private UnstorableGateInterface $gate, - private UncacheableMarkInterface $mark, private UriTagInterface $uriTag = new UriTag(), CacheDependencyInterface|null $cacheDependency = null, + private DeclaredCacheControl $declaredCacheControl = new DeclaredCacheControl(), ) { // Same UriTag as templateKeys(), so a material's tag matches the format purge() emits. $this->cacheDependency = $cacheDependency ?? new CacheDependency($this->uriTag, $this->logger); @@ -149,11 +149,10 @@ private function doPutStatic(ResourceObject $ro, int|null $ttl, int|null $sMaxAg $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); + $donut = ResourceDonut::create($ro, $this->renderer, $keys, $sMaxAge, true)->withStorageState($ttl, $templateKeys); $donut->render($ro, $this->renderer); - // Gate checked against the unmutated response, before setHeaders() mutates it - // (#222): "any mark" anywhere in the content decides storability. - $unstorable = $this->gate->reason($ro); + // Checked against the unmutated response, before setHeaders() mutates it. + $unstorable = $this->declaredCacheControl->reason($ro); $surrogateKeys = $this->setHeaders($keys, $ro, $sMaxAge); $this->logCdnHeaders($ro); $this->logger->event(new PreWriteCleanupContext((string) $ro->uri)); @@ -170,14 +169,6 @@ private function doPutStatic(ResourceObject $ro, int|null $ttl, int|null $sMaxAg $this->resourceStorage->saveDonut($ro->uri, $donut->withContentState($ro), $ttl, $templateKeys); } - /** - * Write the donut, gated against a per-user value baked into the template outside a hole (#222) - * - * The gate is read after create()/render(), since create() runs each hole's own onGet() - * and may itself mark. The template write excludes hole marks - * (reasonExcludingHoleMarks()); the live response does not, since every hole's output - * lands in it too. - */ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void { $this->classifyMaterials($ro); @@ -185,20 +176,19 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void $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); + $donut = ResourceDonut::create($ro, $this->renderer, $keys, $donutTtl, false)->withStorageState($donutTtl, $templateKeys); $donut->render($ro, $this->renderer); - $templateUnstorable = $this->gate->reasonExcludingHoleMarks($ro); - $responseUnstorable = $this->gate->reason($ro); + $unstorable = $this->declaredCacheControl->reason($ro); $keys->setSurrogateHeader($ro); - if ($responseUnstorable !== null) { - ($this->donutHeaderSetter)($ro, null, null); + if ($unstorable !== null) { + ($this->headerSetter)($ro, null, null); } $this->logCdnHeaders($ro); $this->logger->event(new PreWriteCleanupContext((string) $ro->uri)); $this->resourceStorage->invalidateTags([($this->uriTag)($ro->uri)]); - if ($templateUnstorable !== null) { - $this->logger->event(new PutSkippedContext((string) $ro->uri, $templateUnstorable)); + if ($unstorable !== null) { + $this->logger->event(new PutSkippedContext((string) $ro->uri, $unstorable)); return; } @@ -210,7 +200,7 @@ private function doPutDonut(ResourceObject $ro, int|null $donutTtl): void * Register body children the page's onGet() already ran as dependencies, before the template keys read the header * * A child carrying an ETag or Surrogate-Key already ran during onGet() (a material); a hole has not run yet. - * Headers only: running a child here would execute a hole outside its hole scope (#222/#234). + * Headers only: running a child here would execute a hole outside its hole scope. */ private function classifyMaterials(ResourceObject $ro): void { @@ -271,40 +261,23 @@ private function refreshDonut(ResourceObject $ro): ResourceObject|null $this->logger->event(new CacheHitContext('donut')); $this->logger->event(new RefreshDonutContext((string) $ro->uri)); - $donut->refresh($this->resource, $ro, $this->mark); + $donut->refresh($this->resource, $ro); $surrogateKeys = $ro->headers[Header::SURROGATE_KEY] ?? ''; - // refresh() discards any header a hole set on $ro (#222); an unstorable mark - // survives only via the separate per-request mark service, read below instead. - $unstorable = $this->gate->reason($ro); - if (! $donut->isCacheble) { - // Template-only (isCacheble=false): never stored as a page view, so only refresh - // restored headers here (#222) - never invent a new CDN lifetime for this hit. - if ($unstorable !== null) { - ($this->donutHeaderSetter)($ro, null, null); - } - + // Template-only (isCacheble=false): never stored as a page view. $this->logCdnHeaders($ro); - $this->logger->event(new PutSkippedContext((string) $ro->uri, $unstorable ?? 'not-cacheable')); + $this->logger->event(new PutSkippedContext((string) $ro->uri, 'not-cacheable')); return $ro; } // When the recomposed content is byte-identical to the recorded one, carry over // the original Last-Modified instead of advancing it to the recomposition time - $lastModified = $unstorable === null ? $donut->getUnchangedLastModified((string) $ro->view) : null; - // Always run, unstorable or not: strips restored-but-stale "storable" CDN headers from - // an unstorable response and, when a mark fired, also replaces Cache-Control/Pragma (#222). - ($this->donutHeaderSetter)($ro, $donut->ttl, null, $lastModified); - $this->donutHeaderSetter->setCdnCacheControl($ro, $donut->ttl); + $lastModified = $donut->getUnchangedLastModified((string) $ro->view); + ($this->headerSetter)($ro, $donut->ttl, null, $lastModified); + $this->setCdnCacheControl($ro, $donut->ttl); $this->logCdnHeaders($ro); - if ($unstorable !== null) { - $this->logger->event(new PutSkippedContext((string) $ro->uri, $unstorable)); - - return $ro; - } - if ($lastModified === null) { $this->recordContentState($ro, $donut); } @@ -405,12 +378,30 @@ private function setHeaders(SurrogateKeys $keys, ResourceObject $ro, int|null $s { $keys->setSurrogateHeader($ro); $surrogateKeys = $ro->headers[Header::SURROGATE_KEY] ?? ''; - $this->donutHeaderSetter->setCdnCacheControl($ro, $sMaxAge); - ($this->donutHeaderSetter)($ro, 0, null); + $this->setCdnCacheControl($ro, $sMaxAge); + ($this->headerSetter)($ro, 0, null); return $surrogateKeys; } + /** + * Hand the CDN a lifetime for 200 that is also storable + * + * The setters do not read the status code and the Fastly and Akamai defaults are + * 31_536_000 seconds - a year - so an unguarded call would tell every shared cache to + * hold a 301 or a 204 for that long. A response whose Cache-Control already reads + * private/no-store is refused the same way: the check sits here rather than in the + * setters so it also covers an application's own CdnCacheControlHeaderSetterInterface. + */ + private function setCdnCacheControl(ResourceObject $ro, int|null $sMaxAge): void + { + if ($ro->code !== 200 || $this->declaredCacheControl->reason($ro) !== null) { + return; + } + + ($this->cdnCacheControlHeaderSetter)($ro, $sMaxAge); + } + /** @return list */ public function getHeaderKeys(ResourceObject $ro): array { diff --git a/src/DonutRequest.php b/src/DonutRequest.php index d670e39e..332300a2 100644 --- a/src/DonutRequest.php +++ b/src/DonutRequest.php @@ -13,7 +13,7 @@ use function assert; /** - * Runs an embedded hole while a donut template renders; a mark() made then is scoped to the hole (#222) + * Runs an embedded hole while a donut template renders */ final readonly class DonutRequest implements Stringable { @@ -21,27 +21,21 @@ public function __construct( private AbstractRequest $request, private DonutRendererInterface $donutStorage, private SurrogateKeys $etags, - private UncacheableMarkInterface $mark, ) { } #[Override] public function __toString(): string { - // Includes the query, so sibling holes on one path get separate owner slots (#222) + // Includes the query, so sibling holes on one path get distinct cache entries $uri = $this->request->toUri(); - $this->mark->enterHole($uri); - try { - // A Request answers its memoized result, so a child onGet() already read is not run again (#226). - // Other requests are invoked directly: AsyncRequest's jsonSerialize() flushes its batch. - /** @psalm-suppress MixedAssignment $executed - Request::jsonSerialize() answers its memoized ResourceObject */ - $executed = $this->request instanceof Request ? $this->request->jsonSerialize() : ($this->request)(); - assert($executed instanceof ResourceObject); - // Not (string): the cast turns a render failure into '', which would be stored (#237) - $view = $executed->toString(); - } finally { - $this->mark->leaveHole($uri); - } + // A Request answers its memoized result, so a child onGet() already read is not run again (#226). + // Other requests are invoked directly: AsyncRequest's jsonSerialize() flushes its batch. + /** @psalm-suppress MixedAssignment $executed - Request::jsonSerialize() answers its memoized ResourceObject */ + $executed = $this->request instanceof Request ? $this->request->jsonSerialize() : ($this->request)(); + assert($executed instanceof ResourceObject); + // Not (string): the cast turns a render failure into '', which would be stored (#237) + $view = $executed->toString(); $this->etags->addTag($executed); $this->donutStorage->setView($uri, $view); diff --git a/src/Exception/UnstorableResponseDetected.php b/src/Exception/UnstorableResponseDetected.php deleted file mode 100644 index 85d9e28a..00000000 --- a/src/Exception/UnstorableResponseDetected.php +++ /dev/null @@ -1,25 +0,0 @@ -gate->reason($ro) !== null) { + if ($this->declaredCacheControl->reason($ro) !== null) { $this->stripHeaders($ro, self::CDN_FACING_HEADERS); - if ($this->mark->isMarked()) { - $this->stripHeaders($ro, [Header::CACHE_CONTROL, Header::PRAGMA]); - $ro->headers[Header::CACHE_CONTROL] = 'private, no-store'; - $ro->headers[Header::PRAGMA] = 'no-cache'; - } return; } diff --git a/src/Log/Context/ManualStoreResultContext.php b/src/Log/Context/ManualStoreResultContext.php index e54b4e6b..313395a2 100644 --- a/src/Log/Context/ManualStoreResultContext.php +++ b/src/Log/Context/ManualStoreResultContext.php @@ -11,7 +11,7 @@ /** * Close of a manual_store scope: whether the resource was stored. * - * `false`/"failed" also covers a write UnstorableGateInterface refused outright (issue #222): + * `false`/"failed" also covers a write `DeclaredCacheControl::reason()` refused outright: * no save_value/save_view was even attempted, and the nested put_skipped{reason} in the same * scope names which - the same "coarse close, precise nested event" split the package already * uses for a failed save_etag not flipping this result. diff --git a/src/QueryRepository.php b/src/QueryRepository.php index 5ac245cf..1b4ecabe 100644 --- a/src/QueryRepository.php +++ b/src/QueryRepository.php @@ -39,7 +39,7 @@ public function __construct( private ResourceStorageInterface $storage, private Expiry $expiry, private CacheDependencyInterface $cacheDependency, - private UnstorableGateInterface $gate, + private DeclaredCacheControl $declaredCacheControl = new DeclaredCacheControl(), ) { } @@ -92,12 +92,9 @@ private function doPut(ResourceObject $ro): bool $this->logger->event($this->cachePolicy($ro, $cacheable, $ttl)); } - // Checked before headerSetter runs, against the still-unmutated header: only a - // Cache-Control set dynamically (by the method or during rendering above) or a mark - // made during that rendering trips the gate - #[HttpCache]/#[NoHttpCache]'s own - // declared value is exempt (issue #222). headerSetter makes the identical check on - // this same unmutated state to decide what it writes, so the two never disagree. - $unstorable = $this->gate->reason($ro); + // Checked against the still-unmutated header, before headerSetter runs the identical + // check - a dynamically set Cache-Control trips this; #[HttpCache]'s own value is exempt. + $unstorable = $this->declaredCacheControl->reason($ro); ($this->headerSetter)($ro, $ttl, $httpCache); diff --git a/src/QueryRepositoryModule.php b/src/QueryRepositoryModule.php index be89ec54..81c680a3 100644 --- a/src/QueryRepositoryModule.php +++ b/src/QueryRepositoryModule.php @@ -35,8 +35,6 @@ * MatchQueryInterface * UriTag * MatchQueryInterface - * UncacheableMarkInterface - * UnstorableGateInterface * DeclaredCacheControl * * The following module are installed. @@ -81,9 +79,6 @@ protected function configure(): void $this->bind(ResourceStorageSaver::class); // Server context for thread safety (Swoole, RoadRunner, etc.) $this->bind(ServerContextInterface::class)->to(GlobalServerContext::class)->in(Scope::SINGLETON); - // One mark per request (see UncacheableMark); issue #222 - $this->bind(UncacheableMarkInterface::class)->to(UncacheableMark::class)->in(Scope::SINGLETON); - $this->bind(UnstorableGateInterface::class)->to(UnstorableGate::class); $this->bind(DeclaredCacheControl::class); // #[Cacheable] $this->install(new CacheableModule()); diff --git a/src/ResourceDonut.php b/src/ResourceDonut.php index b341030c..b30b0d80 100644 --- a/src/ResourceDonut.php +++ b/src/ResourceDonut.php @@ -75,32 +75,20 @@ public function __construct( ) { } - public function refresh(ResourceInterface $resource, ResourceObject $ro, UncacheableMarkInterface $mark): ResourceObject + public function refresh(ResourceInterface $resource, ResourceObject $ro): ResourceObject { $etags = new SurrogateKeys($ro->uri); - $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags, $mark): string { + $refreshView = preg_replace_callback(self::URI_REGEX, static function (array $matches) use ($resource, $etags): string { // Group 1 is "2" for a new `le2` match (its URI, group 2, is rawurlencode()'d), // or "" for a legacy `le` match (group 2 is its URI, used verbatim). $uri = $matches[1] === '2' ? rawurldecode($matches[2]) : $matches[2]; - // Scoped the same way DonutRequest::__toString() scopes the MISS path: a mark - // made while this hole re-runs (onGet() or its own template render) must be - // attributed to the hole's own owner slot, not TOP_LEVEL - otherwise an - // unrelated sibling hole marking itself here is misread as "the page itself - // marked", and a nested #[DonutCache] hole's OWN template write is wrongly - // refused by reasonExcludingHoleMarks() for a mark that was never its own. - $mark->enterHole($uri); try { $ro = $resource->get($uri); $ro->toString(); } catch (Throwable $e) { - // $ro->toString() does not swallow a Throwable (same non-swallowing render - // create() itself now uses) - converted here, deliberately, to - // CacheStoreFailure: unlike create()'s own render, which lets the Throwable - // travel unwrapped, a HIT's re-fetch degrades instead, so the interceptor's - // read path falls back to a full fresh render (reviewed design, kept as-is). + // toString() lets a render failure travel uncaught; wrapped here so a HIT's + // re-fetch degrades to a fresh render instead of surfacing a broken response. throw CacheStoreFailure::from($e); - } finally { - $mark->leaveHole($uri); } if (array_key_exists(Header::SURROGATE_KEY, $ro->headers)) { @@ -227,21 +215,10 @@ private function hashContent(string $view): string } /** - * `(string) $ro` here would call the page's own `ResourceObject::__toString()`, which - * swallows a render Throwable into an E_USER_WARNING and `''` - storing that would bake - * an empty page into the cache for every later visitor (since 1.17.0). `toString()` is - * the same render with no such catch (vendor/bear/resource), the same method - * `BEAR\Sunday\...\HttpResponder::getOutput()` already renders a response with - so a - * failure in the PAGE's own template propagates exactly as it would for a page with no - * cache attribute at all. A HOLE's failure travels too (DonutRequest wraps it with the - * same non-swallowing render, see DonutRequest::__toString()) - a clean, catchable - * exception, actually BETTER than the no-attribute baseline for an embedded hole: an - * un-donut-wrapped `#[Embed]` renders its child via the raw `AbstractRequest::__toString()`, - * which swallows into a fatal E_USER_ERROR instead (verified by execution, not assumed). - * Either way, the original Throwable - not a reconstruction - travels to the caller, and - * nothing past this call ever stores or returns a template. + * Renders via `toString()`, not `(string) $ro`, which swallows a render failure into + * an `E_USER_WARNING` and `''` - that would store an empty template for every later visitor. */ - public static function create(ResourceObject $ro, DonutRendererInterface $storage, SurrogateKeys $etags, int|null $ttl, bool $isCacheble, UncacheableMarkInterface $mark): self + public static function create(ResourceObject $ro, DonutRendererInterface $storage, SurrogateKeys $etags, int|null $ttl, bool $isCacheble): self { // A resource with nothing to return leaves its body null - a 204 typically does, while // an #[Embed] would have made it an array - and a null body holds no embedded request @@ -251,7 +228,7 @@ public static function create(ResourceObject $ro, DonutRendererInterface $storag /** @var mixed $maybeRequest */ foreach ($ro->body as &$maybeRequest) { if ($maybeRequest instanceof AbstractRequest) { - $maybeRequest = new DonutRequest($maybeRequest, $storage, $etags, $mark); + $maybeRequest = new DonutRequest($maybeRequest, $storage, $etags); } } diff --git a/src/ThrowingUnstorableGate.php b/src/ThrowingUnstorableGate.php deleted file mode 100644 index 719b1366..00000000 --- a/src/ThrowingUnstorableGate.php +++ /dev/null @@ -1,52 +0,0 @@ -throwIfUnstorable($this->gate->reason($ro), $ro); - } - - /** @return 'unstorable'|null */ - #[Override] - public function reasonExcludingHoleMarks(ResourceObject $ro): string|null - { - return $this->throwIfUnstorable($this->gate->reasonExcludingHoleMarks($ro), $ro); - } - - /** - * @param 'unstorable'|null $reason - * - * @return null a non-null $reason always throws - */ - private function throwIfUnstorable(string|null $reason, ResourceObject $ro): null - { - if ($reason !== null) { - throw new UnstorableResponseDetected((string) $ro->uri, $reason); - } - - return null; - } -} diff --git a/src/UncacheableMark.php b/src/UncacheableMark.php deleted file mode 100644 index 0ee0135f..00000000 --- a/src/UncacheableMark.php +++ /dev/null @@ -1,88 +0,0 @@ - */ - private array $stack = []; - - /** @var array */ - private array $markedOwners = []; - - #[Override] - public function mark(string $reason): void - { - $this->markedOwners[$this->currentOwner()] = true; - } - - #[Override] - public function isMarked(): bool - { - return $this->markedOwners !== []; - } - - #[Override] - public function isMarkedForCurrentScope(): bool - { - return isset($this->markedOwners[$this->currentOwner()]); - } - - #[Override] - public function enterHole(string $uri): void - { - $this->stack[] = $uri; - } - - #[Override] - public function leaveHole(string $uri): void - { - // The last (innermost) occurrence of $uri, not the first: enterHole() tolerates - // the same URI nested twice (same path embedded twice), and only the most recent - // push should close on a matching leaveHole(). - $index = null; - foreach ($this->stack as $i => $owner) { - if ($owner === $uri) { - $index = $i; - } - } - - if ($index === null) { - return; - } - - // Truncate to just before that occurrence rather than popping exactly one entry: - // a caller that pushed its own scope and never left it (third-party misuse, or a - // leaked enterHole() above this one) must not permanently block this leaveHole() - // from ever closing ITS OWN scope - a strict "only pop if it is the exact top" - // check would leave this, and every enterHole()/leaveHole() pair below the leak, - // open for the rest of the request. Truncating drops the leaked entry along with - // the one actually being closed. - $this->stack = array_slice($this->stack, 0, $index); - } - - private function currentOwner(): string - { - return $this->stack === [] ? self::TOP_LEVEL : $this->stack[array_key_last($this->stack)]; - } -} diff --git a/src/UncacheableMarkInterface.php b/src/UncacheableMarkInterface.php deleted file mode 100644 index 51136eff..00000000 --- a/src/UncacheableMarkInterface.php +++ /dev/null @@ -1,70 +0,0 @@ -reasonFor($ro, $this->mark->isMarked()); - } - - /** @return 'unstorable'|null */ - #[Override] - public function reasonExcludingHoleMarks(ResourceObject $ro): string|null - { - return $this->reasonFor($ro, $this->mark->isMarkedForCurrentScope()); - } - - /** @return 'unstorable'|null */ - private function reasonFor(ResourceObject $ro, bool $marked): string|null - { - if ($marked) { - return self::REASON; - } - - $name = Header::findKey($ro->headers, Header::CACHE_CONTROL); - $cacheControl = $name === null ? '' : $ro->headers[$name]; - if ($cacheControl === '' || $cacheControl === ($this->declaredCacheControl)($ro)) { - return null; - } - - $lower = strtolower($cacheControl); - if (str_contains($lower, 'no-store') || str_contains($lower, 'private')) { - return self::REASON; - } - - return null; - } -} diff --git a/src/UnstorableGateInterface.php b/src/UnstorableGateInterface.php deleted file mode 100644 index ead6a315..00000000 --- a/src/UnstorableGateInterface.php +++ /dev/null @@ -1,48 +0,0 @@ -getInstance(ResourceInterface::class); - // The resource, asked not to mark itself, gets the CDN header - checked first, - // since the mark is per-request state (here: one shared container for the whole - // test) that cannot be unset once a later call sets it. + // The resource, asked not to set a runtime Cache-Control, gets the CDN header - + // checked first, since the request fixture is one shared container for the whole + // test. $ordinary = $resource->get('page://self/html/unstorable-donut', ['unstorable' => false]); $this->assertArrayHasKey(Header::CDN_CACHE_CONTROL, $ordinary->headers); $this->assertSame('max-age=10, stale-while-revalidate=10', $ordinary->headers[Header::CDN_CACHE_CONTROL]); - // A producer marks this request's response unstorable (issue #222): no + // The resource sets its own Cache-Control to private at runtime: no // CDN-Cache-Control is emitted, and Cache-Control tells the CDN not to keep it. $ro = $resource->get('page://self/html/unstorable-donut'); $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL]); } public function testCdnHeadersSetDirectlyDoNotSurviveAnUnstorableRefusal(): void { // The resource sets CDN-Cache-Control and Surrogate-Control directly in onGet(), - // bypassing CdnCacheControlHeaderSetterInterface entirely, and marks itself - // unstorable: HeaderSetter must strip every CDN-facing header it manages, not just - // skip adding one, or a CDN reading Cache-Control: no-store alongside a surviving - // CDN-Cache-Control could still cache this response at the edge (issue #222). + // bypassing CdnCacheControlHeaderSetterInterface entirely, and sets its own + // Cache-Control to private: HeaderSetter must strip every CDN-facing header it + // manages, not just skip adding one, or a CDN reading a surviving CDN-Cache-Control + // could still cache this response at the edge. $module = $this->getModule(); $injector = new Injector($module, __DIR__ . '/tmp'); $resource = $injector->getInstance(ResourceInterface::class); $ro = $resource->get('page://self/html/unstorable-with-manual-cdn-header'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL]); $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); $this->assertArrayNotHasKey('Surrogate-Control', $ro->headers); } diff --git a/tests/DeclaredCacheControlTest.php b/tests/DeclaredCacheControlTest.php new file mode 100644 index 00000000..243483e5 --- /dev/null +++ b/tests/DeclaredCacheControlTest.php @@ -0,0 +1,80 @@ +assertNull($declaredCacheControl->reason($ro)); + } + + public function testCacheControlNoStoreIsUnstorable(): void + { + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new None(); + $ro->headers[Header::CACHE_CONTROL] = 'no-store'; + + $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + } + + public function testCacheControlPrivateIsUnstorable(): void + { + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new None(); + $ro->headers[Header::CACHE_CONTROL] = 'PRIVATE'; + + $this->assertSame('unstorable', $declaredCacheControl->reason($ro), 'RFC 9111 directives are case-insensitive'); + } + + public function testCacheControlHeaderNameIsMatchedCaseInsensitively(): void + { + // $ro->headers is a plain PHP array, not an HTTP-aware collection - 'cache-control' + // must be found the same way 'Cache-Control' is (RFC 7230 §3.2). + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new None(); + $ro->headers['cache-control'] = 'no-store'; + + $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + } + + public function testCacheControlMatchingTheDeclaredValueIsExempt(): void + { + // #[HttpCache(isPrivate: true)]'s own value is the attribute speaking, not the + // method - #[Cacheable] still stores server-side. + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new HttpCacheControlWithCacheable(); + $ro->headers[Header::CACHE_CONTROL] = 'private'; + + $this->assertNull($declaredCacheControl->reason($ro)); + } + + public function testCacheControlDivergingFromTheDeclaredValueIsNotExempt(): void + { + // A method or render that adds its own no-store on top of (or instead of) the + // declared value still trips the check. + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new HttpCacheControlWithCacheable(); + $ro->headers[Header::CACHE_CONTROL] = 'private, no-store'; + + $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + } + + public function testOrdinaryCacheControlIsStorable(): void + { + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new None(); + $ro->headers[Header::CACHE_CONTROL] = 'max-age=60'; + + $this->assertNull($declaredCacheControl->reason($ro)); + } +} diff --git a/tests/DevUnstorableGateModuleTest.php b/tests/DevUnstorableGateModuleTest.php deleted file mode 100644 index b9820f84..00000000 --- a/tests/DevUnstorableGateModuleTest.php +++ /dev/null @@ -1,55 +0,0 @@ -override(new DevUnstorableGateModule()); - $injector = new Injector($module, __DIR__ . '/tmp'); - - $gate = $injector->getInstance(UnstorableGateInterface::class); - - $this->assertInstanceOf(ThrowingUnstorableGate::class, $gate); - } - - public function testTheResolvedGateActuallyThrowsWhenMarkedUnstorable(): void - { - $module = new QueryRepositoryModule(); - $module->override(new DevUnstorableGateModule()); - $injector = new Injector($module, __DIR__ . '/tmp'); - - $gate = $injector->getInstance(UnstorableGateInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $mark->mark('csrf-token-issued'); - - $this->expectException(UnstorableResponseDetected::class); - $gate->reason(new None()); - } - - public function testPlainInstallDoesNotSwapTheExistingBinding(): void - { - // The negative case this module's docblock warns against: install() silently - // keeps QueryRepositoryModule's own (non-throwing) binding. - $module = new QueryRepositoryModule(); - $module->install(new DevUnstorableGateModule()); - $injector = new Injector($module, __DIR__ . '/tmp'); - - $gate = $injector->getInstance(UnstorableGateInterface::class); - - $this->assertNotInstanceOf(ThrowingUnstorableGate::class, $gate, 'install() must not be documented as working - it silently keeps the original binding'); - } -} diff --git a/tests/DonutCacheTest.php b/tests/DonutCacheTest.php index df6c058e..8beab91a 100644 --- a/tests/DonutCacheTest.php +++ b/tests/DonutCacheTest.php @@ -20,7 +20,6 @@ class DonutCacheTest extends TestCase { private ResourceInterface $resource; - private UncacheableMarkInterface $mark; protected function setUp(): void { @@ -30,7 +29,6 @@ protected function setUp(): void $module->override(new TwigModule([$path])); $injector = new Injector($module, __DIR__ . '/tmp'); $this->resource = $injector->getInstance(ResourceInterface::class); - $this->mark = new UncacheableMark(); parent::setUp(); } @@ -40,7 +38,7 @@ public function testGetState(): void $headers = ['Content-Type' => 'application/xml;']; $donut = new ResourceDonut('cmt=[le:page://self/html/comment]', $headers, null, true); $blog = $this->resource->get('page://self/html/blog-posting'); - $ro = $donut->refresh($this->resource, $blog, $this->mark); + $ro = $donut->refresh($this->resource, $blog); $this->assertInstanceOf(ResourceObject::class, $ro); $this->assertSame('cmt=comment01', $ro->view); $this->assertSame($headers['Content-Type'], $ro->headers['Content-Type']); @@ -181,7 +179,7 @@ public function testDonutReplaysTheStatusCodeItWasStoredWith(): void $donut = new ResourceDonut('tmpl', [], null, true, null, null, null, null, 301); $ro = $this->resource->get('page://self/html/comment'); - $donut->refresh($this->resource, $ro, $this->mark); + $donut->refresh($this->resource, $ro); $this->assertSame(301, $ro->code); } @@ -194,7 +192,7 @@ public function testLegacyDonutPayloadLeavesTheStatusCodeAlone(): void $ro = $this->resource->get('page://self/html/comment'); $ro->code = 301; - $this->legacyDonut()->refresh($this->resource, $ro, $this->mark); + $this->legacyDonut()->refresh($this->resource, $ro); $this->assertSame(301, $ro->code); } diff --git a/tests/DonutMaterialDependencyTest.php b/tests/DonutMaterialDependencyTest.php index 397a565d..64f75939 100644 --- a/tests/DonutMaterialDependencyTest.php +++ b/tests/DonutMaterialDependencyTest.php @@ -73,7 +73,6 @@ protected function setUp(): void $this->logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); MutableComment::$comment = 'comment-a'; - MutableComment::$markUnstorable = false; parent::setUp(); } @@ -81,7 +80,6 @@ protected function setUp(): void protected function tearDown(): void { MutableComment::$comment = 'comment-a'; - MutableComment::$markUnstorable = false; parent::tearDown(); } @@ -301,13 +299,12 @@ public function testAManuallyConstructedRepositoryUsesItsOwnUriTagForTheDefaultC $resource = $injector->getInstance(ResourceInterface::class); $repository = new DonutRepository( $injector->getInstance(QueryRepositoryInterface::class), - $injector->getInstance(DonutHeaderSetter::class), + $injector->getInstance(HeaderSetter::class), $injector->getInstance(ResourceStorageInterface::class), $resource, + $injector->getInstance(CdnCacheControlHeaderSetterInterface::class), $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class), $injector->getInstance(DonutRendererInterface::class), - $injector->getInstance(UnstorableGateInterface::class), - $injector->getInstance(UncacheableMarkInterface::class), new class implements UriTagInterface { #[Override] public function __invoke(AbstractUri $uri): string diff --git a/tests/DonutQueryInterceptorPurgeTest.php b/tests/DonutQueryInterceptorPurgeTest.php index 217cd271..19617240 100644 --- a/tests/DonutQueryInterceptorPurgeTest.php +++ b/tests/DonutQueryInterceptorPurgeTest.php @@ -65,7 +65,6 @@ protected function configure(): void protected function tearDown(): void { MutableComment::$comment = 'comment-a'; - MutableComment::$markUnstorable = false; // Every emitted log entry must conform to the published schema (drift detection) $this->flushAndValidate($this->logger); } @@ -167,53 +166,6 @@ public function testRefreshPersistsChangedContentState(): void $this->assertNotSame($bLastModified, $a2->headers[Header::LAST_MODIFIED], 'B→A advances Last-Modified'); } - /** - * Pins DonutRepository::refreshDonut()'s leak guard: the `return $ro;` inside the - * marked branch of the refresh that stops a marked, changed recomposition from - * reaching saveView(). Mutating that line away (removing the early return) lets the - * marked content fall through to recordContentState()/saveView() and be persisted. - */ - public function testRefreshDoesNotPersistContentMarkedUnstorableDuringTheRefresh(): void - { - // Own pools: the mark this test sets is a per-injector singleton (UncacheableMark) - // and never unsets itself - it must not leak into the shared static injector the - // other test methods in this class reuse. - $injector = self::newInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $repository = $injector->getInstance(QueryRepository::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - $pageUri = 'page://self/html/mutable-blog-posting'; - $commentUri = new Uri('page://self/html/mutable-comment'); - MutableComment::$comment = 'comment-a'; - MutableComment::$markUnstorable = false; - $resource->get($pageUri); - $logger->flush(); // drain the initial put: this test is about the refresh that follows - - // The hole both changes content and marks the recomposition unstorable - e.g. it - // issues a CSRF token while also returning fresh data - exactly the scenario the - // unprotected `return $ro;` in refreshDonut()'s marked branch guards against: - // removing it lets this changed, marked content fall through to saveView(). - MutableComment::$comment = 'comment-b-secret'; - MutableComment::$markUnstorable = true; - $purged = $repository->purge($commentUri); - assert($purged); - - $marked = $resource->get($pageUri); - $this->assertTrue($mark->isMarked(), 'sanity: the hole did mark this recomposition'); - $this->assertStringContainsString('comment-b-secret', (string) $marked->view, 'sanity: the recomposition did pick up the changed, marked content'); - $this->assertSame('private, no-store', $marked->headers[Header::CACHE_CONTROL]); - - // DonutRepository::get() consults exactly this state for EVERY later visitor, - // marked or not - if it now exists and carries the marked content, that content - // leaked into the shared cache for whoever requests this page next. - $state = $repository->get(new Uri($pageUri)); - $this->assertNull($state, 'content recomposed while marked unstorable must not be persisted for a later visitor'); - - $this->flushAndValidate($logger); - } - private function isStateCached(): bool { return $this->repository->get(new Uri('page://self/html/blog-posting')) instanceof ResourceState; diff --git a/tests/DonutRepositoryTest.php b/tests/DonutRepositoryTest.php index 776e9720..11f8798d 100644 --- a/tests/DonutRepositoryTest.php +++ b/tests/DonutRepositoryTest.php @@ -115,15 +115,13 @@ public function testCreatedByDonut(): void $this->assertStringContainsString('"layer":"donut"', $hit); } - public function testUnstorableMarkSkipsDonutStoreAndOrdinaryResponsesStillStore(): void + public function testRuntimePrivateResponseSkipsDonutStoreAndOrdinaryResponsesStillStore(): void { $injector = $this->getInjector(); $resource = $injector->getInstance(ResourceInterface::class); $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - // Ordinary (unmarked) responses store and hit normally - checked first, since the - // mark is per-request state (here: one shared container for the whole test) that - // cannot be unset once a later call sets it. + // Ordinary (no runtime Cache-Control) responses store and hit normally. $ordinary = $resource->get('page://self/html/unstorable-donut', ['unstorable' => false]); $this->assertArrayHasKey(Header::CDN_CACHE_CONTROL, $ordinary->headers); $logger->flush(); @@ -131,11 +129,11 @@ public function testUnstorableMarkSkipsDonutStoreAndOrdinaryResponsesStillStore( $treeOrdinary = $this->flushAndValidate($logger); $this->assertContains('cache_hit', self::collectTypes($treeOrdinary)); - // A producer (a CSRF token issuer) marks this request's response unstorable: the - // donut write is skipped, the live response is still served, and no CDN header is - // emitted (issue #222). + // The resource sets its own Cache-Control to private at runtime (e.g. a CSRF + // token issuer): the donut write is skipped, the live response is still served + // with its own header kept, and no CDN header is emitted. $ro = $resource->get('page://self/html/unstorable-donut'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL]); $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); $tree = $this->flushAndValidate($logger); @@ -143,7 +141,7 @@ public function testUnstorableMarkSkipsDonutStoreAndOrdinaryResponsesStillStore( $this->assertNotNull($putSkipped); $this->assertStringContainsString('"reason":"unstorable"', $putSkipped); - // Nothing was stored: a second GET of the same (still marked) request misses again. + // Nothing was stored: a second GET of the same request misses again. $logger->flush(); $resource->get('page://self/html/unstorable-donut'); $tree2 = $this->flushAndValidate($logger); @@ -151,68 +149,36 @@ public function testUnstorableMarkSkipsDonutStoreAndOrdinaryResponsesStillStore( $this->assertNull(self::eventContextJsonOf($tree2, 'cache_hit')); } - public function testRefreshSkipsSavingContentAndCdnHeaderWhenMarkedUnstorable(): void - { - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $storage = $injector->getInstance(ResourceStorageInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - // Populate the donut template and page state first (ordinary write). - $resource->get('page://self/html/blog-posting'); - // Drop just the page state - same mechanism testCreatedByDonut uses - leaving the - // template behind, so the next GET takes the refresh path. - $storage->invalidateTags([(new UriTag())(new Uri('page://self/html/comment'))]); - $logger->flush(); - - // A hole marked this recomposition unstorable - e.g. it issued a CSRF token. - // ResourceDonut::refresh() restores the stored headers afterwards, but the mark is - // a separate per-request service and survives that (issue #222). - $mark->mark('csrf-token-issued'); - $donutRo = $resource->get('page://self/html/blog-posting'); - - $this->assertSame('private, no-store', $donutRo->headers[Header::CACHE_CONTROL]); - $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $donutRo->headers); - - $tree = $this->flushAndValidate($logger); - $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the page is recomposed from the cached donut'); - $putSkipped = self::eventContextJsonOf($tree, 'put_skipped'); - $this->assertNotNull($putSkipped); - $this->assertStringContainsString('"reason":"unstorable"', $putSkipped); - } - - public function testDonutCacheTemplateSkipsSaveWhenThePageMarksItsOwnBody(): void + public function testDonutCacheTemplateSkipsSaveWhenThePageWritesPrivateIntoItsOwnBody(): void { // A #[DonutCache] template holds only placeholders for an embedded hole's content - // but a page can also write a per-user value straight into its own body/view, - // outside any hole, and that lands in the stored template verbatim (issue #222). + // outside any hole, and that lands in the stored template verbatim. $injector = $this->getInjector(); $resource = $injector->getInstance(ResourceInterface::class); $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - // Ordinary (unmarked) first - the shared mark is sticky once set. A normal - // #[DonutCache] page still saves its template: the next GET recomposes from it - // (refresh_donut), not a fresh donut miss. + // Ordinary first: a normal #[DonutCache] page still saves its template, so the + // next GET recomposes from it (refresh_donut), not a fresh donut miss. $resource->get('page://self/html/unstorable-donut-cache-template', ['unstorable' => false]); $logger->flush(); $resource->get('page://self/html/unstorable-donut-cache-template', ['unstorable' => false]); $treeOrdinary = $this->flushAndValidate($logger); $this->assertNotNull(self::eventContextJsonOf($treeOrdinary, 'refresh_donut'), 'an ordinary #[DonutCache] page still saves and reuses its template'); - // The page marks itself unstorable: the template write must be refused - otherwise - // the secret value, baked verbatim into the stored template, is served to every - // later visitor. + // The page sets its own Cache-Control to private: the template write must be + // refused - otherwise the secret value, baked verbatim into the stored template, + // is served to every later visitor. $ro = $resource->get('page://self/html/unstorable-donut-cache-template'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL]); $tree = $this->flushAndValidate($logger); $putSkipped = self::eventContextJsonOf($tree, 'put_skipped'); $this->assertNotNull($putSkipped); $this->assertStringContainsString('"reason":"unstorable"', $putSkipped); - // Nothing was stored: a second GET of the same (still marked) request is a fresh - // donut miss again, not a refresh_donut recomposed from a leaked template. + // Nothing was stored: a second GET of the same request is a fresh donut miss + // again, not a refresh_donut recomposed from a leaked template. $logger->flush(); $resource->get('page://self/html/unstorable-donut-cache-template'); $tree2 = $this->flushAndValidate($logger); @@ -220,33 +186,6 @@ public function testDonutCacheTemplateSkipsSaveWhenThePageMarksItsOwnBody(): voi $this->assertNotNull(self::eventContextJsonOf($tree2, 'cache_miss')); } - public function testDonutCacheTemplateStoresWhenOnlyAnEmbeddedHoleMarksItself(): void - { - // #[DonutCache] templates hold placeholders only - a hole's content is substituted - // at render/refresh time, never baked into the stored template - so a hole marking - // itself unstorable (its own storage is separately gated by its own #[Cacheable] - // write) must not block the parent template write: this is exactly the "a page - // whose holes contain a form" case issue #222 explicitly keeps working. The live - // response is a different matter - the hole's rendered output IS substituted into - // it, so it still reads Cache-Control: private, no-store (issue #222 MEDIUM) - on - // BOTH the response that wrote the template (cache_miss, doPutDonut) AND every - // later one recomposed from it (cache_hit, refreshDonut): the mark is still set - // for this request (UncacheableMark is a per-injector singleton), and that refusal - // must be read on recomposition too, not just the response that first wrote the - // template. - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - $ro = $resource->get('page://self/html/donut-cache-with-unstorable-hole'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - $logger->flush(); - $refreshed = $resource->get('page://self/html/donut-cache-with-unstorable-hole'); - $tree = $this->flushAndValidate($logger); - $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the template was saved despite the embedded hole marking itself'); - $this->assertSame('private, no-store', $refreshed->headers[Header::CACHE_CONTROL], 'the recomposed (refresh) response must still refuse CDN caching, not just the one that first wrote the template'); - } - public function testDonutCacheWithAMultiParamEmbedRendersAndRefreshesBothQueryParams(): void { // End-to-end through the real #[Embed] pipeline (URI-template expansion, not a @@ -276,136 +215,6 @@ public function testDonutCacheWithAMultiParamEmbedRendersAndRefreshesBothQueryPa $this->assertStringContainsString('1-2', (string) $refreshed->view, 'refresh() re-fetched the hole by its full, multi-param URI, not a partial or query-less one'); } - public function testNestedDonutCacheHoleDoesNotLeakItsOwnMarkedContentToTheSharedCache(): void - { - // The hole (RenderTimeMark, itself #[DonutCache]) runs its own write while still - // nested inside this page's enterHole() scope; its own render-time mark must refuse - // only ITS OWN template - a flat "inside any hole" flag would misattribute that mark - // to "inside a hole" from the hole's own perspective too, and store the leak. - // - // One shared injector/container for both GETs, not two: the mark is request-scoped - // state that legitimately differs per request, but the underlying storage pool must - // be the SAME one a later visitor's request would actually share - two separate - // injectors each get their own in-memory ArrayAdapter (ModuleFactory/FakeEtagPoolModule), - // which would make the second GET below a cold miss regardless of whether the bug - // exists, proving nothing. - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - $resource->get('page://self/html/donut-cache-with-nested-donut-cache-hole'); - $logger->flush(); - - // A later, different visitor requesting the hole's own URI directly, against the - // same shared cache this GET also reads. If the hole's template had been stored - // with the leaked content, this would be a hit serving it. - $ro = $resource->get('page://self/html/render-time-mark'); - - $tree = $this->flushAndValidate($logger); - $this->assertNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'nothing was stored for this later visitor to recompose from'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - } - - public function testRefreshReSavesAnUnrelatedNestedDonutCacheHolesTemplateAfterASiblingHoleMarksDuringTheRefresh(): void - { - // ResourceDonut::refresh() re-fetches each placeholder with $resource->get($uri); - // without scoping that re-fetch to its own URI the way DonutRequest::__toString() - // scopes the MISS path, a sibling hole's own mark (UnstorableValue marks every - // request) is attributed to TOP_LEVEL during the refresh - and an UNRELATED nested - // #[DonutCache] hole (PlainDonutHole, which never marks) reads - // isMarkedForCurrentScope() as true for its own write and wrongly refuses to - // re-save its own purged template (issue #222). - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $storage = $injector->getInstance(ResourceStorageInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - // First GET: MISS. Composes the outer page's own template and, via its own - // nested write, PlainDonutHole's own template too. - $resource->get('page://self/html/donut-cache-with-marking-and-nested-donut-cache-hole'); - $logger->flush(); - - // Drop only the unrelated nested hole's own template, so the outer page's next - // GET - always a refresh for a #[DonutCache] page, which never stores its own - // state - re-fetches it as a fresh MISS that must re-save it. - $storage->invalidateTags([(new UriTag())(new Uri('page://self/html/plain-donut-hole'))]); - - $resource->get('page://self/html/donut-cache-with-marking-and-nested-donut-cache-hole'); - $tree = $this->flushAndValidate($logger); - $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the outer page is recomposed from its own cached donut'); - $this->assertTrue($mark->isMarked(), 'sanity: the sibling hole did mark during this refresh'); - - // A later visitor requesting the unrelated hole directly: if its own template was - // re-saved (the fix), this is a refresh (template hit); if the sibling's mark was - // misattributed to it instead (the bug), this is a fresh MISS again. - $logger->flush(); - $resource->get('page://self/html/plain-donut-hole'); - $laterTree = $this->flushAndValidate($logger); - $this->assertNotNull(self::eventContextJsonOf($laterTree, 'refresh_donut'), "the unrelated hole's own purged template was re-saved during the sibling's marked refresh, not skipped because of the sibling's mark"); - } - - public function testRefreshStillRefusesANestedDonutCacheHolesOwnTemplateWhenItMarksItselfDuringTheRefresh(): void - { - // Same safety property as testNestedDonutCacheHoleDoesNotLeakItsOwnMarkedContentToTheSharedCache - // above, but reached through ResourceDonut::refresh() instead of the initial - // compose (DonutRequest): enterHole()/leaveHole() around the refresh re-fetch must - // still let the hole's own render-time mark refuse ITS OWN template, the same way - // it does on the MISS path - the fix for the unrelated-sibling case above must not - // loosen this. - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - // First GET composes the outer page's own template (MISS); RenderTimeMark marks - // itself every time, so its own write is refused here too. - $resource->get('page://self/html/donut-cache-with-nested-donut-cache-hole'); - $logger->flush(); - - // A #[DonutCache] page never stores its own state, so this second GET is always a - // refresh - re-fetching RenderTimeMark through ResourceDonut::refresh(), not - // DonutRequest. - $resource->get('page://self/html/donut-cache-with-nested-donut-cache-hole'); - $tree = $this->flushAndValidate($logger); - $this->assertNotNull(self::eventContextJsonOf($tree, 'refresh_donut'), 'the outer page is recomposed from its own cached donut'); - - // A later, different visitor requesting the hole's own URI directly. If the - // hole's template had been stored with the leaked content during the refresh - // above, this would be a hit serving it. - $ro = $resource->get('page://self/html/render-time-mark'); - $laterTree = $this->flushAndValidate($logger); - $this->assertNull(self::eventContextJsonOf($laterTree, 'refresh_donut'), "nothing was stored for this later visitor to recompose from, even though the hole's own mark fired during the OUTER page's refresh"); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - } - - public function testDonutCacheTemplateSkipsSaveWhenMarkedWhileItsOwnTemplateRenders(): void - { - // The mark can also happen while the page's own template renders (e.g. a Twig CSRF - // helper), inside the same ResourceDonut::create() call that resolves any embedded - // hole - neither in onGet() (too early) nor inside a hole's own execution (wrongly - // exempt). Both the read order and the hole-exclusion must get this right (issue #222). - $injector = $this->getInjector(); - $resource = $injector->getInstance(ResourceInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - $ro = $resource->get('page://self/html/render-time-mark'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); - - $tree = $this->flushAndValidate($logger); - $putSkipped = self::eventContextJsonOf($tree, 'put_skipped'); - $this->assertNotNull($putSkipped); - $this->assertStringContainsString('"reason":"unstorable"', $putSkipped); - - // Nothing was stored: a second GET is a fresh donut miss again, not a refresh_donut - // recomposed from a leaked template. - $logger->flush(); - $resource->get('page://self/html/render-time-mark'); - $tree2 = $this->flushAndValidate($logger); - $this->assertNull(self::eventContextJsonOf($tree2, 'refresh_donut')); - $this->assertNotNull(self::eventContextJsonOf($tree2, 'cache_miss')); - } - /** * When cache A contains cache B, deleting B will automatically delete A as well. */ diff --git a/tests/DonutRequestTest.php b/tests/DonutRequestTest.php index 891454ba..b8d33a5e 100644 --- a/tests/DonutRequestTest.php +++ b/tests/DonutRequestTest.php @@ -4,14 +4,12 @@ namespace BEAR\QueryRepository; -use BEAR\RepositoryModule\Annotation\CacheLog; use BEAR\Resource\AbstractRequest; use BEAR\Resource\InvokerInterface; use BEAR\Resource\Resource; use BEAR\Resource\ResourceInterface; use BEAR\Resource\ResourceObject; use BEAR\Resource\Uri; -use Koriym\SemanticLogger\SemanticLoggerInterface; use Madapaja\TwigModule\TwigModule; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; @@ -23,8 +21,6 @@ class DonutRequestTest extends TestCase { - use SemanticLogTreeTrait; - private AbstractRequest $request; protected function setUp(): void @@ -46,7 +42,7 @@ protected function setUp(): void public function testToString(): DonutRenderer { $storage = new DonutRenderer(); - $request = new DonutRequest($this->request, $storage, new SurrogateKeys(new Uri('app://self/foo')), new UncacheableMark()); + $request = new DonutRequest($this->request, $storage, new SurrogateKeys(new Uri('app://self/foo'))); $view = (string) $request; $this->assertSame(ResourceDonut::placeholder('page://self/html/comment'), $view); @@ -82,7 +78,7 @@ public function invoke(AbstractRequest $request): ResourceObject }; $request = new FakeNonRequestHole($invoker, $child); $storage = new DonutRenderer(); - $donutRequest = new DonutRequest($request, $storage, new SurrogateKeys(new Uri('app://self/foo')), new UncacheableMark()); + $donutRequest = new DonutRequest($request, $storage, new SurrogateKeys(new Uri('app://self/foo'))); $placeholder = (string) $donutRequest; @@ -91,7 +87,7 @@ public function invoke(AbstractRequest $request): ResourceObject $this->assertSame('hole=non-request-view', $storage->render('hole=' . $placeholder)); } - public function testHoleScopeIsLeftWhenTheHoleThrows(): void + public function testHoleFailureTravelsToTheCaller(): void { $namespace = 'FakeVendor\HelloWorld'; $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); @@ -101,21 +97,10 @@ public function testHoleScopeIsLeftWhenTheHoleThrows(): void $throwingRequest = $resource->get->uri('app://self/throwing-hole'); assert($throwingRequest instanceof AbstractRequest); - // The same instance ThrowingHole itself resolves via DI (UncacheableMarkInterface - // is Scope::SINGLETON): its own mark() call and this test's check share one state. - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $donutRequest = new DonutRequest($throwingRequest, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/foo')), $mark); + $donutRequest = new DonutRequest($throwingRequest, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/foo'))); $this->expectException(RuntimeException::class); - try { - (string) $donutRequest; - } finally { - // ThrowingHole marks itself - attributed to its own scope - before throwing. If - // leaveHole() ran despite the exception, that scope is popped and the current - // scope is top-level, which was never marked; if leaveHole() did not run, the - // hole's scope is still current and this reads as marked, wrongly, from here. - $this->assertFalse($mark->isMarkedForCurrentScope(), 'the hole\'s own mark must not leak into the current (top-level) scope once its scope is left'); - } + (string) $donutRequest; } public function testToStringCarriesAQueryStringSetViaTheRequestsOwnQueryArray(): void @@ -124,8 +109,7 @@ public function testToStringCarriesAQueryStringSetViaTheRequestsOwnQueryArray(): // page's named-parameter binding - not baked into the URI string used to build the // lazy request, so resourceObject->uri->query is still empty here, before // invocation. AbstractRequest::__invoke() is what syncs it (its first statement), - // so a placeholder/view-map URI read before invocation can be missing it (issue - // #222 regression). + // so a placeholder/view-map URI read before invocation can be missing it. $namespace = 'FakeVendor\HelloWorld'; $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); $path = dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'; @@ -133,93 +117,16 @@ public function testToStringCarriesAQueryStringSetViaTheRequestsOwnQueryArray(): $injector = new Injector($module, __DIR__ . '/tmp'); /** @var Resource $resource */ $resource = $injector->getInstance(ResourceInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $request = $resource->get->uri('app://self/query-hole')->withQuery(['token' => 'public-9', 'visibility' => 'public']); + $request = $resource->get->uri('app://self/query-hole')->withQuery(['token' => 'public-9']); assert($request instanceof AbstractRequest); $storage = new DonutRenderer(); - $donutRequest = new DonutRequest($request, $storage, new SurrogateKeys(new Uri('app://self/foo')), $mark); + $donutRequest = new DonutRequest($request, $storage, new SurrogateKeys(new Uri('app://self/foo'))); $placeholder = (string) $donutRequest; - $expectedUri = 'app://self/query-hole?' . http_build_query(['token' => 'public-9', 'visibility' => 'public']); + $expectedUri = 'app://self/query-hole?' . http_build_query(['token' => 'public-9']); $this->assertSame(ResourceDonut::placeholder($expectedUri), $placeholder, 'the placeholder must carry the query string set via the request\'s own query array'); $this->assertSame('public-9', $storage->render(ResourceDonut::placeholder($expectedUri)), 'the stored view must be keyed by the exact same full URI the placeholder carries'); - $this->assertFalse($mark->isMarked(), 'visibility=public must not mark'); - } - - public function testToStringCarriesAQueryStringEvenWhenTheHoleMarksItself(): void - { - // Same as above, with a query that puts the hole in its marking state - the - // query-string fix and the mark/no-mark decision are independent of each other. - $namespace = 'FakeVendor\HelloWorld'; - $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); - $path = dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'; - $module->override(new TwigModule([$path])); - $injector = new Injector($module, __DIR__ . '/tmp'); - /** @var Resource $resource */ - $resource = $injector->getInstance(ResourceInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - - $request = $resource->get->uri('app://self/query-hole')->withQuery(['token' => 'secret-5', 'visibility' => 'private']); - assert($request instanceof AbstractRequest); - - $storage = new DonutRenderer(); - $donutRequest = new DonutRequest($request, $storage, new SurrogateKeys(new Uri('app://self/foo')), $mark); - $placeholder = (string) $donutRequest; - - $expectedUri = 'app://self/query-hole?' . http_build_query(['token' => 'secret-5', 'visibility' => 'private']); - $this->assertSame(ResourceDonut::placeholder($expectedUri), $placeholder, 'the placeholder must carry the query string even though the hole marked itself'); - $this->assertTrue($mark->isMarked(), 'visibility=private must mark'); - } - - public function testSiblingHolesOnTheSamePathWithDifferentQueriesDoNotShareAnOwnerSlot(): void - { - // A query-less enterHole() key would let two embeds of the same path but - // different query args share one owner slot: a #[DonutCache] hole whose own - // template write checks reasonExcludingHoleMarks() (isMarkedForCurrentScope(), - // narrow/scoped) would then read as marked anyway, and refuse ITS OWN template - // write, because a sibling with a different query marked itself (over-refusal). - // Unreachable through #[Embed] itself - EmbedInterceptor expands the URI template - // before building the request, so an #[Embed] request always already carries its - // query - but reachable through two manually built requests with different - // queries on the same path, exactly as issue #222 follow-up code (a worker - // rebinding the mark per request, say) might construct one. - // - // Checked via the semantic log right after the second write, not a later live GET: - // a live response's headers reflect isMarked() (broad, "any mark anywhere in this - // request"), which stays true for the rest of this shared mark instance regardless - // of this fix - only the per-write put_skipped/save_donut event, read at the point - // of the second write itself, observes the scoped template-storage decision. - $namespace = 'FakeVendor\HelloWorld'; - $module = new FakeEtagPoolModule(ModuleFactory::getInstance($namespace)); - $path = dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'; - $module->override(new TwigModule([$path])); - $injector = new Injector($module, __DIR__ . '/tmp'); - /** @var Resource $resource */ - $resource = $injector->getInstance(ResourceInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); - $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); - - // The first hole marks itself - attributed to its own (query-bearing) scope only. - $markedRequest = $resource->get->uri('app://self/query-donut-hole')->withQuery(['token' => 'secret-5', 'visibility' => 'private']); - assert($markedRequest instanceof AbstractRequest); - $storage = new DonutRenderer(); - $etags = new SurrogateKeys(new Uri('app://self/foo')); - (string) new DonutRequest($markedRequest, $storage, $etags, $mark); - $logger->flush(); - - // A sibling on the same path, a different query, never marks itself - wrapped the - // same way #[Embed] would (DonutRequest's own enterHole()/leaveHole()); its own - // #[DonutCache] template write runs here, inside its own scope. - $unmarkedRequest = $resource->get->uri('app://self/query-donut-hole')->withQuery(['token' => 'public-9', 'visibility' => 'public']); - assert($unmarkedRequest instanceof AbstractRequest); - (string) new DonutRequest($unmarkedRequest, $storage, $etags, $mark); - $tree = $this->flushAndValidate($logger); - - $this->assertNull(self::eventContextJsonOf($tree, 'put_skipped'), 'a sibling hole on the same path but a different query must not inherit a mark attributed to another query\'s owner slot'); - $saveDonut = self::eventContextJsonOf($tree, 'save_donut'); - $this->assertNotNull($saveDonut); - $this->assertStringContainsString('"saved":true', $saveDonut, 'the second hole\'s own template write must still go through'); } } diff --git a/tests/Fake/fake-app/src/RenderTimeToken.php b/tests/Fake/fake-app/src/RenderTimeToken.php deleted file mode 100644 index cc213f27..00000000 --- a/tests/Fake/fake-app/src/RenderTimeToken.php +++ /dev/null @@ -1,27 +0,0 @@ -mark->mark('csrf-token-issued'); - - return 'secret-token'; - } -} diff --git a/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php b/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php index 6b1d664f..43a1a696 100644 --- a/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php +++ b/tests/Fake/fake-app/src/Resource/App/HandSetPrivateCacheControl.php @@ -5,30 +5,19 @@ namespace FakeVendor\HelloWorld\Resource\App; use BEAR\QueryRepository\Header; -use BEAR\QueryRepository\UncacheableMarkInterface; use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\Resource\ResourceObject; /** * A #[Cacheable] resource that sets its own Cache-Control to `private` directly in onGet(), - * bypassing #[HttpCache] entirely - the owner-approved regression case (issue #222): this - * still stops the server-side store, but - unlike a producer that calls mark() - keeps the - * developer's own header instead of being overwritten with `private, no-store`. + * bypassing #[HttpCache] entirely: this still stops the server-side store, but keeps the + * developer's own header instead of being overwritten. */ #[Cacheable(expirySecond: 60)] class HandSetPrivateCacheControl extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet(bool $alsoMark = false): static + public function onGet(): static { - if ($alsoMark) { - $this->mark->mark('csrf-token-issued'); - } - $this->headers[Header::CACHE_CONTROL] = 'private'; $this->body = ['token' => 'value']; diff --git a/tests/Fake/fake-app/src/Resource/App/HttpCacheControlWithCacheable.php b/tests/Fake/fake-app/src/Resource/App/HttpCacheControlWithCacheable.php index 10ee4e0e..fcdfd6f5 100644 --- a/tests/Fake/fake-app/src/Resource/App/HttpCacheControlWithCacheable.php +++ b/tests/Fake/fake-app/src/Resource/App/HttpCacheControlWithCacheable.php @@ -13,8 +13,12 @@ #[Cacheable(expirySecond: 10), HttpCache(isPrivate: true)] class HttpCacheControlWithCacheable extends ResourceObject { + public static int $calls = 0; + public function onGet() : ResourceObject { + self::$calls++; + return $this; } } diff --git a/tests/Fake/fake-app/src/Resource/App/QueryDonutHole.php b/tests/Fake/fake-app/src/Resource/App/QueryDonutHole.php deleted file mode 100644 index b7adf990..00000000 --- a/tests/Fake/fake-app/src/Resource/App/QueryDonutHole.php +++ /dev/null @@ -1,34 +0,0 @@ -mark->mark('query-donut-hole-marked'); - } - - $this->body = ['token' => $token]; - - return $this; - } -} diff --git a/tests/Fake/fake-app/src/Resource/App/QueryHole.php b/tests/Fake/fake-app/src/Resource/App/QueryHole.php index 7028355d..33f2017a 100644 --- a/tests/Fake/fake-app/src/Resource/App/QueryHole.php +++ b/tests/Fake/fake-app/src/Resource/App/QueryHole.php @@ -4,30 +4,20 @@ namespace FakeVendor\HelloWorld\Resource\App; -use BEAR\QueryRepository\UncacheableMarkInterface; use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\Resource\ResourceObject; /** * A #[Cacheable] hole whose own identity depends on a query parameter carried in its - * `#[Embed]` src (issue #222 regression test): the donut placeholder and view-map URI must - * carry that query string, or a later ResourceDonut::refresh() re-fetches this hole's - * query-less default instead of the value a composing page actually embedded. + * `#[Embed]` src: the donut placeholder and view-map URI must carry that query string, or a + * later ResourceDonut::refresh() re-fetches this hole's query-less default instead of the + * value a composing page actually embedded. */ #[Cacheable(expirySecond: 60)] class QueryHole extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet(string $token = '', string $visibility = 'public') : ResourceObject + public function onGet(string $token = ''): ResourceObject { - if ($visibility === 'private') { - $this->mark->mark('query-hole-marked'); - } - $this->body = ['token' => $token]; return $this; diff --git a/tests/Fake/fake-app/src/Resource/App/ThrowingHole.php b/tests/Fake/fake-app/src/Resource/App/ThrowingHole.php index 67d2d965..46bd4b77 100644 --- a/tests/Fake/fake-app/src/Resource/App/ThrowingHole.php +++ b/tests/Fake/fake-app/src/Resource/App/ThrowingHole.php @@ -4,26 +4,17 @@ namespace FakeVendor\HelloWorld\Resource\App; -use BEAR\QueryRepository\UncacheableMarkInterface; use BEAR\Resource\ResourceObject; use RuntimeException; /** - * Stands in for a hole that marks itself and then fails mid-render (issue #222 stack-balance - * test): its own mark must be attributed to its own scope, and that scope must be left even - * though the hole never returns normally, or a later mark would be misattributed to it. + * Stands in for a hole that fails mid-render: the failure must travel to the caller + * uncaught, the same way a page with no cache attribute at all would fail. */ class ThrowingHole extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet() : ResourceObject + public function onGet(): ResourceObject { - $this->mark->mark('csrf-token-issued'); - throw new RuntimeException('the hole blew up mid-render'); } } diff --git a/tests/Fake/fake-app/src/Resource/App/UnstorableValue.php b/tests/Fake/fake-app/src/Resource/App/UnstorableValue.php index ebe75f0f..020c4b41 100644 --- a/tests/Fake/fake-app/src/Resource/App/UnstorableValue.php +++ b/tests/Fake/fake-app/src/Resource/App/UnstorableValue.php @@ -4,25 +4,21 @@ namespace FakeVendor\HelloWorld\Resource\App; -use BEAR\QueryRepository\UncacheableMarkInterface; +use BEAR\QueryRepository\Header; use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\Resource\ResourceObject; /** - * Stands in for a #[Cacheable] resource that issues a per-user value (a CSRF token) - issue #222 + * Stands in for a #[Cacheable] resource that issues a per-user value (a CSRF token) by + * setting its own Cache-Control to private at runtime */ #[Cacheable(expirySecond: 60)] class UnstorableValue extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet(bool $unstorable = true) : ResourceObject + public function onGet(bool $unstorable = true): ResourceObject { if ($unstorable) { - $this->mark->mark('csrf-token-issued'); + $this->headers[Header::CACHE_CONTROL] = 'private'; } $this->body = ['token' => 'secret']; diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php b/tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php index b64dfad9..759eae83 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/BlogPostingCacheControl.php @@ -12,6 +12,8 @@ #[CacheableResponse] class BlogPostingCacheControl extends ResourceObject { + public static int $calls = 0; + public $headers = [ RequestHeader::CACHE_CONTROL => CacheControl::NO_STORE ]; @@ -26,6 +28,7 @@ public function __construct( #[Embed(rel: "comment", src: "page://self/html/comment")] public function onGet() { + self::$calls++; $this->body['article'] = '1'; $this->repository->putStatic($this, 100, 10); diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php deleted file mode 100644 index 20a36027..00000000 --- a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.php +++ /dev/null @@ -1,28 +0,0 @@ -body['article'] = '1'; - - return $this; - } -} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithNestedDonutCacheHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithNestedDonutCacheHole.php deleted file mode 100644 index 39172a21..00000000 --- a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithNestedDonutCacheHole.php +++ /dev/null @@ -1,27 +0,0 @@ -body['article'] = '1'; - - return $this; - } -} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithUnstorableHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithUnstorableHole.php deleted file mode 100644 index a56b39be..00000000 --- a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithUnstorableHole.php +++ /dev/null @@ -1,25 +0,0 @@ -body['article'] = '1'; - - return $this; - } -} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php b/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php index 751123cb..bf22ee51 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/MutableComment.php @@ -5,7 +5,6 @@ namespace FakeVendor\HelloWorld\Resource\Page\Html; use BEAR\QueryRepository\Header; -use BEAR\QueryRepository\UncacheableMarkInterface; use BEAR\RepositoryModule\Annotation\Cacheable; use BEAR\Resource\ResourceObject; @@ -13,20 +12,9 @@ class MutableComment extends ResourceObject { public static string $comment = 'comment-a'; - /** Marks this recomposition unstorable, the way a CSRF token issuer would mid-render */ - public static bool $markUnstorable = false; - - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } public function onGet(): static { - if (self::$markUnstorable) { - $this->mark->mark('csrf-token-issued'); - } - $this->body = ['comment' => self::$comment]; $this->headers[Header::SURROGATE_KEY] = 'mutable-comment'; diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/RenderTimeMark.php b/tests/Fake/fake-app/src/Resource/Page/Html/RenderTimeMark.php deleted file mode 100644 index 448e49c1..00000000 --- a/tests/Fake/fake-app/src/Resource/Page/Html/RenderTimeMark.php +++ /dev/null @@ -1,34 +0,0 @@ -body['token'] = new RenderTimeToken($this->mark); - - return $this; - } -} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonut.php b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonut.php index 1c38dad2..bf2cc7da 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonut.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonut.php @@ -4,25 +4,21 @@ namespace FakeVendor\HelloWorld\Resource\Page\Html; -use BEAR\QueryRepository\UncacheableMarkInterface; +use BEAR\QueryRepository\Header; use BEAR\RepositoryModule\Annotation\CacheableResponse; use BEAR\Resource\ResourceObject; /** - * Stands in for a #[CacheableResponse] page that issues a per-user value (a CSRF token) - issue #222 + * Stands in for a #[CacheableResponse] page that issues a per-user value (a CSRF token) by + * setting its own Cache-Control to private at runtime */ #[CacheableResponse] class UnstorableDonut extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet(bool $unstorable = true) : ResourceObject + public function onGet(bool $unstorable = true): ResourceObject { if ($unstorable) { - $this->mark->mark('csrf-token-issued'); + $this->headers[Header::CACHE_CONTROL] = 'private'; } $this->body['token'] = 'secret'; diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonutCacheTemplate.php b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonutCacheTemplate.php index 9eed0b34..5b7a7ba2 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonutCacheTemplate.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableDonutCacheTemplate.php @@ -4,27 +4,22 @@ namespace FakeVendor\HelloWorld\Resource\Page\Html; -use BEAR\QueryRepository\UncacheableMarkInterface; +use BEAR\QueryRepository\Header; use BEAR\RepositoryModule\Annotation\DonutCache; use BEAR\Resource\ResourceObject; /** * A #[DonutCache] page that writes a per-user value straight into its own body/view, outside - * any embedded hole: the "template holds only placeholders" assumption does not cover this, so - * the mark must still stop the template write (issue #222). + * any embedded hole: the "template holds only placeholders" assumption does not cover this, + * so a runtime-private Cache-Control must still stop the template write. */ #[DonutCache] class UnstorableDonutCacheTemplate extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet(bool $unstorable = true) : ResourceObject + public function onGet(bool $unstorable = true): ResourceObject { if ($unstorable) { - $this->mark->mark('csrf-token-issued'); + $this->headers[Header::CACHE_CONTROL] = 'private'; } $this->body['token'] = 'secret'; diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableWithManualCdnHeader.php b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableWithManualCdnHeader.php index e347ff53..bfa5580d 100644 --- a/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableWithManualCdnHeader.php +++ b/tests/Fake/fake-app/src/Resource/Page/Html/UnstorableWithManualCdnHeader.php @@ -4,25 +4,20 @@ namespace FakeVendor\HelloWorld\Resource\Page\Html; -use BEAR\QueryRepository\UncacheableMarkInterface; +use BEAR\QueryRepository\Header; use BEAR\RepositoryModule\Annotation\CacheableResponse; use BEAR\Resource\ResourceObject; /** * Sets CDN-facing headers directly in onGet(), bypassing CdnCacheControlHeaderSetterInterface, - * and marks itself unstorable: those headers must not survive the refusal (issue #222). + * and sets its own Cache-Control to private: those headers must not survive the refusal. */ #[CacheableResponse] class UnstorableWithManualCdnHeader extends ResourceObject { - public function __construct( - private readonly UncacheableMarkInterface $mark, - ) { - } - - public function onGet() : ResourceObject + public function onGet(): ResourceObject { - $this->mark->mark('csrf-token-issued'); + $this->headers[Header::CACHE_CONTROL] = 'private'; $this->headers['CDN-Cache-Control'] = 'max-age=600'; $this->headers['Surrogate-Control'] = 'max-age=600'; $this->body['article'] = '1'; diff --git a/tests/Fake/fake-app/var/templates/App/QueryDonutHole.html.twig b/tests/Fake/fake-app/var/templates/App/QueryDonutHole.html.twig deleted file mode 100644 index 0e9202c9..00000000 --- a/tests/Fake/fake-app/var/templates/App/QueryDonutHole.html.twig +++ /dev/null @@ -1 +0,0 @@ -{{ token }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig deleted file mode 100644 index 9faf9029..00000000 --- a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithMarkingAndNestedDonutCacheHole.html.twig +++ /dev/null @@ -1 +0,0 @@ -marking-and-nested:{{ article }}{{ secret }}{{ plain }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithNestedDonutCacheHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithNestedDonutCacheHole.html.twig deleted file mode 100644 index 9e488e71..00000000 --- a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithNestedDonutCacheHole.html.twig +++ /dev/null @@ -1 +0,0 @@ -nested-parent:{{ article }}{{ nested }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithUnstorableHole.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithUnstorableHole.html.twig deleted file mode 100644 index 7bd73c39..00000000 --- a/tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithUnstorableHole.html.twig +++ /dev/null @@ -1 +0,0 @@ -donut-cache-with-unstorable-hole:{{ article }}{{ secret }} \ No newline at end of file diff --git a/tests/Fake/fake-app/var/templates/Page/Html/RenderTimeMark.html.twig b/tests/Fake/fake-app/var/templates/Page/Html/RenderTimeMark.html.twig deleted file mode 100644 index 10182e77..00000000 --- a/tests/Fake/fake-app/var/templates/Page/Html/RenderTimeMark.html.twig +++ /dev/null @@ -1 +0,0 @@ -render-time-mark:{{ token }} \ No newline at end of file diff --git a/tests/GetInterceptorTest.php b/tests/GetInterceptorTest.php index d5038a5f..888c3efe 100644 --- a/tests/GetInterceptorTest.php +++ b/tests/GetInterceptorTest.php @@ -6,6 +6,7 @@ use BEAR\QueryRepository\Exception\ExpireAtKeyNotExists; use BEAR\Resource\ResourceInterface; +use FakeVendor\HelloWorld\Resource\App\HttpCacheControlWithCacheable; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; @@ -68,8 +69,14 @@ public function testNoHttpCacheAnnotation(): void public function testHttpCacheWithCacheble(): void { + HttpCacheControlWithCacheable::$calls = 0; $ro = $this->resource->get('app://self/http-cache-control-with-cacheable'); $this->assertSame('private, max-age=10', $ro->headers['Cache-Control']); + + // #[HttpCache(isPrivate: true)]'s own declared value is exempt from the runtime-private + // check: a second GET is served from the server-side store, not a fresh onGet() call. + $this->resource->get('app://self/http-cache-control-with-cacheable'); + $this->assertSame(1, HttpCacheControlWithCacheable::$calls, 'the server-side store must still hold this declared-private value'); } public function testHttpCacheOverrideMaxAge(): void diff --git a/tests/HandSetPrivateCacheControlTest.php b/tests/HandSetPrivateCacheControlTest.php index 116ebac2..b18e9fcd 100644 --- a/tests/HandSetPrivateCacheControlTest.php +++ b/tests/HandSetPrivateCacheControlTest.php @@ -5,14 +5,14 @@ namespace BEAR\QueryRepository; use BEAR\Resource\ResourceInterface; +use FakeVendor\HelloWorld\Resource\Page\Html\BlogPostingCacheControl; use PHPUnit\Framework\TestCase; use Ray\Di\Injector; /** - * Owner-approved regression case for issue #222: a #[Cacheable] response whose OWN code sets - * Cache-Control to `private` directly (bypassing #[HttpCache]) is still refused the - * server-side store, but keeps the developer's header - only a mark() forces the package's - * own `private, no-store` + `Pragma: no-cache` onto it. HttpCacheControlWithCacheable + * Owner-approved regression case: a #[Cacheable] response whose OWN code sets Cache-Control + * to `private` directly (bypassing #[HttpCache]) is still refused the server-side store, but + * keeps the developer's header. HttpCacheControlWithCacheable * (GetInterceptorTest::testHttpCacheWithCacheble) already covers the #[HttpCache]-declared * exemption: that value IS stored. */ @@ -44,18 +44,6 @@ public function testHandSetPrivateCacheControlIsNotStoredButKeepsItsOwnHeader(): $this->assertArrayNotHasKey('Age', $second->headers, 'a cache hit would carry Age; onGet() ran again instead'); } - public function testHandSetPrivateCacheControlPlusAMarkGetsThePackagesOwnHeaders(): void - { - // A producer also calling mark() (e.g. the same request issuing a CSRF token - // elsewhere) still gets the package's own refusal headers, unchanged from before - // this fix: the hand-set header alone is the owner-approved exception, a mark is not. - $ro = $this->resource->get('app://self/hand-set-private-cache-control', ['alsoMark' => true]); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - $this->assertSame('no-cache', $ro->headers[Header::PRAGMA]); - $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); - $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $ro->headers); - } - public function testHandSetNoStoreOnADonutResponseKeepsItsHeaderAndStripsCdnHeaders(): void { // BlogPostingCacheControl (#[CacheableResponse]) hand-sets Cache-Control: no-store @@ -63,9 +51,15 @@ public function testHandSetNoStoreOnADonutResponseKeepsItsHeaderAndStripsCdnHead // the #[Cacheable] case above. DonutRepository::setHeaders() sets Surrogate-Key // unconditionally before HeaderSetter runs, so this is not a vacuous check: it // proves the CDN-facing headers are actually stripped, not merely absent. + BlogPostingCacheControl::$calls = 0; $ro = $this->resource->get('page://self/html/blog-posting-cache-control'); $this->assertSame('no-store', $ro->headers[Header::CACHE_CONTROL], "the developer's own header is kept exactly"); $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); $this->assertArrayNotHasKey(Header::SURROGATE_KEY, $ro->headers); + + // Not stored server-side: a second GET re-runs onGet() instead of recomposing + // from a saved donut. + $this->resource->get('page://self/html/blog-posting-cache-control'); + $this->assertSame(2, BlogPostingCacheControl::$calls, 'a stored donut would answer the second GET without running onGet() again'); } } diff --git a/tests/HeaderSetterTest.php b/tests/HeaderSetterTest.php index 4abb90cd..1f9ec011 100644 --- a/tests/HeaderSetterTest.php +++ b/tests/HeaderSetterTest.php @@ -9,9 +9,9 @@ class HeaderSetterTest extends TestCase { - private function headerSetter(UncacheableMarkInterface $mark): HeaderSetter + private function headerSetter(): HeaderSetter { - return new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark); + return new HeaderSetter(new EtagSetter()); } public function testStripHeadersMatchesALowercaseCdnCacheControlHeaderName(): void @@ -19,15 +19,14 @@ public function testStripHeadersMatchesALowercaseCdnCacheControlHeaderName(): vo // $ro->headers is a plain PHP array, not an HTTP-aware collection (RFC 7230 §3.2): // an application that wrote 'cdn-cache-control' (lowercase) must not survive an // unstorable refusal just because it does not match the exact canonical casing. - $mark = new UncacheableMark(); - $mark->mark('csrf-token-issued'); $ro = new None(); + $ro->headers[Header::CACHE_CONTROL] = 'private'; $ro->headers['cdn-cache-control'] = 'max-age=600'; - ($this->headerSetter($mark))($ro, null, null); + ($this->headerSetter())($ro, null, null); $this->assertArrayNotHasKey('cdn-cache-control', $ro->headers); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL], "the developer's own header is kept exactly"); } public function testStripHeadersRemovesEveryCasingWhenTheSameHeaderIsSetTwice(): void @@ -36,13 +35,12 @@ public function testStripHeadersRemovesEveryCasingWhenTheSameHeaderIsSetTwice(): // as two distinct keys. stripHeaders() must remove ALL of them, not just the first // match - a single-match lookup here would leave one behind, a lifetime header // surviving the unstorable refusal. - $mark = new UncacheableMark(); - $mark->mark('csrf-token-issued'); $ro = new None(); + $ro->headers[Header::CACHE_CONTROL] = 'private'; $ro->headers[Header::CDN_CACHE_CONTROL] = 'max-age=600'; $ro->headers['cdn-cache-control'] = 'max-age=600'; - ($this->headerSetter($mark))($ro, null, null); + ($this->headerSetter())($ro, null, null); $this->assertArrayNotHasKey(Header::CDN_CACHE_CONTROL, $ro->headers); $this->assertArrayNotHasKey('cdn-cache-control', $ro->headers); @@ -53,11 +51,10 @@ public function testSetCacheControlMaxAgeFindsAnExistingLowercaseCacheControlHea // An app-set 'cache-control' (lowercase) must be recognized as the SAME header an // exact-key isset() would miss - otherwise this setter adds a second, canonically- // cased Cache-Control header alongside it instead of appending to the existing one. - $mark = new UncacheableMark(); $ro = new None(); $ro->headers['cache-control'] = 'public'; - ($this->headerSetter($mark))($ro, 60, null); + ($this->headerSetter())($ro, 60, null); $this->assertArrayNotHasKey(Header::CACHE_CONTROL, $ro->headers, 'no second, canonically-cased header was added'); $this->assertSame('public, max-age=60', $ro->headers['cache-control']); @@ -65,11 +62,10 @@ public function testSetCacheControlMaxAgeFindsAnExistingLowercaseCacheControlHea public function testSetCacheControlMaxAgeDoesNotDuplicateAnExistingLowercaseMaxAge(): void { - $mark = new UncacheableMark(); $ro = new None(); $ro->headers['cache-control'] = 'max-age=30'; - ($this->headerSetter($mark))($ro, 60, null); + ($this->headerSetter())($ro, 60, null); $this->assertArrayNotHasKey(Header::CACHE_CONTROL, $ro->headers, 'no second, canonically-cased header was added'); $this->assertSame('max-age=30', $ro->headers['cache-control']); diff --git a/tests/QueryRepositoryTest.php b/tests/QueryRepositoryTest.php index af7ac9ba..4df7c5b4 100644 --- a/tests/QueryRepositoryTest.php +++ b/tests/QueryRepositoryTest.php @@ -281,25 +281,21 @@ protected function configure(): void $this->assertLessThanOrEqual(FakeLegacyStateStorage::RESIDENCE + 1, $age); } - public function testUnstorableMarkSkipsStoreAndOrdinaryResponsesStillStore(): void + public function testRuntimePrivateResponseSkipsStoreAndOrdinaryResponsesStillStore(): void { - // Ordinary (unmarked) responses store and hit normally - checked first, since the - // mark is per-request state (here: one shared container for the whole test) that - // cannot be unset once a later call sets it. + // Ordinary (no runtime Cache-Control) responses store and hit normally. $ordinary = $this->resource->get('app://self/unstorable-value', ['unstorable' => false]); - $this->assertArrayNotHasKey(Header::PRAGMA, $ordinary->headers); - $this->assertNotSame('private, no-store', $ordinary->headers[Header::CACHE_CONTROL]); + $this->assertNotSame('private', $ordinary->headers[Header::CACHE_CONTROL] ?? null); $this->logger->flush(); $this->resource->get('app://self/unstorable-value', ['unstorable' => false]); $treeOrdinary = $this->flushAndValidate($this->logger); $this->assertContains('cache_hit', self::collectTypes($treeOrdinary)); - // A producer (a CSRF token issuer) marks this request's response unstorable: the - // write is skipped, the live response is still served, and its headers tell every - // cache - shared or private - not to keep it (issue #222). + // The resource sets its own Cache-Control to private at runtime (e.g. a CSRF + // token issuer): the write is skipped, the live response is still served, and its + // own header is kept exactly, not overwritten. $ro = $this->resource->get('app://self/unstorable-value'); - $this->assertSame('private, no-store', $ro->headers[Header::CACHE_CONTROL]); - $this->assertSame('no-cache', $ro->headers[Header::PRAGMA]); + $this->assertSame('private', $ro->headers[Header::CACHE_CONTROL]); $this->assertArrayNotHasKey(Header::ETAG, $ro->headers); $tree = $this->flushAndValidate($this->logger); @@ -307,7 +303,7 @@ public function testUnstorableMarkSkipsStoreAndOrdinaryResponsesStillStore(): vo $this->assertNotNull($putSkipped); $this->assertStringContainsString('"reason":"unstorable"', $putSkipped); - // Nothing was stored: a second GET of the same (still marked) request misses again. + // Nothing was stored: a second GET of the same request misses again. $this->logger->flush(); $this->resource->get('app://self/unstorable-value'); $tree2 = $this->flushAndValidate($this->logger); @@ -319,11 +315,9 @@ public function testCacheableWithNoHttpCacheStillStores(): void { // #[NoHttpCache] declares its own Cache-Control (private, no-store, no-cache, // must-revalidate) - a header that equals the declared value is the attribute - // speaking, not a per-request value, and must not trip the gate (issue #222 - // regression guard: 1.x stored this). + // speaking, not a per-request value, and must not trip the unstorable check. $ro = $this->resource->get('app://self/cacheable-no-http-cache'); $this->assertStringContainsString('max-age=10', $ro->headers[Header::CACHE_CONTROL]); - $this->assertArrayNotHasKey(Header::PRAGMA, $ro->headers, 'the gate must not override #[NoHttpCache]\'s own headers'); $this->logger->flush(); $this->resource->get('app://self/cacheable-no-http-cache'); diff --git a/tests/ResourceDonutTest.php b/tests/ResourceDonutTest.php index 98340c57..844ee74a 100644 --- a/tests/ResourceDonutTest.php +++ b/tests/ResourceDonutTest.php @@ -31,7 +31,6 @@ class ResourceDonutTest extends TestCase { private ResourceInterface $resource; - private UncacheableMarkInterface $mark; protected function setUp(): void { @@ -40,7 +39,6 @@ protected function setUp(): void $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); $injector = new Injector($module, __DIR__ . '/tmp'); $this->resource = $injector->getInstance(ResourceInterface::class); - $this->mark = new UncacheableMark(); parent::setUp(); } @@ -50,7 +48,7 @@ public function testRefreshResolvesTwoPlaceholdersOnTheSameLine(): void $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut('

[le:page://self/html/comment][le:page://self/html/like]

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro, $this->mark); + $refreshed = $donut->refresh($this->resource, $ro); $this->assertStringContainsString('comment01', (string) $refreshed->view); $this->assertStringContainsString('like0', (string) $refreshed->view); @@ -62,7 +60,7 @@ public function testRefreshResolvesPlaceholdersOnSeparateLines(): void $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut("

[le:page://self/html/comment]

\n

[le:page://self/html/like]

", [], null, false); - $refreshed = $donut->refresh($this->resource, $ro, $this->mark); + $refreshed = $donut->refresh($this->resource, $ro); $this->assertStringContainsString('comment01', (string) $refreshed->view); $this->assertStringContainsString('like0', (string) $refreshed->view); @@ -107,7 +105,7 @@ public function testRefreshDoesNotDoubleDecodeALegacyPlaceholdersQueryValue(stri $query = http_build_query(['token' => $value]); $donut = new ResourceDonut('

[le:app://self/query-hole?' . $query . ']

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro, $this->mark); + $refreshed = $donut->refresh($this->resource, $ro); $this->assertSame('

' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '

', (string) $refreshed->view, 'the legacy placeholder\'s URI must reach BEAR\Resource\Uri exactly as stored, not pre-decoded'); } @@ -123,7 +121,7 @@ public function testRefreshRoundTripsTheNewFormPlaceholdersQueryValue(string $va $placeholder = ResourceDonut::placeholder('app://self/query-hole?' . http_build_query(['token' => $value])); $donut = new ResourceDonut('

' . $placeholder . '

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro, $this->mark); + $refreshed = $donut->refresh($this->resource, $ro); $this->assertSame('

' . htmlspecialchars($value, ENT_QUOTES, 'UTF-8') . '

', (string) $refreshed->view); } @@ -143,7 +141,7 @@ public function testRefreshOfALegacyDuplicateKeyPlaceholderStaysExactlyAsWrongAs $ro = $this->resource->newInstance('page://self/html/blog-posting-donut'); $donut = new ResourceDonut('

[le:app://self/query-hole?token=a&token=b]

', [], null, false); - $refreshed = $donut->refresh($this->resource, $ro, $this->mark); + $refreshed = $donut->refresh($this->resource, $ro); $this->assertSame('

a

', (string) $refreshed->view, 'the first token "wins" exactly as it already did - not "b", and not an exception'); } @@ -152,10 +150,9 @@ public function testRefreshOfALegacyDuplicateKeyPlaceholderStaysExactlyAsWrongAs * A legacy `[le:URI]` placeholder for a hole with two REQUIRED query params, corrupted * the same way, fails the re-fetch with the same underlying cause as a pre-#232-fix * refresh() threw - still a ParameterException, not this fix's rawurldecode() turning it - * into a different, new failure. Unlike before this PR, that cause no longer reaches the - * caller raw: it now arrives wrapped in a CacheStoreFailure, the same conversion any - * re-fetch failure gets, so `AbstractDonutCacheInterceptor`'s read path degrades to a - * fresh render instead of a 500 - an intentional behavior change (see CHANGELOG). + * into a different, new failure. That cause arrives wrapped in a CacheStoreFailure, the + * same conversion any re-fetch failure gets, so `AbstractDonutCacheInterceptor`'s read + * path degrades to a fresh render instead of a 500. */ public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBeforeThisFix(): void { @@ -163,7 +160,7 @@ public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBefore $donut = new ResourceDonut('

[le:app://self/multi-param-query-hole?id=1&tag=2]

', [], null, false); try { - $donut->refresh($this->resource, $ro, $this->mark); + $donut->refresh($this->resource, $ro); $this->fail('expected the malformed legacy placeholder to fail the re-fetch'); } catch (CacheStoreFailure $e) { $this->assertInstanceOf(ParameterException::class, $e->getPrevious()); @@ -171,11 +168,9 @@ public function testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBefore } /** - * A re-fetch that throws must still leave the hole's scope closed, the same way - * `finally` guarantees `leaveHole()` runs: an exception from `$resource->get($uri)` - * (a pool outage, a broken onGet()) must not leave `$uri` stuck on the mark's owner - * stack for the rest of the request - which would misattribute a later, unrelated - * mark() to this hole's scope instead of TOP_LEVEL. + * A re-fetch failure is wrapped into CacheStoreFailure (a RuntimeException subtype), + * so a plain pool outage or a broken onGet() degrades the refresh instead of leaving a + * raw, uncategorized exception for the interceptor's read path to handle. */ public function testRefreshLeavesTheHoleScopeEvenWhenTheReFetchThrows(): void { @@ -250,24 +245,11 @@ public function options(string $uri, array $query = []): ResourceObject }; try { - $donut->refresh($throwingResource, $ro, $this->mark); + $donut->refresh($throwingResource, $ro); $this->fail('expected the re-fetch to throw'); } catch (RuntimeException $e) { $this->assertSame('pool outage', $e->getMessage()); } - - // A balanced stack would make this leaveHole() a no-op (the uri is not on it), so - // the mark() just above stays filed under TOP_LEVEL and is still visible here. A - // leaked entry instead puts $uri back on top: mark() files under $uri, leaveHole() - // pops exactly that entry, and the current scope reverts to TOP_LEVEL - which this - // mark() never touched - making the check below read false instead. - $this->mark->mark('after the throw'); - $this->mark->leaveHole($uri); - - $this->assertTrue( - $this->mark->isMarkedForCurrentScope(), - 'a mark right after the aborted re-fetch must stay filed under TOP_LEVEL, not get displaced by a scope the throw left stuck on the stack', - ); } /** @@ -287,7 +269,7 @@ public function testCreateLetsAHoleThrowTravelAndStoresNothing(): void $ro->body = ['secret' => $throwingRequest]; try { - ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $this->mark); + ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false); $this->fail('expected the hole throw to travel'); } catch (TwigError $e) { $this->assertInstanceOf(RuntimeException::class, $e->getPrevious()); @@ -312,47 +294,13 @@ public function testCreateLetsAHolesOwnRenderFailureTravelAndStoresNothing(): vo $ro->body = ['secret' => $holeRequest]; try { - ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $this->mark); + ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false); $this->fail("expected the hole's own render failure to travel"); } catch (TwigError $e) { $this->assertStringContainsString('HoleWithNoTemplate', $e->getMessage()); } } - /** - * `app://self/throwing-hole` marks itself unstorable, THEN throws (Kimi LOW: document - * which surfaces). The render exception wins, not a dev-gate exception - * (`UnstorableResponseDetected`): the mark only ever affects a SUCCESSFUL render's - * storability, checked by `UnstorableGateInterface::reason()` after `create()` returns - - * which it never does here, so nothing ever reads the mark at all. The scope the mark - * was filed under is still left correctly despite the throw (DonutRequest::__toString()'s - * own finally), confirmed the same way DonutRequestTest's own throw test does. - */ - public function testCreateLetsAHoleThatMarkedItselfThrowTravelAsTheRenderException(): void - { - $ro = $this->resource->newInstance('page://self/html/donut-cache-with-throwing-hole'); - $resource = $this->resource; - assert($resource instanceof Resource); - $throwingRequest = $resource->get->uri('app://self/throwing-hole'); - assert($throwingRequest instanceof AbstractRequest); - $ro->body = ['secret' => $throwingRequest]; - - try { - ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $this->mark); - $this->fail('expected the render exception to travel, not any dev-gate exception'); - } catch (TwigError $e) { - // Not UnstorableResponseDetected or any other gate-related exception: the mark - // ThrowingHole made before throwing is simply never read. - $this->assertInstanceOf(RuntimeException::class, $e->getPrevious()); - $this->assertSame('the hole blew up mid-render', $e->getPrevious()->getMessage()); - } - - // The hole's own scope was left (DonutRequest::__toString()'s finally), so a mark - // right after is filed under TOP_LEVEL, not displaced by a leaked hole scope. - $this->mark->mark('after the throw'); - $this->assertTrue($this->mark->isMarkedForCurrentScope()); - } - /** * An anonymous exception class (`new class extends RuntimeException {}`) must travel * exactly like a named one - Kimi's MEDIUM: a class-name-matching guard would silently @@ -369,7 +317,7 @@ public function testCreateLetsAnAnonymousExceptionHoleThrowTravelAndStoresNothin $ro->body = ['secret' => $throwingRequest]; try { - ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $this->mark); + ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false); $this->fail('expected the anonymous-class hole throw to travel'); } catch (TwigError $e) { $this->assertStringContainsString('anonymous hole blew up', $e->getPrevious()?->getMessage() ?? ''); @@ -391,12 +339,11 @@ protected function configure(): void }); $injector = new Injector($module, __DIR__ . '/tmp'); $resource = $injector->getInstance(ResourceInterface::class); - $mark = $injector->getInstance(UncacheableMarkInterface::class); $ro = $resource->newInstance('page://self/html/plain-donut-hole'); $ro->body = ['value' => 'plain']; try { - ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $mark); + ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false); $this->fail('expected the page template throw to travel'); } catch (FakeTemplateNotFound $e) { $this->assertSame('template not found: user.html.twig', $e->getMessage()); @@ -413,7 +360,7 @@ public function testCreateStillStoresTheTemplateWhenNothingThrows(): void $ro = $this->resource->newInstance('page://self/html/plain-donut-hole'); $ro->body = ['value' => 'plain']; - $donut = ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false, $this->mark); + $donut = ResourceDonut::create($ro, new DonutRenderer(), new SurrogateKeys($ro->uri), null, false); $rendered = $donut->render($ro, new DonutRenderer()); $this->assertStringContainsString('plain-donut-hole:plain', (string) $rendered->view); @@ -422,10 +369,10 @@ public function testCreateStillStoresTheTemplateWhenNothingThrows(): void /** * refresh()'s per-hole `$ro->toString()` (and the `$resource->get($uri)` re-fetch that * runs the hole's onGet()) does not swallow a Throwable either - it propagates directly, - * the same non-swallowing render create() itself now uses. Unlike create(), which lets - * it travel unwrapped, this is a plain try/catch converting it to CacheStoreFailure - * deliberately: a HIT's re-fetch degrades through the interceptor's read path instead, - * falling back to a full fresh render (the reviewed design both reviewers said to keep). + * the same non-swallowing render create() itself uses. Unlike create(), which lets it + * travel unwrapped, this is a plain try/catch converting it to CacheStoreFailure: a + * HIT's re-fetch degrades through the interceptor's read path instead, falling back to + * a full fresh render. */ public function testRefreshRefusesToServeWhenAHoleThrows(): void { @@ -434,20 +381,10 @@ public function testRefreshRefusesToServeWhenAHoleThrows(): void $donut = new ResourceDonut('

' . ResourceDonut::placeholder($uri) . '

', [], null, false); try { - $donut->refresh($this->resource, $ro, $this->mark); + $donut->refresh($this->resource, $ro); $this->fail('expected the hole throw to be refused as a CacheStoreFailure'); } catch (CacheStoreFailure $e) { $this->assertStringContainsString('the hole blew up mid-render', $e->getMessage()); } - - // Same stack-balance check as testRefreshLeavesTheHoleScopeEvenWhenTheReFetchThrows: - // the catch must not skip the finally that leaves the hole's scope. - $this->mark->mark('after the throw'); - $this->mark->leaveHole($uri); - - $this->assertTrue( - $this->mark->isMarkedForCurrentScope(), - 'a mark right after the refused re-fetch must stay filed under TOP_LEVEL, not get displaced by a scope the throw left stuck on the stack', - ); } } diff --git a/tests/ResourceRepositoryTest.php b/tests/ResourceRepositoryTest.php index 02cda4cf..35c44e9a 100644 --- a/tests/ResourceRepositoryTest.php +++ b/tests/ResourceRepositoryTest.php @@ -37,10 +37,9 @@ public function get() return $this->tagAwareAdapter; } }; - $mark = new UncacheableMark(); $this->repository = new Repository( new NullSemanticLogger(), - new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark), + new HeaderSetter(new EtagSetter()), new ResourceStorage( new NullSemanticLogger(), new NullPurger(), @@ -52,7 +51,6 @@ public function get() ), new Expiry(0, 0, 0), new CacheDependency(new UriTag()), - new UnstorableGate($mark, new DeclaredCacheControl()), ); $this->ro = new Index(); $this->ro->uri = new Uri('page://self/user'); @@ -121,10 +119,9 @@ public function get() return $this->tagAwareAdapter; } }; - $mark = new UncacheableMark(); $repository = new Repository( new NullSemanticLogger(), - new HeaderSetter(new EtagSetter(), new UnstorableGate($mark, new DeclaredCacheControl()), $mark), + new HeaderSetter(new EtagSetter()), new ResourceStorage( new NullSemanticLogger(), new NullPurger(), @@ -136,7 +133,6 @@ public function get() ), new Expiry(0, 0, 0), new CacheDependency(new UriTag()), - new UnstorableGate($mark, new DeclaredCacheControl()), ); $this->assertInstanceOf(Repository::class, $repository); } diff --git a/tests/ResourceStorageTest.php b/tests/ResourceStorageTest.php index dbf05849..4b8f08e5 100644 --- a/tests/ResourceStorageTest.php +++ b/tests/ResourceStorageTest.php @@ -74,7 +74,7 @@ protected function setUp(): void public function testSaveGetStatic(): void { - $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false, new UncacheableMark()); + $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false); $this->storage->saveDonut($this->ro->uri, $donut, null, []); $donut = $this->storage->getDonut($this->ro->uri); $this->assertInstanceOf(ResourceDonut::class, $donut); @@ -289,7 +289,7 @@ public function get() $poolProvider, $poolProvider, ); - $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false, new UncacheableMark()); + $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false); $storage->saveDonut($this->ro->uri, $donut, null, []); @@ -302,7 +302,7 @@ public function testEveryStoreClampsNegativeTtlToZero(): void { $logger = new RecordingSemanticLogger(); $storage = self::getResourceStorageInstance($logger); - $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false, new UncacheableMark()); + $donut = ResourceDonut::create($this->ro, new DonutRenderer(), new SurrogateKeys(new Uri('app://self/')), null, false); // The storage is a public interface: a caller handing it a lifetime that already // passed must not produce a negative ttl, which every save_* schema forbids diff --git a/tests/UnstorableGateTest.php b/tests/UnstorableGateTest.php deleted file mode 100644 index 9668f924..00000000 --- a/tests/UnstorableGateTest.php +++ /dev/null @@ -1,274 +0,0 @@ -assertNull($gate->reason($ro)); - } - - public function testMarkedResponseIsUnstorableRegardlessOfHeaders(): void - { - $mark = new UncacheableMark(); - $gate = new UnstorableGate($mark, new DeclaredCacheControl()); - $ro = new None(); - - $mark->mark('csrf-token-issued'); - - $this->assertSame('unstorable', $gate->reason($ro)); - } - - public function testCacheControlNoStoreIsUnstorable(): void - { - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new None(); - $ro->headers[Header::CACHE_CONTROL] = 'no-store'; - - $this->assertSame('unstorable', $gate->reason($ro)); - } - - public function testCacheControlPrivateIsUnstorable(): void - { - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new None(); - $ro->headers[Header::CACHE_CONTROL] = 'PRIVATE'; - - $this->assertSame('unstorable', $gate->reason($ro), 'RFC 9111 directives are case-insensitive'); - } - - public function testCacheControlHeaderNameIsMatchedCaseInsensitively(): void - { - // $ro->headers is a plain PHP array, not an HTTP-aware collection - 'cache-control' - // must be found the same way 'Cache-Control' is (RFC 7230 §3.2). - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new None(); - $ro->headers['cache-control'] = 'no-store'; - - $this->assertSame('unstorable', $gate->reason($ro)); - } - - public function testCacheControlMatchingTheDeclaredValueIsExempt(): void - { - // #[HttpCache(isPrivate: true)]'s own value is the attribute speaking, not the - // method - #[Cacheable] still stores server-side (issue #222). - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new HttpCacheControlWithCacheable(); - $ro->headers[Header::CACHE_CONTROL] = 'private'; - - $this->assertNull($gate->reason($ro)); - } - - public function testCacheControlDivergingFromTheDeclaredValueIsNotExempt(): void - { - // A method or render that adds its own no-store on top of (or instead of) the - // declared value still trips the gate. - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new HttpCacheControlWithCacheable(); - $ro->headers[Header::CACHE_CONTROL] = 'private, no-store'; - - $this->assertSame('unstorable', $gate->reason($ro)); - } - - public function testOrdinaryCacheControlIsStorable(): void - { - $gate = new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl()); - $ro = new None(); - $ro->headers[Header::CACHE_CONTROL] = 'max-age=60'; - - $this->assertNull($gate->reason($ro)); - } - - public function testReasonExcludingHoleMarksIgnoresAMarkMadeInsideAHoleScope(): void - { - $mark = new UncacheableMark(); - $gate = new UnstorableGate($mark, new DeclaredCacheControl()); - $ro = new None(); - - $mark->enterHole('page://self/html/some-hole'); - $mark->mark('csrf-token-issued'); - $mark->leaveHole('page://self/html/some-hole'); - - $this->assertNull($gate->reasonExcludingHoleMarks($ro), 'a hole-scoped mark must not trip the #[DonutCache] template gate'); - $this->assertSame('unstorable', $gate->reason($ro), 'the same mark still trips the gate for paths whose stored content includes hole output'); - } - - public function testReasonExcludingHoleMarksStillCatchesAMarkMadeOutsideAnyHole(): void - { - $mark = new UncacheableMark(); - $gate = new UnstorableGate($mark, new DeclaredCacheControl()); - $ro = new None(); - - $mark->mark('csrf-token-issued'); - - $this->assertSame('unstorable', $gate->reasonExcludingHoleMarks($ro)); - } - - public function testReasonExcludingHoleMarksCatchesANestedHolesOwnMarkForItself(): void - { - // A hole that is itself a #[DonutCache] resource runs its own write while still - // nested inside its embedder's enterHole() scope - its own mark, made while it is - // the current (innermost) scope, must still refuse its OWN template. - $mark = new UncacheableMark(); - $gate = new UnstorableGate($mark, new DeclaredCacheControl()); - $ro = new None(); - - $mark->enterHole('page://self/html/outer-hole'); - $mark->mark('csrf-token-issued'); // the nested hole's own render-time mark - $this->assertSame('unstorable', $gate->reasonExcludingHoleMarks($ro), 'the hole\'s own mark, made while it is the current scope, must refuse its own template'); - $mark->leaveHole('page://self/html/outer-hole'); - - $this->assertNull($gate->reasonExcludingHoleMarks($ro), 'back at the top level, the hole\'s mark (owned by the hole) must not refuse the composing page\'s own template'); - } - - public function testUnbalancedLeaveHoleDoesNotFailOpen(): void - { - // A caller that leaves a hole scope it never entered must not drive the stack - // negative and then silently stop attributing later marks to the page. - $mark = new UncacheableMark(); - $mark->leaveHole('page://self/html/never-entered'); - - $mark->mark('csrf-token-issued'); - - $this->assertTrue($mark->isMarkedForCurrentScope()); - } - - public function testAMismatchedLeaveHoleDoesNotPopAnActiveOuterScope(): void - { - // A stray leaveHole() call a caller makes without a matching enterHole() of its - // own, while nested inside a REAL, still-open outer scope, must not pop that outer - // scope out from under it - or a later mark in the outer scope's remaining - // execution is wrongly attributed to the page instead, reading as not marked where - // the outer resource's own write check expects it to (under-refusal). - $mark = new UncacheableMark(); - $mark->enterHole('page://self/html/outer-hole'); - $mark->leaveHole('page://self/html/some-other-uri-never-entered'); - - $mark->mark('csrf-token-issued'); - - // Properly leave the SAME scope now: if the mismatched call above had already - // (wrongly) popped it, this is a no-op and the mark - now sitting at top level - - // would still read as current here; if the mismatched call was correctly ignored, - // this pop succeeds and takes the mark (attributed to 'outer-hole', not top level) - // down with it, leaving top level unmarked. - $mark->leaveHole('page://self/html/outer-hole'); - - $this->assertFalse($mark->isMarkedForCurrentScope(), 'the mark must have been attributed to the outer scope, not leaked to top level by the earlier mismatched call'); - } - - public function testLeaveHoleSelfHealsALeakedInnerScope(): void - { - // The mirror failure of the mismatch case above: if code nested inside an - // enterHole()/leaveHole() pair pushes its OWN scope and never leaves it (a leak - - // third-party misuse again), the outer leaveHole() must still succeed, taking the - // leaked entry down with it - a strict "only pop if it is the exact top" check - // would instead get permanently stuck behind the leak for the rest of the request. - $mark = new UncacheableMark(); - $mark->enterHole('page://self/html/outer-hole'); - $mark->enterHole('page://self/html/leaked-inner-hole'); // never left - a leak - - $mark->leaveHole('page://self/html/outer-hole'); - $mark->mark('csrf-token-issued'); - - // Re-enter the LEAKED uri specifically: if the stack had not actually been - // unwound, the mark above landed there (still the current top) and this reads - // true; if it was correctly unwound, the mark landed at top level instead and - // this reads false. - $mark->enterHole('page://self/html/leaked-inner-hole'); - $isLeakedScopeMarked = $mark->isMarkedForCurrentScope(); - $mark->leaveHole('page://self/html/leaked-inner-hole'); - - $this->assertFalse($isLeakedScopeMarked, 'the leaked inner scope must not have captured a mark made after the outer scope was correctly (unwound) closed'); - } - - public function testStackStaysBalancedWhenAHoleScopeIsLeftAfterAnException(): void - { - // DonutRequest::__toString() wraps a hole's own execution in try/finally so - // leaveHole() still runs when the hole throws - simulated here directly on the - // mark service. The hole's own mark, made before it "throws", must not leak into - // the current (top-level) scope once that scope is correctly left - checking a - // fresh mark() made only afterward would be tautological: it and the immediately - // following isMarkedForCurrentScope() always resolve through the same - // currentOwner(), so they would agree regardless of whether the stack is balanced. - $mark = new UncacheableMark(); - $mark->enterHole('page://self/html/throwing-hole'); - $mark->mark('csrf-token-issued'); // the hole's own mark, before it "throws" - try { - throw new RuntimeException('the hole blew up mid-render'); - } catch (RuntimeException) { - // swallowed here only to simulate try/finally running leaveHole() on the way out - } finally { - $mark->leaveHole('page://self/html/throwing-hole'); - } - - $this->assertFalse($mark->isMarkedForCurrentScope(), 'the hole\'s own mark must not leak into the current (top-level) scope once its scope is left'); - } - - public function testThrowingGateThrowsWhenUnstorable(): void - { - $mark = new UncacheableMark(); - $gate = new ThrowingUnstorableGate(new UnstorableGate($mark, new DeclaredCacheControl())); - $ro = new None(); - $ro->uri = new Uri('app://self/unstorable-value'); - $mark->mark('csrf-token-issued'); - - $this->expectException(UnstorableResponseDetected::class); - $this->expectExceptionMessage('app://self/unstorable-value is unstorable for this request (unstorable)'); - $gate->reason($ro); - } - - public function testThrowingGateReturnsNullWhenStorable(): void - { - $gate = new ThrowingUnstorableGate(new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl())); - $ro = new None(); - - $this->assertNull($gate->reason($ro)); - } - - public function testThrowingGateReasonExcludingHoleMarksThrowsWhenMarkedForCurrentScope(): void - { - $mark = new UncacheableMark(); - $gate = new ThrowingUnstorableGate(new UnstorableGate($mark, new DeclaredCacheControl())); - $ro = new None(); - $ro->uri = new Uri('app://self/unstorable-value'); - $mark->mark('csrf-token-issued'); - - $this->expectException(UnstorableResponseDetected::class); - $this->expectExceptionMessage('app://self/unstorable-value is unstorable for this request (unstorable)'); - $gate->reasonExcludingHoleMarks($ro); - } - - public function testThrowingGateReasonExcludingHoleMarksReturnsNullWhenStorable(): void - { - $gate = new ThrowingUnstorableGate(new UnstorableGate(new UncacheableMark(), new DeclaredCacheControl())); - $ro = new None(); - - $this->assertNull($gate->reasonExcludingHoleMarks($ro)); - } - - public function testMarkIsStickyOnceSet(): void - { - $mark = new UncacheableMark(); - $this->assertFalse($mark->isMarked()); - - $mark->mark('first'); - $this->assertTrue($mark->isMarked()); - - // Later calls cannot unmark a request; the service carries no reset. - $mark->mark('second'); - $this->assertTrue($mark->isMarked()); - } -} From 5296f33f7beebe1b8ef8d01ccd8264362f743359 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 02:51:02 +0900 Subject: [PATCH 02/11] Document two limits: only the response's own header is inspected, and a per-user value must not arrive via #[ResourceParam] in a cached resource --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 17af8e89..7391b789 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ Declare a dependency with `#[Embed]` and never `unset()` or overwrite the slot - a cached `onGet()` must not fetch dependent data with `$resource->get()`. What a child becomes depends on how the page uses it: -A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response is shared with every visitor, so a per-user value belongs in a non-cacheable `#[DonutCache]` hole, not in the cached response itself. +A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response is shared with every visitor, so a per-user value belongs in a non-cacheable `#[DonutCache]` hole, not in the cached response itself. QR inspects only the response's own header, so a per-user value that reaches the cached output any other way - a child's value read into the body in `onGet()`, a value received through `#[ResourceParam]`, or a value generated in the template (e.g. a Twig function) - is stored and served to every later visitor. In particular, do not receive a per-user value with `#[ResourceParam]` (e.g. `app://self/login#nickname`) in a cached resource - put it in a non-cacheable `#[DonutCache]` hole instead. | Need | Use | Effect of a child purge | |---|---|---| From 00be8d427379bc05769a6ed630d76913e83ef955 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 02:51:45 +0900 Subject: [PATCH 03/11] Shorten the runtime-private guard entry and drop the removed mark API from the put_skipped schema --- README.md | 3 ++- docs/schemas/context/put_skipped.json | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7391b789..7ec18d4c 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,8 @@ - **CDN Integration**: Seamlessly integrates with modern CDNs (e.g., Fastly, Akamai) for shared cache management. - **Conditional Requests with ETag Support**: Reduces network overhead by leveraging `ETag` and `304 Not Modified` responses. - **Distributed Cache Support**: Works with server-side caches (e.g., Redis, APC), shared caches (e.g., CDNs), and client-side caches. -- **Runtime-Private Response Guard**: A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` resource whose own code sets `Cache-Control` to `private` or `no-store` at runtime is not written to the shared cache; its own header is kept exactly and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is exempt - that is the attribute speaking, not a per-request value. This guard reads only the resource's own header, never a descendant's: a per-user value an `#[Embed]`'d child returns is not detected this way, so put it in a non-cacheable `#[DonutCache]` hole instead - the hole's own output is never baked into the stored template and is re-rendered on every hit regardless. This guard only stops *new* writes — deploy a new `CacheVersionModule` version alongside it so this release starts from a fresh cache namespace: entries written by earlier versions may hold per-user content, and an earlier version reading this version's donut cache would otherwise show raw placeholders during a rolling deploy. `CacheVersionModule` only namespaces pools built through this package's own `Storage*Module`s or a `#[CacheNamespace]`-aware adapter; an app binding its own cache adapter directly must clear those pools instead. +- **Runtime-Private Response Guard**: A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` resource whose own code sets `Cache-Control` to `private` or `no-store` at runtime is not written to the shared cache; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. The guard reads only the response's own header (see [Cache dependencies](#cache-dependencies)). + - Deploy this version with a new `CacheVersionModule` version: entries written by earlier versions may hold per-user content, and a rolling deploy would otherwise show raw placeholders. An app that binds its own cache adapter directly must clear those pools instead. ## Cache dependencies diff --git a/docs/schemas/context/put_skipped.json b/docs/schemas/context/put_skipped.json index c0633b1b..839dfcba 100644 --- a/docs/schemas/context/put_skipped.json +++ b/docs/schemas/context/put_skipped.json @@ -13,7 +13,7 @@ "type": "string" }, "reason": { - "description": "why no store happened: the response already carries an ETag (etag-present), is an error response (error-code), the entry kind is not whole-content cacheable (not-cacheable: a donut page served from its template plus inner caches is never stored as a rendered page), or the response is unstorable for this request (unstorable, issue #222: a producer called UncacheableMarkInterface::mark() during this request, or its Cache-Control already contains no-store or private)", + "description": "why no store happened: the response already carries an ETag (etag-present), is an error response (error-code), the entry kind is not whole-content cacheable (not-cacheable: a donut page served from its template plus inner caches is never stored as a rendered page), or the response's own Cache-Control already contains no-store or private, outside what #[HttpCache]/#[NoHttpCache] declares (unstorable)", "type": "string", "enum": ["etag-present", "error-code", "not-cacheable", "unstorable"] }, From ee8035f71dc946544d194b0e7b49478c1d0ef0fb Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 03:02:12 +0900 Subject: [PATCH 04/11] Drop the runtime-private guard entry's third sentence, which duplicates the README --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6e0cf37..9706a64a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,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]`/`#[DonutCache]` response whose own code sets `Cache-Control` to `private` or `no-store` is not stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. This check reads only the response's own header, not a descendant's - put a per-user value in a non-cacheable `#[DonutCache]` hole instead. +- A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response whose own code sets `Cache-Control` to `private` or `no-store` is not stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - 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`. From cd68cf2a0e727770304bc5cd05889f823471130e Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 03:02:36 +0900 Subject: [PATCH 05/11] Trim DeclaredCacheControl's docblock to a one-line summary plus two sentences --- src/DeclaredCacheControl.php | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/DeclaredCacheControl.php b/src/DeclaredCacheControl.php index bc61c765..a8a4f442 100644 --- a/src/DeclaredCacheControl.php +++ b/src/DeclaredCacheControl.php @@ -13,16 +13,13 @@ use function strtolower; /** - * The Cache-Control value `#[HttpCache]` or `#[NoHttpCache]` declares for a resource, and - * whether a response's own code overrode it to something unstorable + * The Cache-Control value `#[HttpCache]`/`#[NoHttpCache]` declares for a resource, and whether + * a response's own code overrode it to something unstorable * - * Mirrors `HttpCacheInterceptor`'s precedence, so `__invoke()` always matches what was written - * to the header. This package is a shared cache in the sense of RFC 9111: whatever it stores - * is served to every later visitor, so reason() refuses a write whose Cache-Control reads - * private or no-store (matched case-insensitively, RFC 7230 §3.2/RFC 9111 §5.2) and was not - * set by that same declaration - the attribute's own client-facing directive is exempt, - * since `#[Cacheable]` combined with `#[HttpCache(noStore: true)]` still stores server-side - * while telling the client not to. + * A declared value is exempt from `reason()`'s check - it is the attribute's own + * client-facing directive, not a per-request value. A runtime `private`/`no-store` outside + * that declared value is refused, since this package is a shared cache: whatever it stores + * is served to every later visitor. */ final readonly class DeclaredCacheControl { From 595a22fab95ee86065ddf169690cb1e7bebd5b28 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 03:07:48 +0900 Subject: [PATCH 06/11] Add a test pinning ResourceDonut::refresh()'s hole re-render (not just the re-fetch) as a degrade-to-fresh-render boundary A hole whose onGet() succeeds but whose own template fails on the refresh re-render previously had no coverage distinct from FlakyHole's onGet()-failure case. Mutating the kept toString() call back to (string) $ro now fails this test (the template failure is swallowed into an empty hole instead of degrading), confirmed and reverted. --- .../App/TemplateFailsOnRefreshHole.php | 22 ++++++++++ .../Resource/App/ThrowingOnStringifyValue.php | 29 ++++++++++++++ ...nutCacheWithTemplateFailsOnRefreshHole.php | 24 +++++++++++ .../App/TemplateFailsOnRefreshHole.html.twig | 1 + ...heWithTemplateFailsOnRefreshHole.html.twig | 1 + tests/GracefulLoggingTest.php | 40 +++++++++++++++++++ 6 files changed, 117 insertions(+) create mode 100644 tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php create mode 100644 tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php create mode 100644 tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithTemplateFailsOnRefreshHole.php create mode 100644 tests/Fake/fake-app/var/templates/App/TemplateFailsOnRefreshHole.html.twig create mode 100644 tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithTemplateFailsOnRefreshHole.html.twig diff --git a/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php b/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php new file mode 100644 index 00000000..113480d0 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php @@ -0,0 +1,22 @@ +toString()` call specifically, not the earlier `$resource->get($uri)` re-fetch. + */ +class TemplateFailsOnRefreshHole extends ResourceObject +{ + public function onGet(): static + { + $this->body['value'] = new ThrowingOnStringifyValue(); + + return $this; + } +} diff --git a/tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php b/tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php new file mode 100644 index 00000000..c03a9544 --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php @@ -0,0 +1,29 @@ + 0) { + self::$failCount--; + + throw new RuntimeException('the hole template is currently broken'); + } + + return 'healthy'; + } +} diff --git a/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithTemplateFailsOnRefreshHole.php b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithTemplateFailsOnRefreshHole.php new file mode 100644 index 00000000..c222907d --- /dev/null +++ b/tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithTemplateFailsOnRefreshHole.php @@ -0,0 +1,24 @@ +{{ flaky }} diff --git a/tests/GracefulLoggingTest.php b/tests/GracefulLoggingTest.php index 68a545af..f63a25d9 100644 --- a/tests/GracefulLoggingTest.php +++ b/tests/GracefulLoggingTest.php @@ -14,6 +14,7 @@ use BEAR\Resource\Uri; use ErrorException; use FakeVendor\HelloWorld\Resource\App\FlakyHole; +use FakeVendor\HelloWorld\Resource\App\ThrowingOnStringifyValue; use FakeVendor\HelloWorld\Resource\Page\Index; use Koriym\SemanticLogger\SemanticLoggerInterface; use Madapaja\TwigModule\TwigModule; @@ -471,6 +472,45 @@ public function testHitRefreshFallsBackToAFreshRenderWhenAHoleTransientlyFails() $this->assertStringContainsString('healthy', (string) $third->view, 'a later request reads the re-stored, healed template directly - no further degrade needed'); } + /** + * HIT refresh, transient hole TEMPLATE failure (not onGet()): `ResourceDonut::refresh()`'s + * own re-fetch runs `$ro->toString()` on the re-fetched hole; `$ro->toString()` does not + * swallow a Throwable the way `(string) $ro` would, so a render failure here degrades and + * falls back to a fresh render, exactly like the onGet() failure above, instead of being + * swallowed into an empty hole embedded in the served page. + */ + public function testHitRefreshFallsBackToAFreshRenderWhenAHolesTemplateTransientlyFails(): void + { + $module = new FakeEtagPoolModule(ModuleFactory::getInstance('FakeVendor\HelloWorld')); + $module->override(new TwigModule([dirname(__DIR__) . '/tests/Fake/fake-app/var/templates'])); + $injector = new Injector($module, __DIR__ . '/tmp'); + $resource = $injector->getInstance(ResourceInterface::class); + $logger = $injector->getInstance(SemanticLoggerInterface::class, CacheLog::class); + + ThrowingOnStringifyValue::$failCount = 0; + $first = $resource->get('page://self/html/donut-cache-with-template-fails-on-refresh-hole'); + $this->assertSame(200, $first->code); + $this->assertStringContainsString('healthy', (string) $first->view); + $logger->flush(); + + ThrowingOnStringifyValue::$failCount = 1; + set_error_handler(static fn (): bool => true); // the fallback degrade still warns + try { + $second = $resource->get('page://self/html/donut-cache-with-template-fails-on-refresh-hole'); + } finally { + restore_error_handler(); + } + + $this->assertSame(200, $second->code); + $this->assertStringContainsString( + 'healthy', + (string) $second->view, + 'the fresh render\'s full body - not an empty hole served because the refresh swallowed the template failure', + ); + $tree = $this->flushAndValidate($logger); + $this->assertNotNull(self::eventContextJsonOf($tree, 'cache_error'), 'the refresh failure is recorded, not silently absorbed'); + } + /** * HIT refresh, persistent hole failure: both the re-fetch and the fresh-render fallback * hit a still-broken hole, so the request fails exactly as the hole's own URI does when From 708ed0b385c3f648839b6f182b1a54687a3d19c0 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 03:10:07 +0900 Subject: [PATCH 07/11] Move ThrowingOnStringifyValue out of the app resource namespace It is a plain Stringable helper, not a ResourceObject - RenderTimeToken (the deleted mark-based equivalent) lived at the fake-app src root under FakeVendor\HelloWorld for the same reason; tests/Fake/fake-app/src/Resource/App/ is reserved for app resources. --- .../fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php | 1 + .../src/{Resource/App => }/ThrowingOnStringifyValue.php | 2 +- tests/GracefulLoggingTest.php | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) rename tests/Fake/fake-app/src/{Resource/App => }/ThrowingOnStringifyValue.php (93%) diff --git a/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php b/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php index 113480d0..2ba576f5 100644 --- a/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php +++ b/tests/Fake/fake-app/src/Resource/App/TemplateFailsOnRefreshHole.php @@ -5,6 +5,7 @@ namespace FakeVendor\HelloWorld\Resource\App; use BEAR\Resource\ResourceObject; +use FakeVendor\HelloWorld\ThrowingOnStringifyValue; /** * A hole whose `onGet()` always succeeds; its own template render is what fails, controlled diff --git a/tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php b/tests/Fake/fake-app/src/ThrowingOnStringifyValue.php similarity index 93% rename from tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php rename to tests/Fake/fake-app/src/ThrowingOnStringifyValue.php index c03a9544..d2e38ea6 100644 --- a/tests/Fake/fake-app/src/Resource/App/ThrowingOnStringifyValue.php +++ b/tests/Fake/fake-app/src/ThrowingOnStringifyValue.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace FakeVendor\HelloWorld\Resource\App; +namespace FakeVendor\HelloWorld; use RuntimeException; use Stringable; diff --git a/tests/GracefulLoggingTest.php b/tests/GracefulLoggingTest.php index f63a25d9..d8230c82 100644 --- a/tests/GracefulLoggingTest.php +++ b/tests/GracefulLoggingTest.php @@ -14,8 +14,8 @@ use BEAR\Resource\Uri; use ErrorException; use FakeVendor\HelloWorld\Resource\App\FlakyHole; -use FakeVendor\HelloWorld\Resource\App\ThrowingOnStringifyValue; use FakeVendor\HelloWorld\Resource\Page\Index; +use FakeVendor\HelloWorld\ThrowingOnStringifyValue; use Koriym\SemanticLogger\SemanticLoggerInterface; use Madapaja\TwigModule\TwigModule; use PHPUnit\Framework\TestCase; From bfaaad673ad2374f36b0b9f319ec3828d5a1b058 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 08:30:38 +0900 Subject: [PATCH 08/11] Make DeclaredCacheControl's declared-value lookup private and mark the class internal --- src/DeclaredCacheControl.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/DeclaredCacheControl.php b/src/DeclaredCacheControl.php index a8a4f442..964876f5 100644 --- a/src/DeclaredCacheControl.php +++ b/src/DeclaredCacheControl.php @@ -20,12 +20,14 @@ * client-facing directive, not a per-request value. A runtime `private`/`no-store` outside * that declared value is refused, since this package is a shared cache: whatever it stores * is served to every later visitor. + * + * @internal */ final readonly class DeclaredCacheControl { private const REASON = 'unstorable'; - public function __invoke(ResourceObject $ro): string|null + private function declared(ResourceObject $ro): string|null { $class = new ReflectionClass($ro); $attributes = $class->getAttributes(HttpCache::class); @@ -51,7 +53,7 @@ public function reason(ResourceObject $ro): string|null { $name = Header::findKey($ro->headers, Header::CACHE_CONTROL); $cacheControl = $name === null ? '' : $ro->headers[$name]; - if ($cacheControl === '' || $cacheControl === $this($ro)) { + if ($cacheControl === '' || $cacheControl === $this->declared($ro)) { return null; } From 351df090d1795ece7e528b86cb23103aeb66b250 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 09:09:59 +0900 Subject: [PATCH 09/11] Check every case variant of the Cache-Control key before storing, and document the property-default no-store change --- CHANGELOG.md | 2 +- src/DeclaredCacheControl.php | 17 +++++++++-------- tests/DeclaredCacheControlTest.php | 13 +++++++++++++ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9706a64a..8ee8cccb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,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]`/`#[DonutCache]` response whose own code sets `Cache-Control` to `private` or `no-store` is not stored server-side; its header is kept and CDN headers are removed. A value declared by `#[HttpCache]`/`#[NoHttpCache]` is still stored. +- A `#[Cacheable]`/`#[CacheableResponse]`/`#[DonutCache]` response whose own code sets `Cache-Control` to `private` or `no-store` is not stored server-side; this includes a `$headers` property default such as `[ResponseHeader::CACHE_CONTROL => CacheControl::NO_STORE]`. To keep server-side caching with a client-facing `no-store`, declare `#[HttpCache(noStore: true)]` instead, which is still stored. - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - 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`. diff --git a/src/DeclaredCacheControl.php b/src/DeclaredCacheControl.php index 964876f5..06b2bd84 100644 --- a/src/DeclaredCacheControl.php +++ b/src/DeclaredCacheControl.php @@ -51,15 +51,16 @@ private function declared(ResourceObject $ro): string|null */ public function reason(ResourceObject $ro): string|null { - $name = Header::findKey($ro->headers, Header::CACHE_CONTROL); - $cacheControl = $name === null ? '' : $ro->headers[$name]; - if ($cacheControl === '' || $cacheControl === $this->declared($ro)) { - return null; - } + $declared = $this->declared($ro); + foreach ($ro->headers as $name => $cacheControl) { + if (strtolower($name) !== strtolower(Header::CACHE_CONTROL) || $cacheControl === $declared) { + continue; + } - $lower = strtolower($cacheControl); - if (str_contains($lower, 'no-store') || str_contains($lower, 'private')) { - return self::REASON; + $lower = strtolower($cacheControl); + if (str_contains($lower, 'no-store') || str_contains($lower, 'private')) { + return self::REASON; + } } return null; diff --git a/tests/DeclaredCacheControlTest.php b/tests/DeclaredCacheControlTest.php index 243483e5..cbe35877 100644 --- a/tests/DeclaredCacheControlTest.php +++ b/tests/DeclaredCacheControlTest.php @@ -77,4 +77,17 @@ public function testOrdinaryCacheControlIsStorable(): void $this->assertNull($declaredCacheControl->reason($ro)); } + + public function testEveryCaseVariantOfTheCacheControlKeyIsChecked(): void + { + // Two components writing differently-cased keys leave two Cache-Control entries in + // the plain array; a private one must be refused whichever key comes first. + $declaredCacheControl = new DeclaredCacheControl(); + $ro = new None(); + $ro->headers = ['Cache-Control' => 'public', 'cache-control' => 'private']; + $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + + $ro->headers = ['cache-control' => 'private', 'Cache-Control' => 'public']; + $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + } } From 2822a429c6db00d039c66017e302149df0ff3674 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 10:09:24 +0900 Subject: [PATCH 10/11] Stop keeping a session only because the logger truncated it --- CHANGELOG.md | 1 - src/Log/KeepMutationsAndFailures.php | 7 +++---- tests/RetentionPolicyTest.php | 13 ------------- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee8cccb..e7e9b70e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `ResourceDonut::FORMAT` is no longer public; use `ResourceDonut::placeholder()`. - 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. - `ResourceDonut::refresh()`: a hole that throws while an already-cached `#[DonutCache]` page is recomposed now degrades to a fresh render instead of failing the request directly from the refresh, the same way a cache-store failure already does. A hole that keeps throwing still fails the request, now from the fresh render instead of the refresh. - `CacheDependency::depends()` no longer unsets the child's own `Surrogate-Key` header after copying it into the parent: a child that is both read by the parent and later rendered as a hole keeps its own key for its own independent cache entry. diff --git a/src/Log/KeepMutationsAndFailures.php b/src/Log/KeepMutationsAndFailures.php index c7a06a45..4c969eb4 100644 --- a/src/Log/KeepMutationsAndFailures.php +++ b/src/Log/KeepMutationsAndFailures.php @@ -30,9 +30,8 @@ * decide the tags, TTLs and CDN keys every later read repeats. * - effects that did not happen: `saved: false`, `invalidate` with `roPool`/`etagPool`/`cdn` * `failed`, a `manual_*` result of `failed`, and `cache_error{operation: write}` - a store or - * an invalidation that was aborted. `semantic_logger_error` and `semantic_logger_truncated` - * join them because they mean the records themselves may be incomplete: past the record - * limit a mutation leaves no record, only the truncation point. + * an invalidation that was aborted. `semantic_logger_error` joins them because it means the + * records themselves may be incomplete. * - a sample, when a rate is configured. * * Deliberately not kept: `cache_error{operation: read}` (availability, monitored elsewhere - @@ -177,7 +176,7 @@ private function isPoint(array $record): bool */ private function isMissingEffect(array $node, mixed $type): bool { - if ($type === 'semantic_logger_error' || $type === 'semantic_logger_truncated') { + if ($type === 'semantic_logger_error') { return true; // the records themselves may be incomplete } diff --git a/tests/RetentionPolicyTest.php b/tests/RetentionPolicyTest.php index ca625478..11314dc0 100644 --- a/tests/RetentionPolicyTest.php +++ b/tests/RetentionPolicyTest.php @@ -20,9 +20,7 @@ use BEAR\QueryRepository\Log\KeepMutationsAndFailures; use BEAR\QueryRepository\Log\SafeSemanticLogger; use Koriym\SemanticLogger\CloseEntry; -use Koriym\SemanticLogger\EventEntry; use Koriym\SemanticLogger\LogJson; -use Koriym\SemanticLogger\OpenEntry; use Koriym\SemanticLogger\SemanticLoggerInterface; use PHPUnit\Framework\TestCase; @@ -187,17 +185,6 @@ public function testASessionWhoseOwnRecordsMayBeIncompleteIsKept(): void $this->assertTrue($this->policy->keeps($this->logger->flush())); } - public function testASessionCutAtTheRecordLimitIsKept(): void - { - // Past the limit the core logger records nothing more, so a command opened after it - // leaves no record at all; the root truncation point is the only trace of it. - $open = new OpenEntry('get_1', 'get', 'https://example.com/get.json', ['uri' => 'page://self/html/blog-posting'], null); - $close = new CloseEntry('cache_hit_1', 'cache_hit', 'https://example.com/hit.json', ['source' => 'view'], 'get_1'); - $truncated = new EventEntry('semantic_logger_truncated_1', 'semantic_logger_truncated', 'https://example.com/t.json', ['limit' => 2, 'dropped' => 2, 'firstDroppedType' => 'command'], null); - - $this->assertTrue($this->policy->keeps(new LogJson('', [$open, $close, $truncated]))); - } - public function testAFailedOutcomeIsFoundWhereTheRootKeepsItAsARecord(): void { // A `close` is one entry on a scope, but a root record carrying `openId` when its From 6e8902dfa2e89136681af0037a0e4121cdcbf936 Mon Sep 17 00:00:00 2001 From: Akihito Koriyama Date: Wed, 28 Oct 2026 10:24:40 +0900 Subject: [PATCH 11/11] Use a fresh resource for the reversed key order so PHPStan does not reuse the narrowed result --- tests/DeclaredCacheControlTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/DeclaredCacheControlTest.php b/tests/DeclaredCacheControlTest.php index cbe35877..9309b9c6 100644 --- a/tests/DeclaredCacheControlTest.php +++ b/tests/DeclaredCacheControlTest.php @@ -87,7 +87,8 @@ public function testEveryCaseVariantOfTheCacheControlKeyIsChecked(): void $ro->headers = ['Cache-Control' => 'public', 'cache-control' => 'private']; $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); - $ro->headers = ['cache-control' => 'private', 'Cache-Control' => 'public']; - $this->assertSame('unstorable', $declaredCacheControl->reason($ro)); + $reversed = new None(); + $reversed->headers = ['cache-control' => 'private', 'Cache-Control' => 'public']; + $this->assertSame('unstorable', $declaredCacheControl->reason($reversed)); } }