Skip to content

Add flexible_content mapping to fields-migrate/fields-generate - #10

Merged
parisek merged 10 commits into
mainfrom
feat/flexible-content-mapper
Jul 25, 2026
Merged

Add flexible_content mapping to fields-migrate/fields-generate#10
parisek merged 10 commits into
mainfrom
feat/flexible-content-mapper

Conversation

@parisek

@parisek parisek commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

Adds flexible_content mapping to fields-migrate / fields-generate, then closes the identity-integrity bug class that building it exposed.

The feature is one commit. The other seven are review rounds — each found a genuine defect in the previous round's fix, all of the same shape: the wp: overlay is merged last with highest precedence, so it could override any property another component consumes. The final commit stops patching guards and removes the capability instead.

Feature

5ebcf02flexible_content support in both directions: ACF layouts ⇄ compact definition, including per-layout sub_fields, layout key/name pinning, and the acf_fc_layout discriminator.

Hardening rounds

Commit Defect found and closed
08e8b8b Layout key collisions, duplicate layouts, display/location loss, schema parity
410b247 Layout key pinning, accordion key-collision guard, lint temp-file collision
92d1cf3 A layout's ACF name was not pinned, only its key — a rename orphaned existing acf_fc_layout postmeta (ACF stores it by layout name, not key)
3300334 Layout name not unique within its field; field name not unique among siblings — both would alias the same postmeta row
2a577b3 wp.key desynced the sibling name→key map, emitting conditional_logic that referenced a key present nowhere in the output (silent — ships and never fires in the editor). wp:{name:''} escaped the sibling-name guard
d5ab6e2 The previous fix re-keyed the accordion exemption to type === 'accordion', which wp:{type:'accordion'} then bypassed — strictly worse, since it was a discoverable trick rather than an edge case. Also wp.key was never validated (null and bogus shipped). Fixed by forbidding key/name/type inside wp:
50f6bc8 That deny-list was applied to fields and layouts but not the root group, and covered only scalar keys. Reproduced bypasses: root wp:{key}; root wp:{fields:[]} silently emitting zero fields from a two-field definition; wp:{sub_fields} smuggling unvalidated nodes one level deeper; wp:{parent_repeater} overwriting a derived cross-reference
d928401 wp.conditional_logic's dangling-reference check only ever inspected the value when it happened to already be is_array() — a scalar fallback, or a rule missing its field key entirely, skipped every check silently. Also closed a split-brain between fields-validate and fields-generate: the former never ran FieldsGenerator at all, so it accepted YAML the latter rejected.

Final design

Identity now has exactly one path. Reserved inside every wp: overlay, at root / field / any nesting depth / flexible_content layout:

key, name, type, fields, sub_fields, layouts, parent_repeater

RESERVED_WP_PROPS in FieldsGenerator and propertyNames.not.enum in the schema's wpOverlay $def are kept in exact agreement — fields-lint and fields-generate take different paths, so a node guarded by only one of them would be a gap.

Two deliberate carve-outs:

  • wp.accordions stays open. It appears 38 times across the committed installed base, so reserving it would be a breaking change for five downstream projects. Its residual-overlay exclusion list was widened to cover the reserved set instead.
  • wp.conditional_logic is validated, not forbidden. AcfJsonReader.php:283 writes it as VisibleWhenMapper's documented fallback for ACF conditional logic too complex to reduce to visible_when (2+ AND conditions, 2+ OR groups, unmapped operator). Forbidding it would remove the migrator's only escape hatch with no replacement. assertConditionalLogicReferencesResolve() now walks the assembled tree and rejects any reference to a key absent from it.

Why a deny-list rather than an allow-list or a redesign: measured against the entire installed base — 174 generatable .yaml definitions across mairateam, fellows, keypers, proficio-de, proficiohub — there are zero occurrences of any reserved property inside a wp: block. The sanctioned alternatives already exist and are validated: top-level key: (^field_ / ^layout_ / ^group_), name derived from the YAML map key, the semantic type: enum, and wp.acf_type as the ACF-type disambiguator.

Round 8 — wp.conditional_logic shape validation + fields-validate/fields-generate parity

