feat(helpers): cache the page-independent half of a formatted menu by parisek · Pull Request #155 · parisek/timber-kit

parisek · 2026-08-27T11:31:58Z

From which project

sloneek (redesign branch). Follow-up to #154 and #156: with those merged, Helpers::formatMenu() is the single largest item left in the render.

Measured

Front page, real Redis object cache, two back-to-back runs of 15 requests each:

	before (v1.43.0)	after
front page, min	402 ms	344–350 ms
`/blog/`, min	907 ms	836–857 ms

That is ~53 ms, 13 % of the request. The isolated cost of the work removed is ~87 ms across five menus (72.5 ms on the 68-item main menu, 14.4 ms on four footer menus), measured by bracketing the call in a real web request. The gap between 87 and 53 is the half that is deliberately not cached — see below.

Rendered HTML is byte-identical once non-deterministic ids are normalized, with a valid control pair on each side.

What is stored, and what is not

The ACF fields on each menu item and on the menu itself are the same on every URL. is_active and in_active_trail are not, and the walk recomputes them on every request including a cache hit.

Caching the whole formatted menu instead would freeze the highlight onto whatever page filled the entry — wrong on every page but one, and invisible to any status check. The split also keeps this to one entry per menu rather than one per page.

The gate proves purity before the work, not after it

This is the part that changed since the first draft, and it is the point of the PR.

The earlier gate watched the render and asked whether formatting had changed anything. That is not a proof of purity. An unregistered [foo] comes back byte-identical, reads as static, and the literal [foo] is stored. The day a plugin registers that shortcode, every page serves the frozen source text instead of its output — with nothing to log and nothing to notice. A reviewer broke the observational gate with a three-line shortcode, and the conclusion was that observing one render cannot prove purity at all.

So each of the three dynamic surfaces in fieldFormatter() is now decided from stored data, before the build:

Surface	Static proof
`do_shortcode()` on `wysiwyg`/`textarea`	no `[` in any of the item's raw meta strings
`field_formatter_{$type}` filters	none registered — no static proof of a callback's purity exists, so one ends it for every menu
CF7 / WPForms `post_object`	none available: the markup carries a nonce, so this one surface stays counted during the build

The meta read is free in practice — wp_get_nav_menu_items() has already primed it — and only runs when the walk can actually store.

The check is deliberately blunt. A bracket in a plain-text field is not a shortcode and the menu is refused anyway: a false refusal costs one uncached menu, a false accept is wrong on every page.

The filter refusal is a default, not a verdict. It goes through timber_kit_cache_menu_fields with everything else, so a project that knows its own formatter is pure can say so.

Rejected

Fingerprinting the registered field_formatter_* tags into the cache key instead of refusing. It catches a callback appearing or disappearing, but it cannot make a page-dependent callback safe — which is the actual risk.
<img in the gate. fieldFormatter() never runs wp_filter_content_tags, so an image tag is not a dynamic surface here. Including it would refuse menus for no reason.
Memoizing CacheSignature::shared(). Two independent reviews found the same mid-request staleness hole from different directions: switch_to_blog(), a user switch and a save in a long-running process each move an input, and a memo would key the second site's menu under the first site's name.

Invalidation

CacheSignature::shared() carries site, language, the current user's roles and a content version from wp_cache_get_last_changed() over posts and terms. WordPress bumps those counters itself, so a saved post or an edited term makes the old key unreachable rather than stale. Nothing has to be flushed and nothing can be forgotten.

The TTL (timber_kit_menu_fields_ttl, 12 h) is not about staleness — it bounds the generations each content change orphans, which nothing else deletes.

Requires a persistent object cache; without one the path is skipped rather than paying for a read and a write that always miss.

Tests

Each gate was verified load-bearing by removing it and confirming the failure:

disabling the stored-value check fails test_shortcode_rendered_output_is_never_stored and test_an_unregistered_shortcode_is_refused_although_nothing_changed
disabling the filter check fails test_a_registered_field_formatter_filter_refuses_every_menu

1811 tests green, PHPStan clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb

