Skip to content

perf(wpml): opt-in guard for ACFML's front-end field-definition translation - #158

Merged
parisek merged 3 commits into
mainfrom
perf/acfml-skip-frontend-field-translation
Aug 27, 2026
Merged

perf(wpml): opt-in guard for ACFML's front-end field-definition translation#158
parisek merged 3 commits into
mainfrom
perf/acfml-skip-frontend-field-translation

Conversation

@parisek

@parisek parisek commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Closes #131.

The problem

ACFML translates ACF field definitions — labels, instructions, placeholders, prepend/append, choices, message — on every request that loads a field group. ACFML\Strings\FieldHooks implements IWPML_Frontend_Action, so it registers on the front end with no is_admin() guard and hooks acf/load_field.

Every field then reaches an unmemoized linear scan over a static array, run through WPML's functional library with a closure allocated per element. On a page view none of those strings is rendered, so the whole walk is discarded.

Measured

Site with 117 field groups and 972 top-level fields:

time
baseline 1.09–1.13 s
group-level translation off 1.04–1.14 s (no change)
field-level translation off 0.29 s

A PHP-FPM slowlog over 3973 slow requests put 71 % of deepest-frame samples inside wpml/fp + wpml/collect. Rendered HTML is byte-identical once per-render uniqueId() values are normalised.

Only the field level is expensive, so the flag switches off both by way of the one filter ACFML provides — the cost lives entirely in the fields.

Off by default, and that is a refusal

Switching it on is wrong for a site that renders field definitions to visitors:

  • a theme calling acf_form() — placeholders, instructions and labels become visitor-facing
  • a template printing a select/radio/checkbox choice label rather than its value

Neither is detectable from the kit, so the consuming site asserts it. Same rule as ADR-0007: where no static proof exists, refuse.

Rejected: auto-detecting acf_form()

It looks available and it is a trap. acf_form_head()acf_enqueue_scripts()ACF_Assets::add_actions(), which guards itself with acf_has_done( 'ACF_Assets::add_actions' ). That function sets the flag it reads — verified in api-helpers.php:

function acf_has_done( $name ) {
	if ( acf_raw_setting( "has_done_{$name}" ) ) { return true; }
	acf_update_setting( "has_done_{$name}", true );
	return false;
}

Calling it from a guard would make ACF's own add_actions() believe it had already run and skip registering its asset actions. The detection would break the exact case it was added to protect.

Four contexts, and naming all four is load-bearing

Admin, AJAX, REST and WP-CLI keep the translation. Gutenberg loads field groups over REST, ACF talks to admin-ajax from inside the editor, and migration scripts run under WP-CLI — none of the three is is_admin().

An is_admin()-only guard therefore shows an editor untranslated labels, which nobody reports as a bug in a performance flag. The test proves the difference: reducing the guard to is_admin() alone fails exactly the AJAX and REST cases and nothing else.

The callback returns the incoming value rather than true, so it stays a veto and cannot overrule another plugin's refusal.

Why this is not #107

#107 was closed unmerged, because a shared workaround would outlive the bug it worked around. Right there, and it does not carry here:

#107 (acf/load_reference leak) this
who pays one caller outside WPML's own plugins every front-end render
fixed upstream yes, acfml 3.0-b.1 no
workaround lifetime would outlive the bug bug has no end date

Verified rather than assumed: on a site running acfml 3.0-b.1 / sitepress 5.0.0-b.1, the seven files on the hot path are byte-identical to 2.2.4 and the bundled wpml/fp has identical MD5s. The same comparison does pick up 3.0-b.1's one change — the remove_filter hook-name fix from #107 — so it is not a blind diff.

Retiring this is a one-line default flip once ACFML memoizes its lookup.

Notes

The REST test runs in its own process: a constant cannot be undefined, and REST_REQUEST left standing made every later test in the run look like REST — four unrelated failures before it was isolated.

Downstream implementation and the original evidence: portadesign/sloneek#103. This is that filter moved up, with the four contexts unchanged.

…lation

ACFML translates ACF field DEFINITIONS -- labels, instructions,
placeholders, prepend/append, choices, message -- on every request that
loads a field group. ACFML\Strings\FieldHooks implements
IWPML_Frontend_Action, so it registers on the front end with no is_admin()
guard and hooks acf/load_field. Every field then reaches an unmemoized
linear scan over a static array, run through WPML's functional library with
a closure allocated per element. On a page view none of those strings is
rendered, so the whole walk is discarded.