Two more findings, same root cause as the table above wearing a different shape:

  • wp.conditional_logic shape was never validated. assertConditionalLogicReferencesResolve() only inspected the fallback when it happened to already be is_array() — a scalar (wp.conditional_logic: 'bogus') or a rule missing its field key entirely (as opposed to carrying an empty one, which was already caught) skipped every check silently and would reach acf.json verbatim. assertConditionalLogicShapeIsValid() now enforces the canonical ACF shape — array of OR-groups, each an array of AND-rules, each rule a map with a non-empty string field key — before reference resolution runs. The pre-existing "off" markers (false for an ordinary field with no visible_when:; 0 for an accordion pseudo-field) stay exempt.
  • fields-validate and fields-generate disagreed on conditional_logic. fields-validate only ran FieldsSchemaValidator + KindLinter — neither knows about conditional_logic reference resolution, which lives in FieldsGenerator. A YAML with a dangling wp.conditional_logic reference passed fields-validate and failed fields-generate. bin/fields-validate now also calls FieldsGenerator::generate() in-memory (no files written — the same side-effect-free call fields-generate makes before persisting) and surfaces any GenerationValidationException as FAIL. This closes the gap for both the new shape check and the pre-existing dangling-reference check, without duplicating FieldsGenerator's key-derivation logic a second time.
  • Exit-code claim checked, not reproduced. Verified directly that both bin/fields-validate and bin/fields-generate already propagate a non-zero exit code on FAIL — added a permanent CLI-level regression test (tests/Schema/FieldsValidateCliTest.php) rather than leaving it as a one-off manual repro.

Regression-gated the same way as every prior round: all 174 installed-base definitions across mairateam/fellows/keypers/proficio-de/proficiohub still fields-generate --dry-run cleanly, and all 40 eprukaz2025 components still pass fields-migrate + fields-generate — zero new failures vs. the 50f6bc8 baseline.

Test plan

  • TDD throughout: each round's tests written first and confirmed to fail at the parent commit for the right reason.
  • vendor/bin/phpunit green — 429 tests, 942 assertions (was 420/922 before round 8).
  • vendor/bin/phpstan analyse green, 0 errors, no baseline or ignores added.
  • Installed base: all 174 generatable definitions across the five downstream projects regenerate identically at this commit and its parent — zero new failures. Copied to scratch; no writes into any live project.
  • Downstream pilot: all 40 eprukaz2025 components fields-migrate + fields-generate cleanly. Round-trip normalisation closes 108 of 110 pre-existing --wpml findings in that project.
  • Control tests assert both directions — legitimate wp: props (toolbar, new_lines, collapsed, ui) still pass, and every reserved property is rejected on the same field shape. The earlier control tested only benign props, which is why it blessed the bypasses that round 8 found.

Breaking change

Reserving these properties rejects input that previously validated. No current consumer authors them (verified against all 174 installed-base definitions), but the constraint is enforced at both schema and generator level, so this warrants a minor bump under 0.x — 0.3.0.

https://claude.ai/code/session_01J51xpeny2K3mm3LCFVGc2w

Adds ACF flexible_content support end-to-end: AbstractTypeMapper /
AbstractTypeReverseMapper, layout recursion in AcfJsonReader (migrate)
and FieldsGenerator (generate), schema support for a layouts: map
keyed by layout name, and MigrationCompletenessAuditor recursion.

Layout name and every field key survive verbatim (the non-negotiable
fidelity requirement) — verified against the two real eprukaz
fixtures that raised issue #9 (split-content, box-price-reference),
copied into tests/fixtures/migration/corpus-sample/.