Anything cached across requests has to answer one question — when are two renders interchangeable — and two callers that answer it differently will disagree about who may be served whose content. `CacheSignature` answers it once: site, language, the current user's roles, and a content version. Roles rather than user id. Role is the axis plugins gate menus on, and it keeps the stored variants to the number of roles rather than the number of accounts, so an editorial team shares one entry instead of filling the cache with a copy each. A logged-in user with no roles is deliberately not 'anon': a plugin can show it something an anonymous visitor must not see. The content version is `wp_cache_get_last_changed()` over `posts` and `terms`. WordPress bumps those itself whenever anything in the group is invalidated, so a saved post changes the key rather than requiring a hook to notice. The alternative — a list of actions to flush on — is only as complete as whoever last added a plugin remembered, and the failure it allows is a stale link on every page with no error and no log. The cost is one rebuild per content change, which is the trade being made. `isAvailable()` is false without a persistent object cache, so callers can skip a read and a write that would always miss, and false without `wp_cache_get_last_changed()`, because a key that cannot go stale on its own is worse than no key. One test per dimension: a dimension that stops separating two worlds is a cache that serves one visitor's content to another, and nothing downstream can notice.

`formatMenu()` spends most of its time on ACF: a 90-item menu costs 61-104 ms per request on the fields attached to its items, out of 529-702 ms of PHP. Those fields are the same on every URL, so they are stored once per menu. **`is_active` and `in_active_trail` stay outside the stored payload.** They are the one part of a formatted menu that does depend on the page, and the walk recomputes them every request. Caching the whole menu instead would freeze the highlight on whatever page filled the entry — a wrong highlight on every page but one, which no status check can see. Keeping them out is also what keeps this to one entry per menu rather than one per page. The payload is assembled during the walk and written once at the end, so a partial walk stores nothing. A menu with no term id, or a site with no persistent object cache, skips the path entirely rather than paying for a read and a write that always miss. Nothing is flushed: `CacheSignature` keys the entry by a content version, so a saved post makes the old key unreachable rather than wrong. `flushMenuFields()` exists for the in-flight assembly state, which a long-running process and a test both outlive. The test counts `wp_get_post_terms()` calls rather than `acf_get_field_groups()` deliberately. The field-group memo on the sibling branch caches the latter per request, so it reads zero on a second render that really did rebuild — an observable that would have started lying the week the two merged. Verified on a branch holding both.

The numbers are the cost of the work a cache hit removes, not a before/after page time. Producing a real hit needs a persistent object cache and the measurement host has none, so claiming a page delta would be claiming something unmeasured. Both figures are stated: 61-104 ms with the field-group memo applied, 154-266 ms without it. The two changes overlap, and quoting the larger number alone would describe a saving that stops existing the week the other one merges.

Two independent reviews of the first version, on separate lenses, and they converged on two things: the open/close protocol was not exception-safe or re-entrant, and `CacheSignature` memoized inputs that move inside a request. Both are fixed here, along with the finding only one of them looked for. **The blocking one: formatted output is not always a function of the stored field.** `fieldFormatter()` expands shortcodes and hands every field to a `field_formatter_{$type}` filter, and either may read the global post, the current query or the current user. The first version cached that output, so one page's rendered shortcode could be replayed onto every other page — and it serializes perfectly, so no type check would have caught it. Dynamism is now detected, and measured as *change* rather than as *opportunity*. Running `do_shortcode()` is not the signal; a shortcode that altered the value is. The blunter rule was written first and rejected the largest menu on the measurement site — 68 items whose fields were provably a function of stored content — which is how the distinction was found. Storability rejects objects, resources and closures separately. One unstorable slot condemns the whole menu rather than storing a payload with a hole that would be replayed as complete. `CacheSignature` no longer memoizes. Three of its four inputs can move inside one request, and a memo would key the second site's menu under the first site's name; term ids collide across sites, so the entry would be found and served. The cache moves out of `Helpers` into `MenuFieldsCache`, which is where the open/close protocol, the depth counting and the storability policy belong. `Helpers::flushMenuFields()` stays as a delegate, because callers reach the menu through `Helpers` and would not look elsewhere for its reset. The walk is closed from a `finally` and only the outermost close writes. `wp_cache_set()` now carries a lifetime. The key already versions content, so this is not about staleness: every content change orphans a generation and nothing deletes it. Verified after the change: all five menus on the measurement site store, 95 slots, none rejected. Each new test fails without the mechanism it covers.

parisek · 2026-08-27T12:07:44Z

Agent review, round 2 — 2026-08-27

Same two reviewers, same lenses, re-run after bede0ee.

Round 1 fixes: both reviewers confirm they hold

Finding	Raised by	Verdict now
`CacheSignature` memoized inputs that move mid-request	both	Fixed beyond doubt. Nothing is memoized; mid-request site and user switches are tested.
open/close not exception-safe, re-entrancy lost the write	both	Fixed for exception, same-menu re-entrancy, nested different menu, repeated calls.
the cache belonged in its own component	Fable	`MenuFieldsCache`; boundary confirmed correct.