Measured on a site with 117 field groups and 972 top-level fields: 1.09 s
with the translation, 0.29 s without. A PHP-FPM slowlog over 3973 slow
requests put 71% of deepest-frame samples inside wpml/fp and wpml/collect.
Only the field level is expensive; the group level measured 1.04 s against
1.11 s, so it is free.

Off by default, and that is a refusal rather than timidity. Switching it on
is wrong for a site that renders field definitions to visitors -- a theme
calling acf_form(), or a template printing a select/radio choice LABEL
rather than its value. Neither is detectable from here, so the consuming
site asserts it. Same rule as ADR-0007: where no static proof exists,
refuse.

Rejected, and worth recording because it looks available: detecting
acf_form() through acf_has_done( 'ACF_Assets::add_actions' ). That function
SETS the flag it reads -- verified in api-helpers.php -- so calling it from
a guard would make ACF's own add_actions() believe it had already run and
skip registering its asset actions. The detection would break the very case
it was added to protect.

Naming four contexts rather than one is_admin() check is load-bearing, and
the test proves it: reducing the guard to is_admin() alone fails exactly the
AJAX and REST cases. Gutenberg loads field groups over REST and ACF talks to
admin-ajax from inside it, so an is_admin()-only guard shows an editor
untranslated labels -- a failure nobody reports as a bug in a performance
flag.

The callback returns the incoming value rather than true, so it stays a veto
and cannot overrule another plugin's refusal.

The REST test runs in its own process. A constant cannot be undefined, and
REST_REQUEST left standing made every later test in the run look like REST
-- four unrelated failures before it was isolated.

Refs #131

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)
@parisek parisek self-assigned this Aug 27, 2026
Two shapes make the guard wrong, and an earlier version of this branch
treated them as one -- both undetectable, so the property defaulted off and
the saving went to whoever read the changelog.

They are not equal. A theme calling acf_form() puts labels, instructions
and placeholders in front of a visitor, and that shape DETECTS ITSELF:
acf_form_head() reaches ACF_Assets::add_actions(), which records
has_done_ACF_Assets::add_actions in ACF's settings. The guard now steps
aside on any front-end request that has set a form up, without being told.

The probe reads with acf_raw_setting() and never acf_has_done(). The
rejection of the latter stands and is unchanged -- it writes the flag it
reads -- but a reviewer pointed out the read-only getter that function
itself calls first, which is what makes the detection available at all. The
earlier commit rejected the whole idea on the strength of the wrong half.

What stays undetectable is a template printing a select/radio/checkbox
choice LABEL rather than its value. That is what the property is still for.
The failure if a site needs it and does not set it is one label in the
source language -- quiet, and easy to mistake for an untranslated string,
so it is named in the README as something to check rather than assume when
moving a large existing site onto this version.

Measured on the reference site, current stack, warm Redis: 479 ms with the
translation, 370 ms without. Then the decisive one -- kit default on with
the site's own functions.php filter REMOVED: 342 ms, against 341 ms with
that filter back. The kit's default fully replaces the downstream copy.

Also in this commit, both from review:

- README gets a properties-table row and a section next to Performance. The
  flag existed only in a changelog line and a docblock, which is where a
  feature goes to not be adopted.
- The method comment claimed an unclassified context keeps the translation.
  The code does the opposite and the opposite is the policy. Corrected.

phpstan-stubs/acf.stub corrects acf_raw_setting()'s signature. ACF documents
it @return void and the generated stubs reproduce that faithfully; its body
is `return acf()->get_setting( $name )`. Fixing the type rather than
silencing the error, because the wrongness is the point: the honest getter
is documented as returning nothing, and its state-mutating sibling is the
one that looks usable.

Refs #131

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)
@parisek

parisek commented Aug 27, 2026

Copy link
Copy Markdown
Owner Author

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

Both reviewed this PR and #93 together. Codex: sound, no correctness defect. Fable's findings were about adoption and shape, and two of them are now fixed.

Fixed

Finding Reviewer What changed
The flag exists only in a changelog line and a docblock — README has an ACF section and a Performance section and neither gets a row Fable properties-table row + a section next to Performance
The method comment says an unclassified context keeps the translation; the code does the opposite Codex corrected — the opposite is the policy
A safe read-only alternative to acf_has_done() exists Codex this is the one that changed the design — see below