flexible_content's own wpml_cf_preferences is deliberately never
auto-reconstructed (unlike group/repeater's always-3): a 5-project
corpus check shows real ACF exports are genuinely inconsistent for
this one field type (absent, or leaf-shaped 1/2, never 3) — captured
verbatim via the existing wp: escape hatch instead of forcing a
default that would be wrong for roughly half the observed corpus.

Closes #9.
@parisek parisek self-assigned this Jul 24, 2026
…ts, display/location loss, schema parity

Addresses two independent adversarial reviews on PR #10:

- CRITICAL: FieldsGenerator now enforces global key uniqueness across an
  entire generated field group (including flexible_content layouts and
  their sub-fields), throwing GenerationValidationException instead of
  silently emitting two ACF fields aliasing the same postmeta key.
- CRITICAL: AcfJsonReader::readLayouts() throws on duplicate layout names
  instead of silently overwriting the earlier layout (and its key).
  MigrationCompletenessAuditor independently detects the same duplicate,
  fixing a masking bug where identically-shaped duplicate layouts produced
  zero violations.
- CRITICAL: layout `display` (block|table|row) and `location` are now
  captured verbatim by the reader (via the layout's own `wp:` escape
  hatch) and replayed by the generator, instead of being hardcoded to
  'block' / null.
- HIGH: component.fields.schema.json's layout $defs now requires `label`
  and constrains layout map keys to ^[a-z][a-z0-9_]*$, matching
  parisek/acf-json-schema's field-flexible_content.schema.json.
- Added regression pins: flexible_content nested inside another
  flexible_content's layout (works), layout-level non-sentinel min/max
  round-tripping through the full reader->generator pipeline.
- Wired parisek/acf-json-schema's AcfLinter into the test suite
  (tests/Integration/AcfLintValidationTest.php): every real-world
  migration fixture's regenerated acf.json is now validated against the
  ecosystem's canonical ACF-shape schema as part of `composer test` (and
  therefore CI) — closing the gap that let the display/location and
  schema-parity regressions ship unnoticed.

Every finding has a test that failed before the fix and passes after.
Verified end-to-end against all 40 real flexible_content-eligible
components in eprukaz (copies, in a temp dir — eprukaz itself untouched):
all 40 migrate and regenerate cleanly, all pass `acf-lint --strict`, and
zero flexible_content/layout-related diffs against the originals.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7
@parisek

parisek commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Pushed fixes for both adversarial reviews (Fable + Codex):

  • A (CRITICAL) — global key-uniqueness guard in FieldsGenerator (throws GenerationValidationException on collision, incl. across flexible_content layouts).
  • B (CRITICAL)AcfJsonReader::readLayouts() throws on duplicate layout names instead of silently overwriting; MigrationCompletenessAuditor independently detects the same duplicate (previously maskable when both layouts shared an identical shape).
  • C (CRITICAL) — layout display/location now round-trip verbatim via a layout-level wp: escape hatch, instead of being hardcoded to block/null.
  • D (HIGH)component.fields.schema.json layout $defs now requires label and constrains layout-map keys to ^[a-z][a-z0-9_]*$, matching parisek/acf-json-schema.
  • E — added regression pins for FC-nested-in-FC-layout and layout-level non-sentinel min/max (both already worked; now covered).
  • F — CHANGELOG entry added under [Unreleased].

Wiring: parisek/acf-json-schema's AcfLinter is now exercised in this repo's own test suite (tests/Integration/AcfLintValidationTest.php) against every real migration fixture's regenerated acf.json — part of composer test/CI going forward. One pre-existing, out-of-scope residual (store-locator's legacy gallery field has wpml_cf_preferences: 2 vs. the schema's const 1) is explicitly skipped with justification in the test — unrelated to flexible_content, predates this PR.