The new finding, and it is the one that matters

Codex broke the dynamism detector with a counterexample I cannot answer:

add_shortcode('contextual', fn() => is_singular('product') ? 'Buy now' : '[contextual]');

Out of context the shortcode returns its own source text. The value does not change, the detector reads it as static, and the literal [contextual] is stored. On a product page the cache hit bypasses do_shortcode() entirely and renders the literal.

The same shape defeats the field_formatter_* comparison: a callback that varies by post, user or capability can look inert on the seeding request.

The general point is right and is not fixable by a better detector: observing one render cannot prove purity across renders. Round 1's detector was too blunt (it rejected the largest menu on the measurement site); round 2's is too permissive. There is no middle setting, because the property being tested is not observable from one sample.

Codex also found that a nested incomplete close is forgotten — close($id, false) at depth > 1 only decrements, so an inner walk that threw and was caught still allows the outer write. Real, and separate from the above.

Why this is not being patched

The obvious fix is Codex's: cache raw ACF values and format on every request. Measured on the front page, that would still capture most of the win — the lookup is 56-102 ms of the 64-118 ms per-item cost, so formatting is only ~12 % of it.

But there are no raw values at this boundary. getFieldObjectsByScreen() fills $field['value'] from get_field(), and ACF's own format_value runs the_content filters for wysiwyg — shortcodes included. Caching "before our formatting" still caches shortcode output. The safe boundary is get_field( …, false ) plus live acf_format_value(), which changes what every downstream consumer of formatFields() receives.

That is a larger and riskier change than the 12 % of one page it would buy, on top of #154 which already takes the larger bite with no staleness surface at all.

Recommendation: do not merge; park it

Marked draft. The work is not wasted — CacheSignature, the depth-counted protocol, the storability policy and the tests are all sound, and they are what a later attempt at the deeper boundary would build on.

Fable's smaller findings, for the record

Helpers::$dynamic_format_count is never reset in tests. Harmless today because only a delta is read, but a trap for the first test that reads it absolutely.
No test covers menu A whose formatting re-enters formatMenu() for menu B; the mechanism handles it, nothing pins it.
The CF7/WPForms branches increment unconditionally while everything else compares — correct in practice, inconsistent with the stated doctrine, and unexplained in the code.

All three would be worth fixing if this were shipping. They are recorded rather than done.

`getFieldObjectsByScreen()` asked ACF for a group's fields once per group per screen. On the sloneek front page that is 348 calls producing 21 distinct answers, because a 90-item menu resolves the same group ninety times. Measured on the work removed rather than on the page: 26-38 ms falls to 5-8 ms. End to end the page moved 16-34 ms across two rounds; the machine drifted between them, so the direct measurement is the one to trust. **This needs no invalidation, and that is the point.** Field definitions are configuration — theme JSON, not anything a visitor changes — so the memo lives for the request and nothing has to notice when it should die. A field *value* is the opposite, which is why the parked #155 could not do the same thing one layer up. The key carries the blog id and the language for the reasons the screen memo's does: groups are registered per site, and ACFML translates a field's label, instructions and choices. A group carrying neither a key nor an id has no identity and is asked every time, rather than sharing one entry with the next anonymous group. Output is byte-identical against a control pair of runs.

