Do not store a donut template whose render threw by koriym · Pull Request #237 · bearsunday/BEAR.QueryRepository

koriym · 2026-10-25T03:08:23Z

Redesign (both reviewers rejected the warning-regex + error-handler approach)

ResourceDonut::create() renders with $ro->toString() instead of (string) $ro -
toString() has no catch (unlike magic __toString()), so a render Throwable just
propagates, same as HttpResponder::getOutput() already renders with. No handler, no
regex, nothing to reconstruct or evade. DonutRequest::__toString()'s hole render gets the
same treatment ($executed->toString()). Nothing stores because nothing past the render
call runs. refresh()'s existing CacheStoreFailure conversion (both reviewers: keep) is
unchanged.

Second bug, found by executing the required tests

AbstractDonutCacheInterceptor's degraded-read fallback returned proceed()'s raw,
un-rendered result - a persistently-broken hole on refresh silently served an empty 200.
Fixed by sharing the render+store path a normal MISS already uses.

Verification

Named/anonymous hole exceptions, page-template exceptions, warning→exception-converter
parity vs. a no-attribute page, transient/persistent HIT-refresh - each on actual output,
mutation-proved. cs/sa/phpmd clean, 100% coverage, BeMart e2e diffed clean (only
already-documented #236/ETag deltas).

coderabbitai · 2026-10-25T03:08:30Z

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 27574057-6478-4f35-92eb-2fa84f9225aa

Commits

Reviewing files that changed from the base of the PR and between b34ee63 and dcef167.

Files selected for processing: 18

CHANGELOG.md
src/AbstractDonutCacheInterceptor.php
src/DonutRequest.php
src/Exception/CacheStoreFailure.php
src/ResourceDonut.php
tests/Fake/FakeWarningEmittingRenderer.php
tests/Fake/fake-app/src/Resource/App/AnonymousThrowingHole.php
tests/Fake/fake-app/src/Resource/App/FlakyHole.php
tests/Fake/fake-app/src/Resource/App/HoleWithNoTemplate.php
tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithFlakyHole.php
tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithThrowingHole.php
tests/Fake/fake-app/src/Resource/Page/Html/PlainNoCacheAttribute.php
tests/Fake/fake-app/var/templates/App/FlakyHole.html.twig
tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithFlakyHole.html.twig
tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithThrowingHole.html.twig
tests/Fake/fake-app/var/templates/Page/Html/PlainNoCacheAttribute.html.twig
tests/GracefulLoggingTest.php
tests/ResourceDonutTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Rendering failures now propagate from donut creation and request rendering. Cache refresh wraps hole failures so the interceptor can perform a fresh render and apply the normal response and cache-write handling.

Changes: Donut failure handling

Layer / File(s)	Summary
Propagate rendering failures `src/DonutRequest.php`, `src/ResourceDonut.php`, `tests/Fake/`, `tests/Fake/fake-app/src/Resource/{App,Page/Html}/`, `tests/Fake/fake-app/var/templates/{App,Page/Html}/*`, `tests/GracefulLoggingTest.php`, `tests/ResourceDonutTest.php`	`DonutRequest` and `ResourceDonut::create()` call `toString()` directly, allowing rendering exceptions to propagate. Tests cover page and hole failures, warning handling, and successful rendering.
Refresh failure and fallback `src/ResourceDonut.php`, `src/AbstractDonutCacheInterceptor.php`, `src/Exception/CacheStoreFailure.php`, `tests/Fake/fake-app/src/Resource/App/FlakyHole.php`, `tests/Fake/fake-app/src/Resource/Page/Html/DonutCacheWithFlakyHole.php`, `tests/Fake/fake-app/var/templates/App/FlakyHole.html.twig`, `tests/Fake/fake-app/var/templates/Page/Html/DonutCacheWithFlakyHole.html.twig`, `tests/GracefulLoggingTest.php`, `tests/ResourceDonutTest.php`, `CHANGELOG.md`	`ResourceDonut::refresh()` wraps hole failures in `CacheStoreFailure`. The interceptor handles fallback renders through the shared response path. Tests cover transient fallback, persistent failure, and cache-write behavior.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: jingu

Merge Risk: Minimal · up to dcef1

The rendering changes have no identified merge-blocking issue after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 52.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 13 files. (5 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description check		The description directly explains the render propagation changes, degraded-read fallback fix, and verification results. It is clearly related to the changeset.
Title check		The title clearly summarizes the primary change: preventing storage of donut templates when rendering throws.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 52.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 13 files. (5 skipped: 5 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

koriym · 2026-10-25T03:08:43Z

@koriym Evidence for this PR (head b4dc71bf00c10aa5115818929e99fa0d711cd2d3):

Mutation proofs (break → red → restore)

Handler returns true instead of false on match (suppresses the original warning)
→ ResourceDonutTest::testCreateRefusesToStoreTheTemplateWhenAHoleThrows and
testCreateRefusesToStoreTheTemplateWhenThePageTemplateItselfThrows both failed
(error_get_last() no longer populated - PHP's normal handling never ran). Restored.
throw CacheStoreFailure::from(...) disabled (reproduces the original bug: an empty
template would be stored) → 4 tests failed: the two create() guard tests
(expectException never thrown) and both GracefulLoggingTest integration tests
(cache_error event missing). Restored.
Regex trace anchor changed from #0 to #9 (simulates a BEAR.Resource warning-shape
drift) → the Twig-integration test happened to still pass by accident (a deep Twig stack
can contain a coincidental )\n#9 elsewhere), which is why I added a dedicated
testSwallowedRenderWarningRegexMatchesPreciselyAndRejectsUnrelatedWarnings unit test
against the regex constant directly (via ReflectionClassConstant) on a controlled
3-frame trace - that one failed correctly (preg_match returned 0). Restored.
refresh()'s catch (Throwable $e) { throw CacheStoreFailure::from($e); } removed
(reverts to the pre-fix behaviour) → testRefreshRefusesToServeWhenAHoleThrows (new) and
testRefreshOfALegacyMultiParamPlaceholderFailsTheSameWayAsBeforeThisFix (updated) both
failed with the raw, unwrapped exception. Restored.

Gate

composer cs    → 61/61, no violations
composer sa    → psalm: no errors; phpstan: no errors
composer phpmd → exit 0, no violations (only pre-existing tool-internal PHP 8.4
                 deprecation noise from phpmd/pdepend itself)
vendor/bin/phpunit → OK: 478 tests, 1408 assertions, 0 failures
                      (13 skipped / 12 PHPUnit deprecations are pre-existing,
                      unchanged with/without this branch's commit)

Coverage

php -d pcov.enabled=1 vendor/bin/phpunit --coverage-text --coverage-filter=src
  Classes: 100.00% (108/108)
  Methods: 100.00% (295/295)
  Lines:   100.00% (1397/1397)

(Exception\DonutRenderAborted has no executable lines of its own - same as its sibling
one-line exception classes - so it does not appear as a separate coverage-report row, matching
existing package convention.)

koriym · 2026-10-25T03:08:46Z

@coderabbitai review

coderabbitai · 2026-10-25T03:08:52Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

codecov · 2026-10-25T03:09:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (b34ee63) to head (dcef167).

Additional details and impacted files

@@             Coverage Diff             @@
##                 1.x      #237   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       649       651    +2     
===========================================
  Files            111       111           
  Lines           1745      1750    +5     
===========================================
+ Hits            1745      1750    +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…fy CacheStoreFailure

koriym · 2026-10-25T03:16:15Z

@koriym Follow-up (head 9449c6be9d1415bf5979b332fac98b2621e76e62) addressing review feedback:

CHANGELOG: added a Changed entry for ResourceDonut::refresh() - a hole that throws
while an already-cached page is recomposed now degrades (recorded as cache_error, served
via a fresh non-donut render) instead of the exception reaching the caller. Called out
explicitly that a permanently-broken hole (e.g. the Donut placeholder breaks for an #[Embed] hole with two or more query parameters #232 corrupted-legacy-placeholder
ParameterException) now degrades to 200 on every refresh instead of a 500, and updated that
test's docblock to state this intentional change rather than leave it implicit.
CacheStoreFailure docblock: added a paragraph reconciling the donut-render-abort case
with the class's "only the storage boundary raises this" principle - by the time
ResourceObject::__toString() returns, BEAR.Resource has already discarded the real
Throwable (only a warning + '' remain), so there is no genuine exception left to let
"travel" the way testARenderFailureOnTheWritePathKeepsTravelling does for plain
#[Cacheable]; degrading is what keeps the response servable without fabricating a 500 from
a guessed cause.
Tests: replaced the ReflectionClassConstant-based regex pin test with public-API
assertions on the caught CacheStoreFailure's message (exact match for the
FakeThrowingRenderer case; prefix/contains/not-contains-#0 /.php( for the Twig-wrapped
hole case) - pins precision without touching the private constant.
Added a direct ResourceStorageInterface::getDonut() check right after the first request in
the integration test, independent of the second request's cache_miss outcome, so "nothing
was stored" and "a later request isn't served a stale empty page" are each provable on their
own.

Gate (re-run after these changes)

composer cs    → 61/61
composer sa    → psalm + phpstan: no errors
vendor/bin/phpunit → OK: 477 tests, 1406 assertions
php -d pcov.enabled=1 vendor/bin/phpunit --coverage-text --coverage-filter=src
  Classes: 100.00% (108/108)  Methods: 100.00% (295/295)  Lines: 100.00% (1397/1397)

(Previous mutation proofs from the earlier commit still hold - none of this round's edits
touched the guarded code paths themselves, only docs/tests.)

# Conflicts: # CHANGELOG.md

…g-matching error handler

koriym · 2026-10-25T04:41:18Z

@koriym Evidence for the redesign (head 07939b8c31fcd6a361e7178a3165d544757662f7), rebuilt on 1.x after #236 merged:

Mutation proofs (break → red → restore)

ResourceDonut::create(): reverted $ro->toString() back to (string) $ro → the hole-throw
and page-template-throw tests pass silently again (empty template stored) - confirms the
guard is this one call, not incidental.
DonutRequest::__toString(): reverted $executed->toString() to (string) $executed →
testCreateLetsAHolesOwnRenderFailureTravelAndStoresNothing (a hole whose onGet()
succeeds but has no template) failed correctly - this is the test that specifically pins
that line, since ThrowingHole fails earlier, inside ($this->request)(), and never
reaches it.
AbstractDonutCacheInterceptor::handleProceedResult(): reverted to the old bare
return $invocation->proceed(); fallback → testHitRefreshFailsTheSameWayAsNoDonutCache...
failed (the persistently-broken hole silently returned 200 with an un-rendered body instead
of failing) - this is the real second bug this redesign found by actually running the
brief's required HIT-refresh tests, not by inspection.
A $this->fail() inside a try is NOT safely caught by a broad catch (RuntimeException) -
PHPUnit\Framework\Exception extends \RuntimeException - found and fixed in my own new
persistent-failure test (flag-then-assert-after pattern instead).

Verified by execution, not assumption

No-cache-attribute baseline: $resource->get('page://self/html/plain-no-cache-attribute')
alone does NOT render (no toString() call happens without a cache attribute) - the
converter-comparison test explicitly calls ->toString() to match what
HttpResponder::getOutput() does.
A no-attribute page's #[Embed] of a throwing hole, rendered via ->toString(), hits BEAR.
Resource's own AbstractRequest::__toString() swallow into a fatal E_USER_ERROR (confirmed
via a standalone script) - genuinely worse than the donut path after this fix, which gets a
clean, catchable exception. CHANGELOG/docblocks worded accordingly, not overclaiming
"identical".

Gate

composer cs    → 62/62
composer sa    → psalm + phpstan: no errors
composer phpmd → exit 0, no violations
vendor/bin/phpunit → OK: 512 tests, 1511 assertions, 0 failures
php -d pcov.enabled=1 vendor/bin/phpunit --coverage-text --coverage-filter=src
  Classes: 100.00% (111/111)  Methods: 100.00% (320/320)  Lines: 100.00% (1524/1524)

BeMart e2e smoke (`~/git/bemart-e2e/run.sh`, vendor src swapped in then restored exactly via

composer install from the lock file - vendor/ is gitignored in that worktree, confirmed restored)

out/trial-donut-redesign vs out/trial-qr-once (the post-#236, pre-this-PR baseline):
only two kinds of difference, both explained and expected, zero body/status differences:

page://self/ (#[CacheableResponse]+#[Embed]) ETag shifts - re-weaving
AbstractDonutCacheInterceptor changes the Ray.Aop class identity the same way Embedした子のURIタグは、描画されなくても親に付けてよいのでは #226 already
documented for a different interceptor change; bodies byte-identical (verified via raw diff).
3 .hitmiss files show fewer CACHE_HIT lines for the stock resource - already present
identically in out/trial-qr-once (the Execute each embedded donut child once and tag executed children (#226) #236-only baseline), so entirely Execute each embedded donut child once and tag executed children (#226) #236's embed-dedup,
not this PR's.

koriym · 2026-10-25T04:41:21Z

@coderabbitai review

coderabbitai · 2026-10-25T04:41:27Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…ent legacy-placeholder case

koriym · 2026-10-25T05:05:11Z

@koriym Applied the three LOW fixes from review (head 4143dad730dad32dbf96edb41192e500dba6b905):

CHANGELOG's refresh() entry now presents the corrupted-legacy-placeholder case as
transient - verified by execution: both the first and second request return 200 with the
full body, and the stored template is re-saved with a correct [le2:...] placeholder.
"Still fails" is now limited to a hole that keeps throwing.
Removed the added sentence claiming this PR's AbstractDonutCacheInterceptor change
shifts #[CacheableResponse]+#[Embed] ETags again - kept only the existing Embedした子のURIタグは、描画されなくても親に付けてよいのでは #226 note.
Reworded the stale (string) $ro/warning-shape-guard comments at
src/ResourceDonut.php (refresh()'s catch) and tests/ResourceDonutTest.php (the
no-throw and refresh-hole-throw test docblocks) to match the current non-swallowing
toString() design.

Gate: cs 62/62, sa clean, phpunit 512/512, coverage 100%.

koriym · 2026-10-25T05:07:53Z

@coderabbitai full review

The PR was redesigned (07939b8, 4143dad): it now renders with the non-swallowing ResourceObject::toString() instead of matching warnings in an error handler. Please review the full current diff.

coderabbitai · 2026-10-25T05:07:59Z

⚠️ Action not completed

Head commit changed.

… re-stores the template

koriym · 2026-10-25T05:08:12Z

@koriym Added one cheap, mutation-proved assertion (head dcef1672fefd146d67b861fdd7ee8ee54b714679): testHitRefreshFallsBackToAFreshRenderWhenAHoleTransientlyFails now also checks getDonut() is non-null after the degraded request (proving the fresh render re-stores the template, not just that the response looks right) and that a third request reads the healed entry directly with no further degrade needed - backing the CHANGELOG's self-healing claim with a permanent test rather than only the throwaway script used to verify the wording. Reverting the interceptor fix makes this test fail as expected.

Gate: cs 62/62, sa clean, phpunit 512/512, coverage 100%.

koriym · 2026-10-25T05:39:38Z

@coderabbitai review

coderabbitai · 2026-10-25T05:39:45Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…he existing hole-exclusion predicate and never replaces a genuine store failure

Do not store a donut template whose render threw

b4dc71b

Address review: document refresh() semantics, strengthen tests, clari…

9449c6b

…fy CacheStoreFailure

koriym added 2 commits October 25, 2026 13:14

Merge remote-tracking branch 'upstream/1.x' into donut-aborted-render

8e7ea1f

# Conflicts: # CHANGELOG.md

Redesign: detect an aborted donut render via toString(), not a warnin…

07939b8

…g-matching error handler

Reword docs/comments for the current toString() design and the transi…

4143dad

…ent legacy-placeholder case

Add a direct storage assertion proving the transient-failure fallback…

dcef167

… re-stores the template

coderabbitai Bot approved these changes Oct 25, 2026

View reviewed changes

koriym mentioned this pull request Oct 25, 2026

Scope mark() refusals to the marked output and report dev-gate skips once (#234) #238

Closed

koriym merged commit 5ddd3fe into bearsunday:1.x Oct 25, 2026
20 checks passed

koriym added a commit to koriym/BEAR.QueryRepository that referenced this pull request Oct 25, 2026

Prove the bearsunday#237-merged donut degraded-read fallback honors t…

1aebe7e

…he existing hole-exclusion predicate and never replaces a genuine store failure

coderabbitai Bot mentioned this pull request Oct 25, 2026

Make an embedded child execute once without an extra interceptor (#226) #239

Merged

koriym mentioned this pull request Oct 27, 2026

Prepare 1.18.0: drop the unreleased mark guard, keep the runtime-private guard #242

Merged

Conversation

koriym commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Redesign (both reviewers rejected the warning-regex + error-handler approach)

Second bug, found by executing the required tests

Verification

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koriym commented Oct 25, 2026

Mutation proofs (break → red → restore)

Gate

Coverage

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

koriym commented Oct 25, 2026

Gate (re-run after these changes)

Uh oh!

koriym commented Oct 25, 2026

Mutation proofs (break → red → restore)

Verified by execution, not assumption

Gate

BeMart e2e smoke (~/git/bemart-e2e/run.sh, vendor src swapped in then restored exactly via

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

koriym commented Oct 25, 2026

Uh oh!

coderabbitai Bot commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

koriym commented Oct 25, 2026 •

edited

Loading

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

codecov Bot commented Oct 25, 2026 •

edited

Loading

BeMart e2e smoke (`~/git/bemart-e2e/run.sh`, vendor src swapped in then restored exactly via

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading

coderabbitai Bot commented Oct 25, 2026 •

edited

Loading