Real-world proof: ran fields-migrate/fields-generate against copies of all 40 real components with acf.json in eprukaz (not 42 — that's the actual count; eprukaz itself was never touched, everything ran against a temp-dir copy). All 40 round-trip cleanly, all pass acf-lint --strict, zero flexible_content/layout-related diffs against the originals for the 2 real components that use it (box-price-reference, split-content).

Every finding has a test that failed before the fix and passed after (TDD). composer test (376 tests, 1 skipped/justified) and composer phpstan both green.

parisek and others added 2 commits July 24, 2026 13:44
…rd, display/location round-trip proof, lint temp-file collision, wpml_cf_preferences data fix

- Finding 1 (CRITICAL): layout keys are now ALWAYS pinned at migration
  time, not only when they deviate from the derived convention. A layout
  whose key happens to match the convention was previously left
  unpinned, so renaming its YAML map key silently re-derived a different
  ACF key on next generation — orphaning acf_fc_layout postmeta. Ordinary
  field-key omission is untouched (that's separate, deliberately tested
  doctrine: a field's `name` IS its identity, a layout's map key isn't).

- Finding 2 (HIGH): the global key-uniqueness guard now runs over the
  FINAL assembled fields list (after RootFieldGroupBuilder interleaves
  accordion pseudo-fields), not the pre-accordion list. A duplicate key
  hidden in wp.accordions[].key previously slipped past undetected.

- Finding 3/4 (MODERATE): added a value-level round-trip test (not just
  schema validity) proving non-default layout `display`/`location`
  survive read -> generate unchanged, using a synthetic fixture (no real
  corpus fixture happens to author non-default values, so the existing
  full-diff round-trip tests never exercised this path and would have
  stayed green through a Finding-C reversion). Clarified the reader's
  docblock: this is deliberate value-level canonicalisation, not
  presence-loss — real ACF exports always carry both keys.

- Finding 5 (MODERATE): investigated the store-locator gallery
  wpml_cf_preferences disagreement on the merits instead of accepting
  the "two packages disagree" framing. parisek/acf-json-schema's
  `const: 1` for gallery/image matches this project's own documented
  doctrine (non-translatable media); definition-kit's WpmlTranslatableMapper
  faithfully round-trips whatever raw value a source has (by design), so
  the anomalous `2` was simply bad legacy data in this repo's OWN test
  fixture (not the live site). Fixed the fixture, dropped the skip
  entirely — acf-lint now passes for every migration fixture, no
  exceptions. acf-json-schema was not touched.

- Finding 6 (LOW): AcfLintValidationTest now writes its temp acf.json
  into a unique per-test directory instead of a shared, fixed
  `sys_get_temp_dir()/acf.json` path — fixes a collision under parallel
  test runs.

- Round-1 finding #5 (empty button_label) re-examined, not fixed:
  documented why the deferral holds (matches the project-wide `''` =
  "not authored" sentinel convention; no real fixture exercises it; ACF
  itself doesn't distinguish absent vs empty button_label at runtime).

Verified against the real eprukaz corpus (40 components, read-only
copies): all 40 regenerate and pass `acf-lint --strict`; the 2 real
flexible_content components (split-content, box-price-reference) show
zero layouts/display/location diffs against the original, by parsed
value.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7
Finding 1 (CRITICAL) — round 3 solved the wrong half. It pinned a
layout's `key` unconditionally but left `name` derived from the YAML
map key (FieldsGenerator::buildLayouts() emitted `'name' =>
(string) $layoutName`). WordPress stores `acf_fc_layout` postmeta BY
NAME, not by key, so renaming the map key — an innocent-looking
refactor — silently changed the regenerated ACF name, orphaning every
`acf_fc_layout` value already stored in production and breaking every
`{% if layout.acf_fc_layout == '...' %}` Twig branch. The existing
round-3 regression test asserted this WRONG behaviour as correct
(`assertSame('heading', $regeneratedLayout['name'])` after a rename) —
that assertion is now removed/replaced.

Fix: AcfJsonReader::readLayouts() now pins `name` verbatim in the
layout definition, exactly like `key` — unconditionally, not only when
it deviates from the map key. FieldsGenerator::buildLayouts() reads
this pinned `name` when present, falling back to the map key only for
a layout authored directly in YAML (never migrated, so the map key IS
the only name that has ever existed). schemas/component.fields.schema.json
gained the `name` property on the `layout` $def (previously
`additionalProperties: false` with no `name` key at all).

New failing-first test:
test_flexible_content_layout_name_is_pinned_so_renaming_the_map_key_does_not_orphan_acf_fc_layout
— reproduces the NAME change specifically (not just the key), fails
before the fix (`'heading'` regenerated) and passes after (`'title'`
regenerated, matching the original ACF export).

Verified: composer test (382 tests, 833 assertions) and composer
phpstan (0 errors) both green. Real-world re-run against all 40
eprukaz components with acf.json (read-only copies): migrate ->
generate -> lint with acf-lint --strict — all 40 pass, zero lint
failures, compared by parsed value with a canary proving the diff
detector isn't a silent no-op. The two real flexible_content
components (split-content, box-price-reference) show zero
layouts/display/location diffs against their originals.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7
@parisek

parisek commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Round-4 fixes (92d1cf3)

Finding 1 (CRITICAL) — round 3 solved the wrong half

Confirmed by reproduction: round 3 pinned a flexible_content layout's key unconditionally but left name derived from the YAML map key. WordPress stores acf_fc_layout postmeta by name, not by key — renaming the map key silently changed the regenerated name, orphaning production postmeta. The round-3 regression test had asserted this as correct (assertSame('heading', $regeneratedLayout['name']) post-rename) — that's now fixed to assert the original name (title) survives.

Fix: AcfJsonReader::readLayouts() now pins name verbatim, unconditionally, exactly like key. FieldsGenerator::buildLayouts() reads the pinned name when present, falls back to the map key only for a layout that's never been migrated. Schema gained name on the layout $def.

New test (fails before fix, passes after): test_flexible_content_layout_name_is_pinned_so_renaming_the_map_key_does_not_orphan_acf_fc_layout.

Finding 2 — doctrine adjudication

Traced the store-locator wpml_cf_preferences dispute to its root: parisek/acf-json-schema's field-image/field-gallery refs hardcode const: 1 unconditionally, but tailwind-base's wordpress/gutenberg.md + wordpress/wpml.md both document 2 for every value field on an ACF Options Page (ACFML permanently locks a 1-flagged field on options pages, since there's no post duplication to copy from). The schema was the thing that was wrong — verified the schema has no location-context mechanism to express "1 outside options pages, 2 on them," so both refs now accept enum: [1, 2] (matching how field-link/field-select/field-url already have no per-type override and fall through to the general enum: [0,1,2,3]).

Opened as a draft PR: parisek/acf-json-schema#29 (assignee @parisek, no reviewer per the self-review-drop convention). Failing-first tests included; composer test + composer phpstan green in that repo.

Revisited round 3's store-locator fixture edit (2 → 1) in light of this: defensible as-is. store-locator's location is block == acf/store-locator (a Gutenberg block, not an options page) — 1 is the doctrine-correct value for that context, unrelated to the options-page exception. Confirmed no eprukaz or local-machine project has an actual options-page image/gallery field with wpml_cf_preferences: 2 today, so I can't point at a concretely-broken live component for that specific combination — but the schema gap itself was real and is now fixed upstream.

Acceptance evidence

  • composer test: 382 tests, 833 assertions, green (definition-kit). composer phpstan: 0 errors.
  • acf-json-schema worktree: 149 tests (1 skipped, needs live WP), green. composer phpstan: 0 errors.
  • Real-world re-run: all 40 eprukaz components with acf.json (read-only copies, eprukaz working tree left clean — verified via git status), migrated → regenerated → linted with acf-lint --strict: 40/40 pass, zero lint failures, compared by parsed value with a canary proving the diff engine actually runs (not a silent no-op — this run genuinely detected diffs in 40/40 components, all attributable to documented, deliberate canonicalizations: ACF-default boilerplate fill-in, wpml_cf_preferences stamped where absent in legacy JSON-only-authored fields, and a pre-existing drift in the corpus itself between twig name: metadata and the acf.json title field — not a definition-kit defect, but worth knowing before mass-regenerating). The two real flexible_content components (split-content, box-price-reference) show zero layouts/display/location diffs.
  • store-locator as it stands today: passes acf-lint --strict (verified via the existing AcfLintValidationTest data-provider case) — it does not exist as a live component in eprukaz or elsewhere on this machine; it's a corpus-sample test fixture only.

Not merged; PR stays draft.

parisek and others added 6 commits July 24, 2026 14:22
Enumerates the full set of WordPress data-identity invariants this
YAML->ACF mapping must hold (docblock on FieldsGenerator) and closes
the two still-open holes:

- Two layouts in the same flexible_content field pinning different
  `key`s but the same `name` produced indistinguishable
  `acf_fc_layout` values (name, not key, is what ACF matches a saved
  row's layout against). Guarded per-field in buildLayouts().
- The schema's `wp:` overlay is fully open and always wins, including
  over `name` -- two sibling fields authored under different
  definition-map keys (hence different derived `key`s) could still
  collide on the actual ACF `name` (= the WordPress postmeta key) via
  `wp: {name: ...}`. Guarded per-level in collectKeys(), with
  accordion pseudo-fields (canonical `name: ''`) exempted.

Each guard has a failing-first test plus a sanity-control test proving
it isn't over-broad (same layout name across different
flexible_content fields, same overridden name at different nesting
levels).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7
…e-collision guard

Two confirmed defects in FieldsGenerator, both reproduced with a failing
test before the fix (TDD):

Defect 1 (HIGH) — generate() builds the sibling name=>key map used to
resolve `visible_when` -> `conditional_logic` from the RAW semantic
fields via siblingKeyMap()/deriveOrPinKey(), which only read a field's
top-level `key:` prop. buildField()'s own final merge gives the open
`wp:` overlay (and therefore `wp.key`) priority over that same prop, so
a field pinning its key via `wp: {key: ...}` produced a
`conditional_logic` entry referencing the DERIVED key while the field
itself shipped under the OVERRIDDEN key — a dangling reference to a key
that exists nowhere in the generated tree, with no exception raised.
Confirmed via the exact repro in the task brief (toggle field with
`wp.key`, sibling `visible_when` referencing it) before touching any
code. Fixed by making deriveOrPinKey() resolve `wp.key` first, matching
buildField()'s own precedence, so the single function is the shared
source of truth for both the map-building and field-building passes at
every nesting level (root, group/repeater sub_fields, flexible_content
layout sub_fields all go through the same deriveOrPinKey()).

Regression tests assert that every key referenced by any emitted
conditional_logic exists somewhere in the whole generated tree (walking
fields/sub_fields/layouts), rather than asserting one hardcoded expected
key — a hardcoded expectation would keep passing even if both sides of
the computation silently drifted onto the same wrong value together.
Covers both the top-level case and one nested inside a group.

Defect 2 (MEDIUM) — collectKeys()'s sibling-name-uniqueness guard
exempted any field whose final `name` is `''`, assuming only accordion
pseudo-fields have an empty name. Since `wp:` is a fully open bag, an
ordinary field can set `wp: {name: ''}` and escape the guard by value;
confirmed with two sibling text fields each carrying `wp: {name: ''}`
generating with no exception (the control case, `wp: {name: 'dup'}` on
both, correctly throws — only the empty-name carve-out was wrong).
Fixed by discriminating on field shape (`type === 'accordion'`, per
RootFieldGroupBuilder::accordionBaseline()) instead of the name value.
Regression tests cover both directions: two wp:{name:''} ordinary
fields must now throw, and two genuine accordions must still coexist
(pre-existing accordion path unaffected).

Also rewrote the class docblock's invariant enumeration: invariant #5
previously claimed there was no internal key/name consistency invariant
to violate "by construction" — false, and exactly what let Defect 1
through undetected for five prior rounds. Replaced it with the real
invariant (every conditional_logic reference must resolve against the
emitted tree) plus an explicit note that `wp.key` is a second,
independent key-setting path parallel to the top-level `key:` prop.
Stopped claiming the enumeration is complete; added a closing note
naming the structural reason new ones keep appearing (the open `wp:`
bag can override any property this class emits, and several of those
properties are independently consumed downstream by a second
component — every such property is a candidate for the same bug class).

Before: 386 tests / 839 assertions, phpstan clean.
After:  390 tests / 846 assertions, phpstan clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7
… path

Six review rounds each found a new identity bug of the same shape: `wp:`
is an unconstrained escape-hatch object merged with HIGHEST precedence in
FieldsGenerator::buildField(), so wp.key / wp.name / wp.type each gave a
field's identity a second, silently-diverging path onto a value the
top-level `key:` prop / the YAML field-map key / the top-level `type:`
prop already owned:

- wp.key desynced the sibling name=>key map, producing a dangling
  conditional_logic reference (round 6's own fix).
- wp.name bypassed the sibling-name-uniqueness guard.
- Round 6's re-keyed accordion exemption (type === 'accordion') was
  itself bypassable via wp: {type: 'accordion', name: 'clash'}.
- wp.key with a bogus or null value shipped a broken key end-to-end.

Rather than patch each symptom, this closes the whole class structurally:
`key`, `name`, and `type` are now forbidden inside every `wp:` bag, at
both the JSON Schema layer (component.fields.schema.json's new
`wpOverlay` $def, referenced from the field-level and layout-level `wp:`
props) and FieldsGenerator's own belt-and-braces check
(assertNoIdentityPropsInWpOverlay(), run before any key derivation),
since in-process callers that build a tree in memory never go through
FieldsSchemaValidator. Each rejection names the sanctioned alternative.

deriveOrPinKey()'s wp.key read path — dead now that wp.key can never
reach it — is removed, leaving exactly one key-resolution path.

Verified safe against the installed base: none of the 174 committed
component <name>.yaml definitions across mairateam/fellows/keypers/
proficio-de/proficiohub set key/name/type inside any wp: block, and
AcfJsonReader already routes a non-derivable key/name/type to the
validated top-level channel, never into wp: (confirmed: 'type', 'name',
'key' are unconditionally added to $consumed before the leftover diff,
so $leftover can never carry them) — so the deny-list cannot break
migration. Root-level wp: (group description/accordions) is untouched;
it's a separate, already-safe mechanism (AcfJsonReader never routes a
non-derivable group key through wp: either).

Layout wp: gets the same three-prop deny-list for symmetry, even though
buildLayouts() never reads layout.wp.type/name/key today — it already
reads layout.wp.display/location, so the divergence hazard is latent
there too.

397 -> 403 tests (+13 including 4 schema-layer tests), phpstan clean.
Round 7 (d5ab6e2) forbade key/name/type inside wp: but missed four more
bypasses of the same "identity has exactly one path" shape:

- ROOT wp: was never walked by FieldsGenerator's own belt-and-braces
  check (only $definitionTree['fields'] was) or covered by the schema
  (root `wp` was a bare {"type":"object"}). RootFieldGroupBuilder merges
  root wp: with HIGHEST precedence over the key/fields it just
  assembled, so wp.key silently repointed the group's key and
  wp.fields silently zeroed the ENTIRE generated fields list — a
  2-field definition emitted an empty acf.json with no error anywhere.
- wp.sub_fields / wp.layouts on an ORDINARY (non-container,
  non-flexible_content) field survived to generated output: buildField()
  merges wpOverrides before the container branch would overwrite
  sub_fields/layouts, but a leaf field never reaches that branch.
- wp.parent_repeater on a repeater's child is merged AFTER the real,
  ACF-derived value is assigned, silently overwriting it.

Fix widens the reserved set from {key, name, type} to also include the
structural containers that carry a NESTED node's identity
(fields, sub_fields, layouts) and parent_repeater (ACF-computed
metadata with no legitimate authoring path at all — reserved outright,
not given an "alternative"). The schema's `wpOverlay` $def and
FieldsGenerator's own belt-and-braces check now share one deny-list
(RESERVED_WP_PROPS) enforced identically at root, field (any depth),
and layout level via one shared assertion helper. Root `wp` now `$ref`s
the same wpOverlay def as field/layout `wp`.

RootFieldGroupBuilder's accordion-residual overlay (wp.accordions[])
previously excluded only key/label/open/before, letting a captured
accordion element impersonate an arbitrary field via type/name (or
smuggle fields/sub_fields/layouts/parent_repeater) despite
accordionBaseline() fixing type: 'accordion' / name: '' immediately
above. Widened to the same reserved set. wp.accordions itself is
untouched — still the sole legitimate way to author accordions (38
installed-base uses).

wp.conditional_logic is deliberately NOT added to the reserved set.
Investigated Migration\AcfJsonReader:283, which writes
wp.conditional_logic as VisibleWhenMapper's documented fallback for raw
ACF conditional logic too complex to reduce to visible_when (2+ AND
conditions, 2+ OR groups, unmapped operator) — a real round-trip path,
not a bypass. None of the 174 installed-base component definitions
across mairateam/fellows/keypers/proficio-de/proficiohub currently
author it (grepped; only vendor/parisek/definition-kit's own schema
docs mention the string), so forbidding it outright would not break any
current consumer — but it would permanently remove the migrator's only
escape hatch for components with genuinely complex ACF conditional
logic, with no sanctioned replacement. Per the correctness gap that
actually matters (a dangling `field` reference ACF's editor would show
as broken with zero diagnostic), FieldsGenerator now validates every
wp.conditional_logic fallback's referenced keys resolve against the
final assembled tree, throwing GenerationValidationException on a
dangling reference instead of silently shipping one.

Tests: 17 new (91 -> new counts across the two files; repo-wide 403 ->
420 tests, 866 -> 922 assertions), covering each reserved prop at every
level (root/field/nested-depth-2/layout), the four reproduced findings
[1]-[4], a strengthened control test asserting legitimate installed-base
props (toolbar/new_lines/collapsed/ui) still pass AND every newly
reserved prop is rejected on the same field shape, genuine accordions
still working end to end, and both directions of the
conditional_logic dangling-reference gate.

phpstan: clean (0 errors), same as baseline.

Installed-base regeneration gate: all 174 generatable .yaml definitions
across mairateam(49)/fellows(41)/keypers(40)/proficio-de(34)/
proficiohub(10) still fields-generate cleanly (0 failed, same as
baseline d5ab6e2 — zero new failures). All 40 eprukaz components with
acf.json still fields-migrate + fields-generate cleanly (0 failed).
Both runs against read-only copies under scratch — no writes into
~/Sites/wordpress/*.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J51xpeny2K3mm3LCFVGc2w
…ith fields-generate

Fix 1 — assertConditionalLogicReferencesResolve() only ever inspected
`conditional_logic` when it happened to already be `is_array()`, so a
malformed fallback (a scalar, or a rule missing its `field` key) skipped
every check silently and reached acf.json verbatim. Add
assertConditionalLogicShapeIsValid(), enforcing the canonical ACF shape
(array of OR-groups, each an array of AND-rules, each rule a map with a
non-empty string `field` key) before reference resolution runs. Falsy
markers (`false` — FieldReconstructor's "no visible_when" default; `0` —
RootFieldGroupBuilder's accordion "off" marker) stay exempt, matching
existing behaviour.

Fix 2 — bin/fields-validate previously only ran FieldsSchemaValidator +
KindLinter, so it accepted YAML that fields-generate would reject via
FieldsGenerator's conditional_logic checks (both the new shape check and
the existing dangling-reference check). fields-validate now also calls
FieldsGenerator::generate() in-memory (no files written — the same
side-effect-free call fields-generate makes before persisting) and
surfaces any GenerationValidationException as FAIL. This gives full
parity for conditional_logic between the two tools, including reference
resolution, without duplicating FieldsGenerator's key-derivation logic.

Fix 3 — verified bin/fields-validate and bin/fields-generate already
propagate a non-zero exit code on FAIL; added a permanent CLI-level
regression test (tests/Schema/FieldsValidateCliTest.php) rather than
leaving it as a one-off manual repro.

Regression-gated against real definitions: 174/174 real components
across mairateam/fellows/keypers/proficio-de/proficiohub still generate
cleanly (fields-generate --dry-run), and 40/40 eprukaz components still
pass fields-migrate + fields-generate, with zero new failures vs the
50f6bc8 baseline.

Tests: 420 -> 429 (922 -> 942 assertions). PHPStan clean.
…ogic work

The [Unreleased] section documented only the flexible_content feature and
the first review rounds. The most consequential change on the branch -- the
breaking reservation of identity and cross-reference properties inside wp:,
plus the conditional_logic shape/reference validation and the
fields-validate/fields-generate parity fix -- was missing entirely.

Sections reordered to Added/Changed/Fixed, matching 0.2.0's convention.
@parisek
parisek marked this pull request as ready for review July 25, 2026 08:58
@parisek
parisek merged commit e7ac21e into main Jul 25, 2026
3 checks passed
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.

1 participant