## From which project `sloneek`, from the profiling that produced #154. **Stacked on #154** — the base is `perf/memoize-capability-probes`, so this diff shows only the new change; GitHub retargets to `main` when #154 merges. ## The finding `getFieldObjectsByScreen()` asks ACF for a group's fields once per group **per screen**. A 90-item menu resolves the same group ninety times. | | calls | time | |---|---:|---:| | before | 348 | 26-38 ms | | after | **21** | **5-8 ms** | That is measured on the work removed. End to end the page moved 16-34 ms across two rounds — the same saving through a noisier instrument, and the machine drifted between them, so the component figure is the one to trust. Rendered HTML is byte-identical, checked against a control pair of runs of the unchanged code because the page is not deterministic without one. ## Why this one needs no invalidation **Field definitions are configuration, not content.** They come from the theme's JSON, they do not depend on the page being rendered, and nothing a visitor does changes them. The memo lives for the request, and there is nothing for a hook to notice. A field *value* is the opposite, which is exactly why #155 is parked: caching one layer up meant caching formatted output, and formatting runs shortcodes and arbitrary filters that can read the current page. The two look like the same optimisation and are not. The key carries the blog id and the language for the same reasons the screen memo's does — groups are registered per site, ACFML translates a field's label, instructions and choices. A group carrying neither a key nor an id has no identity to memoize on and is asked every time, rather than sharing an entry with the next anonymous group. ## What was tried and did not work Recorded so nobody repeats it. All measured on the front page, all rejected: | Idea | Result | |---|---| | Prime the post-meta cache for all menu items | Already warm. 0 ms, query count unchanged. | | Skip `get_field()` where the item has no stored meta | 206 of 272 calls skipped, time unchanged — ACF already short-circuits. | | Prime the term-relationship cache | Already warm, and added 5 queries. | | Cache the formatted field payload | #155, parked: formatting is not a function of the stored value. | What remains unclaimed on this path is `get_field()` for the 66 menu-item fields that hold a value (37-68 ms) and `wp_get_post_terms()` (9-16 ms, removable only by threading the known menu id through `formatFields()`). ## Tests Nine, each verified to fail without the memo where it is the mechanism under test: one group asked once, identity by `key` and by `ID`, an anonymous group asked every time, separation by language and by blog, a non-array answer normalized, and the flush.

…-payload # Conflicts: # CHANGELOG.md # README.md # tests/Unit/HelpersTestCase.php

…one render The gate this replaces watched the render and asked whether formatting had changed anything. That is not a proof of purity. An unregistered `[foo]` comes back byte-identical, reads as static, and the literal `[foo]` gets stored -- and the day a plugin registers that shortcode, every page serves the frozen source text instead of its output, with nothing to log. Each of the three dynamic surfaces in `fieldFormatter()` is now decided before the work runs: - `do_shortcode()` needs an opening bracket, so an item whose raw meta holds no `[` formats to a function of what it stores. The meta is already primed by `wp_get_nav_menu_items()`, so this is a memory read. - A `field_formatter_*` callback may read anything and no static proof of its purity exists, so one registered callback refuses every menu. - A rendered CF7/WPForms embed carries a nonce and is dynamic whatever is stored, so that one surface stays counted during the build. The check is deliberately blunt: a bracket in a plain-text field is not a shortcode and the menu is refused anyway. A false refusal costs one uncached menu; a false accept is wrong on every page. Rejected, and worth saying because the diff cannot: fingerprinting the registered `field_formatter_*` tags into the cache key instead of refusing. It would catch a callback appearing or disappearing, but it cannot make a page-dependent callback safe -- which is the actual risk. Refusing is the smaller and honest answer, and the filter is still there for a project that knows its own formatter is pure. Also not done: `<img` in the gate. `fieldFormatter()` never runs `wp_filter_content_tags`, so an image tag is not a dynamic surface here and rejecting it would refuse menus for no reason. Measured against a real Redis object cache on sloneek, 90 items across five menus: front page 402 ms -> 344-350 ms over two back-to-back runs, `/blog/` 907 ms -> 836-857 ms. Rendered HTML byte-identical, valid control pair either side. The README and CHANGELOG carried an estimate of removed work from when the measurement host had no object cache; both now carry the end-to-end number, which is the smaller claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

The gate read raw post meta and concluded that an item with no `[` could not reach `do_shortcode()`. That is the wrong store. Values arrive through `get_field()`, and ACF can supply one no meta row holds: `acf/pre_load_value` short-circuits the database entirely, `default_value` fills in for an absent row, `acf/load_value` replaces what was loaded, and a group or clone sub-field takes its own default the same way. Verified against acf-value-functions.php::acf_get_value() in the installed copy, not inferred. So a menu whose meta was provably bracket-free could still put `[contextual]` through do_shortcode(), and this cache would store one request's rendered output and replay it on every other page. Found by an adversarial review; the counter-test it specified is test_a_shortcode_acf_supplies_but_no_meta_row_holds_is_refused, which keeps the meta empty on purpose and fails against the previous commit. The check now sits at the do_shortcode() call site and inspects its INPUT. That keeps the property the meta check was reaching for -- decided before the dynamic call, never from whether its output differed -- while seeing the value ACF actually produced, whatever produced it. It is also less code and one fewer meta read per item. Added alongside: MenuFieldsCache::flushStored() / Helpers::flushStoredMenuFields(), a group-scoped delete for the case where an entry is believed wrong and the next content change is too late. BlockRenderer already took this route; the absence here left wp_cache_flush() -- every group, every site on shared infrastructure -- as the only lever. A backend without flush_group support returns false rather than looking successful. Still open, deliberately not fixed here: ACF field-group JSON changes bump neither the posts nor the terms last-changed counter, so a deploy that edits a default value leaves entries reachable for up to the TTL. And an acf/load_value callback that reads the current user is undetectable by any of this. Both are for the owner to weigh against making the whole path opt-in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