Codex found the half I got wrong when rejecting auto-detection

The rejection of acf_has_done() stands: it writes the flag it reads, verified again in the installed ACF. But Codex pointed at the getter that function itself calls first — acf_raw_setting() — which reads the same state without touching it.

So acf_form() is detectable safely, and the earlier commit rejected the whole idea on the strength of the wrong half. The guard now steps aside on any front-end request that has set a form up, without being told.

That is what makes the default flip defensible, and the default is now on.

Two shapes, and they were never equal

  • acf_form() — detects itself, handled above.
  • A template printing a select/radio choice label rather than its value — still undetectable, and still what the property is for. Named in the README as something to check rather than assume when moving a large existing site onto this version.

Measured on the current stack

min
translation on 479 ms
translation off 370 ms

And the decisive one — kit default on, with the reference site's own functions.php filter removed: 342 ms, against 341 ms with that filter back. The kit's default fully replaces the downstream copy, which is what licenses deleting it.

Rejected

  • A Site Health check listing field groups that use select/radio/checkbox (Fable). Presence is not usage — most sites have choice fields and never print a label, so the check would be noise on every site and silence on none.
  • Renaming to a positive $acfml_translate_frontend_fields (Fable). It reads better in isolation and worse at the call site: the property would then have to default false to mean the same thing, and "translate = false" is a subtler double negative than "skip = true".

Where a reviewer was wrong, for calibration

Fable's cross-cutting point 2 argued PR #93 should be a StarterBase property like this one, on the grounds that a property "can be flipped once for the whole site" while a parameter needs timber_context() edited. Both need timber_context() edited — the kit does not call formatFields('option') itself; the downstream timber_context() does. The conclusion is still worth acting on for a different reason (a third positional boolean invites a fourth), but the argument given for it does not hold.

Codex could not reach the GitHub API and reviewed from local branches, commit messages and installed plugin sources. Its separate-process REST test and PHPStan run were blocked by a read-only sandbox, not by the code — both pass here.

@parisek
parisek marked this pull request as ready for review August 27, 2026 16:29
@parisek
parisek merged commit d669e9b into main Aug 27, 2026
6 checks passed
@parisek
parisek deleted the perf/acfml-skip-frontend-field-translation branch August 27, 2026 16:29
parisek added a commit that referenced this pull request Aug 27, 2026
…pped it

The release-stamp workflow cut v1.44.0 at 16:23. PR #158 merged at 16:29,
and its Unreleased entry landed under the heading the stamp had just
written -- so CHANGELOG.md credited 1.44.0 with
$acfml_skip_frontend_field_translation while `git show v1.44.0:src/StarterBase.php`
does not contain it.

Nobody would have caught that by reading the file. It was found by checking
the tag before cutting the next release, which is now the reason to keep
doing that.

The entry moves to Unreleased, where it is true, and ships with the next
version alongside this branch's own change.

Carried in this PR rather than its own because the merge with main already
put this file in conflict here, and a two-line move does not earn a third
pull request. Recorded so the next reader knows why an unrelated section
moved in a 404 change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMYy6JHLf4mU4H4Hd47spb (petr@pari.cz)
parisek added a commit that referenced this pull request Aug 27, 2026
`redirect_guess_404_permalink()` matches the requested slug as a PREFIX —
`post_name LIKE 'about%'` — and redirects to whatever comes back first. A reader
following a dead link is told the page moved and then shown something else. That
is worse than being told it is gone: a 404 is a fact, a wrong 301 is an answer.

The query has a leading value and a trailing wildcard, so it cannot use the
`post_name` index, and it runs on every 404 that carries a name. That is a scan
any visitor can ask for as often as they like, with no authentication and no
rate limit.

Genuine canonical redirects are untouched, checked rather than assumed: the
guess is the last thing `redirect_canonical()` tries, after trailing-slash,
`?p=ID`-to-slug and category-base have each had their turn.

`$disable_404_permalink_guess` is on by default, which reverses core. Set it
false on a site that renames slugs without leaving redirects behind and relies
on the guess to catch the fallout.

Also carried here: the ACFML entry moves out of `## [1.44.0]`, which never
shipped it. The release stamp cut 1.44.0 six minutes before #158 merged, and the
merge put that entry under the heading the stamp had just written. Found by
checking the tag before cutting the next release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(wpml): opt-in guard for ACFML's front-end ACF field-definition translation

1 participant