parisek · 2026-08-27T13:37:14Z

Agent review, 2026-08-27 — Codex (gpt-5.3-codex) and Fable, run blind to each other

Two reviewers, different lenses: Codex on cache soundness and the purity gate, Fable on downstream blast radius, observability and API surface. Neither saw the other's output.

They converged on one thing, from opposite directions

The ACF value-load layer. Codex reached it directly: the gate read raw post meta, but values arrive through get_field(), and ACF can supply one no meta row holds. Fable reached the same layer from the other end, arguing the roles-only audience signature cannot model two same-role visitors getting different values.

That convergence is what makes it credible, and it was verified against the installed ACF source before acting — acf-value-functions.php::acf_get_value() really does apply acf/pre_load_value first, fall back to default_value when no row exists, and then run acf/load_value. Group and clone sub-fields take their own defaults the same way.

So a menu whose meta was provably bracket-free could still put [contextual] through do_shortcode(), and this cache would have stored one request's rendered output onto every page. My design, my error — the previous commit's gate was unsound.

Fixed

Finding	Reviewer	What changed
Gate reads the wrong store (critical)	Codex	The check moved to the `do_shortcode()` call site and inspects its input. Same property — decided before the dynamic call, never from whether output differed — but on the value ACF actually produced. Less code, one fewer meta read per item.
No way to purge a wrong entry now	Fable	`MenuFieldsCache::flushStored()` / `Helpers::flushStoredMenuFields()`, group-scoped, guarded by `wp_cache_supports('flush_group')`. Fable correctly pointed out `BlockRenderer.php:396` already did exactly this — the precedent was in the same codebase and unused here. Without it the only lever was `wp_cache_flush()`: every group, every site on shared infrastructure.
The counter-test	Codex	`test_a_shortcode_acf_supplies_but_no_meta_row_holds_is_refused` keeps meta empty on purpose. It fails against the previous commit.

Each gate re-verified load-bearing by removal: disabling the input check fails exactly the three shortcode tests and nothing else.

Rejected

Refusing when any acf/load_value callback is registered, mirroring the field_formatter_* treatment. ACFML registers one on every WPML site, and its callback converts link targets by language — and language is already in the key, so it is a function of inputs the key carries. A blanket refusal would disable the feature precisely where it was measured, to guard against a callback shape neither reviewer found in practice.
Fingerprinting registered formatter tags into the key rather than refusing. Catches a callback appearing or disappearing; cannot make a page-dependent callback safe, which is the actual risk.

Reviewer errors worth recording

Fable's blast-radius list named membership and personalization plugins that filter wp_get_nav_menu_items as leak vectors. They are not. The payload is keyed per menu item id and holds only that item's ACF fields; a plugin that removes items changes which slots are asked for, not what a slot contains. The mechanism Fable identified is real, but its examples were the wrong ones — the actual vector is the ACF value layer Codex found. Worth knowing when calibrating the rest of that report.

Codex could not reach the GitHub API and reviewed from the branch diff and commit message rather than the PR description.

Open, and needing an owner's decision rather than a commit

ACF field-group JSON changes bump neither posts nor terms last-changed, so a deploy that edits a default_value or a return format leaves entries reachable for up to the TTL. Codex rated this High. A config version in the signature would fix it, at a per-request cost.
An acf/load_value callback reading the current user is undetectable by any static means. Fable's argument that this makes on-by-default the wrong call is coherent, and is the strongest case for shipping the whole path opt-in for one release.
No observability. No hit/miss signal, and timber_kit_cache_menu_fields receives a bool and a menu id but never the reason the default said no.

Both reviewers were right that these matter. I have not decided them here because (1) and (2) trade correctness against the reach the feature was built for, which is the owner's call, not a reviewer's and not mine.

Field groups load from theme JSON, not from `wp_posts`. A deploy that edits a `default_value` or a return format therefore moves neither the `posts` nor the `terms` last-changed counter, and the cached menu kept serving what the previous definition produced -- until the entry expired. A lifetime is not a correctness bound. Raised as High by an adversarial review. ACF stamps each group with `modified`, and its local-JSON loader takes that from the file, so hashing the groups a menu reads is exact in both directions: an admin edit moves it and so does a deploy. It asks the `nav_menu` screen only. Item groups are located by the same rule -- ACF_Location_Nav_Menu_Item::match() confirms the key is set and hands the decision straight to `nav_menu` -- so one screen covers both, and the answer comes from the memo #154 added rather than from a fresh acf_get_field_groups() walk. An earlier draft asked a second screen and would have spent the 8-10 ms that memo exists to remove. Not added, and worth saying because the diff cannot: the `acf/update_field_group` flush hook proposed alongside this. It is redundant. `flushFieldGroups()` already drops the memo on all four ACF verbs, so the next version read sees the new `modified` by itself. Not covered: a site registering its own ACF location type that narrows groups to specific menu items. sharesNavMenuItemFieldGroups() already detects that shape for the memo, and a config change confined to such a group would not move this token. Both directions are tested. The mirror test matters as much as the first: a version that moved on its own would make every request a miss, and the cache would look like it works while doing nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

The shortcode check reads the value ACF produced, which closes the surface it can see. It cannot see this one: `acf/pre_load_value` short-circuits the database and `acf/load_value` replaces what was read, both before the value exists. A callback there reading the current user leaves nothing behind to detect. Refusing on presence -- the treatment `field_formatter_*` gets -- would be wrong here, and measurably so. ACF registers its own plumbing on this hook: `_acf_apply_hook_variations()` is what makes `acf/load_value/type=wysiwyg` fire at all. ACFML registers one on every WPML site. Measured on the reference site, both are present, so a presence test switches the cache off exactly where it was measured. Callbacks are therefore judged by where they are defined, the same test `navMenuItemSharingIsSafe()` already applies to ACF location types. ACF's own code is trusted because it is the mechanism rather than a policy; ACFML's because what it varies on is the language, and the language is already in the key. `timber_kit_trusted_value_load_roots` lets a project vouch for its own, so the refusal is not a dead end. The variation tags are scanned too, and that is load-bearing rather than thorough: ACF fans the base hook out into `acf/load_value/type=`, `/name=` and `/key=`, so a project callback can sit on a tag the base name never mentions. Measured on the reference site: fourteen variation tags live and not one is named `acf/load_value`. A scan of the two literal tags would report "nothing registered" and store. Anything unreflectable counts as untrusted, internal functions included. A callback whose file cannot be named is precisely the one there is no case for trusting. One test in this commit was written wrong first and is worth recording: it was named "a trusted callback still caches" and asserted that nothing was stored. It passed, for the opposite of its stated reason. The replacement needs a real fixture file, because the gate reflects to a filename and neither a closure declared in the test nor an internal function can stand in for "defined under a trusted root". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

`isCacheable()` computed the objection and threw it away, so "why is my menu not cached" was a source-reading exercise on a package a hundred sites consume. `Helpers::menuCacheDecisions()` returns the decision per menu for the request: 'cache', 'filtered-off', or the objection itself. The reason also reaches `timber_kit_cache_menu_fields` as a third argument, which is the part that changes what a project can express. A boolean lets an author say "cache this whatever you found". A reason lets them say "that formatter is mine and it is pure" while leaving every other refusal standing -- and only the second is a claim an author can honestly make. The third argument is additive: a callback registered with two accepted args keeps receiving two. Ordered cheapest first and stops at the first objection, so a site with no object cache is never asked to walk the filter registry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

…othing The commit that added the field-config version asked the `nav_menu` screen only, on the reasoning that item groups are located by the same rule. ACF_Location_Nav_Menu_Item::match() does delegate to `nav_menu` -- but only after confirming `nav_menu_item` is SET. With that key absent the screen matches no item group at all. Measured on the reference site: the `nav_menu` screen returns zero groups; the item screen returns the one that holds every menu field. So the token was the constant 'nogroups' on every menu, and the version check read as working while versioning nothing. Visible only because the Redis key is printable. Both screens are asked now. The item screen costs no extra walk: fieldGroupsMemoKey() normalizes `nav_menu_item` to a presence marker wherever sharing is safe, so the placeholder id lands on the same memo entry the per-item lookups use. That is what the earlier commit assumed without checking, and it removed the screen that was carrying the answer. The tests passed either way, which is the more useful finding. The stub returned the same group for any screen, so it could not distinguish the two implementations. The new test reproduces the asymmetry the real site has and fails against the previous commit. `nogroups` remains the correct answer for a menu with no ACF fields at all -- four of the five on the reference site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

parisek · 2026-08-27T14:42:45Z

The three open items are now four commits

Each is independent and can be rejected on its own.

Commit	What
`ba657df`	field-config version in the key
`7e19936`	ACF value-load callbacks judged by where they are defined
`5a99dd7`	the refusal reason is reported instead of discarded
`7f87923`	fixes `ba657df`, which versioned nothing — see below

1. Field definitions change without touching content

The key carries a hash of the modified stamps of the groups the menu reads. ACF's local-JSON loader takes modified from the file, so an admin edit moves it and so does a deploy.

The first attempt at this shipped broken and the tests did not catch it. It asked the nav_menu screen only, reasoning that item groups are located by the same rule. ACF_Location_Nav_Menu_Item::match() does delegate to nav_menu — but only after confirming nav_menu_item is set. With that key absent it matches no item group at all. Measured: the nav_menu screen returns zero groups on the reference site; the item screen returns the one holding every menu field. So the token was the constant nogroups on every menu.

It was visible only because the Redis key is printable:

menu-fields-478|nogroups|b1|len|aanon|p0.795…|t0.832…     ← before
menu-fields-478|2b141291ff39|b1|len|aanon|p0.795…|t0.832… ← after

The unit tests passed against both implementations, because the stub returned the same group for any screen. 7f87923 adds one that reproduces the real asymmetry and fails against ba657df.

nogroups is still the right answer for a menu carrying no ACF fields — four of the five here.

2. `acf/load_value` — judged by origin, not by presence

Refusing on presence would have been wrong and measurably so. Both hooks are occupied on the reference site: ACF's own _acf_apply_hook_variations(), which is what makes acf/load_value/type=… fire at all, and ACFML's link converter. A presence test switches the cache off exactly where it was measured.

So callbacks are judged by where they are defined — the same test navMenuItemSharingIsSafe() already applies to location types. ACF is trusted because it is the mechanism, not a policy. ACFML because it varies on the language, and the language is already in the key. timber_kit_trusted_value_load_roots lets a project vouch for its own.

The variation tags are scanned too, and that is load-bearing. The reference site has fourteen of them live and not one is named acf/load_value. A scan of the two literal tags would report "nothing registered" and store.

Verified against the live site after the change — all five menus still cache:

array ( 478 => 'cache', 482 => 'cache', 483 => 'cache', 484 => 'cache', 75 => 'cache' )

3. Observability

Helpers::menuCacheDecisions() returns the decision per menu. The reason also reaches timber_kit_cache_menu_fields as a third argument, which is what changes what a project can express: a boolean only says "cache this whatever you found", a reason lets an author say "that formatter is mine and it is pure" while leaving every other refusal standing.

Additive — a callback registered with two accepted args keeps receiving two.

Not done, deliberately

The acf/update_field_group flush hook proposed alongside item 1. Redundant: flushFieldGroups() already drops the memo on all four ACF verbs, so the next version read sees the new modified by itself.
Making the whole path opt-in. The two gates above close the surfaces the reviews actually found, and the owner's position is that a cache useful to every site should not need a filter to turn on. A site with an unrecognised value-load callback now gets the refusal automatically instead.

Measured, after all five commits

	before (v1.43.0)	after
front page, min	410 ms	371 ms
median	427 ms	400 ms

Rendered HTML byte-identical, valid control pair. 1823 tests, PHPStan clean, CI green. Every gate re-verified load-bearing by removal.

…nder Two gate designs failed on this branch before the third held, and both failures are the kind a future reader would otherwise have to reconstruct from the diff. Watching the render cannot distinguish a pure formatter from one that happened not to fire: an unregistered shortcode is handed straight back, the check reports "nothing moved", and the literal gets stored until a plugin registers it. Reading the stored meta proved the right thing about the wrong data: values arrive through get_field(), and acf/pre_load_value, default_value and acf/load_value can each supply one no meta row holds. The ADR states the rule both violate -- decide every purity claim from an input, before the work, and from the input the dynamic call actually receives -- and tabulates where each surface's claim is decided, so a new dynamic surface has somewhere to be added rather than being left to a value inspection that happens to catch it today. It also records the two corollaries that are easy to erode: refuse where no static proof exists, but judge by origin where refusing on presence would disable the cache almost everywhere (ACF and ACFML both occupy the value-load hooks). And keep the checks blunt -- narrowing one to "is that bracket really a shortcode" reintroduces a claim only a render could support. Consequences name the guard that failed: the config-version check shipped for one commit reading the wrong ACF screen, versioning nothing, and the tests passed either way because the stub answered any screen identically. A gate that cannot be shown to fail is not a gate. Linked from MenuFieldsCache's own docblock, because the class is where a reader meets the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

…g it The check refused any value holding `[`, defended as "blunt on purpose". It is not conservative, it is imprecise: a menu label reading "Ceník [2026]" cost a site its cache and bought nothing, because core would never have treated it as a shortcode either. `do_shortcode()` answers this exactly and cheaply before doing any work -- it returns its input untouched unless a REGISTERED tag name appears in it. The same early exit now decides here: same order, same regex, quoted at the call site so the two can be compared. That precision is only sound with its second half, which is the part worth reading. "Does this string hold a shortcode" is not a property of the string; it is a property of the string AND the registry. Store the answer without carrying the registry and the literal `[foo]` is stored while `foo` is unregistered, then served forever after a plugin registers it -- with no content change to make the entry unreachable. So $shortcode_tags rides in the key. Only the names: a callback swapped behind an unchanged name does not change whether the value was expandable, and closures do not hash. A test asserted the opposite of this and had to be reversed, which is the honest summary of the change: an unregistered shortcode is genuinely static and now caches, and registering the tag makes the old entry unreachable rather than wrong. Both halves are verified by removal -- dropping the key axis fails the reversal test, restoring the blunt check fails that one and the prose test. ADR-0007 is amended in the same commit. It was written an hour earlier and said checks should stay blunt; that was the wrong rule, stated too broadly. The rule it now states: a check may be as precise as its source of truth allows, provided every input that precision depends on is in the key. The narrowing that stays forbidden is the one that cannot be discharged that way -- deciding a registered shortcode is harmless, or that a formatter callback looks pure. Measured on the reference site, 20 registered shortcodes: front page min 362 ms against 410 ms on v1.43.0. Output byte-identical, control pair valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)

parisek added 3 commits August 27, 2026 13:31

parisek self-assigned this Aug 27, 2026

parisek marked this pull request as draft August 27, 2026 12:07

parisek mentioned this pull request Aug 27, 2026

perf(helpers): memoize acf_get_fields() per group #156

Merged

parisek and others added 3 commits August 27, 2026 15:10

Merge remote-tracking branch 'origin/main' into feat/cache-menu-field…

c0f0b41

…-payload # Conflicts: # CHANGELOG.md # README.md # tests/Unit/HelpersTestCase.php

parisek and others added 4 commits August 27, 2026 16:30

parisek marked this pull request as ready for review August 27, 2026 14:50

parisek and others added 2 commits August 27, 2026 16:50

parisek merged commit 412e713 into main Aug 27, 2026
6 checks passed

parisek deleted the feat/cache-menu-field-payload branch August 27, 2026 15:41

parisek mentioned this pull request Aug 27, 2026

perf: the options-page formatFields() is what is left of the context rebuild #147

Open

Navigation Menu

feat(helpers): cache the page-independent half of a formatted menu - #155

feat(helpers): cache the page-independent half of a formatted menu#155
parisek merged 13 commits into
mainfrom
feat/cache-menu-field-payload

parisek commented Aug 27, 2026 •

edited

Loading

Uh oh!

parisek commented Aug 27, 2026

Uh oh!

parisek commented Aug 27, 2026

Uh oh!

parisek commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

parisek commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

From which project

Measured

What is stored, and what is not

The gate proves purity before the work, not after it

Rejected

Invalidation

Tests

Uh oh!

parisek commented Aug 27, 2026

Agent review, round 2 — 2026-08-27

Round 1 fixes: both reviewers confirm they hold

The new finding, and it is the one that matters

Why this is not being patched

Recommendation: do not merge; park it

Fable's smaller findings, for the record

Uh oh!

parisek commented Aug 27, 2026

Agent review, 2026-08-27 — Codex (gpt-5.3-codex) and Fable, run blind to each other

They converged on one thing, from opposite directions

Fixed

Rejected

Reviewer errors worth recording

Open, and needing an owner's decision rather than a commit

Uh oh!

parisek commented Aug 27, 2026

The three open items are now four commits

1. Field definitions change without touching content

2. acf/load_value — judged by origin, not by presence

3. Observability

Not done, deliberately

Measured, after all five commits

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

parisek commented Aug 27, 2026 •

edited

Loading

2. `acf/load_value` — judged by origin, not by presence