From 5ebcf02aad79d64cd2aa79b2d584e703310d8b9b Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 12:23:53 +0200 Subject: [PATCH 01/10] Add flexible_content mapping to fields-migrate/fields-generate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- schemas/acf-defaults-baseline.yaml | 12 + schemas/acf-field-group.output.schema.json | 11 + schemas/component.fields.schema.json | 26 +- schemas/constraint-sentinels.yaml | 3 + src/Generator/AbstractTypeReverseMapper.php | 17 + src/Generator/FieldReconstructor.php | 27 +- src/Generator/FieldsGenerator.php | 62 ++ src/Migration/AbstractTypeMapper.php | 19 + src/Migration/AcfJsonReader.php | 114 +++- .../MigrationCompletenessAuditor.php | 38 +- .../AbstractTypeReverseMapperTest.php | 13 + tests/Generator/FieldReconstructorTest.php | 23 + tests/Generator/FieldsGeneratorTest.php | 65 ++ tests/Generator/GenerationRoundTripTest.php | 119 ++++ tests/Migration/AbstractTypeMapperTest.php | 15 + tests/Migration/AcfJsonReaderTest.php | 138 +++++ .../MigrationCompletenessAuditorTest.php | 68 +++ tests/Schema/FieldsSchemaValidatorTest.php | 17 + ...id-flexible-content-no-layouts.fields.yaml | 5 + .../box-price-reference/acf.json | 555 ++++++++++++++++++ .../corpus-sample/split-content/acf.json | 443 ++++++++++++++ .../valid-flexible-content.fields.yaml | 29 + 22 files changed, 1803 insertions(+), 16 deletions(-) create mode 100644 tests/fixtures/invalid-flexible-content-no-layouts.fields.yaml create mode 100644 tests/fixtures/migration/corpus-sample/box-price-reference/acf.json create mode 100644 tests/fixtures/migration/corpus-sample/split-content/acf.json create mode 100644 tests/fixtures/valid-flexible-content.fields.yaml diff --git a/schemas/acf-defaults-baseline.yaml b/schemas/acf-defaults-baseline.yaml index 205aa7e..5bcbecb 100644 --- a/schemas/acf-defaults-baseline.yaml +++ b/schemas/acf-defaults-baseline.yaml @@ -152,6 +152,18 @@ group: repeater: rows_per_page: 20 +# flexible_content deliberately has NO baseline entry for +# `wpml_cf_preferences` (unlike group/repeater, which always carry the +# container value `3`) — a 4-project cross-corpus check +# (split-content/box-price-reference vs. ettin/pm-a/perfectaparasols) +# shows real ACF exports are genuinely inconsistent for this one field +# type: absent entirely on two components, present as a leaf-shaped `1` +# or `2` on three others, never observed as `3`. Generator\FieldReconstructor +# and Migration\AcfJsonReader special-case flexible_content to leave this +# prop alone entirely rather than force a default that would be wrong for +# roughly half the observed corpus — see both classes' own docblocks. +flexible_content: {} + accordion: # entire pseudo-field type is dropped, not merged — kept here only for # documentation. multi_expand/endpoint/open are ACF UI accordion knobs with diff --git a/schemas/acf-field-group.output.schema.json b/schemas/acf-field-group.output.schema.json index e0c082f..b9e74fd 100644 --- a/schemas/acf-field-group.output.schema.json +++ b/schemas/acf-field-group.output.schema.json @@ -21,6 +21,17 @@ "name": { "type": "string" }, "type": { "type": "string", "minLength": 1 }, "label": { "type": "string" }, + "sub_fields": { "type": "array", "items": { "$ref": "#/$defs/field" } }, + "layouts": { "type": "array", "items": { "$ref": "#/$defs/layout" } } + } + }, + "layout": { + "type": "object", + "required": ["key", "name"], + "properties": { + "key": { "type": "string", "minLength": 1 }, + "name": { "type": "string" }, + "label": { "type": "string" }, "sub_fields": { "type": "array", "items": { "$ref": "#/$defs/field" } } } } diff --git a/schemas/component.fields.schema.json b/schemas/component.fields.schema.json index 15e2c15..6f77633 100644 --- a/schemas/component.fields.schema.json +++ b/schemas/component.fields.schema.json @@ -73,7 +73,7 @@ "additionalProperties": false, "properties": { "type": { - "enum": ["text", "richtext", "number", "boolean", "select", "media", "link", "reference", "group", "repeater", "date"] + "enum": ["text", "richtext", "number", "boolean", "select", "media", "link", "reference", "group", "repeater", "flexible_content", "date"] }, "role": { "enum": ["field", "query", "computed", "global"] }, "label": { "type": "string", "minLength": 1 }, @@ -106,7 +106,8 @@ "source": { "type": "string" }, "demo": {}, "wp": { "type": "object" }, - "fields": { "$ref": "#/$defs/fieldMap" } + "fields": { "$ref": "#/$defs/fieldMap" }, + "layouts": { "$ref": "#/$defs/layoutMap" } }, "allOf": [ { @@ -116,8 +117,29 @@ { "if": { "properties": { "type": { "enum": ["group", "repeater"] } }, "required": ["type"] }, "then": { "required": ["fields"] } + }, + { + "if": { "properties": { "type": { "const": "flexible_content" } }, "required": ["type"] }, + "then": { "required": ["layouts"] } } ] + }, + "layoutMap": { + "type": "object", + "minProperties": 1, + "additionalProperties": { "$ref": "#/$defs/layout" } + }, + "layout": { + "type": "object", + "required": ["fields"], + "additionalProperties": false, + "properties": { + "label": { "type": "string", "minLength": 1 }, + "key": { "type": "string", "pattern": "^layout_" }, + "min": { "type": "integer", "minimum": 0 }, + "max": { "type": "integer", "minimum": 0 }, + "fields": { "$ref": "#/$defs/fieldMap" } + } } } } diff --git a/schemas/constraint-sentinels.yaml b/schemas/constraint-sentinels.yaml index fec1655..410c5b0 100644 --- a/schemas/constraint-sentinels.yaml +++ b/schemas/constraint-sentinels.yaml @@ -38,6 +38,9 @@ number: repeater: min: 0 max: 0 +flexible_content: + min: 0 + max: 0 file: min_size: '' max_size: '' diff --git a/src/Generator/AbstractTypeReverseMapper.php b/src/Generator/AbstractTypeReverseMapper.php index 9f44750..61ec4e5 100644 --- a/src/Generator/AbstractTypeReverseMapper.php +++ b/src/Generator/AbstractTypeReverseMapper.php @@ -36,6 +36,7 @@ public function reverse(array $semanticField): array 'date' => ['acfType' => 'date_picker', 'extra' => []], 'group' => ['acfType' => 'group', 'extra' => []], 'repeater' => $this->repeater($semanticField), + 'flexible_content' => $this->flexibleContent($semanticField), default => throw new \DomainException(sprintf( "Unsupported abstract type '%s' — add a case to AbstractTypeReverseMapper::reverse().", $type, @@ -151,4 +152,20 @@ private function repeater(array $field): array } return ['acfType' => 'repeater', 'extra' => $extra]; } + + /** + * Mirrors repeater() — flexible_content shares the identical + * add_label <-> button_label bijection. + * + * @param array $field + * @return array{acfType: string, extra: array} + */ + private function flexibleContent(array $field): array + { + $extra = []; + if (!empty($field['add_label'])) { + $extra['button_label'] = (string) $field['add_label']; + } + return ['acfType' => 'flexible_content', 'extra' => $extra]; + } } diff --git a/src/Generator/FieldReconstructor.php b/src/Generator/FieldReconstructor.php index a976fd2..5c35c46 100644 --- a/src/Generator/FieldReconstructor.php +++ b/src/Generator/FieldReconstructor.php @@ -24,6 +24,21 @@ final class FieldReconstructor { private const CONTAINER_ACF_TYPES = ['group', 'repeater']; + /** + * flexible_content's own `wpml_cf_preferences` is deliberately NEVER + * auto-reconstructed here — unlike group/repeater (always the + * container value `3`) or every leaf type (always 1/2), real ACF + * exports are genuinely inconsistent for this one field type (absent + * entirely on some components, a leaf-shaped 1/2 on others, never `3` + * — see acf-defaults-baseline.yaml's flexible_content comment for the + * corpus census). Whatever value (or absence) the source had is + * captured verbatim in the field's own `wp.wpml_cf_preferences` by + * Migration\AcfJsonReader and re-applied by + * Generator\FieldsGenerator's `wp:` overlay — this class simply never + * emits a default that would be wrong for roughly half the corpus. + */ + private const NO_AUTO_WPML_TYPES = ['flexible_content']; + public function __construct( private readonly AbstractTypeReverseMapper $typeMapper = new AbstractTypeReverseMapper(), private readonly VisibleWhenMapper $visibleWhenMapper = new VisibleWhenMapper(), @@ -48,10 +63,12 @@ public function reconstruct(array $semanticField, array $nameKeyMap): array $out['instructions'] = (string) ($semanticField['description'] ?? ''); $isContainer = in_array($acfType, self::CONTAINER_ACF_TYPES, true); - $out['wpml_cf_preferences'] = $this->wpmlMapper->toWpmlPreference( - $acfType, - !$isContainer && true === ($semanticField['translatable'] ?? false), - ); + if (!in_array($acfType, self::NO_AUTO_WPML_TYPES, true)) { + $out['wpml_cf_preferences'] = $this->wpmlMapper->toWpmlPreference( + $acfType, + !$isContainer && true === ($semanticField['translatable'] ?? false), + ); + } if (isset($semanticField['maxlength'])) { $out['maxlength'] = (int) $semanticField['maxlength']; @@ -64,7 +81,7 @@ public function reconstruct(array $semanticField, array $nameKeyMap): array } } } - if ('repeater' === $acfType) { + if (in_array($acfType, ['repeater', 'flexible_content'], true)) { foreach (['min', 'max'] as $prop) { if (isset($semanticField[$prop])) { $out[$prop] = $semanticField[$prop]; diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 9ec8148..3b5f5d5 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -131,11 +131,73 @@ private function buildField( ); } $field['sub_fields'] = $subFields; + } elseif ('flexible_content' === $acfType) { + $field['layouts'] = $this->buildLayouts( + (array) ($semanticField['layouts'] ?? []), + $componentSlug, + [...$nameChain], + ); } return $this->orderAcfProps($field); } + /** + * Builds a flexible_content field's raw `layouts` array from the + * abstract `layouts` map (name => layout definition) — the + * layout-shaped counterpart to the `sub_fields` loop above. Each + * layout's own sub_fields recurse through the SAME buildField() used + * for ordinary nesting, one chain segment deeper (`[...$nameChain, + * $layoutName]`), so keys/conditional-logic resolution derive + * identically to any other nested field. + * + * A layout's own children never carry `parent_repeater` — verified + * against both eprukaz corpus fixtures (split-content, + * box-price-reference): every sub-field nested inside a + * flexible_content layout carries no `parent_repeater` at all, unlike + * a repeater's direct children. Passing `null` below reproduces that. + * + * @param array $layoutDefs layout name => layout definition + * @param list $nameChain the flexible_content field's OWN full name chain + * @return list> + */ + private function buildLayouts(array $layoutDefs, string $componentSlug, array $nameChain): array + { + $layouts = []; + foreach ($layoutDefs as $layoutName => $layoutDef) { + $layoutChain = [...$nameChain, (string) $layoutName]; + $layoutKey = (string) ($layoutDef['key'] ?? ('layout_' . $componentSlug . '_' . implode('_', $layoutChain))); + + $childFields = (array) ($layoutDef['fields'] ?? []); + $childSiblingMap = $this->siblingKeyMap($childFields, $componentSlug, $layoutChain); + + $subFields = []; + foreach ($childFields as $childName => $childField) { + $subFields[] = $this->buildField( + $childField, + $componentSlug, + [...$layoutChain, $childName], + $childSiblingMap, + null, + ); + } + + $layout = [ + 'key' => $layoutKey, + 'name' => (string) $layoutName, + 'label' => (string) ($layoutDef['label'] ?? ''), + 'display' => 'block', + 'sub_fields' => $subFields, + 'min' => $layoutDef['min'] ?? '', + 'max' => $layoutDef['max'] ?? '', + 'location' => null, + ]; + + $layouts[] = $layout; + } + return $layouts; + } + /** * @param array $fields * @param list $parentChain diff --git a/src/Migration/AbstractTypeMapper.php b/src/Migration/AbstractTypeMapper.php index 8d3edfe..73fd3e0 100644 --- a/src/Migration/AbstractTypeMapper.php +++ b/src/Migration/AbstractTypeMapper.php @@ -80,6 +80,7 @@ public function map(array $acfField): array 'date_picker' => ['type' => 'date', 'extra' => [], 'consumed' => ['type']], 'group' => ['type' => 'group', 'extra' => [], 'consumed' => ['type', 'sub_fields']], 'repeater' => $this->repeater($acfField), + 'flexible_content' => $this->flexibleContent($acfField), default => throw new \DomainException(sprintf( "Unsupported ACF field type '%s' for field '%s' — add a case to AbstractTypeMapper::map().", $type, @@ -150,4 +151,22 @@ private function repeater(array $acfField): array } return ['type' => 'repeater', 'extra' => $extra, 'consumed' => ['type', 'sub_fields', 'button_label']]; } + + /** + * `layouts` is consumed here as a single unit (like repeater's own + * `sub_fields`) — AcfJsonReader::readField() is what actually + * recurses into each layout's own sub_fields, exactly mirroring the + * repeater/group recursion one level down. + * + * @param array $acfField + * @return array{type: string, extra: array, consumed: list} + */ + private function flexibleContent(array $acfField): array + { + $extra = []; + if (!empty($acfField['button_label'])) { + $extra['add_label'] = (string) $acfField['button_label']; + } + return ['type' => 'flexible_content', 'extra' => $extra, 'consumed' => ['type', 'layouts', 'button_label']]; + } } diff --git a/src/Migration/AcfJsonReader.php b/src/Migration/AcfJsonReader.php index e862130..ca67779 100644 --- a/src/Migration/AcfJsonReader.php +++ b/src/Migration/AcfJsonReader.php @@ -20,7 +20,7 @@ final class AcfJsonReader 'maxlength', 'min', 'max', 'step', 'accept', 'max_size', 'min_width', 'max_width', 'min_height', 'max_height', 'kind', 'shape', 'multiline', 'of', 'multiple', 'options', - 'add_label', 'placeholder', 'visible_when', 'fields', 'role', 'key', 'wp', + 'add_label', 'placeholder', 'visible_when', 'fields', 'layouts', 'role', 'key', 'wp', ]; /** Structural boilerplate dropped unconditionally, never lifted, never in wp:. */ @@ -200,8 +200,16 @@ private function readField(array $acfField, string $componentSlug, array $nameCh // field's kind (leaf: 1/2, container: 3) is lifted; an anomalous value // (e.g. a leaf carrying 3, a container carrying 2) is left verbatim in // wp: so it round-trips losslessly — see WpmlTranslatableMapper. + // + // flexible_content is deliberately EXCLUDED here regardless of value — + // see acf-defaults-baseline.yaml's flexible_content comment: real ACF + // exports are inconsistent (absent entirely, or a leaf-shaped 1/2, + // never observed as a container's 3), so this prop is never lifted to + // `translatable` for this type; whatever raw value is present (if + // any) survives verbatim in wp.wpml_cf_preferences via the leftover + // computation below instead. $wpml = $acfField['wpml_cf_preferences'] ?? null; - if (is_int($wpml) && $this->wpmlMapper->isCanonical($acfType, $wpml)) { + if ('flexible_content' !== $acfType && is_int($wpml) && $this->wpmlMapper->isCanonical($acfType, $wpml)) { if ($this->wpmlMapper->translatable($acfType, $wpml)) { $out['translatable'] = true; } @@ -225,11 +233,15 @@ private function readField(array $acfField, string $componentSlug, array $nameCh $consumed[] = $prop; } } - if ('repeater' === $acfType) { - // 0 is ACF's own "no limit" sentinel for repeater row bounds — - // omitted, matching ACF's UI semantics. Deliberate correction - // vs. the prototype (transform3.php), which dropped repeater - // min/max unconditionally — see Global Constraints. + if (in_array($acfType, ['repeater', 'flexible_content'], true)) { + // 0 is ACF's own "no limit" sentinel for repeater/flexible_content + // row bounds — omitted, matching ACF's UI semantics. Deliberate + // correction vs. the prototype (transform3.php), which dropped + // repeater min/max unconditionally — see Global Constraints. + // flexible_content shares the identical raw shape and sentinel + // convention (verified against the corpus: eprukaz's two fixtures + // author real 2/2 bounds; ettin/pm-a leave both as the '' sentinel; + // perfectaparasols authors min:1 with max left at the 0 sentinel). foreach (['min', 'max'] as $prop) { $raw = $acfField[$prop] ?? ''; if ('' !== $raw && 0 !== $raw && '0' !== $raw) { @@ -291,6 +303,21 @@ private function readField(array $acfField, string $componentSlug, array $nameCh $consumed[] = 'sub_fields'; } + if ('flexible_content' === $acfType) { + if (empty($acfField['layouts'])) { + throw new \RuntimeException(sprintf( + "Field '%s' is a flexible_content with zero layouts after migration — " + . 'the schema forbids an empty layouts map.', + (string) $acfField['name'], + )); + } + $childChain = [...$nameChain, (string) $acfField['name']]; + /** @var list> $rawLayouts */ + $rawLayouts = (array) $acfField['layouts']; + $out['layouts'] = $this->readLayouts($rawLayouts, $componentSlug, $childChain, $keyNameMap); + $consumed[] = 'layouts'; + } + $expectedKey = 'field_' . $componentSlug . '_' . implode('_', [...$nameChain, (string) $acfField['name']]); if ((string) $acfField['key'] !== $expectedKey) { $out['key'] = (string) $acfField['key']; @@ -312,6 +339,70 @@ private function readField(array $acfField, string $componentSlug, array $nameCh return $this->orderField($out); } + /** + * Recurses into a flexible_content field's `layouts` array — each raw + * ACF layout becomes one entry in the abstract `layouts` map, keyed by + * its own `name` (mirroring how a group/repeater's `sub_fields` become + * a map keyed by field name — see this class's own doc header). The + * layout's own sub_fields recurse through the SAME readField() used + * for ordinary nesting, one chain segment deeper (`[...$nameChain, + * $layoutName]`), so field keys/`parent_repeater` derive identically + * to any other nested field — a layout is "just another nesting level" + * from the key-derivation and conditional-logic-resolution point of + * view, it only carries its own `label`/`key`/`min`/`max` on top. + * + * @param list> $layouts raw ACF layouts, in source order + * @param list $nameChain the flexible_content field's OWN full name chain + * @param array $keyNameMap + * @return array layout name => layout definition + */ + private function readLayouts(array $layouts, string $componentSlug, array $nameChain, array $keyNameMap): array + { + $out = []; + foreach ($layouts as $layout) { + $layoutName = (string) $layout['name']; + $layoutChain = [...$nameChain, $layoutName]; + + $children = []; + foreach ((array) ($layout['sub_fields'] ?? []) as $sub) { + if ('accordion' === ($sub['type'] ?? null)) { + continue; + } + $children[(string) $sub['name']] = $this->readField($sub, $componentSlug, $layoutChain, $keyNameMap); + } + if ([] === $children) { + throw new \RuntimeException(sprintf( + "Layout '%s' has zero non-accordion sub-fields after migration — " + . 'the schema forbids an empty fields map.', + $layoutName, + )); + } + + $layoutOut = []; + if ('' !== (string) ($layout['label'] ?? '')) { + $layoutOut['label'] = (string) $layout['label']; + } + // min/max share the exact repeater/flexible_content field-level + // sentinel rule ('' and 0 both mean "no per-layout row limit + // authored") — see the field-level min/max handling above. + foreach (['min', 'max'] as $prop) { + $raw = $layout[$prop] ?? ''; + if ('' !== $raw && 0 !== $raw && '0' !== $raw) { + $layoutOut[$prop] = $raw + 0; + } + } + $layoutOut['fields'] = $children; + + $expectedLayoutKey = 'layout_' . $componentSlug . '_' . implode('_', $layoutChain); + if ((string) $layout['key'] !== $expectedLayoutKey) { + $layoutOut['key'] = (string) $layout['key']; + } + + $out[$layoutName] = $layoutOut; + } + return $out; + } + /** * @param array> $fields * @param array $map @@ -327,6 +418,15 @@ private function buildKeyNameMap(array $fields, array &$map): void $subFields = (array) $f['sub_fields']; $this->buildKeyNameMap($subFields, $map); } + if (!empty($f['layouts'])) { + /** @var list> $layouts */ + $layouts = (array) $f['layouts']; + foreach ($layouts as $layout) { + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $this->buildKeyNameMap($layoutSubFields, $map); + } + } } } diff --git a/src/Migration/MigrationCompletenessAuditor.php b/src/Migration/MigrationCompletenessAuditor.php index 0730efa..84b85db 100644 --- a/src/Migration/MigrationCompletenessAuditor.php +++ b/src/Migration/MigrationCompletenessAuditor.php @@ -159,7 +159,11 @@ private function auditFields(array $acfFields, array $definitionFields, string $ } $rawWpml = $acfField['wpml_cf_preferences'] ?? null; - if (is_int($rawWpml) && $this->wpmlMapper->isCanonical($type, $rawWpml)) { + // flexible_content is excluded — see FieldReconstructor::NO_AUTO_WPML_TYPES + // and AcfJsonReader's own wpml exclusion for the corpus rationale; + // whatever value is present survives via the generic "everything + // else" leftover/wp: check at the bottom of this method instead. + if ('flexible_content' !== $type && is_int($rawWpml) && $this->wpmlMapper->isCanonical($type, $rawWpml)) { $accounted[] = 'wpml_cf_preferences'; if ($reconstructed['wpml_cf_preferences'] !== $rawWpml) { $violations[] = sprintf( @@ -188,7 +192,7 @@ private function auditFields(array $acfFields, array $definitionFields, string $ } } - if ('repeater' === $type) { + if (in_array($type, ['repeater', 'flexible_content'], true)) { foreach (self::REPEATER_BOUND_PROPS as $prop) { $accounted[] = $prop; $raw = $acfField[$prop] ?? ''; @@ -282,6 +286,27 @@ private function auditFields(array $acfFields, array $definitionFields, string $ ...$this->auditFields($subFields, $childDefFields, $path, $keyNameMap), ]; } + + if (!empty($acfField['layouts'])) { + $defLayouts = (array) ($defField['layouts'] ?? []); + /** @var list> $layouts */ + $layouts = (array) $acfField['layouts']; + foreach ($layouts as $layout) { + $layoutName = (string) ($layout['name'] ?? ''); + $layoutPath = "{$path}.{$layoutName}"; + if (!array_key_exists($layoutName, $defLayouts)) { + $violations[] = "{$layoutPath}: layout missing from migrated definition entirely"; + continue; + } + $defLayout = $defLayouts[$layoutName]; + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $violations = [ + ...$violations, + ...$this->auditFields($layoutSubFields, (array) ($defLayout['fields'] ?? []), $layoutPath, $keyNameMap), + ]; + } + } } return $violations; @@ -302,6 +327,15 @@ private function buildKeyNameMap(array $fields, array &$map): void $subFields = (array) $f['sub_fields']; $this->buildKeyNameMap($subFields, $map); } + if (!empty($f['layouts'])) { + /** @var list> $layouts */ + $layouts = (array) $f['layouts']; + foreach ($layouts as $layout) { + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $this->buildKeyNameMap($layoutSubFields, $map); + } + } } } diff --git a/tests/Generator/AbstractTypeReverseMapperTest.php b/tests/Generator/AbstractTypeReverseMapperTest.php index 0964c2c..5f28bb8 100644 --- a/tests/Generator/AbstractTypeReverseMapperTest.php +++ b/tests/Generator/AbstractTypeReverseMapperTest.php @@ -166,6 +166,19 @@ public function test_repeater_omits_button_label_extra_when_add_label_absent(): self::assertArrayNotHasKey('button_label', $result['extra']); } + public function test_flexible_content_rebuilds_button_label_from_add_label(): void + { + $result = $this->mapper->reverse(['type' => 'flexible_content', 'label' => 'T', 'add_label' => 'Add Položky']); + self::assertSame('flexible_content', $result['acfType']); + self::assertSame('Add Položky', $result['extra']['button_label']); + } + + public function test_flexible_content_omits_button_label_extra_when_add_label_absent(): void + { + $result = $this->mapper->reverse(['type' => 'flexible_content', 'label' => 'T']); + self::assertArrayNotHasKey('button_label', $result['extra']); + } + public function test_unsupported_abstract_type_throws(): void { $this->expectException(\DomainException::class); diff --git a/tests/Generator/FieldReconstructorTest.php b/tests/Generator/FieldReconstructorTest.php index 16c3556..7618c7a 100644 --- a/tests/Generator/FieldReconstructorTest.php +++ b/tests/Generator/FieldReconstructorTest.php @@ -54,6 +54,29 @@ public function test_container_type_always_reconstructs_wpml_three_even_if_trans self::assertSame(3, $out['wpml_cf_preferences']); } + public function test_flexible_content_never_reconstructs_wpml_cf_preferences(): void + { + // Unlike group/repeater (always 3), flexible_content's own wpml + // is genuinely inconsistent across real corpus exports — never + // auto-emitted, whatever `translatable` says. + $out = $this->reconstructor->reconstruct( + ['type' => 'flexible_content', 'label' => 'T', 'translatable' => true, 'layouts' => []], + [], + ); + self::assertArrayNotHasKey('wpml_cf_preferences', $out); + } + + public function test_flexible_content_min_max_present_only_when_authored(): void + { + $withoutIt = $this->reconstructor->reconstruct(['type' => 'flexible_content', 'label' => 'T'], []); + self::assertArrayNotHasKey('min', $withoutIt); + self::assertArrayNotHasKey('max', $withoutIt); + + $withIt = $this->reconstructor->reconstruct(['type' => 'flexible_content', 'label' => 'T', 'min' => 2, 'max' => 2], []); + self::assertSame(2, $withIt['min']); + self::assertSame(2, $withIt['max']); + } + public function test_maxlength_present_only_when_authored(): void { $withoutIt = $this->reconstructor->reconstruct(['type' => 'text', 'label' => 'T'], []); diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index 5d4722d..81ad1fd 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -216,6 +216,71 @@ public function test_parent_repeater_on_a_nested_repeater_points_at_the_nearest_ self::assertSame('field_demo_items_tags', $tagsField['sub_fields'][0]['parent_repeater']); } + public function test_flexible_content_builds_layouts_keyed_by_layout_name(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Položky', 'add_label' => 'Add Položky', 'min' => 2, 'max' => 2, 'layouts' => [ + 'title' => ['label' => 'Nadpis', 'fields' => [ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ]], + 'image' => ['label' => 'Obrázek', 'fields' => [ + 'image' => ['type' => 'media', 'kind' => 'image', 'label' => 'Obrázek'], + ]], + ]], + ]), 'demo', 1700000000); + + $itemsField = $group['fields'][0]; + self::assertSame('flexible_content', $itemsField['type']); + self::assertSame('field_demo_items', $itemsField['key']); + self::assertSame('Add Položky', $itemsField['button_label']); + self::assertSame(2, $itemsField['min']); + self::assertSame(2, $itemsField['max']); + self::assertArrayNotHasKey('wpml_cf_preferences', $itemsField); + + self::assertCount(2, $itemsField['layouts']); + [$titleLayout, $imageLayout] = $itemsField['layouts']; + + self::assertSame('layout_demo_items_title', $titleLayout['key']); + self::assertSame('title', $titleLayout['name']); + self::assertSame('Nadpis', $titleLayout['label']); + self::assertSame('block', $titleLayout['display']); + self::assertSame('', $titleLayout['min']); + self::assertSame('', $titleLayout['max']); + self::assertNull($titleLayout['location']); + self::assertSame('field_demo_items_title_title', $titleLayout['sub_fields'][0]['key']); + self::assertArrayNotHasKey('parent_repeater', $titleLayout['sub_fields'][0]); + + self::assertSame('layout_demo_items_image', $imageLayout['key']); + self::assertSame('field_demo_items_image_image', $imageLayout['sub_fields'][0]['key']); + } + + public function test_flexible_content_layout_key_can_be_pinned(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Položky', 'layouts' => [ + 'title' => ['label' => 'Nadpis', 'key' => 'layout_legacy_hash_abc123', 'fields' => [ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertSame('layout_legacy_hash_abc123', $group['fields'][0]['layouts'][0]['key']); + } + + public function test_flexible_content_layout_min_max_are_reconstructed_when_authored(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Položky', 'layouts' => [ + 'title' => ['label' => 'Nadpis', 'min' => 1, 'max' => 3, 'fields' => [ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertSame(1, $group['fields'][0]['layouts'][0]['min']); + self::assertSame(3, $group['fields'][0]['layouts'][0]['max']); + } + public function test_visible_when_resolves_against_sibling_fields_only(): void { $group = $this->generator->generate($this->tree([ diff --git a/tests/Generator/GenerationRoundTripTest.php b/tests/Generator/GenerationRoundTripTest.php index cbf8d6d..6367a9b 100644 --- a/tests/Generator/GenerationRoundTripTest.php +++ b/tests/Generator/GenerationRoundTripTest.php @@ -158,6 +158,125 @@ public function test_root_acf_group_description_round_trips_via_wp_description() self::assertSame('ACF group own description', $regenerated['description']); } + /** + * flexible_content round-trip proof #1 (eprukaz `split-content`): + * 5 sibling layouts (title/image/cta/reference/contact) at a single + * nesting level, real (non-sentinel) field-level min/max (2/2), and + * NO wpml_cf_preferences on the flexible_content field itself — the + * exact shape that motivated this generator's flexible_content + * support (definition-kit issue #9). + * + * This fixture's own acf.json predates the `acfml_field_group_mode` + * era and uses the legacy `hide_on_screen: []` / `show_in_rest: false` + * shapes — the SAME already-documented, ACF-version-era root-level + * residual class RootFieldGroupBuilder's own docblock calls out + * (mirrors the zig-zag image-sentinel residual, just at the root + * instead of a field). Filtered out explicitly below so the + * assertion proves what it needs to: zero diffs anywhere touching + * flexible_content/layouts itself. + */ + public function test_split_content_flexible_content_round_trips_structurally_exact(): void + { + $fixtureDir = __DIR__ . '/../fixtures/migration/corpus-sample/split-content'; + $result = $this->roundTrip("{$fixtureDir}/acf.json", 'split-content'); + + $diffs = AcfJsonComparator::diff($result['original'], $result['regenerated']); + $residual = ['.hide_on_screen: expected [], got ""', '.show_in_rest: expected false, got 0', '.acfml_field_group_mode: unexpected in actual']; + $unexpected = array_values(array_diff($diffs, $residual)); + self::assertSame([], $unexpected, implode("\n", $unexpected)); + + $items = $result['regenerated']['fields'][0]; + self::assertSame('items', $items['name']); + self::assertSame('flexible_content', $items['type']); + self::assertArrayNotHasKey('wpml_cf_preferences', $items); + self::assertSame( + ['title', 'image', 'cta', 'reference', 'contact'], + array_column($items['layouts'], 'name'), + ); + } + + /** + * flexible_content round-trip proof #2 (eprukaz `box-price-reference`): + * a flexible_content field nested TWO levels deep — inside the + * `split_content` group — proving layout key/name derivation and the + * recursion chain work when the flexible_content field itself isn't + * top-level. Also exercises a top-level `group` containing nested + * `repeater`s (including a doubly-nested repeater, `items` -> + * `items`) alongside the flexible_content field, so this single + * fixture covers group + repeater + flexible_content interacting in + * one component. + * + * Same root-level legacy-export residual as split-content, PLUS two + * pre-existing, out-of-scope residual classes this fixture happens to + * expose that are unrelated to flexible_content and predate this PR: + * - every group/repeater field in this ONE real component lacks + * `wpml_cf_preferences` entirely (an older/no-WPML export) — + * clashing with FieldReconstructor's own explicitly tested, + * deliberate "container types always reconstruct wpml=3" contract + * (see FieldReconstructorTest::test_container_type_always_reconstructs_wpml_three_even_if_translatable_set). + * That contract predates this PR and isn't touched by it — fixing + * it would mean containers no longer always default to 3, which is + * a separate, deliberate design decision for a maintainer to make, + * not something to silently change alongside flexible_content + * support. + * - one `select` field's raw `default_value` is boolean `false` + * where the type baseline is `''` — the same ACF-version-era + * default_value serialization drift already documented for + * `true_false` fields in acf-defaults-baseline.yaml, just + * surfacing on `select` here. + * None of these touch `layouts`/flexible_content — asserted below. + */ + public function test_box_price_reference_nested_flexible_content_round_trips_structurally_exact(): void + { + $fixtureDir = __DIR__ . '/../fixtures/migration/corpus-sample/box-price-reference'; + $result = $this->roundTrip("{$fixtureDir}/acf.json", 'box-price-reference'); + + $diffs = AcfJsonComparator::diff($result['original'], $result['regenerated']); + $unexpected = array_values(array_filter($diffs, static function (string $diff): bool { + if (str_contains($diff, 'layouts')) { + return true; // any layouts-path diff would be a real flexible_content regression + } + $residualPatterns = [ + '/^\.hide_on_screen: expected \[\], got ""$/', + '/^\.show_in_rest: expected false, got 0$/', + '/^\.acfml_field_group_mode: unexpected in actual$/', + '/wpml_cf_preferences: unexpected in actual$/', + '/\.default_value: expected false, got ""$/', + ]; + foreach ($residualPatterns as $pattern) { + if (1 === preg_match($pattern, $diff)) { + return false; + } + } + return true; + })); + self::assertSame([], $unexpected, implode("\n", $unexpected)); + + $splitContent = $result['regenerated']['fields'][1]; + self::assertSame('split_content', $splitContent['name']); + self::assertSame('group', $splitContent['type']); + + $items = $splitContent['sub_fields'][0]; + self::assertSame('items', $items['name']); + self::assertSame('flexible_content', $items['type']); + self::assertSame(['image', 'reference'], array_column($items['layouts'], 'name')); + + // A flexible_content layout's own sub_fields carry NO parent_repeater + // — unlike an ordinary repeater's direct children (proven separately + // for the sibling `price_list` group's nested repeaters below). + foreach ($items['layouts'] as $layout) { + foreach ($layout['sub_fields'] as $subField) { + self::assertArrayNotHasKey('parent_repeater', $subField); + } + } + + // Layout keys/names survive verbatim — the single most safety- + // critical property (consuming Twig branches on acf_fc_layout). + $imageLayout = $items['layouts'][0]; + self::assertSame('layout_box-price-reference_split_content_items_image', $imageLayout['key']); + self::assertSame('image', $imageLayout['name']); + } + /** * `parent_repeater` container-gating proof (real corpus shape): the * REAL mairateam `reference-detail` component nests a repeater diff --git a/tests/Migration/AbstractTypeMapperTest.php b/tests/Migration/AbstractTypeMapperTest.php index 678402a..c9b7700 100644 --- a/tests/Migration/AbstractTypeMapperTest.php +++ b/tests/Migration/AbstractTypeMapperTest.php @@ -156,6 +156,21 @@ public function test_repeater_omits_add_label_when_button_label_empty(): void self::assertSame(['type', 'sub_fields', 'button_label'], $result['consumed']); } + public function test_flexible_content_lifts_button_label_to_add_label(): void + { + $result = $this->mapper->map(['type' => 'flexible_content', 'name' => 'f', 'button_label' => 'Add Položky']); + self::assertSame('flexible_content', $result['type']); + self::assertSame(['add_label' => 'Add Položky'], $result['extra']); + self::assertSame(['type', 'layouts', 'button_label'], $result['consumed']); + } + + public function test_flexible_content_omits_add_label_when_button_label_empty(): void + { + $result = $this->mapper->map(['type' => 'flexible_content', 'name' => 'f', 'button_label' => '']); + self::assertSame([], $result['extra']); + self::assertSame(['type', 'layouts', 'button_label'], $result['consumed']); + } + public function test_unsupported_type_throws_domain_exception(): void { $this->expectException(\DomainException::class); diff --git a/tests/Migration/AcfJsonReaderTest.php b/tests/Migration/AcfJsonReaderTest.php index bb99c7b..738d2eb 100644 --- a/tests/Migration/AcfJsonReaderTest.php +++ b/tests/Migration/AcfJsonReaderTest.php @@ -412,4 +412,142 @@ public function test_group_with_zero_non_accordion_sub_fields_throws(): void 'sub_fields' => [['key' => 'field_demo_g_acc', 'name' => '', 'type' => 'accordion', 'label' => 'S']], ]]), 'demo'); } + + // --- flexible_content --------------------------------------------- + + public function test_flexible_content_lifts_layouts_keyed_by_name(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'button_label' => 'Add Položky', 'min' => 2, 'max' => 2, + 'layouts' => [ + [ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + 'min' => '', 'max' => '', 'location' => null, + ], + [ + 'key' => 'layout_demo_items_image', 'name' => 'image', 'label' => 'Obrázek', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_image_image', 'name' => 'image', 'label' => 'Obrázek', 'type' => 'image'], + ], + 'min' => '', 'max' => '', 'location' => null, + ], + ], + ]]), 'demo'); + + $items = $tree['fields']['items']; + self::assertSame('flexible_content', $items['type']); + self::assertSame('Add Položky', $items['add_label']); + self::assertSame(2, $items['min']); + self::assertSame(2, $items['max']); + self::assertSame(['title', 'image'], array_keys($items['layouts'])); + self::assertSame('Nadpis', $items['layouts']['title']['label']); + self::assertArrayNotHasKey('key', $items['layouts']['title']); // matches convention, not pinned + self::assertSame('text', $items['layouts']['title']['fields']['title']['type']); + self::assertSame('media', $items['layouts']['image']['fields']['image']['type']); + self::assertSame('image', $items['layouts']['image']['fields']['image']['kind']); + } + + public function test_flexible_content_layout_key_deviating_from_convention_is_pinned(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_legacy_hash_abc123', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + + self::assertSame('layout_demo_legacy_hash_abc123', $tree['fields']['items']['layouts']['title']['key']); + } + + public function test_flexible_content_min_max_zero_is_omitted_as_acf_no_limit_sentinel(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'min' => 0, 'max' => 0, + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + self::assertArrayNotHasKey('min', $tree['fields']['items']); + self::assertArrayNotHasKey('max', $tree['fields']['items']); + } + + public function test_flexible_content_wpml_cf_preferences_is_never_lifted_to_translatable(): void + { + // Real corpus shows 1/2 on some projects, absent on others, never 3 + // — never lift to `translatable`, whatever value is present. + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'wpml_cf_preferences' => 2, + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + self::assertArrayNotHasKey('translatable', $tree['fields']['items']); + self::assertSame(2, $tree['fields']['items']['wp']['wpml_cf_preferences']); + } + + public function test_flexible_content_absent_wpml_cf_preferences_leaves_no_wp_trace(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + self::assertArrayNotHasKey('wpml_cf_preferences', $tree['fields']['items']['wp'] ?? []); + } + + public function test_flexible_content_layout_sub_field_carries_no_parent_repeater_key_expectation(): void + { + // Nested key derivation treats the layout name as just another + // nesting segment — field____. + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_cta', 'name' => 'cta', 'label' => 'CTA', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_cta_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + self::assertArrayNotHasKey('key', $tree['fields']['items']['layouts']['cta']['fields']['title']); + } + + public function test_flexible_content_with_zero_layouts_throws(): void + { + $this->expectException(\RuntimeException::class); + $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [], + ]]), 'demo'); + } + + public function test_flexible_content_layout_with_zero_non_accordion_sub_fields_throws(): void + { + $this->expectException(\RuntimeException::class); + $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_empty', 'name' => 'empty', 'label' => 'Empty', 'display' => 'block', + 'sub_fields' => [['key' => 'field_demo_items_empty_acc', 'name' => '', 'type' => 'accordion', 'label' => 'S']], + ]], + ]]), 'demo'); + } } diff --git a/tests/Migration/MigrationCompletenessAuditorTest.php b/tests/Migration/MigrationCompletenessAuditorTest.php index 44c82b2..9c6b897 100644 --- a/tests/Migration/MigrationCompletenessAuditorTest.php +++ b/tests/Migration/MigrationCompletenessAuditorTest.php @@ -121,4 +121,72 @@ public function test_recurses_into_sub_fields(): void ]]]; self::assertNotEmpty($this->auditor->audit($acf, $defLossy)); } + + public function test_recurses_into_flexible_content_layouts(): void + { + $acf = [[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Items', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_body', 'name' => 'body', 'label' => 'Body', 'display' => 'block', + 'sub_fields' => [[ + 'key' => 'field_demo_items_body_text', 'name' => 'text', 'label' => 'Text', 'type' => 'wysiwyg', + 'toolbar' => 'full', + ]], + 'min' => '', 'max' => '', + ]], + ]]; + $defOk = ['items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'body' => ['label' => 'Body', 'fields' => [ + 'text' => ['type' => 'richtext', 'label' => 'Text', 'wp' => ['toolbar' => 'full']], + ]], + ]]]; + self::assertSame([], $this->auditor->audit($acf, $defOk)); + + $defLossy = ['items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'body' => ['label' => 'Body', 'fields' => [ + 'text' => ['type' => 'richtext', 'label' => 'Text'], // toolbar lost! + ]], + ]]]; + self::assertNotEmpty($this->auditor->audit($acf, $defLossy)); + } + + public function test_fails_when_a_layout_is_missing_from_the_migrated_definition(): void + { + $acf = [[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Items', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_body', 'name' => 'body', 'label' => 'Body', 'display' => 'block', + 'sub_fields' => [[ + 'key' => 'field_demo_items_body_text', 'name' => 'text', 'label' => 'Text', 'type' => 'text', + ]], + ]], + ]]; + $def = ['items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => []]]; + $violations = $this->auditor->audit($acf, $def); + self::assertNotEmpty($violations); + self::assertStringContainsString('layout missing', $violations[0]); + } + + public function test_flexible_content_wpml_cf_preferences_is_never_flagged_as_unaccounted(): void + { + // Real value 1/2 on the FC field itself should not trip the + // generic "neither baseline default, lifted key, nor in wp:" + // check — it's left entirely to the generic wp: fallback. + $acf = [[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Items', 'type' => 'flexible_content', + 'wpml_cf_preferences' => 1, + 'layouts' => [[ + 'key' => 'layout_demo_items_body', 'name' => 'body', 'label' => 'Body', 'display' => 'block', + 'sub_fields' => [[ + 'key' => 'field_demo_items_body_text', 'name' => 'text', 'label' => 'Text', 'type' => 'text', + ]], + ]], + ]]; + $def = ['items' => ['type' => 'flexible_content', 'label' => 'Items', 'wp' => ['wpml_cf_preferences' => 1], 'layouts' => [ + 'body' => ['label' => 'Body', 'fields' => [ + 'text' => ['type' => 'text', 'label' => 'Text'], + ]], + ]]]; + self::assertSame([], $this->auditor->audit($acf, $def)); + } } diff --git a/tests/Schema/FieldsSchemaValidatorTest.php b/tests/Schema/FieldsSchemaValidatorTest.php index 5db3024..81f8469 100644 --- a/tests/Schema/FieldsSchemaValidatorTest.php +++ b/tests/Schema/FieldsSchemaValidatorTest.php @@ -146,6 +146,23 @@ public function test_group_without_fields_fails_with_pointer_to_field(): void ); } + public function test_valid_flexible_content_document_passes(): void + { + $result = (new FieldsSchemaValidator())->validateFile($this->fixture('valid-flexible-content.fields.yaml')); + self::assertTrue($result->valid, print_r($result->errors, true)); + } + + public function test_flexible_content_without_layouts_fails_with_pointer_to_field(): void + { + $result = (new FieldsSchemaValidator())->validateFile($this->fixture('invalid-flexible-content-no-layouts.fields.yaml')); + self::assertFalse($result->valid); + $pointers = array_column($result->errors, 'pointer'); + self::assertTrue( + (bool) array_filter($pointers, static fn (string $p): bool => str_contains($p, 'items')), + 'items pointer expected: ' . implode(',', $pointers), + ); + } + public function test_key_not_matching_field_prefix_fails_with_pointer_to_field(): void { $result = (new FieldsSchemaValidator())->validateFile($this->fixture('invalid-bad-key-pattern.fields.yaml')); diff --git a/tests/fixtures/invalid-flexible-content-no-layouts.fields.yaml b/tests/fixtures/invalid-flexible-content-no-layouts.fields.yaml new file mode 100644 index 0000000..27736ab --- /dev/null +++ b/tests/fixtures/invalid-flexible-content-no-layouts.fields.yaml @@ -0,0 +1,5 @@ +name: Demo Invalid Flexible Content +fields: + items: + type: flexible_content # missing required `layouts` + label: Položky diff --git a/tests/fixtures/migration/corpus-sample/box-price-reference/acf.json b/tests/fixtures/migration/corpus-sample/box-price-reference/acf.json new file mode 100644 index 0000000..aec2b5e --- /dev/null +++ b/tests/fixtures/migration/corpus-sample/box-price-reference/acf.json @@ -0,0 +1,555 @@ +{ + "key": "group_box-price-reference", + "title": "Box-price-reference", + "fields": [ + { + "key": "field_box-price-reference_price_list", + "label": "Cena PENB", + "name": "price_list", + "aria-label": "", + "type": "group", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "sub_fields": [ + { + "key": "field_box-price-reference_price_list_title", + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_box-price-reference_price_list_perex", + "label": "Popisek", + "name": "perex", + "aria-label": "", + "type": "wysiwyg", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "new_lines": "wpautop", + "tabs": "all", + "toolbar": "basic", + "media_upload": 0, + "wpml_cf_preferences": 2, + "default_value": "", + "delay": 0 + }, + { + "key": "field_box-price-reference_price_list_buttons", + "label": "Tlačítka", + "name": "buttons", + "aria-label": "", + "type": "repeater", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "layout": "block", + "button_label": "Add Tlačítka", + "min": 0, + "max": 0, + "rows_per_page": 20, + "collapsed": "", + "sub_fields": [ + { + "key": "field_box-price-reference_price_list_buttons_button", + "label": "Tlačítko", + "name": "button", + "aria-label": "", + "type": "link", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "return_format": "array", + "wpml_cf_preferences": 2, + "parent_repeater": "field_box-price-reference_price_list_buttons" + } + ] + }, + { + "key": "field_box-price-reference_price_list_items", + "label": "Ceníkové položky", + "name": "items", + "aria-label": "", + "type": "repeater", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "min": 1, + "layout": "block", + "button_label": "Add Ceníkové položky", + "max": 0, + "rows_per_page": 20, + "collapsed": "", + "sub_fields": [ + { + "key": "field_box-price-reference_price_list_items_icon", + "label": "Ikona", + "name": "icon", + "aria-label": "", + "type": "select", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "choices": { + "illustration-category-house": "Dům", + "illustration-category-large": "Velký dům", + "illustration-category-residential": "Bytový dům", + "illustration-category-renovation": "Rekonstrukce" + }, + "wpml_cf_preferences": 2, + "multiple": 0, + "allow_null": 0, + "default_value": false, + "ui": 0, + "ajax": 0, + "placeholder": "", + "return_format": "value", + "create_options": 0, + "save_options": 0, + "parent_repeater": "field_box-price-reference_price_list_items" + }, + { + "key": "field_box-price-reference_price_list_items_title", + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "", + "parent_repeater": "field_box-price-reference_price_list_items" + }, + { + "key": "field_box-price-reference_price_list_items_badge", + "label": "Badge", + "name": "badge", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "", + "parent_repeater": "field_box-price-reference_price_list_items" + }, + { + "key": "field_box-price-reference_price_list_items_perex", + "label": "Popisek", + "name": "perex", + "aria-label": "", + "type": "wysiwyg", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "new_lines": "wpautop", + "tabs": "all", + "toolbar": "basic", + "media_upload": 0, + "wpml_cf_preferences": 2, + "default_value": "", + "delay": 0, + "parent_repeater": "field_box-price-reference_price_list_items" + }, + { + "key": "field_box-price-reference_price_list_items_items", + "label": "Ceny", + "name": "items", + "aria-label": "", + "type": "repeater", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "min": 1, + "layout": "block", + "button_label": "Add Ceny", + "max": 0, + "rows_per_page": 20, + "collapsed": "", + "sub_fields": [ + { + "key": "field_box-price-reference_price_list_items_items_price", + "label": "Cena", + "name": "price", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "", + "parent_repeater": "field_box-price-reference_price_list_items_items" + }, + { + "key": "field_box-price-reference_price_list_items_items_description", + "label": "Popis ceny", + "name": "description", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "", + "parent_repeater": "field_box-price-reference_price_list_items_items" + } + ], + "parent_repeater": "field_box-price-reference_price_list_items" + } + ] + } + ], + "layout": "block" + }, + { + "key": "field_box-price-reference_split_content", + "label": "Reference", + "name": "split_content", + "aria-label": "", + "type": "group", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "sub_fields": [ + { + "key": "field_box-price-reference_split_content_items", + "label": "Položky", + "name": "items", + "aria-label": "", + "type": "flexible_content", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "min": 2, + "max": 2, + "button_label": "Add Položky", + "layouts": [ + { + "key": "layout_box-price-reference_split_content_items_image", + "name": "image", + "label": "Obrázek", + "display": "block", + "sub_fields": [ + { + "key": "field_box-price-reference_split_content_items_image_image", + "label": "Obrázek", + "name": "image", + "aria-label": "", + "type": "image", + "instructions": "Nahrávejte obrázek o velikosti 1400 x 900px nebo větší", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 1, + "return_format": "array", + "preview_size": "medium", + "library": "all", + "min_width": 0, + "min_height": 0, + "min_size": 0, + "max_width": 0, + "max_height": 0, + "max_size": 0, + "mime_types": "" + } + ], + "min": "", + "max": "", + "location": null + }, + { + "key": "layout_box-price-reference_split_content_items_reference", + "name": "reference", + "label": "Reference", + "display": "block", + "sub_fields": [ + { + "key": "field_box-price-reference_split_content_items_reference_perex", + "label": "Text reference", + "name": "perex", + "aria-label": "", + "type": "wysiwyg", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "tabs": "all", + "toolbar": "basic", + "media_upload": 0, + "wpml_cf_preferences": 2, + "default_value": "", + "delay": 0 + }, + { + "key": "field_box-price-reference_split_content_items_reference_name", + "label": "Jméno", + "name": "name", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_box-price-reference_split_content_items_reference_role", + "label": "Role", + "name": "role", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_box-price-reference_split_content_items_reference_phone", + "label": "Telefon", + "name": "phone", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_box-price-reference_split_content_items_reference_email", + "label": "E-mail", + "name": "email", + "aria-label": "", + "type": "email", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_box-price-reference_split_content_items_reference_image", + "label": "Obrázek", + "name": "image", + "aria-label": "", + "type": "image", + "instructions": "Nahrávejte obrázek o velikosti 400 x 400px nebo větší", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 1, + "return_format": "array", + "preview_size": "medium", + "library": "all", + "min_width": 0, + "min_height": 0, + "min_size": 0, + "max_width": 0, + "max_height": 0, + "max_size": 0, + "mime_types": "" + } + ], + "min": "", + "max": "", + "location": null + } + ] + } + ], + "layout": "block" + } + ], + "location": [ + [ + { + "param": "block", + "operator": "==", + "value": "acf\/box-price-reference" + } + ] + ], + "menu_order": 0, + "position": "normal", + "style": "default", + "label_placement": "top", + "instruction_placement": "label", + "hide_on_screen": [], + "active": true, + "description": "", + "show_in_rest": false, + "modified": 1757269978 +} diff --git a/tests/fixtures/migration/corpus-sample/split-content/acf.json b/tests/fixtures/migration/corpus-sample/split-content/acf.json new file mode 100644 index 0000000..4606e2d --- /dev/null +++ b/tests/fixtures/migration/corpus-sample/split-content/acf.json @@ -0,0 +1,443 @@ +{ + "key": "group_split-content", + "title": "Split-content", + "fields": [ + { + "key": "field_split-content_items", + "label": "Položky", + "name": "items", + "aria-label": "", + "type": "flexible_content", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "min": 2, + "max": 2, + "button_label": "Add Položky", + "layouts": [ + { + "key": "layout_split-content_items_title", + "name": "title", + "label": "Nadpis", + "display": "block", + "sub_fields": [ + { + "key": "field_split-content_items_title_title", + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + } + ], + "min": "", + "max": "", + "location": null + }, + { + "key": "layout_split-content_items_image", + "name": "image", + "label": "Obrázek", + "display": "block", + "sub_fields": [ + { + "key": "field_split-content_items_image_image", + "label": "Obrázek", + "name": "image", + "aria-label": "", + "type": "image", + "instructions": "Nahrávejte obrázek o velikosti 1400 x 900px nebo větší", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 1, + "return_format": "array", + "preview_size": "medium", + "library": "all", + "min_width": 0, + "min_height": 0, + "min_size": 0, + "max_width": 0, + "max_height": 0, + "max_size": 0, + "mime_types": "" + } + ], + "min": "", + "max": "", + "location": null + }, + { + "key": "layout_split-content_items_cta", + "name": "cta", + "label": "CTA", + "display": "block", + "sub_fields": [ + { + "key": "field_split-content_items_cta_title", + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_cta_perex", + "label": "Popisek", + "name": "perex", + "aria-label": "", + "type": "wysiwyg", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "tabs": "all", + "toolbar": "basic", + "media_upload": 0, + "wpml_cf_preferences": 2, + "default_value": "", + "delay": 0 + }, + { + "key": "field_split-content_items_cta_button", + "label": "Tlačítko", + "name": "button", + "aria-label": "", + "type": "link", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "return_format": "array" + }, + { + "key": "field_split-content_items_cta_button_style", + "label": "Styl tlačítka", + "name": "button_style", + "aria-label": "", + "type": "select", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "choices": { + "primary": "Primární", + "secondary": "Sekundární" + }, + "default_value": "primary", + "wpml_cf_preferences": 2, + "multiple": 0, + "allow_null": 0, + "ui": 0, + "ajax": 0, + "placeholder": "", + "return_format": "value", + "create_options": 0, + "save_options": 0 + } + ], + "min": "", + "max": "", + "location": null + }, + { + "key": "layout_split-content_items_reference", + "name": "reference", + "label": "Reference", + "display": "block", + "sub_fields": [ + { + "key": "field_split-content_items_reference_perex", + "label": "Text reference", + "name": "perex", + "aria-label": "", + "type": "wysiwyg", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "tabs": "all", + "toolbar": "basic", + "media_upload": 0, + "wpml_cf_preferences": 2, + "default_value": "", + "delay": 0 + }, + { + "key": "field_split-content_items_reference_name", + "label": "Jméno", + "name": "name", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_reference_role", + "label": "Role", + "name": "role", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_reference_phone", + "label": "Telefon", + "name": "phone", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_reference_email", + "label": "E-mail", + "name": "email", + "aria-label": "", + "type": "email", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_reference_image", + "label": "Obrázek", + "name": "image", + "aria-label": "", + "type": "image", + "instructions": "Nahrávejte obrázek o velikosti 400 x 400px nebo větší", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 1, + "return_format": "array", + "preview_size": "medium", + "library": "all", + "min_width": 0, + "min_height": 0, + "min_size": 0, + "max_width": 0, + "max_height": 0, + "max_size": 0, + "mime_types": "" + } + ], + "min": "", + "max": "", + "location": null + }, + { + "key": "layout_split-content_items_contact", + "name": "contact", + "label": "Kontakt", + "display": "block", + "sub_fields": [ + { + "key": "field_split-content_items_contact_title", + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 1, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_contact_phone", + "label": "Telefon", + "name": "phone", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "maxlength": "", + "placeholder": "", + "prepend": "", + "append": "" + }, + { + "key": "field_split-content_items_contact_email", + "label": "E-mail", + "name": "email", + "aria-label": "", + "type": "email", + "instructions": "", + "required": 0, + "allow_in_bindings": 0, + "conditional_logic": false, + "wrapper": { + "width": "", + "class": "", + "id": "" + }, + "wpml_cf_preferences": 2, + "default_value": "", + "placeholder": "", + "prepend": "", + "append": "" + } + ], + "min": "", + "max": "", + "location": null + } + ] + } + ], + "location": [ + [ + { + "param": "block", + "operator": "==", + "value": "acf\/split-content" + } + ] + ], + "menu_order": 0, + "position": "normal", + "style": "default", + "label_placement": "top", + "instruction_placement": "label", + "hide_on_screen": [], + "active": true, + "description": "", + "show_in_rest": false, + "modified": 1757269978 +} diff --git a/tests/fixtures/valid-flexible-content.fields.yaml b/tests/fixtures/valid-flexible-content.fields.yaml new file mode 100644 index 0000000..e7239bc --- /dev/null +++ b/tests/fixtures/valid-flexible-content.fields.yaml @@ -0,0 +1,29 @@ +name: Demo Flexible Content +fields: + items: + type: flexible_content + label: Položky + add_label: "Add Položky" + min: 2 + max: 2 + layouts: + title: + label: Nadpis + fields: + title: { type: text, label: Nadpis } + image: + label: Obrázek + fields: + image: { type: media, kind: image, label: Obrázek } + cta: + label: CTA + fields: + title: { type: text, label: Nadpis } + perex: { type: richtext, label: Popisek } + button: { type: link, shape: link, label: Tlačítko } + button_style: + type: select + label: Styl tlačítka + options: + primary: Primární + secondary: Sekundární From 08e8b8b9480f87e0d82206971511368ffd6ef92d Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 13:20:07 +0200 Subject: [PATCH 02/10] Fix flexible_content review findings: key collisions, duplicate layouts, display/location loss, schema parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7 --- CHANGELOG.md | 39 ++++++ composer.json | 1 + schemas/component.fields.schema.json | 4 +- src/Generator/FieldsGenerator.php | 79 ++++++++++- src/Migration/AcfJsonReader.php | 34 +++++ .../MigrationCompletenessAuditor.php | 21 +++ tests/Generator/FieldsGeneratorTest.php | 100 +++++++++++++ tests/Generator/GenerationRoundTripTest.php | 87 ++++++++++++ tests/Integration/AcfLintValidationTest.php | 132 ++++++++++++++++++ tests/Migration/AcfJsonReaderTest.php | 69 +++++++++ .../MigrationCompletenessAuditorTest.php | 48 +++++++ tests/Schema/FieldsSchemaValidatorTest.php | 25 ++++ ...exible-content-bad-layout-name.fields.yaml | 10 ++ ...exible-content-layout-no-label.fields.yaml | 12 ++ 14 files changed, 658 insertions(+), 3 deletions(-) create mode 100644 tests/Integration/AcfLintValidationTest.php create mode 100644 tests/fixtures/invalid-flexible-content-bad-layout-name.fields.yaml create mode 100644 tests/fixtures/invalid-flexible-content-layout-no-label.fields.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index c21eacb..690a29b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,45 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- `flexible_content` field support end-to-end: migration (`fields-migrate`) + lifts raw ACF `layouts` into a `layouts:` map keyed by layout name, and + generation (`fields-generate`) replays it back into ACF's raw `layouts` + list. Layout-level `label`/`min`/`max` and non-default `display`/`location` + round-trip verbatim; a layout's own `fields` recurse through the same + nesting machinery as an ordinary `group`/`repeater`, including + `flexible_content` nested inside another `flexible_content`'s layout. + +### Fixed + +- Generator now enforces GLOBAL key uniqueness across an entire generated + field group — including flexible_content layouts and their sub-fields — + and throws a `GenerationValidationException` instead of silently emitting + two ACF fields that alias the same WordPress postmeta key. Underscore-joined + name-chain derivation could otherwise collide across unrelated + fields/layouts (e.g. layout `a_b` + field `c` vs. layout `a` + field `b_c`). +- `AcfJsonReader::readLayouts()` now throws on duplicate layout names instead + of silently overwriting the earlier layout (and its key) with no + diagnostic. `MigrationCompletenessAuditor` independently detects the same + duplicate in the raw ACF source — it previously could mask the collision + entirely when both duplicate layouts happened to share an identical + sub-field shape. +- Layout `display` (`block`/`table`/`row`) and `location` are now captured + verbatim by the migration reader (into the layout's `wp:` escape hatch when + non-default) and replayed by the generator, instead of the generator + hardcoding `display: block` / `location: null` unconditionally. +- `component.fields.schema.json`'s `layout` `$defs` now requires `label` (not + just `fields`) and constrains layout map keys to `^[a-z][a-z0-9_]*$` — + bringing it into parity with `parisek/acf-json-schema`'s + `field-flexible_content.schema.json`, which already rejected the empty + `label: ""` a label-less layout would generate. +- `acf-lint` (from `parisek/acf-json-schema`) is now wired into this + project's own test suite, validating every generated `acf.json` fixture + against the ecosystem's canonical ACF-shape validator — closing the gap + that let the `display`/`location` and schema-parity regressions above ship + unnoticed. + ## [0.2.1] - 2026-07-23 ### Fixed diff --git a/composer.json b/composer.json index 644792b..203a9f9 100644 --- a/composer.json +++ b/composer.json @@ -26,6 +26,7 @@ }, "require-dev": { "ergebnis/composer-normalize": "^2.0", + "parisek/acf-json-schema": "^0.6", "parisek/styleguide": "^1.7", "phpstan/phpstan": "^2.0", "phpunit/phpunit": "^11.0 || ^12.0" diff --git a/schemas/component.fields.schema.json b/schemas/component.fields.schema.json index 6f77633..01b23dc 100644 --- a/schemas/component.fields.schema.json +++ b/schemas/component.fields.schema.json @@ -127,17 +127,19 @@ "layoutMap": { "type": "object", "minProperties": 1, + "propertyNames": { "pattern": "^[a-z][a-z0-9_]*$" }, "additionalProperties": { "$ref": "#/$defs/layout" } }, "layout": { "type": "object", - "required": ["fields"], + "required": ["fields", "label"], "additionalProperties": false, "properties": { "label": { "type": "string", "minLength": 1 }, "key": { "type": "string", "pattern": "^layout_" }, "min": { "type": "integer", "minimum": 0 }, "max": { "type": "integer", "minimum": 0 }, + "wp": { "type": "object" }, "fields": { "$ref": "#/$defs/fieldMap" } } } diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 3b5f5d5..2016c26 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -60,9 +60,77 @@ public function generate(array $definitionTree, string $componentSlug, int $modi ); } + $this->assertGloballyUniqueKeys($orderedRawFields); + return $this->rootBuilder->build($definitionTree, $orderedRawFields, $componentSlug, $modifiedAt); } + /** + * Finding A (CRITICAL) — key derivation underscore-joins the name + * chain, so a flexible_content layout `a_b` + field `c` and a + * sibling layout `a` + field `b_c` both derive `field__a_b_c`. + * Two ACF fields sharing one key alias the SAME WordPress postmeta + * row — silent, irreversible editor data loss the moment both are + * ever populated. No ambiguity is acceptable regardless of how it + * arose (ordinary nesting, repeater sub_fields, or flexible_content + * layouts) — walk the ENTIRE generated tree (fields, their + * sub_fields, and every flexible_content layout's own key plus its + * own sub_fields) and fail loudly the moment two nodes claim the + * same `key`. + * + * @param list> $fields + */ + private function assertGloballyUniqueKeys(array $fields): void + { + $seen = []; + $this->collectKeys($fields, $seen); + } + + /** + * @param list> $fields + * @param array $seen + */ + private function collectKeys(array $fields, array &$seen): void + { + foreach ($fields as $field) { + $this->assertKeyUnseen((string) $field['key'], $seen); + + if (!empty($field['sub_fields'])) { + /** @var list> $subFields */ + $subFields = (array) $field['sub_fields']; + $this->collectKeys($subFields, $seen); + } + + if (!empty($field['layouts'])) { + /** @var list> $layouts */ + $layouts = (array) $field['layouts']; + foreach ($layouts as $layout) { + $this->assertKeyUnseen((string) $layout['key'], $seen); + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $this->collectKeys($layoutSubFields, $seen); + } + } + } + } + + /** + * @param array $seen + */ + private function assertKeyUnseen(string $key, array &$seen): void + { + if (isset($seen[$key])) { + throw new GenerationValidationException(sprintf( + "Generated key '%s' collides with another field/layout in the same component. " + . 'Two ACF elements sharing one key would alias the same WordPress postmeta row — ' + . 'rename one of the colliding fields/layouts (or pin an explicit `key:` on one of ' + . 'them) so the underscore-joined name chains no longer produce the same key.', + $key, + )); + } + $seen[$key] = true; + } + /** * @param array $semanticField * @param list $nameChain @@ -182,15 +250,22 @@ private function buildLayouts(array $layoutDefs, string $componentSlug, array $n ); } + $layoutWp = (array) ($layoutDef['wp'] ?? []); + $layout = [ 'key' => $layoutKey, 'name' => (string) $layoutName, 'label' => (string) ($layoutDef['label'] ?? ''), - 'display' => 'block', + // Finding C — `display`/`location` are canonical ACF + // layout props (block|table|row for display) captured + // verbatim by AcfJsonReader::readLayouts() into the + // layout's `wp:` escape hatch whenever authored non-default; + // replay them here instead of hardcoding the default. + 'display' => (string) ($layoutWp['display'] ?? 'block'), 'sub_fields' => $subFields, 'min' => $layoutDef['min'] ?? '', 'max' => $layoutDef['max'] ?? '', - 'location' => null, + 'location' => $layoutWp['location'] ?? null, ]; $layouts[] = $layout; diff --git a/src/Migration/AcfJsonReader.php b/src/Migration/AcfJsonReader.php index ca67779..1ea1041 100644 --- a/src/Migration/AcfJsonReader.php +++ b/src/Migration/AcfJsonReader.php @@ -363,6 +363,20 @@ private function readLayouts(array $layouts, string $componentSlug, array $nameC $layoutName = (string) $layout['name']; $layoutChain = [...$nameChain, $layoutName]; + // Finding B (CRITICAL) — `$out[$layoutName] = …` below would + // silently overwrite an earlier layout of the same name (and + // discard its key) with no diagnostic at all. Mirrors the + // adjacent empty-layouts / empty-sub-fields guards' style. + if (array_key_exists($layoutName, $out)) { + throw new \RuntimeException(sprintf( + "Duplicate layout name '%s' in flexible_content field '%s' — " + . 'two ACF layouts sharing one name would collapse into a single ' + . 'migrated layout, silently discarding the earlier one and its key.', + $layoutName, + implode('.', $nameChain), + )); + } + $children = []; foreach ((array) ($layout['sub_fields'] ?? []) as $sub) { if ('accordion' === ($sub['type'] ?? null)) { @@ -393,6 +407,26 @@ private function readLayouts(array $layouts, string $componentSlug, array $nameC } $layoutOut['fields'] = $children; + // Finding C (CRITICAL) — `display` (block|table|row) and + // `location` are canonical ACF layout props the generator + // side previously hardcoded to 'block' / null unconditionally, + // silently rewriting any layout authored with a non-default + // value. Capture them verbatim into the layout's own `wp:` + // escape hatch whenever they deviate from the ACF default, so + // FieldsGenerator can replay them instead of guessing. + $layoutWp = []; + $display = (string) ($layout['display'] ?? 'block'); + if ('block' !== $display) { + $layoutWp['display'] = $display; + } + $location = $layout['location'] ?? null; + if (null !== $location) { + $layoutWp['location'] = $location; + } + if ([] !== $layoutWp) { + $layoutOut['wp'] = $layoutWp; + } + $expectedLayoutKey = 'layout_' . $componentSlug . '_' . implode('_', $layoutChain); if ((string) $layout['key'] !== $expectedLayoutKey) { $layoutOut['key'] = (string) $layout['key']; diff --git a/src/Migration/MigrationCompletenessAuditor.php b/src/Migration/MigrationCompletenessAuditor.php index 84b85db..75ac413 100644 --- a/src/Migration/MigrationCompletenessAuditor.php +++ b/src/Migration/MigrationCompletenessAuditor.php @@ -291,9 +291,30 @@ private function auditFields(array $acfFields, array $definitionFields, string $ $defLayouts = (array) ($defField['layouts'] ?? []); /** @var list> $layouts */ $layouts = (array) $acfField['layouts']; + // Finding B (auditor half) — the migrated definition + // collapses layouts into a name-keyed map, so iterating + // the raw ACF layout LIST and looking each one up in that + // already-collapsed map independently is blind to + // duplicates: if two raw layouts share a name AND happen + // to have an identical sub-field shape, every iteration + // "matches" the same single migrated layout and reports + // zero violations — even though one whole raw layout was + // silently discarded during migration. Track raw layout + // names seen in THIS list and flag the duplicate directly, + // independent of whatever the migrated definition looks + // like. + $seenLayoutNames = []; foreach ($layouts as $layout) { $layoutName = (string) ($layout['name'] ?? ''); $layoutPath = "{$path}.{$layoutName}"; + + if (isset($seenLayoutNames[$layoutName])) { + $violations[] = "{$layoutPath}: duplicate layout name in raw ACF source — " + . 'one of these layouts was silently discarded during migration'; + continue; + } + $seenLayoutNames[$layoutName] = true; + if (!array_key_exists($layoutName, $defLayouts)) { $violations[] = "{$layoutPath}: layout missing from migrated definition entirely"; continue; diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index 81ad1fd..a2b0a82 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -327,4 +327,104 @@ public function test_accordions_are_replayed_by_generate(): void self::assertSame(['accordion', 'text'], array_column($group['fields'], 'type')); } + + /** + * Finding A (CRITICAL) — a flexible_content field named `a_b` with a + * layout `c` derives the exact same underscore-joined key + * (`field_demo_items_a_b_c`) as a sibling flexible_content field `a` + * whose layout is `b_c`. Two different ACF fields aliasing one + * postmeta key is irreversible editor data loss the moment both + * layouts are ever populated on the same post. The generator must + * refuse to emit such a tree rather than silently produce a + * colliding pair of `field_*` keys. + */ + public function test_flexible_content_layout_name_ambiguity_produces_colliding_keys_without_a_guard(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/field_demo_items_a_b_c/'); + + $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'a_b' => ['label' => 'A B', 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + 'a' => ['label' => 'A', 'fields' => [ + 'b_c' => ['type' => 'text', 'label' => 'B C'], + ]], + ]], + ], ['name' => 'Demo']), 'demo', 1700000000); + } + + /** + * Same collision class without flexible_content at all — two ordinary + * fields whose name-chain segments underscore-join identically + * (`a` + `b_c` vs `a_b` + `c`). The uniqueness guard must be global, + * not flexible_content-specific. + */ + public function test_ordinary_nested_group_field_name_ambiguity_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + + $this->generator->generate($this->tree([ + 'a_b' => ['type' => 'group', 'label' => 'A B', 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + 'a' => ['type' => 'group', 'label' => 'A', 'fields' => [ + 'b_c' => ['type' => 'text', 'label' => 'B C'], + ]], + ]), 'demo', 1700000000); + } + + /** + * Sanity control — the same fixture with distinguishable names must + * keep working (the guard must not be over-broad / false-positive). + */ + public function test_distinct_flexible_content_layout_names_generate_without_collision(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'alpha' => ['label' => 'Alpha', 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + 'other' => ['type' => 'flexible_content', 'label' => 'Other', 'layouts' => [ + 'beta' => ['label' => 'Beta', 'fields' => [ + 'd' => ['type' => 'text', 'label' => 'D'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertCount(2, $group['fields']); + } + + /** + * Finding C (CRITICAL) — layout `display` and `location` must be + * captured verbatim when authored non-default, not hardcoded to + * `block` / `null`. + */ + public function test_flexible_content_layout_display_is_reconstructed_when_non_default(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Položky', 'layouts' => [ + 'title' => ['label' => 'Nadpis', 'wp' => ['display' => 'table'], 'fields' => [ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertSame('table', $group['fields'][0]['layouts'][0]['display']); + } + + public function test_flexible_content_layout_display_defaults_to_block_when_not_authored(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Položky', 'layouts' => [ + 'title' => ['label' => 'Nadpis', 'fields' => [ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertSame('block', $group['fields'][0]['layouts'][0]['display']); + } } diff --git a/tests/Generator/GenerationRoundTripTest.php b/tests/Generator/GenerationRoundTripTest.php index 6367a9b..b79fa63 100644 --- a/tests/Generator/GenerationRoundTripTest.php +++ b/tests/Generator/GenerationRoundTripTest.php @@ -340,4 +340,91 @@ public function test_reference_detail_nested_repeater_container_carries_parent_r self::assertArrayNotHasKey('parent_repeater', $headingChild); } } + + /** + * Finding E(i) — a synthetic two-level fixture: a flexible_content + * field whose layout itself contains ANOTHER flexible_content field. + * An adversarial reviewer built this by hand and confirmed it works; + * this pins the behaviour as a regression test rather than leaving it + * as an unverified claim. + */ + public function test_flexible_content_nested_inside_another_flexible_content_layout_round_trips(): void + { + $original = [ + 'key' => 'group_nested_fc', + 'title' => 'Nested FC', + 'fields' => [[ + 'key' => 'field_nested_outer', 'name' => 'outer', 'label' => 'Outer', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_nested_outer_wrap', 'name' => 'wrap', 'label' => 'Wrap', 'display' => 'block', + 'min' => '', 'max' => '', 'location' => null, + 'sub_fields' => [[ + 'key' => 'field_nested_outer_wrap_inner', 'name' => 'inner', 'label' => 'Inner', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_nested_outer_wrap_inner_leaf', 'name' => 'leaf', 'label' => 'Leaf', 'display' => 'block', + 'min' => '', 'max' => '', 'location' => null, + 'sub_fields' => [[ + 'key' => 'field_nested_outer_wrap_inner_leaf_title', 'name' => 'title', 'label' => 'Title', 'type' => 'text', + ]], + ]], + ]], + ]], + ]], + ]; + + $tree = (new AcfJsonReader())->read($original, 'nested'); + $regenerated = (new FieldsGenerator())->generate($tree, 'nested', 1700000000); + + // Not a full structural-exact diff (this hand-typed minimal + // fixture doesn't carry every real-ACF baseline prop) — the + // regression this test pins is specifically that a nested + // flexible_content-inside-a-flexible_content-layout round-trips + // its own layouts/keys/names correctly, which the assertions + // below verify directly against both original and regenerated. + $outer = $regenerated['fields'][0]; + self::assertSame('outer', $outer['name']); + self::assertSame('flexible_content', $outer['type']); + self::assertSame('wrap', $outer['layouts'][0]['name']); + + $innerFcField = $outer['layouts'][0]['sub_fields'][0]; + self::assertSame('inner', $innerFcField['name']); + self::assertSame('flexible_content', $innerFcField['type']); + self::assertSame('leaf', $innerFcField['layouts'][0]['name']); + self::assertSame('title', $innerFcField['layouts'][0]['sub_fields'][0]['name']); + self::assertSame('field_nested_outer_wrap_inner_leaf_title', $innerFcField['layouts'][0]['sub_fields'][0]['key']); + } + + /** + * Finding E(ii) — layout-level non-sentinel `min`/`max` round-trip + * through the FULL reader -> generator pipeline (each half is + * unit-tested separately in AcfJsonReaderTest / + * FieldsGeneratorTest already, but the combined pipeline wasn't + * pinned end-to-end until this test). + */ + public function test_flexible_content_layout_non_sentinel_min_max_round_trips_end_to_end(): void + { + $original = [ + 'key' => 'group_layout_bounds', + 'title' => 'Layout Bounds', + 'fields' => [[ + 'key' => 'field_bounds_items', 'name' => 'items', 'label' => 'Items', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_bounds_items_title', 'name' => 'title', 'label' => 'Title', 'display' => 'block', + 'min' => 1, 'max' => 3, 'location' => null, + 'sub_fields' => [[ + 'key' => 'field_bounds_items_title_title', 'name' => 'title', 'label' => 'Title', 'type' => 'text', + ]], + ]], + ]], + ]; + + $tree = (new AcfJsonReader())->read($original, 'bounds'); + self::assertSame(1, $tree['fields']['items']['layouts']['title']['min']); + self::assertSame(3, $tree['fields']['items']['layouts']['title']['max']); + + $regenerated = (new FieldsGenerator())->generate($tree, 'bounds', 1700000000); + $layout = $regenerated['fields'][0]['layouts'][0]; + self::assertSame(1, $layout['min']); + self::assertSame(3, $layout['max']); + } } diff --git a/tests/Integration/AcfLintValidationTest.php b/tests/Integration/AcfLintValidationTest.php new file mode 100644 index 0000000..8097a0c --- /dev/null +++ b/tests/Integration/AcfLintValidationTest.php @@ -0,0 +1,132 @@ + generate -> + * lint the regenerated acf.json with `acf-lint`'s own validator class. A + * projection that fails the ecosystem validator must fail this test (and, + * wired the same way in CI, the build). + */ +final class AcfLintValidationTest extends TestCase +{ + /** + * Pre-existing, out-of-scope data residuals this wiring surfaced that + * predate this PR and have nothing to do with flexible_content: the + * `store-locator` corpus fixture's raw `gallery` field carries + * `wpml_cf_preferences: 2`, while `parisek/acf-json-schema` requires + * `const 1` for `gallery` (non-translatable-media convention). This is + * a legacy corpus-data quirk (the fixture has no flexible_content + * field at all — `post_object`/`gallery`/`file`/`google_map`/ + * `date_picker` only) that a maintainer should fix in the fixture or + * relax in the schema on its own merits — NOT something this PR's + * flexible_content work should silently paper over. Skipping it here + * (with this exact justification, mirroring + * GenerationRoundTripTest's own documented-residual pattern) keeps the + * wiring's actual job — catching flexible_content regressions — + * intact without blocking on an unrelated finding. + * + * @var list + */ + private const KNOWN_PRE_EXISTING_RESIDUALS = ['store-locator']; + + private AcfLinter $linter; + + protected function setUp(): void + { + $this->linter = new AcfLinter(__DIR__ . '/../../vendor/parisek/acf-json-schema/schemas'); + } + + /** + * @return list + */ + private static function migrationFixtureAcfJsonPaths(): array + { + $root = __DIR__ . '/../fixtures/migration'; + $paths = []; + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($root, \FilesystemIterator::SKIP_DOTS), + ); + foreach ($iterator as $file) { + if ($file->isFile() && 'acf.json' === $file->getFilename()) { + $paths[] = $file->getPathname(); + } + } + sort($paths); + return $paths; + } + + /** + * @return array + */ + public static function migrationFixtureProvider(): array + { + $cases = []; + foreach (self::migrationFixtureAcfJsonPaths() as $path) { + $slug = basename(dirname($path)); + $cases[$slug] = [$path, $slug]; + } + return $cases; + } + + #[DataProvider('migrationFixtureProvider')] + public function test_regenerated_acf_json_passes_acf_lint(string $acfJsonPath, string $slug): void + { + if (in_array($slug, self::KNOWN_PRE_EXISTING_RESIDUALS, true)) { + self::markTestSkipped("{$slug}: known pre-existing, out-of-scope data residual — see class docblock"); + } + + $original = json_decode((string) file_get_contents($acfJsonPath), true, flags: JSON_THROW_ON_ERROR); + + $twigPath = dirname($acfJsonPath) . "/{$slug}.twig"; + $twigSource = is_file($twigPath) ? file_get_contents($twigPath) : null; + + $tree = (new AcfJsonReader())->read($original, $slug, $twigSource ?: null); + $regenerated = (new FieldsGenerator())->generate($tree, $slug, (int) $original['modified']); + + $tmpPath = sys_get_temp_dir() . '/dk-acf-lint-' . $slug . '-' . bin2hex(random_bytes(4)) . '.json'; + file_put_contents( + $tmpPath, + json_encode($regenerated, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE) . "\n", + ); + + try { + // acf-lint dispatches by filename for block.json; anything else + // with fields+location is treated as acf.json shape regardless + // of the temp file's actual basename, so a random tmp name is + // safe here (confirmed via AcfLinter::dispatch()). + $renamed = dirname($tmpPath) . '/acf.json'; + rename($tmpPath, $renamed); + $result = $this->linter->lintFile($renamed, fix: false); + + self::assertTrue( + $result->skipped === false, + "acf-lint skipped '{$slug}' — dispatch() didn't recognize the regenerated shape as ACF", + ); + self::assertTrue( + $result->valid, + "acf-lint found violations in regenerated acf.json for '{$slug}': " + . json_encode($result->errors, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), + ); + } finally { + @unlink($renamed ?? $tmpPath); + } + } +} diff --git a/tests/Migration/AcfJsonReaderTest.php b/tests/Migration/AcfJsonReaderTest.php index 738d2eb..f4e16a8 100644 --- a/tests/Migration/AcfJsonReaderTest.php +++ b/tests/Migration/AcfJsonReaderTest.php @@ -550,4 +550,73 @@ public function test_flexible_content_layout_with_zero_non_accordion_sub_fields_ ]], ]]), 'demo'); } + + /** + * Finding B (CRITICAL) — two layouts sharing the same `name` collapse + * into one PHP array key (`$out[$layoutName] = …`) with no collision + * check: the earlier layout AND its key silently vanish. Reproduced + * live against a synthetic ACF export by an adversarial reviewer. + * The reader must throw, matching the style of the adjacent + * empty-layouts / empty-sub-fields guards above. + */ + public function test_flexible_content_duplicate_layout_names_throws_instead_of_silently_overwriting(): void + { + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessageMatches('/duplicate/i'); + + $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [ + [ + 'key' => 'layout_demo_items_title_v1', 'name' => 'title', 'label' => 'Nadpis V1', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_v1_a', 'name' => 'a', 'label' => 'A', 'type' => 'text'], + ], + ], + [ + 'key' => 'layout_demo_items_title_v2', 'name' => 'title', 'label' => 'Nadpis V2', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_v2_b', 'name' => 'b', 'label' => 'B', 'type' => 'text'], + ], + ], + ], + ]]), 'demo'); + } + + /** + * Finding C (CRITICAL) — a layout authored with a non-default + * `display` (`table` / `row`) must round-trip verbatim, not be + * silently dropped (the generator side currently hardcodes `block` + * unconditionally, so the reader must actually capture the raw + * value for the round-trip to be possible at all). + */ + public function test_flexible_content_layout_non_default_display_is_captured(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'table', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + + self::assertSame('table', $tree['fields']['items']['layouts']['title']['wp']['display']); + } + + public function test_flexible_content_layout_default_display_leaves_no_wp_trace(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + + self::assertArrayNotHasKey('wp', $tree['fields']['items']['layouts']['title']); + } } diff --git a/tests/Migration/MigrationCompletenessAuditorTest.php b/tests/Migration/MigrationCompletenessAuditorTest.php index 9c6b897..87cf6d1 100644 --- a/tests/Migration/MigrationCompletenessAuditorTest.php +++ b/tests/Migration/MigrationCompletenessAuditorTest.php @@ -189,4 +189,52 @@ public function test_flexible_content_wpml_cf_preferences_is_never_flagged_as_un ]]]; self::assertSame([], $this->auditor->audit($acf, $def)); } + + /** + * Finding B (CRITICAL, auditor half) — AcfJsonReader::readLayouts() + * collapses duplicate layout names via `$out[$layoutName] = …` + * (fixed elsewhere to throw), but the auditor has an INDEPENDENT + * masking bug: it iterates the raw ACF `$layouts` list one at a + * time and looks each one up in `$defLayouts` (already-collapsed to + * one key per name). When both duplicate layouts happen to share an + * identical sub-field shape, every iteration "matches" the same + * single migrated layout and the auditor reports zero violations — + * even though one whole raw layout was silently discarded. The + * auditor must independently detect the duplicate in the raw ACF + * source, regardless of what the reader does. + */ + public function test_duplicate_layout_names_with_identical_shape_are_flagged_not_masked(): void + { + $acf = [[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Items', 'type' => 'flexible_content', + 'layouts' => [ + [ + 'key' => 'layout_demo_items_body_v1', 'name' => 'body', 'label' => 'Body', 'display' => 'block', + 'sub_fields' => [[ + 'key' => 'field_demo_items_body_v1_text', 'name' => 'text', 'label' => 'Text', 'type' => 'text', + ]], + ], + [ + 'key' => 'layout_demo_items_body_v2', 'name' => 'body', 'label' => 'Body', 'display' => 'block', + 'sub_fields' => [[ + 'key' => 'field_demo_items_body_v2_text', 'name' => 'text', 'label' => 'Text', 'type' => 'text', + ]], + ], + ], + ]]; + // Same shape as EITHER raw layout — this is exactly the case that + // previously slipped through undetected. + $def = ['items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'body' => ['label' => 'Body', 'fields' => [ + 'text' => ['type' => 'text', 'label' => 'Text'], + ]], + ]]]; + + $violations = $this->auditor->audit($acf, $def); + self::assertNotEmpty($violations, 'duplicate layout name must be flagged, not silently masked'); + self::assertTrue( + (bool) array_filter($violations, static fn (string $v): bool => str_contains(strtolower($v), 'duplicate')), + 'expected a duplicate-layout violation, got: ' . implode(' | ', $violations), + ); + } } diff --git a/tests/Schema/FieldsSchemaValidatorTest.php b/tests/Schema/FieldsSchemaValidatorTest.php index 81f8469..3117d59 100644 --- a/tests/Schema/FieldsSchemaValidatorTest.php +++ b/tests/Schema/FieldsSchemaValidatorTest.php @@ -163,6 +163,31 @@ public function test_flexible_content_without_layouts_fails_with_pointer_to_fiel ); } + /** + * Finding D (HIGH) — this schema currently requires only `["fields"]` + * on a layout, so a layout without `label` passes here but the + * generator emits `"label": ""`, which fails + * parisek/acf-json-schema's field-flexible_content.schema.json + * (`required: ["key","name","label"]`, `label` `minLength: 1`). + * Bring the two schemas into parity. + */ + public function test_flexible_content_layout_without_label_fails(): void + { + $result = (new FieldsSchemaValidator())->validateFile($this->fixture('invalid-flexible-content-layout-no-label.fields.yaml')); + self::assertFalse($result->valid); + } + + /** + * Finding D (HIGH, second half) — layout map keys had no pattern + * constraint while downstream (parisek/acf-json-schema) requires + * `^[a-z][a-z0-9_]*$` for ACF `name` values. + */ + public function test_flexible_content_layout_name_must_match_acf_name_pattern(): void + { + $result = (new FieldsSchemaValidator())->validateFile($this->fixture('invalid-flexible-content-bad-layout-name.fields.yaml')); + self::assertFalse($result->valid); + } + public function test_key_not_matching_field_prefix_fails_with_pointer_to_field(): void { $result = (new FieldsSchemaValidator())->validateFile($this->fixture('invalid-bad-key-pattern.fields.yaml')); diff --git a/tests/fixtures/invalid-flexible-content-bad-layout-name.fields.yaml b/tests/fixtures/invalid-flexible-content-bad-layout-name.fields.yaml new file mode 100644 index 0000000..589ab79 --- /dev/null +++ b/tests/fixtures/invalid-flexible-content-bad-layout-name.fields.yaml @@ -0,0 +1,10 @@ +name: Demo Invalid Flexible Content Layout Name +fields: + items: + type: flexible_content + label: Položky + layouts: + Title-Layout: # uppercase + hyphen -- must match ^[a-z][a-z0-9_]*$ + label: Nadpis + fields: + title: { type: text, label: Nadpis } diff --git a/tests/fixtures/invalid-flexible-content-layout-no-label.fields.yaml b/tests/fixtures/invalid-flexible-content-layout-no-label.fields.yaml new file mode 100644 index 0000000..bbef8d3 --- /dev/null +++ b/tests/fixtures/invalid-flexible-content-layout-no-label.fields.yaml @@ -0,0 +1,12 @@ +name: Demo Invalid Flexible Content Layout Label +fields: + items: + type: flexible_content + label: Položky + layouts: + title: + # missing required `label` -- generator would emit "label": "" + # which parisek/acf-json-schema's field-flexible_content schema + # rejects (required: ["key","name","label"], label minLength 1). + fields: + title: { type: text, label: Nadpis } From 410b24708ed729f37247da4691e4f90e07638b51 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 13:44:41 +0200 Subject: [PATCH 03/10] Round-3 review fixes: layout key pinning, accordion key-collision guard, display/location round-trip proof, lint temp-file collision, wpml_cf_preferences data fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7 --- src/Generator/FieldsGenerator.php | 16 +++- src/Migration/AcfJsonReader.php | 40 +++++++- tests/Generator/FieldsGeneratorTest.php | 50 ++++++++++ tests/Generator/GenerationRoundTripTest.php | 45 +++++++++ tests/Integration/AcfLintValidationTest.php | 75 +++++++++------ tests/Migration/AbstractTypeMapperTest.php | 28 ++++++ tests/Migration/AcfJsonReaderTest.php | 57 ++++++++++- .../corpus-sample/store-locator/acf.json | 2 +- .../non-default-layout-display/acf.json | 94 +++++++++++++++++++ 9 files changed, 369 insertions(+), 38 deletions(-) create mode 100644 tests/fixtures/migration/non-default-layout-display/acf.json diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 2016c26..93b884d 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -60,9 +60,21 @@ public function generate(array $definitionTree, string $componentSlug, int $modi ); } - $this->assertGloballyUniqueKeys($orderedRawFields); + $built = $this->rootBuilder->build($definitionTree, $orderedRawFields, $componentSlug, $modifiedAt); - return $this->rootBuilder->build($definitionTree, $orderedRawFields, $componentSlug, $modifiedAt); + // Finding 2 (round 3, HIGH) — the uniqueness scan MUST run over the + // final assembled `fields` list (built['fields']), not + // $orderedRawFields. RootFieldGroupBuilder::build() interleaves + // accordion pseudo-fields (from root `wp.accordions`) into that + // list — an accordion's own `key` (or a collision between two + // accordions, or an accordion and an ordinary field) would + // otherwise slip past this guard entirely, since accordions don't + // exist yet at the point $orderedRawFields is assembled. + /** @var list> $builtFields */ + $builtFields = $built['fields']; + $this->assertGloballyUniqueKeys($builtFields); + + return $built; } /** diff --git a/src/Migration/AcfJsonReader.php b/src/Migration/AcfJsonReader.php index 1ea1041..6b1ab8e 100644 --- a/src/Migration/AcfJsonReader.php +++ b/src/Migration/AcfJsonReader.php @@ -414,6 +414,20 @@ private function readLayouts(array $layouts, string $componentSlug, array $nameC // value. Capture them verbatim into the layout's own `wp:` // escape hatch whenever they deviate from the ACF default, so // FieldsGenerator can replay them instead of guessing. + // + // Deliberate canonicalisation, not presence-loss (round 3, + // finding 3): this is VALUE-level round-tripping, not + // presence-level. A layout's `display`/`location` keys are + // always present in real ACF Local JSON exports — every real + // corpus fixture with flexible_content confirms it (see + // `test_flexible_content_layout_non_default_display_and_location_round_trip_by_value` + // in GenerationRoundTripTest) — so "absent" never actually + // happens; the only thing this canonicalises is an EXPLICITLY + // authored default value (`display: "block"`, `location: null`) + // down to omitting the `wp:` entry, which regenerates the + // identical default value either way. A non-default value is + // never coerced — it always round-trips byte-for-byte via the + // `wp:` bag below. $layoutWp = []; $display = (string) ($layout['display'] ?? 'block'); if ('block' !== $display) { @@ -427,10 +441,28 @@ private function readLayouts(array $layouts, string $componentSlug, array $nameC $layoutOut['wp'] = $layoutWp; } - $expectedLayoutKey = 'layout_' . $componentSlug . '_' . implode('_', $layoutChain); - if ((string) $layout['key'] !== $expectedLayoutKey) { - $layoutOut['key'] = (string) $layout['key']; - } + // Finding 1 (round 3, CRITICAL) — unlike an ordinary field's key + // (only pinned when it deviates from the derived convention — + // that omit-if-matching behaviour is deliberate, long-tested + // doctrine for FIELDS, see `test_field_key_omitted_when_matches_convention`- + // style assertions elsewhere in this class), a LAYOUT's key is + // ALWAYS pinned verbatim, unconditionally. The two cases aren't + // symmetric: a field's `name` is itself the ACF-authoritative + // identity (renaming the YAML map key IS renaming the field, + // same postmeta key either way), but a flexible_content layout's + // map key is a purely cosmetic authoring label — the *real* + // ACF/WordPress identity of a layout is its `key` + // (`acf_fc_layout` values stored in postmeta reference the + // layout's `name`, and the layout's OWN identity for Admin + // "Sync" routing is `key`). If the key is pinned only when it + // deviates from the derived convention, renaming a layout whose + // key happens to already match the convention re-derives a + // DIFFERENT key on next generation — silently orphaning every + // `acf_fc_layout` value already stored for that layout, with no + // diagnostic. Always emitting `key` here freezes that identity + // at migration time regardless of what the YAML map key is + // renamed to afterwards. + $layoutOut['key'] = (string) $layout['key']; $out[$layoutName] = $layoutOut; } diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index a2b0a82..f1bc6a2 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -375,6 +375,56 @@ public function test_ordinary_nested_group_field_name_ambiguity_is_rejected(): v ]), 'demo', 1700000000); } + /** + * Finding 2 (round 3, HIGH) — `generate()` runs + * `assertGloballyUniqueKeys($orderedRawFields)` BEFORE + * `RootFieldGroupBuilder::build()` re-inserts accordion pseudo-fields + * from root `wp.accordions` into the final assembled `fields` list. + * A duplicate key hidden in an accordion (colliding with either an + * ordinary field's key or another accordion's key) therefore slips + * straight past the "global" uniqueness guard and reaches the + * generated acf.json — exactly the postmeta-aliasing hazard the guard + * exists to catch, just via a different injection point than + * fields/layouts. The guard must see the FINAL assembled fields list, + * accordions included, not just the ordinary fields it validates + * today. + */ + public function test_accordion_key_colliding_with_an_ordinary_field_key_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/field_demo_title/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['accordions' => [ + ['key' => 'field_demo_title', 'label' => 'Section', 'open' => 0, 'before' => 'title'], + ]], + ]), 'demo', 1700000000); + } + + /** + * Same collision class, but between two accordions' own keys — no + * ordinary field involved at all, proving the guard must scan the + * accordion list itself, not just cross-check it against fields. + */ + public function test_two_accordions_sharing_the_same_key_are_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/field_demo_dup_accordion/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['accordions' => [ + ['key' => 'field_demo_dup_accordion', 'label' => 'A', 'open' => 0, 'before' => 'title'], + ['key' => 'field_demo_dup_accordion', 'label' => 'B', 'open' => 0], + ]], + ]), 'demo', 1700000000); + } + /** * Sanity control — the same fixture with distinguishable names must * keep working (the guard must not be over-broad / false-positive). diff --git a/tests/Generator/GenerationRoundTripTest.php b/tests/Generator/GenerationRoundTripTest.php index b79fa63..0853f95 100644 --- a/tests/Generator/GenerationRoundTripTest.php +++ b/tests/Generator/GenerationRoundTripTest.php @@ -427,4 +427,49 @@ public function test_flexible_content_layout_non_sentinel_min_max_round_trips_en self::assertSame(1, $layout['min']); self::assertSame(3, $layout['max']); } + + /** + * Finding 4 (round 3, MODERATE) — the value-level round-trip proof + * `AcfLintValidationTest` is missing. Schema validity (which that test + * checks) is not equivalence: `display: 'table'`/`display: 'row'` and + * a real non-null `location` are just as schema-valid as the + * hardcoded `display: 'block'` / `location: null` defaults would be — + * a regression that silently reverted Finding C (this generator + * hardcoding the default unconditionally again) would pass + * `acf-lint --strict` without a single failure. None of the two real + * corpus fixtures with flexible_content (`split-content`, + * `box-price-reference`) happen to author non-default display/ + * location, so even `GenerationRoundTripTest`'s own full-diff + * assertions above don't exercise this path — they'd stay green + * through the same reversion. + * + * This fixture is therefore a definition-kit-authored synthetic + * corpus sample (not a real eprukaz/mairateam component) whose sole + * job is to carry non-default `display` (`table`/`row`) and a real, + * non-null `location` on two sibling layouts, then assert those exact + * VALUES survive read -> generate unchanged. Reverting either + * AcfJsonReader::readLayouts()'s capture or + * FieldsGenerator::buildLayouts()'s replay back to a hardcoded + * 'block'/null breaks this assertion immediately. + */ + public function test_flexible_content_layout_non_default_display_and_location_round_trip_by_value(): void + { + $fixtureDir = __DIR__ . '/../fixtures/migration/non-default-layout-display'; + $result = $this->roundTrip("{$fixtureDir}/acf.json", 'non-default-layout-display'); + + $originalLayouts = $result['original']['fields'][0]['layouts']; + $regeneratedLayouts = $result['regenerated']['fields'][0]['layouts']; + + self::assertSame(['title', 'image'], array_column($regeneratedLayouts, 'name')); + + self::assertSame('table', $originalLayouts[0]['display']); + self::assertSame($originalLayouts[0]['display'], $regeneratedLayouts[0]['display']); + self::assertSame($originalLayouts[0]['location'], $regeneratedLayouts[0]['location']); + self::assertNotNull($regeneratedLayouts[0]['location']); + + self::assertSame('row', $originalLayouts[1]['display']); + self::assertSame($originalLayouts[1]['display'], $regeneratedLayouts[1]['display']); + self::assertSame($originalLayouts[1]['location'], $regeneratedLayouts[1]['location']); + self::assertNull($regeneratedLayouts[1]['location']); + } } diff --git a/tests/Integration/AcfLintValidationTest.php b/tests/Integration/AcfLintValidationTest.php index 8097a0c..581a033 100644 --- a/tests/Integration/AcfLintValidationTest.php +++ b/tests/Integration/AcfLintValidationTest.php @@ -28,25 +28,38 @@ final class AcfLintValidationTest extends TestCase { /** - * Pre-existing, out-of-scope data residuals this wiring surfaced that - * predate this PR and have nothing to do with flexible_content: the - * `store-locator` corpus fixture's raw `gallery` field carries - * `wpml_cf_preferences: 2`, while `parisek/acf-json-schema` requires - * `const 1` for `gallery` (non-translatable-media convention). This is - * a legacy corpus-data quirk (the fixture has no flexible_content - * field at all — `post_object`/`gallery`/`file`/`google_map`/ - * `date_picker` only) that a maintainer should fix in the fixture or - * relax in the schema on its own merits — NOT something this PR's - * flexible_content work should silently paper over. Skipping it here - * (with this exact justification, mirroring - * GenerationRoundTripTest's own documented-residual pattern) keeps the - * wiring's actual job — catching flexible_content regressions — - * intact without blocking on an unrelated finding. + * Finding 5 (round 3, MODERATE) — round 2 skipped `store-locator` here, + * framing it as "two first-party packages disagreeing" (definition-kit + * regenerating `wpml_cf_preferences: 2` for a `gallery` field vs. + * `parisek/acf-json-schema` requiring `const 1`). Investigated on the + * merits instead of accepting that framing: * - * @var list + * - `parisek/acf-json-schema`'s `field-gallery.schema.json` / + * `field-image.schema.json` both hardcode `const: 1` — deliberate, + * matching this project's OWN documented doctrine + * (`.claude/rules/wordpress/gutenberg.md` § ACF Field Type Mapping: + * media fields are non-translatable, `wpml_cf_preferences: 1`). + * - definition-kit's `WpmlTranslatableMapper` treats every non-container + * leaf type uniformly (`isCanonical()` accepts 1 OR 2 for ANY leaf), + * with no per-type awareness that media types are non-translatable- + * only — so it faithfully round-trips whatever raw value the source + * JSON had, including an anomalous `2` on a `gallery` field. + * - The `store-locator` fixture's `photos` gallery field ("Galerie", + * no locale-specific instructions, no per-language distinguishing + * context) is an ordinary photo gallery with no plausible reason to + * be marked "Translate" in WPML — it is legacy real-world ACF + * Admin-authored data that itself violates the project's own + * doctrine, not a case the doctrine failed to anticipate. + * + * Conclusion: the schema is right, definition-kit's round-trip is + * (correctly) faithful to a flawed input, and the fixture's data was + * simply wrong. Fixed the COPIED test fixture in this repo (not the + * live eprukaz/mairateam site — this repo's fixture is a static + * snapshot) from `wpml_cf_preferences: 2` to `1` on the `photos` + * field, and dropped the skip entirely — acf-lint now passes for every + * fixture with no exceptions. `parisek/acf-json-schema` was NOT + * touched; no draft PR was needed because the schema was correct. */ - private const KNOWN_PRE_EXISTING_RESIDUALS = ['store-locator']; - private AcfLinter $linter; protected function setUp(): void @@ -89,10 +102,6 @@ public static function migrationFixtureProvider(): array #[DataProvider('migrationFixtureProvider')] public function test_regenerated_acf_json_passes_acf_lint(string $acfJsonPath, string $slug): void { - if (in_array($slug, self::KNOWN_PRE_EXISTING_RESIDUALS, true)) { - self::markTestSkipped("{$slug}: known pre-existing, out-of-scope data residual — see class docblock"); - } - $original = json_decode((string) file_get_contents($acfJsonPath), true, flags: JSON_THROW_ON_ERROR); $twigPath = dirname($acfJsonPath) . "/{$slug}.twig"; @@ -101,19 +110,24 @@ public function test_regenerated_acf_json_passes_acf_lint(string $acfJsonPath, s $tree = (new AcfJsonReader())->read($original, $slug, $twigSource ?: null); $regenerated = (new FieldsGenerator())->generate($tree, $slug, (int) $original['modified']); - $tmpPath = sys_get_temp_dir() . '/dk-acf-lint-' . $slug . '-' . bin2hex(random_bytes(4)) . '.json'; + // Finding 6 (round 3, LOW) — acf-lint dispatches by filename for + // block.json; anything else with fields+location is treated as + // acf.json shape regardless of the actual basename (confirmed via + // AcfLinter::dispatch()). It still needs a FILE named exactly + // `acf.json` though, so a unique per-test DIRECTORY (not just a + // unique filename) is required — the previous implementation + // renamed into the shared `sys_get_temp_dir()` root as a fixed + // `acf.json`, which collides the moment two data-provider cases + // run concurrently (parallel test runners, e.g. paratest). + $tmpDir = sys_get_temp_dir() . '/dk-acf-lint-' . $slug . '-' . bin2hex(random_bytes(8)); + mkdir($tmpDir); + $renamed = "{$tmpDir}/acf.json"; file_put_contents( - $tmpPath, + $renamed, json_encode($regenerated, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE) . "\n", ); try { - // acf-lint dispatches by filename for block.json; anything else - // with fields+location is treated as acf.json shape regardless - // of the temp file's actual basename, so a random tmp name is - // safe here (confirmed via AcfLinter::dispatch()). - $renamed = dirname($tmpPath) . '/acf.json'; - rename($tmpPath, $renamed); $result = $this->linter->lintFile($renamed, fix: false); self::assertTrue( @@ -126,7 +140,8 @@ public function test_regenerated_acf_json_passes_acf_lint(string $acfJsonPath, s . json_encode($result->errors, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), ); } finally { - @unlink($renamed ?? $tmpPath); + @unlink($renamed); + @rmdir($tmpDir); } } } diff --git a/tests/Migration/AbstractTypeMapperTest.php b/tests/Migration/AbstractTypeMapperTest.php index c9b7700..578fdb3 100644 --- a/tests/Migration/AbstractTypeMapperTest.php +++ b/tests/Migration/AbstractTypeMapperTest.php @@ -149,6 +149,34 @@ public function test_repeater_lifts_button_label_to_add_label(): void self::assertSame(['type', 'sub_fields', 'button_label'], $result['consumed']); } + /** + * Round-1 finding #5, re-examined in round 3 (not fixed — deferral + * confirmed, not merely re-asserted): an explicitly-authored empty + * `button_label: ""` is treated identically to an absent one, so + * `add_label` is never lifted and the empty string isn't captured + * anywhere — not even the `wp:` escape hatch. Two things support + * keeping this as-is rather than treating it as a real defect: + * + * 1. This mirrors the SAME `'' === "not authored"` sentinel + * convention already established project-wide for every other + * ACF text-ish constraint (`maxlength`, `min`/`max`/`step`, + * repeater/flexible_content row bounds — see + * `schemas/constraint-sentinels.yaml` and this class's own + * min/max handling). `button_label` isn't a special case that + * accidentally fell through a gap; it consistently follows the + * rule every other prop in this codebase follows. + * 2. No real corpus fixture in this repo authors an explicit empty + * `button_label` (`rg '"button_label": ""'` across + * `tests/fixtures/` finds nothing) — ACF's own admin UI/JS falls + * back to its stock "Add Row" text whether the prop is absent OR + * an empty string, so there is no distinguishable production + * behaviour being lost by treating them the same. + * + * If a future corpus fixture is found with a genuinely meaningful + * empty `button_label` (e.g. a deliberately blanked button showing + * icon-only), re-open this — but as of round 3 there's no evidence + * that's a real authored state, so the low-risk deferral holds. + */ public function test_repeater_omits_add_label_when_button_label_empty(): void { $result = $this->mapper->map(['type' => 'repeater', 'name' => 'f', 'button_label' => '']); diff --git a/tests/Migration/AcfJsonReaderTest.php b/tests/Migration/AcfJsonReaderTest.php index f4e16a8..b135970 100644 --- a/tests/Migration/AcfJsonReaderTest.php +++ b/tests/Migration/AcfJsonReaderTest.php @@ -445,7 +445,14 @@ public function test_flexible_content_lifts_layouts_keyed_by_name(): void self::assertSame(2, $items['max']); self::assertSame(['title', 'image'], array_keys($items['layouts'])); self::assertSame('Nadpis', $items['layouts']['title']['label']); - self::assertArrayNotHasKey('key', $items['layouts']['title']); // matches convention, not pinned + // Finding 1 (round 3) — a layout's key is ALWAYS pinned, even when + // it matches the derived convention. Unlike ordinary fields (whose + // `name` IS the identity, so omit-if-matching is safe), a layout's + // YAML map key is cosmetic — only `key` is the real ACF identity. + // Always pinning means renaming the map key later can never + // silently re-derive a different key. See + // test_flexible_content_layout_key_is_pinned_even_when_matching_convention_so_renaming_the_map_key_is_safe. + self::assertSame('layout_demo_items_title', $items['layouts']['title']['key']); self::assertSame('text', $items['layouts']['title']['fields']['title']['type']); self::assertSame('media', $items['layouts']['image']['fields']['image']['type']); self::assertSame('image', $items['layouts']['image']['fields']['image']['kind']); @@ -466,6 +473,54 @@ public function test_flexible_content_layout_key_deviating_from_convention_is_pi self::assertSame('layout_demo_legacy_hash_abc123', $tree['fields']['items']['layouts']['title']['key']); } + /** + * Finding 1 (round 3, CRITICAL) — the regression this pinning fix + * exists for. Migrate a layout whose ACF key happens to already match + * the derived convention (so, pre-fix, AcfJsonReader would NOT have + * pinned it), then simulate a maintainer renaming the layout's YAML + * map key (`title` -> `heading`) — a routine, innocent-looking + * refactor. Assert the migrated layout's `key` is present and + * unchanged by that rename: FieldsGenerator must regenerate the exact + * same `layout_demo_items_title` key regardless of the map key, + * because AcfJsonReader always pins it verbatim at migration time. + * Before the fix, this key would silently re-derive to + * `layout_demo_items_heading`, orphaning any `acf_fc_layout: "title"` + * postmeta already stored in production. + */ + public function test_flexible_content_layout_key_is_pinned_even_when_matching_convention_so_renaming_the_map_key_is_safe(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + + $migratedLayout = $tree['fields']['items']['layouts']['title']; + self::assertSame('layout_demo_items_title', $migratedLayout['key']); + + // Simulate the maintainer renaming the map key post-migration — + // the YAML the generator now sees carries the SAME pinned `key` + // under a different map key. + $renamedLayouts = ['heading' => $migratedLayout]; + $renamedTree = $tree; + $renamedTree['fields']['items']['layouts'] = $renamedLayouts; + + $generated = (new \Parisek\DefinitionKit\Generator\FieldsGenerator())->generate($renamedTree, 'demo', 1); + $regeneratedLayout = $generated['fields'][0]['layouts'][0]; + + self::assertSame('heading', $regeneratedLayout['name']); + self::assertSame( + 'layout_demo_items_title', + $regeneratedLayout['key'], + 'Renaming the layout map key must not change its ACF key — otherwise ' + . 'every acf_fc_layout="title" value already stored in postmeta is orphaned.', + ); + } + public function test_flexible_content_min_max_zero_is_omitted_as_acf_no_limit_sentinel(): void { $tree = $this->reader->read($this->group([[ diff --git a/tests/fixtures/migration/corpus-sample/store-locator/acf.json b/tests/fixtures/migration/corpus-sample/store-locator/acf.json index 83a35f0..2267531 100644 --- a/tests/fixtures/migration/corpus-sample/store-locator/acf.json +++ b/tests/fixtures/migration/corpus-sample/store-locator/acf.json @@ -33,7 +33,7 @@ "required": 0, "conditional_logic": false, "wrapper": { "width": "", "class": "", "id": "" }, - "wpml_cf_preferences": 2, + "wpml_cf_preferences": 1, "return_format": "array", "preview_size": "medium", "insert": "append", diff --git a/tests/fixtures/migration/non-default-layout-display/acf.json b/tests/fixtures/migration/non-default-layout-display/acf.json new file mode 100644 index 0000000..87a8776 --- /dev/null +++ b/tests/fixtures/migration/non-default-layout-display/acf.json @@ -0,0 +1,94 @@ +{ + "key": "group_non-default-layout-display", + "title": "Non-Default Layout Display", + "fields": [ + { + "key": "field_non-default-layout-display_items", + "allow_in_bindings": 0, + "label": "Items", + "name": "items", + "aria-label": "", + "type": "flexible_content", + "instructions": "", + "required": 0, + "conditional_logic": false, + "wrapper": { "width": "", "class": "", "id": "" }, + "layouts": [ + { + "key": "layout_non-default-layout-display_items_title", + "name": "title", + "label": "Nadpis", + "display": "table", + "sub_fields": [ + { + "key": "field_non-default-layout-display_items_title_title", + "allow_in_bindings": 0, + "label": "Nadpis", + "name": "title", + "aria-label": "", + "type": "text", + "instructions": "", + "required": 0, + "conditional_logic": false, + "wrapper": { "width": "", "class": "", "id": "" }, + "wpml_cf_preferences": 2, + "default_value": "", + "placeholder": "", + "maxlength": "" + } + ], + "min": "", + "max": "", + "location": [[{"param": "post_type", "operator": "==", "value": "page"}]] + }, + { + "key": "layout_non-default-layout-display_items_image", + "name": "image", + "label": "Obrázek", + "display": "row", + "sub_fields": [ + { + "key": "field_non-default-layout-display_items_image_image", + "allow_in_bindings": 0, + "label": "Obrázek", + "name": "image", + "aria-label": "", + "type": "image", + "instructions": "", + "required": 0, + "conditional_logic": false, + "wrapper": { "width": "", "class": "", "id": "" }, + "wpml_cf_preferences": 1, + "return_format": "array", + "library": "all", + "min_width": "", + "min_height": "", + "min_size": "", + "max_width": "", + "max_height": "", + "max_size": "", + "mime_types": "", + "preview_size": "medium" + } + ], + "min": "", + "max": "", + "location": null + } + ], + "wpml_cf_preferences": 3 + } + ], + "location": [[{"param": "block", "operator": "==", "value": "acf/non-default-layout-display"}]], + "menu_order": 0, + "position": "normal", + "style": "default", + "label_placement": "top", + "instruction_placement": "label", + "hide_on_screen": "", + "active": true, + "description": "", + "show_in_rest": 0, + "acfml_field_group_mode": "advanced", + "modified": 1700000000 +} From 92d1cf3173ee76f58c4abbc99c4f86e8e52df730 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 14:07:40 +0200 Subject: [PATCH 04/10] Round-4 fix: pin a flexible_content layout's ACF name, not just its key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7 --- schemas/component.fields.schema.json | 1 + src/Generator/FieldsGenerator.php | 16 +++++++-- src/Migration/AcfJsonReader.php | 13 +++++++ tests/Migration/AcfJsonReaderTest.php | 50 ++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 3 deletions(-) diff --git a/schemas/component.fields.schema.json b/schemas/component.fields.schema.json index 01b23dc..194562b 100644 --- a/schemas/component.fields.schema.json +++ b/schemas/component.fields.schema.json @@ -137,6 +137,7 @@ "properties": { "label": { "type": "string", "minLength": 1 }, "key": { "type": "string", "pattern": "^layout_" }, + "name": { "type": "string", "minLength": 1 }, "min": { "type": "integer", "minimum": 0 }, "max": { "type": "integer", "minimum": 0 }, "wp": { "type": "object" }, diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 93b884d..9af2100 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -245,7 +245,19 @@ private function buildLayouts(array $layoutDefs, string $componentSlug, array $n { $layouts = []; foreach ($layoutDefs as $layoutName => $layoutDef) { - $layoutChain = [...$nameChain, (string) $layoutName]; + // Finding 1 (round 4, CRITICAL) — the ACF `name` (what WordPress + // stores in `acf_fc_layout` postmeta) is pinned verbatim by + // AcfJsonReader::readLayouts() via the layout's own `name:` key, + // exactly like `key` already was per round 3. The YAML map key + // (`$layoutName`) is a cosmetic authoring label a maintainer can + // rename freely; trusting it as the ACF name would silently + // rewrite postmeta identity on every rename. Prefer the pinned + // `name:` when present; fall back to the map key only for + // hand-authored definitions that never went through migration + // (a brand-new layout authored directly in YAML, where the map + // key genuinely IS the only name that has ever existed). + $acfName = (string) ($layoutDef['name'] ?? $layoutName); + $layoutChain = [...$nameChain, $acfName]; $layoutKey = (string) ($layoutDef['key'] ?? ('layout_' . $componentSlug . '_' . implode('_', $layoutChain))); $childFields = (array) ($layoutDef['fields'] ?? []); @@ -266,7 +278,7 @@ private function buildLayouts(array $layoutDefs, string $componentSlug, array $n $layout = [ 'key' => $layoutKey, - 'name' => (string) $layoutName, + 'name' => $acfName, 'label' => (string) ($layoutDef['label'] ?? ''), // Finding C — `display`/`location` are canonical ACF // layout props (block|table|row for display) captured diff --git a/src/Migration/AcfJsonReader.php b/src/Migration/AcfJsonReader.php index 6b1ab8e..0a4d240 100644 --- a/src/Migration/AcfJsonReader.php +++ b/src/Migration/AcfJsonReader.php @@ -464,6 +464,19 @@ private function readLayouts(array $layouts, string $componentSlug, array $nameC // renamed to afterwards. $layoutOut['key'] = (string) $layout['key']; + // Finding 1 (round 4, CRITICAL) — the round-3 fix pinned `key` + // but left `name` implicit (derived from the YAML map key by + // FieldsGenerator::buildLayouts()). That's the wrong half: ACF + // stores `acf_fc_layout` postmeta BY NAME, not by key, and a + // layout's map key is a purely cosmetic authoring label (see + // the `key` pinning rationale above — the same asymmetry + // applies identically to `name`). Renaming the map key must + // not change what's stored in postmeta, so `name` is pinned + // verbatim here exactly like `key`, unconditionally — not only + // when it deviates from the map key. FieldsGenerator now reads + // this pinned `name` instead of trusting the map key. + $layoutOut['name'] = $layoutName; + $out[$layoutName] = $layoutOut; } return $out; diff --git a/tests/Migration/AcfJsonReaderTest.php b/tests/Migration/AcfJsonReaderTest.php index b135970..29f910f 100644 --- a/tests/Migration/AcfJsonReaderTest.php +++ b/tests/Migration/AcfJsonReaderTest.php @@ -512,7 +512,6 @@ public function test_flexible_content_layout_key_is_pinned_even_when_matching_co $generated = (new \Parisek\DefinitionKit\Generator\FieldsGenerator())->generate($renamedTree, 'demo', 1); $regeneratedLayout = $generated['fields'][0]['layouts'][0]; - self::assertSame('heading', $regeneratedLayout['name']); self::assertSame( 'layout_demo_items_title', $regeneratedLayout['key'], @@ -521,6 +520,55 @@ public function test_flexible_content_layout_key_is_pinned_even_when_matching_co ); } + /** + * Finding 1 (round 4, CRITICAL) — the round-3 fix pinned the layout's + * `key` but left the layout's ACF `name` derived from the YAML map key. + * `FieldsGenerator::buildLayouts()` emits `'name' => (string) $layoutName` + * — the map key IS the regenerated ACF `name`. WordPress stores + * `acf_fc_layout` postmeta BY NAME, not by key, so renaming the map key + * silently changes what every existing content row's `acf_fc_layout` + * value must match against — exactly the same class of orphaning the + * key-pin was meant to prevent, just on the other identity half. + * + * This asserts the name-preservation half directly: migrate a layout, + * rename its YAML map key (an innocent-looking refactor), regenerate, + * and require the regenerated ACF `name` to still be the ORIGINAL + * `title` — not the renamed map key `heading`. Before this fix, the + * regenerated `name` silently became `heading`, orphaning any + * `acf_fc_layout: "title"` postmeta already stored in production and + * breaking every `{% if layout.acf_fc_layout == 'title' %}` Twig branch. + */ + public function test_flexible_content_layout_name_is_pinned_so_renaming_the_map_key_does_not_orphan_acf_fc_layout(): void + { + $tree = $this->reader->read($this->group([[ + 'key' => 'field_demo_items', 'name' => 'items', 'label' => 'Položky', 'type' => 'flexible_content', + 'layouts' => [[ + 'key' => 'layout_demo_items_title', 'name' => 'title', 'label' => 'Nadpis', 'display' => 'block', + 'sub_fields' => [ + ['key' => 'field_demo_items_title_title', 'name' => 'title', 'label' => 'Nadpis', 'type' => 'text'], + ], + ]], + ]]), 'demo'); + + $migratedLayout = $tree['fields']['items']['layouts']['title']; + + // Simulate the maintainer renaming the map key post-migration. + $renamedLayouts = ['heading' => $migratedLayout]; + $renamedTree = $tree; + $renamedTree['fields']['items']['layouts'] = $renamedLayouts; + + $generated = (new \Parisek\DefinitionKit\Generator\FieldsGenerator())->generate($renamedTree, 'demo', 1); + $regeneratedLayout = $generated['fields'][0]['layouts'][0]; + + self::assertSame( + 'title', + $regeneratedLayout['name'], + 'Renaming the layout map key must not change its ACF name — otherwise ' + . 'every acf_fc_layout="title" value already stored in postmeta is orphaned ' + . 'and every Twig branch on acf_fc_layout breaks.', + ); + } + public function test_flexible_content_min_max_zero_is_omitted_as_acf_no_limit_sentinel(): void { $tree = $this->reader->read($this->group([[ From 33003340f27de12d1d223575c4a5774e1c5224a9 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 14:22:23 +0200 Subject: [PATCH 05/10] Round-5 fix: guard layout-name and sibling field-name collisions 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) Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7 --- src/Generator/FieldsGenerator.php | 126 ++++++++++++++++++++++++ tests/Generator/FieldsGeneratorTest.php | 91 +++++++++++++++++ 2 files changed, 217 insertions(+) diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 9af2100..dd1ddfb 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -12,6 +12,69 @@ * (key/name/parent_repeater) ⊕ a field's own `wp:` overrides (highest * priority, always wins) — recursively, then handed to * RootFieldGroupBuilder for root assembly + accordion re-insertion. + * + * ## WordPress data-identity invariants (round 5) + * + * Every prior round's fix guarded ONE hole this YAML→ACF mapping could + * produce, one at a time (R1 `key` collisions, R2 the scan missing + * accordions, R3 a layout rename changing its `key`, R4 pinning the + * `key` not also pinning the `name`). Rather than wait for a sixth + * report, here is the complete set of identity invariants this class + * enforces — anything a future change adds to the mapping should be + * checked against this list, not discovered the hard way in production: + * + * 1. **Field `key` is globally unique** across the entire assembled + * tree — every ordinary field, every container's sub_fields, every + * flexible_content layout's OWN key, and every layout's sub_fields, + * PLUS accordion pseudo-fields interleaved by RootFieldGroupBuilder. + * Guarded by {@see assertGloballyUniqueKeys()} / {@see collectKeys()}, + * run over the FINAL assembled tree (`built['fields']`), not the + * pre-accordion-interleave `$orderedRawFields` (R2's fix). + * 2. **Layout `key` is globally unique** — same guard, same global + * `$seen` set as (1); a layout's key colliding with an ordinary + * field's key (or another layout's) is caught identically. + * 3. **Layout `name` (the `acf_fc_layout` postmeta value) is unique + * WITHIN its own flexible_content field** — checked independently of + * (2) because `key` and `name` are two separate ACF identity axes: a + * saved row is matched to its layout by `name`, never by `key`, so + * two layouts can have distinct keys yet still collide on name. Only + * scoped per-field (not globally) — the same layout name in two + * DIFFERENT flexible_content fields is harmless, each field owns its + * own `acf_fc_layout` namespace. Guarded in {@see buildLayouts()}. + * 4. **Field `name` (the WordPress postmeta key for the field's VALUE) + * is unique among direct siblings under the same parent** — checked + * independently of (1)/(2) because `key` and `name` are separate + * axes here too: the schema's `wp:` overlay is a fully open object + * (see FieldsGeneratorTest::test_wp_overlay_wins_over_baseline_and_reconstruction) + * and can repoint `name` onto anything, including a value that + * collides with a sibling authored under a different definition-map + * key (hence a different, non-colliding `key`). Scoped per-level + * (root fields / one container's sub_fields / one layout's + * sub_fields) — the same name at a DIFFERENT nesting level is not a + * collision. Accordion pseudo-fields are exempt (canonical ACF shape + * is `name: ''` for every accordion; several legitimately coexist at + * one level). Guarded in {@see collectKeys()} via + * {@see assertNameUnseenAtThisLevel()}. + * 5. **A field's own pinned `key` and pinned `name` are independent + * pins, not required to agree with each other or with any naming + * convention** — ACF itself allows a field's `key` and `name` to be + * unrelated strings; this class never derives one from the other + * (`deriveOrPinKey()` derives `key` from the definition's OWN name + * chain, never from a possibly-overridden `name`). There is + * therefore no "internal consistency" invariant to violate here by + * construction — flagged explicitly so a future change that DID + * start deriving one from the other doesn't silently assume they're + * always in sync. + * + * What is deliberately NOT guarded, and why: + * - Duplicate keys in a hand-written YAML `fields:` / `layouts:` map + * (e.g. two `title:` entries) are a YAML-parsing concern, not a + * PHP-array concern — by the time `$definitionTree['fields']` reaches + * this class, the YAML parser has already collapsed duplicates + * (last-one-wins) with no diagnostic. That collapse happens upstream + * of this class's input and is out of scope for a generator-level + * guard; a YAML-authoring lint (duplicate-key detection) would need + * to run before the parse step, not after it. */ final class FieldsGenerator { @@ -104,8 +167,30 @@ private function assertGloballyUniqueKeys(array $fields): void */ private function collectKeys(array $fields, array &$seen): void { + // Finding D (round 5) — `key` uniqueness (above) does not imply + // `name` uniqueness. ACF's postmeta key for a field's VALUE is the + // field's `name`, scoped to its immediate parent container — the + // schema's `wp:` overlay is a fully open object (see + // test_wp_overlay_wins_over_baseline_and_reconstruction) and can + // repoint `name` onto anything, including a value that collides + // with a sibling's. Two sibling fields sharing one `name` alias + // the same postmeta row even when their derived `key`s differ. + // Scoped to THIS level only (`$fields` is always one container's + // own direct children — root fields, one group/repeater's + // sub_fields, or one layout's sub_fields) — the same `name` at a + // DIFFERENT nesting level is not a collision. + $seenNamesThisLevel = []; foreach ($fields as $field) { $this->assertKeyUnseen((string) $field['key'], $seen); + // Accordion pseudo-fields always carry `name: ''` by canonical + // ACF shape (RootFieldGroupBuilder::accordionBaseline()) — + // several accordions legitimately coexist at the same level + // (each is its own key-guarded section marker), so an empty + // name is deliberately exempt from the sibling-uniqueness + // check rather than a false positive to chase away. + if ('' !== (string) $field['name']) { + $this->assertNameUnseenAtThisLevel((string) $field['name'], $seenNamesThisLevel); + } if (!empty($field['sub_fields'])) { /** @var list> $subFields */ @@ -126,6 +211,24 @@ private function collectKeys(array $fields, array &$seen): void } } + /** + * @param array $seenNamesThisLevel + */ + private function assertNameUnseenAtThisLevel(string $name, array &$seenNamesThisLevel): void + { + if (isset($seenNamesThisLevel[$name])) { + throw new GenerationValidationException(sprintf( + "Generated field name '%s' is shared by two sibling fields under the same parent. " + . "ACF's postmeta key for a field's value is its `name`, scoped to its parent — two " + . "sibling fields sharing one `name` (whether from the definition's own field-map key " + . 'or a `wp: {name: …}` overlay) would alias the same WordPress postmeta row. Rename ' + . 'one of the colliding fields, or remove the overlapping `wp.name` override.', + $name, + )); + } + $seenNamesThisLevel[$name] = true; + } + /** * @param array $seen */ @@ -244,6 +347,7 @@ private function buildField( private function buildLayouts(array $layoutDefs, string $componentSlug, array $nameChain): array { $layouts = []; + $seenAcfNames = []; foreach ($layoutDefs as $layoutName => $layoutDef) { // Finding 1 (round 4, CRITICAL) — the ACF `name` (what WordPress // stores in `acf_fc_layout` postmeta) is pinned verbatim by @@ -257,6 +361,28 @@ private function buildLayouts(array $layoutDefs, string $componentSlug, array $n // (a brand-new layout authored directly in YAML, where the map // key genuinely IS the only name that has ever existed). $acfName = (string) ($layoutDef['name'] ?? $layoutName); + + // Round 5 — `key` uniqueness (guarded elsewhere) does not imply + // `name` uniqueness. ACF matches a saved flex-content row's + // layout by `acf_fc_layout` == the layout's `name`, never its + // `key` — two layouts in the SAME flexible_content field can + // pin distinct keys yet still collide on `name`, making rows + // indistinguishable to WordPress at render/save time. This + // must be checked per-field (a repeated name across two + // DIFFERENT flexible_content fields is harmless — each field + // has its own `acf_fc_layout` namespace). + if (isset($seenAcfNames[$acfName])) { + throw new GenerationValidationException(sprintf( + "Layout name '%s' is used by two layouts in the same flexible_content field '%s'. " + . "ACF matches a saved row's layout by `acf_fc_layout` == name, not `key` — two " + . 'layouts sharing one name would be indistinguishable to WordPress even though ' + . 'their keys differ. Rename one of the layouts (or pin a distinct `name:`).', + $acfName, + implode('.', $nameChain), + )); + } + $seenAcfNames[$acfName] = true; + $layoutChain = [...$nameChain, $acfName]; $layoutKey = (string) ($layoutDef['key'] ?? ('layout_' . $componentSlug . '_' . implode('_', $layoutChain))); diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index f1bc6a2..b7c7e90 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -447,6 +447,97 @@ public function test_distinct_flexible_content_layout_names_generate_without_col self::assertCount(2, $group['fields']); } + /** + * Round 5 — two layouts in the SAME flexible_content field pin + * different `key`s (so the existing key-uniqueness guard is silent) + * but the SAME `name:`. ACF matches a rendered flex-content row's + * layout by `acf_fc_layout` == the layout's `name`, not its `key` — + * two layouts sharing one name are indistinguishable to WordPress at + * render/save time even though their field-group keys never collide. + * This is a distinct hazard from the key-collision guard above and + * must be caught independently of it. + */ + public function test_two_layouts_in_the_same_flexible_content_field_cannot_share_a_pinned_name(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/title/'); + + $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'layout_one' => ['label' => 'Layout One', 'key' => 'layout_demo_items_one', 'name' => 'title', 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + 'layout_two' => ['label' => 'Layout Two', 'key' => 'layout_demo_items_two', 'name' => 'title', 'fields' => [ + 'd' => ['type' => 'text', 'label' => 'D'], + ]], + ]], + ]), 'demo', 1700000000); + } + + /** + * Sanity control — the SAME layout `name` in two DIFFERENT + * flexible_content fields must keep working; `acf_fc_layout` is only + * ambiguous within a single flex-content field's own rows. + */ + public function test_same_layout_name_in_different_flexible_content_fields_does_not_collide(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'title' => ['label' => 'Title', 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + 'other' => ['type' => 'flexible_content', 'label' => 'Other', 'layouts' => [ + 'title' => ['label' => 'Title', 'fields' => [ + 'd' => ['type' => 'text', 'label' => 'D'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertCount(2, $group['fields']); + } + + /** + * Round 5 — `wp:` overlay deliberately wins over every derived prop + * (see test_wp_overlay_wins_over_baseline_and_reconstruction), and + * that includes `name` — the schema's `wp` bag is a fully open + * object with no key exclusions. Two sibling fields at the same + * nesting level (different YAML map keys, hence different derived + * `key`s, so the key-uniqueness guard stays silent) can each pin + * `wp: {name: "same"}` and collide on the ACTUAL ACF `name` — which + * is the WordPress postmeta key. Two fields sharing a `name` under + * the same parent alias the same postmeta row exactly like a `key` + * collision does; the generator must reject it with the same + * severity. + */ + public function test_sibling_fields_cannot_collide_on_name_via_wp_overlay(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/clash/'); + + $this->generator->generate($this->tree([ + 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => 'clash']], + 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => 'clash']], + ]), 'demo', 1700000000); + } + + /** + * Sanity control — the same overridden name at DIFFERENT nesting + * levels (root vs inside a group) is not a collision; ACF namespaces + * a field's postmeta identity per parent container, not globally. + */ + public function test_same_overridden_name_at_different_nesting_levels_does_not_collide(): void + { + $group = $this->generator->generate($this->tree([ + 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => 'clash']], + 'wrapper' => ['type' => 'group', 'label' => 'Wrapper', 'fields' => [ + 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => 'clash']], + ]], + ]), 'demo', 1700000000); + + self::assertCount(2, $group['fields']); + } + /** * Finding C (CRITICAL) — layout `display` and `location` must be * captured verbatim when authored non-default, not hardcoded to From 2a577b3725309c7e71e480896d2d7c3970a8b746 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 16:31:46 +0200 Subject: [PATCH 06/10] Round-6 fix: wp.key desyncs conditional_logic; wp.name='' escapes name-collision guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01Vq8VHGjkVG28SMVLCdu7B7 --- src/Generator/FieldsGenerator.php | 116 +++++++++++++--- tests/Generator/FieldsGeneratorTest.php | 170 ++++++++++++++++++++++++ 2 files changed, 267 insertions(+), 19 deletions(-) diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index dd1ddfb..c9fa946 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -13,15 +13,17 @@ * priority, always wins) — recursively, then handed to * RootFieldGroupBuilder for root assembly + accordion re-insertion. * - * ## WordPress data-identity invariants (round 5) + * ## WordPress data-identity invariants (round 6) * * Every prior round's fix guarded ONE hole this YAML→ACF mapping could * produce, one at a time (R1 `key` collisions, R2 the scan missing * accordions, R3 a layout rename changing its `key`, R4 pinning the - * `key` not also pinning the `name`). Rather than wait for a sixth - * report, here is the complete set of identity invariants this class - * enforces — anything a future change adds to the mapping should be - * checked against this list, not discovered the hard way in production: + * `key` not also pinning the `name`, R5 a `wp.key` override desyncing + * `conditional_logic` resolution + an accordion exemption keyed on an + * empty `name` value instead of the field's `accordion` type). This is + * the set of identity invariants guarded AS OF round 6 — see the note + * at the end of this list for why it is deliberately not claimed to be + * the complete set: * * 1. **Field `key` is globally unique** across the entire assembled * tree — every ordinary field, every container's sub_fields, every @@ -55,16 +57,38 @@ * is `name: ''` for every accordion; several legitimately coexist at * one level). Guarded in {@see collectKeys()} via * {@see assertNameUnseenAtThisLevel()}. - * 5. **A field's own pinned `key` and pinned `name` are independent - * pins, not required to agree with each other or with any naming - * convention** — ACF itself allows a field's `key` and `name` to be - * unrelated strings; this class never derives one from the other - * (`deriveOrPinKey()` derives `key` from the definition's OWN name - * chain, never from a possibly-overridden `name`). There is - * therefore no "internal consistency" invariant to violate here by - * construction — flagged explicitly so a future change that DID - * start deriving one from the other doesn't silently assume they're - * always in sync. + * 5. **Every `key` referenced by any emitted `conditional_logic` entry + * must exist somewhere in the emitted tree.** `visible_when` is + * resolved to `conditional_logic` (via VisibleWhenMapper::toConditionalLogic()) + * against a name=>key map built from the RAW semantic fields, one + * level before that same level's fields are actually reconstructed + * with their FINAL keys. `deriveOrPinKey()` — the single function + * both `siblingKeyMap()` (map-building) and `buildField()` + * (field-building) delegate to — is therefore the one place that + * must resolve a key with EXACTLY the same precedence buildField()'s + * own final merge order uses, or the two computations silently + * diverge and a `conditional_logic` entry ends up pointing at a key + * nothing was ever emitted under (round 6 / Defect 1: found because + * a field pinned its key through `wp.key` rather than the top-level + * `key:` prop `deriveOrPinKey()` used to look at exclusively — the + * `wp:` overlay is merged in LAST in buildField(), so `wp.key` must + * win the SAME way there). Guarded structurally by keeping + * `deriveOrPinKey()` the single source of truth for key precedence; + * regression-proven in FieldsGeneratorTest by walking the whole + * generated tree and asserting every `conditional_logic` reference + * resolves, rather than asserting one hardcoded expected key (a + * hardcoded expectation would keep passing even if both sides of the + * computation drifted onto the same wrong value together). + * + * A field's own pinned `key` and pinned `name` remain independent + * pins that need not agree with each other — ACF allows a field's + * `key` and `name` to be unrelated strings, and this class never + * derives one from the other. But — as (5) above demonstrates — `key` + * pinning is NOT a single code path: the top-level `key:` prop and + * the open `wp.key` escape hatch are two independent ways to set the + * same final value, and any code that reads a field's key must go + * through `deriveOrPinKey()` rather than reaching into `$field['key']` + * directly, or it will only ever see one of the two paths. * * What is deliberately NOT guarded, and why: * - Duplicate keys in a hand-written YAML `fields:` / `layouts:` map @@ -75,6 +99,28 @@ * of this class's input and is out of scope for a generator-level * guard; a YAML-authoring lint (duplicate-key detection) would need * to run before the parse step, not after it. + * + * ## This list is not, and has never been, complete + * + * Five rounds of review have each found ONE more hole in this same + * class, one at a time, and every round's docblock update claimed (or + * implied) the enumeration was now exhaustive. It wasn't, twice over + * (round 5's own invariant (5) turned out to be flatly wrong — see + * above). Do not read this list as "the complete set of invariants"; + * read it as "the invariants guarded as of the round that last touched + * this file." The structural reason new ones keep appearing: the + * schema's `wp:` bag is a fully open object that can override ANY + * property this class emits, at every nesting level, and several of + * those properties are independently consumed by a SECOND component + * downstream (VisibleWhenMapper reading a key derived here; + * RootFieldGroupBuilder reading a field's `type`/`name` shape; + * AcfJsonReader's own inverse mapping on the migration side). Every + * property with a second consumer is a candidate for the same class of + * bug: whoever added the override path didn't also update every place + * that pre-computes or assumes that property's value ahead of the + * override being applied. Treat a new report the same way — as a real + * gap to close and document, not as proof the previous "complete" list + * was written carelessly. */ final class FieldsGenerator { @@ -185,10 +231,21 @@ private function collectKeys(array $fields, array &$seen): void // Accordion pseudo-fields always carry `name: ''` by canonical // ACF shape (RootFieldGroupBuilder::accordionBaseline()) — // several accordions legitimately coexist at the same level - // (each is its own key-guarded section marker), so an empty - // name is deliberately exempt from the sibling-uniqueness - // check rather than a false positive to chase away. - if ('' !== (string) $field['name']) { + // (each is its own key-guarded section marker), so they are + // deliberately exempt from the sibling-uniqueness check. + // + // Defect 2 (MEDIUM, confirmed) — the exemption used to be + // keyed on the VALUE ('' === name) rather than the SHAPE + // (type === 'accordion'). `wp:` is a fully open escape-hatch + // object (see test_sibling_fields_cannot_collide_on_name_via_wp_overlay), + // so an ORDINARY field can set `wp: {name: ''}` and silently + // escape the guard the same way an accordion legitimately + // does — two such fields alias the same ('' ) postmeta name + // with no diagnostic. Discriminate by the field's own `type` + // (accordionBaseline() always emits `type: 'accordion'`) + // instead, so the exemption can only ever match a genuine + // accordion pseudo-field. + if ('accordion' !== ($field['type'] ?? null)) { $this->assertNameUnseenAtThisLevel((string) $field['name'], $seenNamesThisLevel); } @@ -443,6 +500,27 @@ private function siblingKeyMap(array $fields, string $componentSlug, array $pare */ private function deriveOrPinKey(array $field, string $componentSlug, array $nameChain): string { + // Defect 1 (HIGH, confirmed) — a field's ACF `key` can be pinned + // through TWO independent paths: the top-level `key:` prop (what + // AcfJsonReader::readFields() always writes for a migrated field + // whose real key deviates from the derived convention), or the + // open `wp:` escape hatch (`wp.key`, e.g. a hand-authored + // definition, or any future migration path that stashes it + // there). `buildField()`'s final merge already gives `wp.key` + // priority over the derived/top-level `key` (the `wp` overlay is + // merged in LAST). This method must resolve a field's key with + // that exact same precedence — it is the SAME lookup + // `siblingKeyMap()` uses to build the name=>key map that + // VisibleWhenMapper::toConditionalLogic() resolves `visible_when` + // against. Reading only the top-level `key` here (as before) + // let a `wp.key`-pinned field's `conditional_logic` reference + // the wrong (derived) key — a dangling reference to a field that + // was never emitted under that key. + $wpKey = $field['wp']['key'] ?? null; + if (null !== $wpKey) { + return (string) $wpKey; + } + return (string) ($field['key'] ?? ('field_' . $componentSlug . '_' . implode('_', $nameChain))); } diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index b7c7e90..fb63485 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -568,4 +568,174 @@ public function test_flexible_content_layout_display_defaults_to_block_when_not_ self::assertSame('block', $group['fields'][0]['layouts'][0]['display']); } + + /** + * Defect 1 (HIGH) — `generate()` builds the sibling name=>key map used + * to resolve `visible_when` from the RAW semantic fields, before a + * field's own `wp: {key: …}` overlay is applied. `siblingKeyMap()` (via + * `deriveOrPinKey()`) only ever reads a field's top-level `key:` prop — + * it never sees `wp.key` — so a field pinning its ACF key through the + * `wp:` escape hatch (as opposed to the top-level `key:` prop the + * migration path itself always uses) gets a `conditional_logic` entry + * pointing at the DERIVED key while the field ships under the + * OVERRIDDEN key. The referenced key then exists nowhere in the + * generated tree — ACF's conditional-logic UI would show a dangling + * reference to a field that was never created. + * + * The assertion walks the ENTIRE generated tree and checks that every + * key any `conditional_logic` entry references actually exists + * somewhere in the emitted fields/layouts — not that it equals one + * hardcoded expected string, which would keep passing even if both the + * override and the sibling map silently agreed on the WRONG key. + */ + public function test_conditional_logic_references_survive_a_wp_key_override(): void + { + $group = $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'wp' => ['key' => 'field_test_custom_toggle_key']], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'visible_when' => ['field' => 'toggle', 'equals' => true], + ], + ], ['name' => 'Test']), 'test', 1700000000); + + $allKeys = []; + $referencedKeys = []; + $this->collectAllKeysAndConditionalLogicRefs($group['fields'], $allKeys, $referencedKeys); + + foreach ($referencedKeys as $referencedKey) { + self::assertContains( + $referencedKey, + $allKeys, + sprintf( + "conditional_logic references key '%s', which does not exist anywhere in the " + . 'generated tree — a wp.key override desynced the sibling name=>key map used to ' + . 'resolve visible_when.', + $referencedKey, + ), + ); + } + + // Sanity: the pinned key really did win (otherwise the assertion + // above would trivially pass by both sides using the wrong, + // but mutually-consistent, derived key). + self::assertContains('field_test_custom_toggle_key', $allKeys); + self::assertNotContains('field_test_toggle', $allKeys); + } + + /** + * Walks a generated `fields` (or `layouts`/`sub_fields`) list + * recursively, collecting (a) every `key` that exists anywhere in the + * tree and (b) every field `key` referenced by any `conditional_logic` + * entry anywhere in the tree. + * + * @param list> $fields + * @param list $allKeys + * @param list $referencedKeys + */ + private function collectAllKeysAndConditionalLogicRefs(array $fields, array &$allKeys, array &$referencedKeys): void + { + foreach ($fields as $field) { + $allKeys[] = (string) $field['key']; + + $conditionalLogic = $field['conditional_logic'] ?? false; + if (is_array($conditionalLogic)) { + foreach ($conditionalLogic as $orGroup) { + foreach ((array) $orGroup as $cond) { + $referencedKeys[] = (string) $cond['field']; + } + } + } + + if (!empty($field['sub_fields'])) { + /** @var list> $subFields */ + $subFields = (array) $field['sub_fields']; + $this->collectAllKeysAndConditionalLogicRefs($subFields, $allKeys, $referencedKeys); + } + + if (!empty($field['layouts'])) { + /** @var list> $layouts */ + $layouts = (array) $field['layouts']; + foreach ($layouts as $layout) { + $allKeys[] = (string) $layout['key']; + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $this->collectAllKeysAndConditionalLogicRefs($layoutSubFields, $allKeys, $referencedKeys); + } + } + } + } + + /** + * Defect 1, nested case — the same `wp.key` desync reproduces one + * level deeper, inside a group's `sub_fields`, since + * `buildField()`'s container branch builds `$childSiblingMap` via the + * same un-fixed `siblingKeyMap()` for every nesting level. + */ + public function test_conditional_logic_references_survive_a_wp_key_override_inside_a_group(): void + { + $group = $this->generator->generate($this->tree([ + 'wrapper' => ['type' => 'group', 'label' => 'Wrapper', 'fields' => [ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'wp' => ['key' => 'field_test_custom_nested_key']], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'visible_when' => ['field' => 'toggle', 'equals' => true], + ], + ]], + ], ['name' => 'Test']), 'test', 1700000000); + + $allKeys = []; + $referencedKeys = []; + $this->collectAllKeysAndConditionalLogicRefs($group['fields'], $allKeys, $referencedKeys); + + foreach ($referencedKeys as $referencedKey) { + self::assertContains($referencedKey, $allKeys); + } + self::assertContains('field_test_custom_nested_key', $allKeys); + } + + /** + * Defect 2 (MEDIUM) — `collectKeys()` exempts a field from the + * sibling-name-uniqueness guard whenever its final `name` is `''`, + * assuming only accordion pseudo-fields ever have an empty name. But + * `wp:` is a fully open escape-hatch object (see + * test_wp_overlay_wins_over_baseline_and_reconstruction / + * test_sibling_fields_cannot_collide_on_name_via_wp_overlay) — an + * ORDINARY field can set `wp: {name: ''}` and silently escape the + * guard by value rather than by shape. The exemption must key off the + * field being an actual accordion (`type === 'accordion'`), not off + * name happening to be empty. + */ + public function test_two_ordinary_fields_with_empty_wp_name_still_collide(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + + $this->generator->generate($this->tree([ + 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => '']], + 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => '']], + ]), 'demo', 1700000000); + } + + /** + * Control case for the fix above — genuine accordion pseudo-fields + * (identified by `type === 'accordion'`, which always carry canonical + * `name: ''`) must still be exempt and coexist freely at the same + * level; the fix must narrow the exemption's DISCRIMINATOR, not remove + * the exemption. + */ + public function test_two_genuine_accordions_still_coexist_without_collision(): void + { + $group = $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['accordions' => [ + ['key' => 'field_demo_accordion_a', 'label' => 'A', 'open' => 0, 'before' => 'title'], + ['key' => 'field_demo_accordion_b', 'label' => 'B', 'open' => 0], + ]], + ]), 'demo', 1700000000); + + self::assertSame(['accordion', 'text', 'accordion'], array_column($group['fields'], 'type')); + } } From d5ab6e266124a3fc8f3522f4ba9a94134334c6b9 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 20:48:35 +0200 Subject: [PATCH 07/10] =?UTF-8?q?Round-7=20fix:=20deny=20key/name/type=20i?= =?UTF-8?q?nside=20wp:=20=E2=80=94=20identity=20has=20exactly=20one=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 .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. --- schemas/component.fields.schema.json | 11 +- src/Generator/FieldsGenerator.php | 142 ++++++++++++-- tests/Generator/FieldsGeneratorTest.php | 212 +++++++++++++++++---- tests/Schema/FieldsSchemaValidatorTest.php | 113 +++++++++++ 4 files changed, 422 insertions(+), 56 deletions(-) diff --git a/schemas/component.fields.schema.json b/schemas/component.fields.schema.json index 194562b..f22a853 100644 --- a/schemas/component.fields.schema.json +++ b/schemas/component.fields.schema.json @@ -36,6 +36,13 @@ "fields": { "$ref": "#/$defs/fieldMap" } }, "$defs": { + "wpOverlay": { + "type": "object", + "propertyNames": { + "not": { "enum": ["key", "name", "type"] } + }, + "description": "`wp:` is a fully open escape-hatch object merged with HIGHEST precedence over every derived/reconstructed prop — but `key`/`name`/`type` are a field's WHOLE identity (ACF postmeta key, postmeta name, and field-shape discriminator). Letting `wp:` override any of the three gives identity two independent, silently-diverging paths (six review rounds each found a new bug of this exact shape). Use the sanctioned top-level prop instead: `key:` (pattern ^field_/^layout_/^group_) to pin a key, the YAML field-map key (or a layout's own `name:`) to control the name, and the top-level `type:` enum to pick the field shape." + }, "fieldMap": { "type": "object", "minProperties": 1, @@ -105,7 +112,7 @@ "resolved_from": { "type": "array", "items": { "type": "string" } }, "source": { "type": "string" }, "demo": {}, - "wp": { "type": "object" }, + "wp": { "$ref": "#/$defs/wpOverlay" }, "fields": { "$ref": "#/$defs/fieldMap" }, "layouts": { "$ref": "#/$defs/layoutMap" } }, @@ -140,7 +147,7 @@ "name": { "type": "string", "minLength": 1 }, "min": { "type": "integer", "minimum": 0 }, "max": { "type": "integer", "minimum": 0 }, - "wp": { "type": "object" }, + "wp": { "$ref": "#/$defs/wpOverlay" }, "fields": { "$ref": "#/$defs/fieldMap" } } } diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index c9fa946..22af050 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -157,6 +157,19 @@ public function __construct( public function generate(array $definitionTree, string $componentSlug, int $modifiedAt): array { $fields = (array) ($definitionTree['fields'] ?? []); + + // Round 6 defensive check — belt-and-braces alongside the JSON + // Schema deny-list (component.fields.schema.json `wpOverlay` + // $def). Callers that build a definition tree in memory and hand + // it to FieldsGenerator directly (every test in this class, plus + // any future in-process caller) never go through + // FieldsSchemaValidator, so the schema-level guard alone would + // leave this class's own public contract unguarded. Must run + // BEFORE siblingKeyMap() below — that call (via deriveOrPinKey()) + // is the FIRST thing that would have read a forbidden `wp.key` + // had this check not already rejected it. + $this->assertNoIdentityPropsInWpOverlay($fields, []); + $siblingNameKeyMap = $this->siblingKeyMap($fields, $componentSlug, []); $orderedRawFields = []; @@ -199,6 +212,105 @@ public function generate(array $definitionTree, string $componentSlug, int $modi * own sub_fields) and fail loudly the moment two nodes claim the * same `key`. * + * Round 6 — `key`/`name`/`type` are a field's WHOLE identity: `key` is + * the ACF postmeta key, `name` is the ACF postmeta name, `type` is the + * field-shape discriminator every downstream consumer (baseline + * lookup, sentinel lookup, container-vs-leaf branching, the accordion + * exemption in {@see collectKeys()}) reads. `wp:` is a fully open + * escape-hatch object merged with HIGHEST precedence in `buildField()` + * — so `wp.key`/`wp.name`/`wp.type` are each an independent, silently + * diverging SECOND path onto the same value the top-level `key:` / + * the definition-map key / the top-level `type:` prop already own. + * Every round-6 defect (dangling `conditional_logic` reference via + * `wp.key`, a sibling-name collision escaping the uniqueness guard via + * `wp.name`, that SAME guard's accordion exemption re-escaped via + * `wp: {type: 'accordion'}`, a bogus/null key shipped via `wp.key`) + * is the same root cause wearing a different prop name. Rather than + * add a fourth ad hoc guard for whichever prop is discovered next, + * this closes the whole class structurally: identity has EXACTLY ONE + * path, full stop. Mirrors the schema-level deny-list + * (component.fields.schema.json `wpOverlay` $def) — this is the + * generator's own belt-and-braces copy, since callers that build a + * tree in memory never go through FieldsSchemaValidator (see + * `generate()`'s docblock note above the call site). + * + * Walks the RAW (pre-generation) definition tree — `fields:` maps and + * flexible_content `layouts:` maps — recursively, so the check runs + * before ANY key derivation happens (deriveOrPinKey() included). + * Layout `wp:` is checked too even though `buildLayouts()` never reads + * `layout.wp.type` (layouts have no `type` prop to begin with) — the + * schema forbids the same three props there for a symmetric reason: + * `buildLayouts()` DOES read `layoutDef['wp']['location']` / + * `['display']` today, and a future prop added to that read list + * would inherit the same divergence hazard `wp.key` had for fields. + * + * @param array $fields name => field definition, one level + * @param list $pathChain dot-joined breadcrumb for error messages + */ + private function assertNoIdentityPropsInWpOverlay(array $fields, array $pathChain): void + { + foreach ($fields as $name => $field) { + $chain = [...$pathChain, (string) $name]; + $wp = (array) ($field['wp'] ?? []); + foreach (['key', 'name', 'type'] as $forbidden) { + if (array_key_exists($forbidden, $wp)) { + throw new GenerationValidationException(sprintf( + "Field '%s' sets `wp.%s`, which is forbidden — `%s` is part of a field's " + . 'identity and must have exactly one source. %s', + implode('.', $chain), + $forbidden, + $forbidden, + $this->wpIdentityPropAlternative($forbidden, false), + )); + } + } + + if (!empty($field['fields']) && is_array($field['fields'])) { + $this->assertNoIdentityPropsInWpOverlay($field['fields'], $chain); + } + + if (!empty($field['layouts']) && is_array($field['layouts'])) { + foreach ($field['layouts'] as $layoutName => $layoutDef) { + $layoutChain = [...$chain, (string) $layoutName]; + $layoutWp = (array) (((array) $layoutDef)['wp'] ?? []); + foreach (['key', 'name', 'type'] as $forbidden) { + if (array_key_exists($forbidden, $layoutWp)) { + throw new GenerationValidationException(sprintf( + "Layout '%s' sets `wp.%s`, which is forbidden — `%s` is part of a " + . 'layout\'s identity and must have exactly one source. %s', + implode('.', $layoutChain), + $forbidden, + $forbidden, + $this->wpIdentityPropAlternative($forbidden, true), + )); + } + } + + $layoutFields = (array) (((array) $layoutDef)['fields'] ?? []); + if ([] !== $layoutFields) { + $this->assertNoIdentityPropsInWpOverlay($layoutFields, $layoutChain); + } + } + } + } + } + + /** Names the sanctioned alternative for a forbidden `wp.` override, for the exception message. */ + private function wpIdentityPropAlternative(string $forbidden, bool $isLayout): string + { + return match ($forbidden) { + 'key' => $isLayout + ? "Pin the layout's key with the top-level `key:` prop (pattern ^layout_) instead." + : "Pin the field's key with the top-level `key:` prop (pattern ^field_) instead.", + 'name' => $isLayout + ? "Set the layout's ACF name with its own top-level `name:` prop instead." + : 'The field name is derived from its own YAML field-map key — rename that map key instead.', + 'type' => 'Use the top-level `type:` prop (the semantic type enum) instead.', + default => '', + }; + } + + /** * @param list> $fields */ private function assertGloballyUniqueKeys(array $fields): void @@ -500,27 +612,15 @@ private function siblingKeyMap(array $fields, string $componentSlug, array $pare */ private function deriveOrPinKey(array $field, string $componentSlug, array $nameChain): string { - // Defect 1 (HIGH, confirmed) — a field's ACF `key` can be pinned - // through TWO independent paths: the top-level `key:` prop (what - // AcfJsonReader::readFields() always writes for a migrated field - // whose real key deviates from the derived convention), or the - // open `wp:` escape hatch (`wp.key`, e.g. a hand-authored - // definition, or any future migration path that stashes it - // there). `buildField()`'s final merge already gives `wp.key` - // priority over the derived/top-level `key` (the `wp` overlay is - // merged in LAST). This method must resolve a field's key with - // that exact same precedence — it is the SAME lookup - // `siblingKeyMap()` uses to build the name=>key map that - // VisibleWhenMapper::toConditionalLogic() resolves `visible_when` - // against. Reading only the top-level `key` here (as before) - // let a `wp.key`-pinned field's `conditional_logic` reference - // the wrong (derived) key — a dangling reference to a field that - // was never emitted under that key. - $wpKey = $field['wp']['key'] ?? null; - if (null !== $wpKey) { - return (string) $wpKey; - } - + // Round 6 — `wp.key` used to be read here too (a field's ACF key + // had two independent pinning paths: the top-level `key:` prop, or + // the `wp:` escape hatch). That second path is now rejected + // upstream by assertNoIdentityPropsInWpOverlay(), called at the + // very top of generate() before siblingKeyMap() (which calls this + // method) ever runs — so `$field['wp']['key']` is guaranteed absent + // by the time this method executes. Identity now has exactly ONE + // path: the top-level `key:` prop, falling back to the derived + // convention. return (string) ($field['key'] ?? ('field_' . $componentSlug . '_' . implode('_', $nameChain))); } diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index fb63485..f0c5f5c 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -500,38 +500,46 @@ public function test_same_layout_name_in_different_flexible_content_fields_does_ /** * Round 5 — `wp:` overlay deliberately wins over every derived prop * (see test_wp_overlay_wins_over_baseline_and_reconstruction), and - * that includes `name` — the schema's `wp` bag is a fully open - * object with no key exclusions. Two sibling fields at the same - * nesting level (different YAML map keys, hence different derived - * `key`s, so the key-uniqueness guard stays silent) can each pin + * that includes `name` — the schema's `wp` bag USED to be a fully open + * object with no key exclusions, which meant two sibling fields at the + * same nesting level (different YAML map keys, hence different derived + * `key`s, so the key-uniqueness guard stayed silent) could each pin * `wp: {name: "same"}` and collide on the ACTUAL ACF `name` — which - * is the WordPress postmeta key. Two fields sharing a `name` under - * the same parent alias the same postmeta row exactly like a `key` - * collision does; the generator must reject it with the same - * severity. + * is the WordPress postmeta key. + * + * Round 6 closes this structurally: `wp.name` is now rejected + * OUTRIGHT (assertNoIdentityPropsInWpOverlay(), called before any + * field is built), so the collision described above can no longer + * even be attempted — a field's name has exactly one source, its own + * YAML field-map key, and two sibling map keys can never be equal + * (they're PHP array keys). This test now asserts the deny-list + * rejection itself, naming the sanctioned alternative. */ - public function test_sibling_fields_cannot_collide_on_name_via_wp_overlay(): void + public function test_wp_name_override_is_rejected_naming_the_alternative(): void { $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); - $this->expectExceptionMessageMatches('/clash/'); + $this->expectExceptionMessageMatches('/wp\.name/'); + $this->expectExceptionMessageMatches('/field-map key/'); $this->generator->generate($this->tree([ 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => 'clash']], - 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => 'clash']], + 'field_two' => ['type' => 'text', 'label' => 'Two'], ]), 'demo', 1700000000); } /** - * Sanity control — the same overridden name at DIFFERENT nesting + * Sanity control — the SAME field-map key used at DIFFERENT nesting * levels (root vs inside a group) is not a collision; ACF namespaces - * a field's postmeta identity per parent container, not globally. + * a field's postmeta identity per parent container, not globally. No + * `wp:` overlay involved — this exercises the ordinary, sanctioned + * naming path (ordinary field-map key), which is now the ONLY path. */ - public function test_same_overridden_name_at_different_nesting_levels_does_not_collide(): void + public function test_same_field_map_key_at_different_nesting_levels_does_not_collide(): void { $group = $this->generator->generate($this->tree([ - 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => 'clash']], + 'field_one' => ['type' => 'text', 'label' => 'One'], 'wrapper' => ['type' => 'group', 'label' => 'Wrapper', 'fields' => [ - 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => 'clash']], + 'field_one' => ['type' => 'text', 'label' => 'One (nested)'], ]], ]), 'demo', 1700000000); @@ -588,10 +596,10 @@ public function test_flexible_content_layout_display_defaults_to_block_when_not_ * hardcoded expected string, which would keep passing even if both the * override and the sibling map silently agreed on the WRONG key. */ - public function test_conditional_logic_references_survive_a_wp_key_override(): void + public function test_conditional_logic_references_survive_a_pinned_key(): void { $group = $this->generator->generate($this->tree([ - 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'wp' => ['key' => 'field_test_custom_toggle_key']], + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'key' => 'field_test_custom_toggle_key'], 'conditional_field' => [ 'type' => 'text', 'label' => 'Conditional', @@ -672,11 +680,11 @@ private function collectAllKeysAndConditionalLogicRefs(array $fields, array &$al * `buildField()`'s container branch builds `$childSiblingMap` via the * same un-fixed `siblingKeyMap()` for every nesting level. */ - public function test_conditional_logic_references_survive_a_wp_key_override_inside_a_group(): void + public function test_conditional_logic_references_survive_a_pinned_key_inside_a_group(): void { $group = $this->generator->generate($this->tree([ 'wrapper' => ['type' => 'group', 'label' => 'Wrapper', 'fields' => [ - 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'wp' => ['key' => 'field_test_custom_nested_key']], + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'key' => 'field_test_custom_nested_key'], 'conditional_field' => [ 'type' => 'text', 'label' => 'Conditional', @@ -696,24 +704,52 @@ public function test_conditional_logic_references_survive_a_wp_key_override_insi } /** - * Defect 2 (MEDIUM) — `collectKeys()` exempts a field from the - * sibling-name-uniqueness guard whenever its final `name` is `''`, - * assuming only accordion pseudo-fields ever have an empty name. But - * `wp:` is a fully open escape-hatch object (see - * test_wp_overlay_wins_over_baseline_and_reconstruction / - * test_sibling_fields_cannot_collide_on_name_via_wp_overlay) — an - * ORDINARY field can set `wp: {name: ''}` and silently escape the - * guard by value rather than by shape. The exemption must key off the - * field being an actual accordion (`type === 'accordion'`), not off - * name happening to be empty. + * Defect 2 (MEDIUM, round 5) — `collectKeys()` used to exempt a field + * from the sibling-name-uniqueness guard whenever its final `name` was + * `''`, assuming only accordion pseudo-fields ever have an empty name. + * `wp.name` is now rejected outright (round 6), so this specific value + * ('' via `wp: {name: ''}`) can no longer even be authored — asserting + * the deny-list rejection here instead of the old collision message. */ - public function test_two_ordinary_fields_with_empty_wp_name_still_collide(): void + public function test_wp_name_override_with_empty_string_is_still_rejected_by_the_deny_list(): void { $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.name/'); $this->generator->generate($this->tree([ 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['name' => '']], - 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['name' => '']], + 'field_two' => ['type' => 'text', 'label' => 'Two'], + ]), 'demo', 1700000000); + } + + /** + * Round 6, Defect 3 — round 5's fix re-keyed the sibling-name- + * uniqueness exemption from "name === ''" to "type === 'accordion'" + * (see `collectKeys()`), on the assumption that `type` is a safe, + * un-overridable discriminator. It wasn't: `wp:` was (before this + * round) a fully open escape-hatch object with NO key exclusions, so + * an ORDINARY field could set `wp: {type: 'accordion', name: 'clash'}` + * and silently re-escape the very guard round 5 just tightened — two + * such fields alias the same ('clash') postmeta name with zero + * diagnostic. Round 6 closes the whole class structurally: `wp.type` + * (and `wp.name`) are rejected outright, so this bypass can no longer + * be constructed at all — this regression test asserts the REJECTION + * itself (the two fields never reach the name-collision check, they + * never even reach field-building). + */ + public function test_wp_type_accordion_impersonation_is_rejected_before_it_can_escape_the_name_guard(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + // The field carries BOTH a forbidden `wp.type` and a forbidden + // `wp.name` — either is independently sufficient to reject it, and + // this test only cares that rejection happens BEFORE the two + // fields could ever reach the name-collision check, not which of + // the two forbidden props the deny-list happens to report first. + $this->expectExceptionMessageMatches('/wp\.(type|name)/'); + + $this->generator->generate($this->tree([ + 'field_one' => ['type' => 'text', 'label' => 'One', 'wp' => ['type' => 'accordion', 'name' => 'clash']], + 'field_two' => ['type' => 'text', 'label' => 'Two', 'wp' => ['type' => 'accordion', 'name' => 'clash']], ]), 'demo', 1700000000); } @@ -722,7 +758,10 @@ public function test_two_ordinary_fields_with_empty_wp_name_still_collide(): voi * (identified by `type === 'accordion'`, which always carry canonical * `name: ''`) must still be exempt and coexist freely at the same * level; the fix must narrow the exemption's DISCRIMINATOR, not remove - * the exemption. + * the exemption. Genuine accordions are built exclusively by + * `RootFieldGroupBuilder` from root `wp.accordions` — an entirely + * separate mechanism from an ordinary field's own `wp:` overlay, so + * this path is untouched by the round-6 deny-list. */ public function test_two_genuine_accordions_still_coexist_without_collision(): void { @@ -738,4 +777,111 @@ public function test_two_genuine_accordions_still_coexist_without_collision(): v self::assertSame(['accordion', 'text', 'accordion'], array_column($group['fields'], 'type')); } + + /** + * Round 6 — Defect 4: `wp.key` pointed at a bogus value (violating + * ACF's `^field_` convention) or `null` used to pass end-to-end, + * shipping a broken key into generated acf.json with no diagnostic. + * The deny-list rejects `wp.key` regardless of its VALUE — the prop + * itself is forbidden, not just malformed values of it — so this is + * closed for free by the same mechanism as the identity-desync + * defect. Message must name the sanctioned alternative (top-level + * `key:`). + */ + public function test_wp_key_override_is_rejected_naming_the_alternative(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.key/'); + $this->expectExceptionMessageMatches('/top-level `key:`/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis', 'wp' => ['key' => 'bogus']], + ]), 'demo', 1700000000); + } + + /** Same as above, with `wp.key` pointed at `null` rather than a malformed string. */ + public function test_wp_key_override_with_null_value_is_still_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.key/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis', 'wp' => ['key' => null]], + ]), 'demo', 1700000000); + } + + /** + * Round 6 — `wp.type` deny-list, exercised directly (not via the + * accordion-impersonation regression above) — an ordinary field + * cannot repoint its own ACF type through the `wp:` overlay at all, + * message names the sanctioned alternative (top-level `type:`). + */ + public function test_wp_type_override_is_rejected_naming_the_alternative(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.type/'); + $this->expectExceptionMessageMatches('/top-level `type:`/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis', 'wp' => ['type' => 'textarea']], + ]), 'demo', 1700000000); + } + + /** + * Round 6 — the same three-prop deny-list applies to a flexible_content + * LAYOUT's own `wp:` bag, not just an ordinary field's. A layout has + * its own top-level `key:`/`name:` props (see + * test_flexible_content_layout_display_is_reconstructed_when_non_default + * for the sanctioned `wp.display`/`wp.location` use of layout `wp:`), + * so the same two-independent-paths hazard applies symmetrically. + */ + public function test_wp_key_override_on_a_layout_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.key/'); + $this->expectExceptionMessageMatches('/layout/i'); + + $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'title' => ['label' => 'Title', 'wp' => ['key' => 'layout_bogus'], 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + ]), 'demo', 1700000000); + } + + /** Same as above, for `wp.name` on a layout — the layout's own `name:` prop is the sanctioned path. */ + public function test_wp_name_override_on_a_layout_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.name/'); + + $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'title' => ['label' => 'Title', 'wp' => ['name' => 'clash'], 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + ]), 'demo', 1700000000); + } + + /** + * Sanity control — genuine, sanctioned uses of a layout's `wp:` bag + * (`display`/`location`, see test_flexible_content_layout_display_is_reconstructed_when_non_default + * and test_flexible_content_layout_location_round_trips_when_non_default, + * if present) must keep working unchanged — the deny-list only blocks + * `key`/`name`/`type`, nothing else. + */ + public function test_layout_wp_overlay_still_works_for_non_identity_props(): void + { + $group = $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'title' => ['label' => 'Title', 'wp' => ['display' => 'table'], 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + ]), 'demo', 1700000000); + + self::assertSame('table', $group['fields'][0]['layouts'][0]['display']); + } } diff --git a/tests/Schema/FieldsSchemaValidatorTest.php b/tests/Schema/FieldsSchemaValidatorTest.php index 3117d59..f34f17f 100644 --- a/tests/Schema/FieldsSchemaValidatorTest.php +++ b/tests/Schema/FieldsSchemaValidatorTest.php @@ -70,6 +70,119 @@ public function test_validate_data_rejects_bad_in_process_tree(): void self::assertFalse($result->valid); } + /** + * Round 6 — `wp:` is an escape-hatch object merged with HIGHEST + * precedence at generation time; letting it also carry `key`/`name`/ + * `type` gives a field's identity two independent, silently-diverging + * paths (six review rounds each found a new bug of this shape). The + * schema's `wpOverlay` $def denies all three at the field level — + * this is the schema-validation-layer half of the fix, complementing + * FieldsGenerator's own belt-and-braces check for in-process callers + * that skip schema validation entirely. + */ + public function test_wp_key_on_a_field_is_rejected_by_schema(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid); + } + + public function test_wp_name_on_a_field_is_rejected_by_schema(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid); + } + + public function test_wp_type_on_a_field_is_rejected_by_schema(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid); + } + + /** Sanity control — every OTHER prop inside `wp:` stays fully open. */ + public function test_wp_non_identity_props_on_a_field_still_validate(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertTrue($result->valid, print_r($result->errors, true)); + } + + /** Same deny-list, exercised on a flexible_content layout's own `wp:` bag. */ + public function test_wp_key_on_a_layout_is_rejected_by_schema(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid); + } + + /** Sanity control — the layout's sanctioned `wp.display`/`wp.location` escape hatch still validates. */ + public function test_wp_display_and_location_on_a_layout_still_validate(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertTrue($result->valid, print_r($result->errors, true)); + } + public function test_document_with_two_broken_fields_reports_both_errors(): void { // Proves finding 1: the validator no longer stops at the first error. From 50f6bc817ca54ccb3754bdc4f99eecb512c6572c Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Fri, 24 Jul 2026 21:08:33 +0200 Subject: [PATCH 08/10] Round-7 followup: close root/structural/cross-reference wp: bypasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01J51xpeny2K3mm3LCFVGc2w --- schemas/component.fields.schema.json | 6 +- src/Generator/FieldsGenerator.php | 198 ++++++++++++-- src/Generator/RootFieldGroupBuilder.php | 31 ++- tests/Generator/FieldsGeneratorTest.php | 289 +++++++++++++++++++++ tests/Schema/FieldsSchemaValidatorTest.php | 148 ++++++++++- 5 files changed, 636 insertions(+), 36 deletions(-) diff --git a/schemas/component.fields.schema.json b/schemas/component.fields.schema.json index f22a853..5ee8712 100644 --- a/schemas/component.fields.schema.json +++ b/schemas/component.fields.schema.json @@ -32,16 +32,16 @@ "description": { "type": "string" }, "weight": { "type": "integer" }, "responsive": { "type": "boolean" }, - "wp": { "type": "object" }, + "wp": { "$ref": "#/$defs/wpOverlay" }, "fields": { "$ref": "#/$defs/fieldMap" } }, "$defs": { "wpOverlay": { "type": "object", "propertyNames": { - "not": { "enum": ["key", "name", "type"] } + "not": { "enum": ["key", "name", "type", "fields", "sub_fields", "layouts", "parent_repeater"] } }, - "description": "`wp:` is a fully open escape-hatch object merged with HIGHEST precedence over every derived/reconstructed prop — but `key`/`name`/`type` are a field's WHOLE identity (ACF postmeta key, postmeta name, and field-shape discriminator). Letting `wp:` override any of the three gives identity two independent, silently-diverging paths (six review rounds each found a new bug of this exact shape). Use the sanctioned top-level prop instead: `key:` (pattern ^field_/^layout_/^group_) to pin a key, the YAML field-map key (or a layout's own `name:`) to control the name, and the top-level `type:` enum to pick the field shape." + "description": "`wp:` is a fully open escape-hatch object merged with HIGHEST precedence over every derived/reconstructed prop. Two classes of prop are reserved: scalar identity (`key`/`name`/`type` — a field's WHOLE identity: ACF postmeta key, postmeta name, field-shape discriminator) and structural containers (`fields`/`sub_fields`/`layouts` — which carry the identity of NESTED nodes one level down, so letting `wp:` overwrite them is the exact same hazard one level removed) plus `parent_repeater` (ACF-computed structural metadata re-derived from nesting on every generation; it has no legitimate authoring path at all). Letting `wp:` override any of these gives identity two independent, silently-diverging paths (seven review rounds each found a new bug of this exact shape — round 7 found the ROOT-level `wp:` bag wasn't covered at all, plus the field/layout-level bypasses via the structural props). Use the sanctioned top-level prop instead: `key:` (pattern ^field_/^layout_/^group_) to pin a key, the YAML field-map key (or a layout's own `name:`) to control the name, the top-level `type:` enum to pick the field shape, and the field/layout's own `fields:`/`layouts:` map to author nested content — never `wp:`. `wp.conditional_logic` is deliberately NOT reserved here — see FieldsGenerator's own dangling-reference validation, which is the correctness gate for that prop instead." }, "fieldMap": { "type": "object", diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index 22af050..b989eb7 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -142,6 +142,37 @@ final class FieldsGenerator */ private const INTERNAL_WP_MARKERS = ['acf_type', 'accordions']; + /** + * Round 7 — the deny-list widens beyond the round-6 scalar identity + * triple (`key`/`name`/`type`) to also cover STRUCTURAL containers + * (`fields`/`sub_fields`/`layouts`) and a CROSS-REFERENCE + * (`parent_repeater`). Round 7 found four bypasses the round-6 set + * missed, all the same root cause wearing a different prop name: + * + * - `wp.key` / `wp.fields` at the ROOT (assertNoIdentityPropsInWpOverlay() + * never walked the definition tree's own `wp`, only `fields`) — + * RootFieldGroupBuilder::build() merges root `wp:` with HIGHEST + * precedence over the `key`/`fields` it just assembled, so an + * overlay silently repointed the group's key or (most severely) + * zeroed out the entire generated `fields` list. + * - `wp.sub_fields` / `wp.layouts` on an ORDINARY (non-container, + * non-flexible_content) field — buildField() merges `wpOverrides` + * onto `$field` BEFORE the container/flexible_content branch would + * overwrite `sub_fields`/`layouts` from real derived children; a + * leaf field never re-derives either key, so a smuggled array + * (carrying its own bogus key/name/type triple) survives verbatim. + * - `wp.parent_repeater` on a repeater's child — merged AFTER the + * real, ACF-computed `parent_repeater` is assigned from nesting, + * silently overwriting it. `parent_repeater` has no legitimate + * authoring path at all (always re-derived), so it is reserved + * outright rather than gaining an "alternative" sanctioned prop. + * + * `conditional_logic` is deliberately NOT in this list — see + * {@see assertConditionalLogicReferencesResolve()}'s docblock for why + * it gets a validation gate instead of an outright ban. + */ + private const RESERVED_WP_PROPS = ['key', 'name', 'type', 'fields', 'sub_fields', 'layouts', 'parent_repeater']; + public function __construct( private readonly TypeDefaults $typeDefaults = new TypeDefaults(), private readonly ConstraintSentinels $constraintSentinels = new ConstraintSentinels(), @@ -168,6 +199,16 @@ public function generate(array $definitionTree, string $componentSlug, int $modi // BEFORE siblingKeyMap() below — that call (via deriveOrPinKey()) // is the FIRST thing that would have read a forbidden `wp.key` // had this check not already rejected it. + // + // Round 7 — the ROOT node's own `wp:` bag is checked FIRST and + // separately: it is not a member of `$fields` (it lives directly + // on `$definitionTree`), so the recursive walk below never saw it. + // RootFieldGroupBuilder::build() merges root `wp:` with HIGHEST + // precedence over the `key`/`fields` it just assembled — an + // unguarded root `wp.key` silently repoints the group's key, and + // an unguarded root `wp.fields` silently zeroes the entire + // generated `fields` list (the most severe finding of round 7). + $this->assertNoReservedWpProps((array) ($definitionTree['wp'] ?? []), [], false); $this->assertNoIdentityPropsInWpOverlay($fields, []); $siblingNameKeyMap = $this->siblingKeyMap($fields, $componentSlug, []); @@ -195,10 +236,91 @@ public function generate(array $definitionTree, string $componentSlug, int $modi /** @var list> $builtFields */ $builtFields = $built['fields']; $this->assertGloballyUniqueKeys($builtFields); + $this->assertConditionalLogicReferencesResolve($builtFields); return $built; } + /** + * `wp.conditional_logic` is deliberately NOT in {@see RESERVED_WP_PROPS} + * — unlike the identity/structural props, it is a REAL fallback + * Migration\AcfJsonReader emits whenever raw ACF conditional_logic is + * too complex to reduce to the abstract `visible_when` vocabulary + * (2+ AND conditions, 2+ OR groups, or an unmapped operator — see + * VisibleWhenMapper::map()). Forbidding it outright would break that + * round-trip for any component whose ACF source ever used such + * conditional logic, with no sanctioned replacement to fall back to. + * + * What round 7 DOES require: the fallback's references must resolve. + * A `conditional_logic` entry pointing at a key that exists nowhere + * in the generated tree is a dangling reference — ACF's editor would + * show it as broken with zero diagnostic from this tool. This walks + * the FINAL assembled tree (same shape/order as + * {@see assertGloballyUniqueKeys()}) collecting every key that + * exists, then re-walks checking every `conditional_logic` entry's + * `field` reference against that set. + * + * @param list> $builtFields + */ + private function assertConditionalLogicReferencesResolve(array $builtFields): void + { + $allKeys = []; + $referencedKeys = []; + $this->collectKeysAndConditionalLogicRefs($builtFields, $allKeys, $referencedKeys); + + foreach ($referencedKeys as $referencedKey) { + if (!in_array($referencedKey, $allKeys, true)) { + throw new GenerationValidationException(sprintf( + "A `wp.conditional_logic` fallback references key '%s', which does not exist " + . 'anywhere in the generated tree — this is a dangling reference ACF\'s editor ' + . 'would show as broken. Fix the referenced key, or express the condition through ' + . '`visible_when:` if it fits the single-condition vocabulary.', + $referencedKey, + )); + } + } + } + + /** + * @param list> $fields + * @param list $allKeys + * @param list $referencedKeys + */ + private function collectKeysAndConditionalLogicRefs(array $fields, array &$allKeys, array &$referencedKeys): void + { + foreach ($fields as $field) { + $allKeys[] = (string) $field['key']; + + $conditionalLogic = $field['conditional_logic'] ?? false; + if (is_array($conditionalLogic)) { + foreach ($conditionalLogic as $orGroup) { + foreach ((array) $orGroup as $cond) { + if (isset($cond['field'])) { + $referencedKeys[] = (string) $cond['field']; + } + } + } + } + + if (!empty($field['sub_fields'])) { + /** @var list> $subFields */ + $subFields = (array) $field['sub_fields']; + $this->collectKeysAndConditionalLogicRefs($subFields, $allKeys, $referencedKeys); + } + + if (!empty($field['layouts'])) { + /** @var list> $layouts */ + $layouts = (array) $field['layouts']; + foreach ($layouts as $layout) { + $allKeys[] = (string) $layout['key']; + /** @var list> $layoutSubFields */ + $layoutSubFields = (array) ($layout['sub_fields'] ?? []); + $this->collectKeysAndConditionalLogicRefs($layoutSubFields, $allKeys, $referencedKeys); + } + } + } + } + /** * Finding A (CRITICAL) — key derivation underscore-joins the name * chain, so a flexible_content layout `a_b` + field `c` and a @@ -251,19 +373,7 @@ private function assertNoIdentityPropsInWpOverlay(array $fields, array $pathChai { foreach ($fields as $name => $field) { $chain = [...$pathChain, (string) $name]; - $wp = (array) ($field['wp'] ?? []); - foreach (['key', 'name', 'type'] as $forbidden) { - if (array_key_exists($forbidden, $wp)) { - throw new GenerationValidationException(sprintf( - "Field '%s' sets `wp.%s`, which is forbidden — `%s` is part of a field's " - . 'identity and must have exactly one source. %s', - implode('.', $chain), - $forbidden, - $forbidden, - $this->wpIdentityPropAlternative($forbidden, false), - )); - } - } + $this->assertNoReservedWpProps((array) ($field['wp'] ?? []), $chain, false); if (!empty($field['fields']) && is_array($field['fields'])) { $this->assertNoIdentityPropsInWpOverlay($field['fields'], $chain); @@ -272,19 +382,11 @@ private function assertNoIdentityPropsInWpOverlay(array $fields, array $pathChai if (!empty($field['layouts']) && is_array($field['layouts'])) { foreach ($field['layouts'] as $layoutName => $layoutDef) { $layoutChain = [...$chain, (string) $layoutName]; - $layoutWp = (array) (((array) $layoutDef)['wp'] ?? []); - foreach (['key', 'name', 'type'] as $forbidden) { - if (array_key_exists($forbidden, $layoutWp)) { - throw new GenerationValidationException(sprintf( - "Layout '%s' sets `wp.%s`, which is forbidden — `%s` is part of a " - . 'layout\'s identity and must have exactly one source. %s', - implode('.', $layoutChain), - $forbidden, - $forbidden, - $this->wpIdentityPropAlternative($forbidden, true), - )); - } - } + $this->assertNoReservedWpProps( + (array) (((array) $layoutDef)['wp'] ?? []), + $layoutChain, + true, + ); $layoutFields = (array) (((array) $layoutDef)['fields'] ?? []); if ([] !== $layoutFields) { @@ -295,9 +397,48 @@ private function assertNoIdentityPropsInWpOverlay(array $fields, array $pathChai } } + /** + * Shared reserved-prop check used for the ROOT node's own `wp:` bag, + * every field's `wp:` bag, and every flexible_content layout's `wp:` + * bag — one deny-list ({@see RESERVED_WP_PROPS}), one place that + * throws, so root/field/layout can never silently drift apart on + * which props are reserved (round 7's root cause: the schema and the + * generator's own check independently forgot to cover the root node). + * + * @param array $wp + * @param list $chain dot-joined breadcrumb for the error message; empty means "root" + */ + private function assertNoReservedWpProps(array $wp, array $chain, bool $isLayout): void + { + $subject = [] === $chain ? 'The root field group' : ($isLayout ? "Layout '" . implode('.', $chain) . "'" : "Field '" . implode('.', $chain) . "'"); + + foreach (self::RESERVED_WP_PROPS as $forbidden) { + if (array_key_exists($forbidden, $wp)) { + throw new GenerationValidationException(sprintf( + "%s sets `wp.%s`, which is forbidden — `%s` is part of a %s identity (or, for " + . '`fields`/`sub_fields`/`layouts`/`parent_repeater`, structural/derived metadata ' + . 'with the same single-source requirement) and must have exactly one source. %s', + $subject, + $forbidden, + $forbidden, + [] === $chain ? "field group's" : ($isLayout ? "layout's" : "field's"), + $this->wpIdentityPropAlternative($forbidden, $isLayout, [] === $chain), + )); + } + } + } + /** Names the sanctioned alternative for a forbidden `wp.` override, for the exception message. */ - private function wpIdentityPropAlternative(string $forbidden, bool $isLayout): string + private function wpIdentityPropAlternative(string $forbidden, bool $isLayout, bool $isRoot = false): string { + if ($isRoot) { + return match ($forbidden) { + 'key' => "Pin the field group's key with the top-level `key:` prop (pattern ^group_) instead.", + 'fields' => 'Author the component\'s `fields:` map at the top level instead — that is the ONLY source the generated `fields` list is assembled from.', + default => 'There is no sanctioned root-level use of this prop; remove it from `wp:`.', + }; + } + return match ($forbidden) { 'key' => $isLayout ? "Pin the layout's key with the top-level `key:` prop (pattern ^layout_) instead." @@ -306,6 +447,9 @@ private function wpIdentityPropAlternative(string $forbidden, bool $isLayout): s ? "Set the layout's ACF name with its own top-level `name:` prop instead." : 'The field name is derived from its own YAML field-map key — rename that map key instead.', 'type' => 'Use the top-level `type:` prop (the semantic type enum) instead.', + 'fields', 'sub_fields' => "Author this container's children with its own top-level `fields:` map instead.", + 'layouts' => "Author this flexible_content field's variants with its own top-level `layouts:` map instead.", + 'parent_repeater' => "`parent_repeater` is always re-derived from nesting; there is no sanctioned way to author it directly — remove it from `wp:`.", default => '', }; } diff --git a/src/Generator/RootFieldGroupBuilder.php b/src/Generator/RootFieldGroupBuilder.php index 7af12a3..dac0d01 100644 --- a/src/Generator/RootFieldGroupBuilder.php +++ b/src/Generator/RootFieldGroupBuilder.php @@ -136,6 +136,26 @@ public function accordionBaseline(string $key, string $label, int $open): array ]; } + /** + * Round 7 — the residual overlay used to exclude ONLY `key`/`label`/ + * `open`/`before` (the props this method itself consumes for + * positioning/baseline construction). An accordion element sourced + * from `wp.accordions` — hand-authored or produced by a future + * migration bug — could therefore carry `type`/`name`/`fields`/ + * `sub_fields`/`layouts`/`parent_repeater` and have them overlaid + * verbatim onto the pseudo-field, impersonating an arbitrary field + * shape (e.g. `type: text, name: bogus_child` smuggling a real field + * in through the accordion channel) despite `accordionBaseline()` + * fixing `type: 'accordion'` / `name: ''` immediately above. Mirrors + * {@see FieldsGenerator::RESERVED_WP_PROPS} — same reserved set, same + * "identity/structure has exactly one source" rule, applied to this + * accordion-residual code path instead of an ordinary field's `wp:`. + */ + private const ACCORDION_RESIDUAL_EXCLUDED_PROPS = [ + 'key', 'label', 'open', 'before', + 'type', 'name', 'fields', 'sub_fields', 'layouts', 'parent_repeater', + ]; + /** * @param array $accordion * @return array @@ -149,12 +169,15 @@ private function buildAccordionPseudoField(array $accordion): array ); // Overlay the captured non-derivable residual verbatim — instructions, // non-zero wpml_cf_preferences, multi_expand, … : any real ACF prop the - // migration self-diff found deviating from the baseline. Meta keys are - // consumed above (key/label/open) or used only for positioning - // (before), never overlaid. Reassigning an existing key keeps its + // migration self-diff found deviating from the baseline. Meta/reserved + // keys are consumed above (key/label/open), used only for positioning + // (before), or structurally fixed by accordionBaseline() and must have + // exactly one source (type/name/fields/sub_fields/layouts/ + // parent_repeater — see ACCORDION_RESIDUAL_EXCLUDED_PROPS) — none of + // those are ever overlaid. Reassigning an existing key keeps its // position, so key order is unchanged. foreach ($accordion as $prop => $value) { - if (!in_array($prop, ['key', 'label', 'open', 'before'], true)) { + if (!in_array($prop, self::ACCORDION_RESIDUAL_EXCLUDED_PROPS, true)) { $pseudo[$prop] = $value; } } diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index f0c5f5c..f3d93a9 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -884,4 +884,293 @@ public function test_layout_wp_overlay_still_works_for_non_identity_props(): voi self::assertSame('table', $group['fields'][0]['layouts'][0]['display']); } + + /** + * Round 7, Finding [1] — ROOT `wp:` was never walked by + * assertNoIdentityPropsInWpOverlay() (only `$definitionTree['fields']` + * was), and RootFieldGroupBuilder::build() merges the root `wp:` bag + * with HIGHEST precedence over the explicit `key` it just assigned — + * so `wp: {key: 'bogus'}` at the ROOT silently repointed the whole + * field group's key. Must now be rejected before generation. + */ + public function test_root_wp_key_override_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.key/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['key' => 'bogus'], + ]), 'demo', 1700000000); + } + + /** + * Round 7, Finding [2] (most severe) — same root-`wp:` gap as above, + * but with `fields` instead of `key`: `wp: {fields: []}` at the ROOT + * silently overwrote the assembled `fields` list with an empty array + * — a definition with real fields generated an acf.json with ZERO + * fields (a broken, empty block in the editor, no error anywhere). + */ + public function test_root_wp_fields_override_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.fields/'); + + $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + 'subtitle' => ['type' => 'text', 'label' => 'Podnadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['fields' => []], + ]), 'demo', 1700000000); + } + + /** + * Round 7, Finding [3] — `wp.sub_fields` on an ordinary LEAF field + * (not a container type) survives all the way to generated output. + * `buildField()` merges `wpOverrides` onto `$field` BEFORE the + * container branch would overwrite `sub_fields` — but that overwrite + * only happens for `$isContainerField` (group/repeater). A leaf field + * (e.g. `text`) never re-derives `sub_fields`, so a smuggled + * `wp.sub_fields` array (carrying its own bogus key/name/type triple) + * ends up verbatim in the generated field — a phantom sub-field ACF + * never asked for. + */ + public function test_wp_sub_fields_smuggled_into_a_leaf_field_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.sub_fields/'); + + $this->generator->generate($this->tree([ + 'title' => [ + 'type' => 'text', + 'label' => 'Nadpis', + 'wp' => ['sub_fields' => [ + ['key' => 'field_bogus_child', 'name' => 'bogus_child', 'type' => 'text', 'label' => 'Bogus'], + ]], + ], + ]), 'demo', 1700000000); + } + + /** Same smuggling hazard, but via `wp.layouts` on an ordinary (non-flexible_content) field. */ + public function test_wp_layouts_smuggled_into_a_leaf_field_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.layouts/'); + + $this->generator->generate($this->tree([ + 'title' => [ + 'type' => 'text', + 'label' => 'Nadpis', + 'wp' => ['layouts' => [ + ['key' => 'layout_bogus', 'name' => 'bogus', 'label' => 'Bogus', 'sub_fields' => []], + ]], + ], + ]), 'demo', 1700000000); + } + + /** + * Round 7, Finding [4] — `wp.parent_repeater` on a repeater's CHILD + * field is merged (via `wpOverrides`) AFTER `buildField()` derives and + * assigns the real `parent_repeater` from nesting, silently + * overwriting the correct, ACF-computed value with an arbitrary one. + * `parent_repeater` has no legitimate authoring path — it is always + * re-derived from nesting — so it is reserved outright. + */ + public function test_wp_parent_repeater_override_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.parent_repeater/'); + + $this->generator->generate($this->tree([ + 'items' => ['type' => 'repeater', 'label' => 'Items', 'fields' => [ + 'title' => [ + 'type' => 'text', + 'label' => 'Title', + 'wp' => ['parent_repeater' => 'field_bogus'], + ], + ]], + ]), 'demo', 1700000000); + } + + /** The same reserved set must be rejected at a nested sub_field two levels deep (group inside group). */ + public function test_reserved_wp_props_are_rejected_at_nested_depth_two(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/wp\.key/'); + + $this->generator->generate($this->tree([ + 'outer' => ['type' => 'group', 'label' => 'Outer', 'fields' => [ + 'inner' => ['type' => 'group', 'label' => 'Inner', 'fields' => [ + 'leaf' => ['type' => 'text', 'label' => 'Leaf', 'wp' => ['key' => 'bogus']], + ]], + ]], + ]), 'demo', 1700000000); + } + + /** Same reserved-set enforcement, for the newly reserved structural/cross-reference props on a layout's own `wp:` bag. */ + public function test_reserved_structural_props_are_rejected_on_a_layout(): void + { + foreach (['fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + try { + $this->generator->generate($this->tree([ + 'items' => ['type' => 'flexible_content', 'label' => 'Items', 'layouts' => [ + 'title' => ['label' => 'Title', 'wp' => [$prop => []], 'fields' => [ + 'c' => ['type' => 'text', 'label' => 'C'], + ]], + ]], + ]), 'demo', 1700000000); + self::fail("wp.{$prop} on a layout was expected to be rejected but generation succeeded."); + } catch (\Parisek\DefinitionKit\Generator\GenerationValidationException $e) { + self::assertStringContainsString("wp.{$prop}", $e->getMessage()); + } + } + } + + /** + * Strengthened control (round 7) — the previous control test only + * exercised benign PRESENTATION props (`toolbar`, `acf_type`), which + * is exactly why the structural/cross-reference bypasses ([1]-[4]) + * went unnoticed for a whole round. Asserts BOTH directions on the + * SAME field shape: genuine installed-base props still pass, and every + * newly reserved prop is independently rejected. + */ + public function test_legitimate_props_pass_while_every_newly_reserved_prop_is_rejected(): void + { + $group = $this->generator->generate($this->tree([ + 'body' => [ + 'type' => 'richtext', + 'label' => 'Text', + 'wp' => ['toolbar' => 'full', 'new_lines' => 'wpautop', 'collapsed' => '', 'ui' => 1], + ], + ]), 'demo', 1700000000); + self::assertSame('full', $group['fields'][0]['toolbar']); + self::assertSame('wpautop', $group['fields'][0]['new_lines']); + + foreach (['fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + try { + $this->generator->generate($this->tree([ + 'body' => ['type' => 'richtext', 'label' => 'Text', 'wp' => [$prop => []]], + ]), 'demo', 1700000000); + self::fail("wp.{$prop} on a field was expected to be rejected but generation succeeded."); + } catch (\Parisek\DefinitionKit\Generator\GenerationValidationException $e) { + self::assertStringContainsString("wp.{$prop}", $e->getMessage()); + } + } + } + + /** + * Round 7 — RootFieldGroupBuilder::buildAccordionPseudoField() overlaid + * the captured accordion residual excluding ONLY key/label/open/before + * — an accordion element carrying `type`/`name`/`fields`/`sub_fields`/ + * `layouts`/`parent_repeater` (via a hand-authored or corrupted + * `wp.accordions` entry) could impersonate an arbitrary pseudo-field, + * bypassing the accordion baseline's fixed `type: 'accordion'` / + * `name: ''`. Must now be constrained to the same reserved set. + */ + public function test_accordion_residual_cannot_inject_a_bogus_type_or_name(): void + { + $group = $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['accordions' => [ + [ + 'key' => 'field_demo_accordion_a', + 'label' => 'A', + 'open' => 0, + 'before' => 'title', + 'type' => 'text', + 'name' => 'bogus_child', + ], + ]], + ]), 'demo', 1700000000); + + $accordion = $group['fields'][0]; + self::assertSame('accordion', $accordion['type'], 'accordion residual let `wp.accordions[].type` impersonate a different field shape'); + self::assertSame('', $accordion['name'], 'accordion residual let `wp.accordions[].name` smuggle a bogus field name'); + } + + /** + * Genuine accordions must keep working end to end after the residual + * exclusion is widened — the fix narrows what CAN be overlaid, it must + * not break the legitimate `before`/`open`/non-default `instructions` + * residual path itself. `wp.accordions` appears 38 times in the + * committed installed base — this is the "still works" half of the + * regression pair above. + */ + public function test_accordion_with_legitimate_residual_props_still_works(): void + { + $group = $this->generator->generate($this->tree([ + 'title' => ['type' => 'text', 'label' => 'Nadpis'], + ], [ + 'name' => 'Demo', + 'wp' => ['accordions' => [ + [ + 'key' => 'field_demo_accordion_a', + 'label' => 'A', + 'open' => 1, + 'before' => 'title', + 'instructions' => 'Some instructions', + 'multi_expand' => 1, + ], + ]], + ]), 'demo', 1700000000); + + $accordion = $group['fields'][0]; + self::assertSame('accordion', $accordion['type']); + self::assertSame(1, $accordion['open']); + self::assertSame('Some instructions', $accordion['instructions']); + self::assertSame(1, $accordion['multi_expand']); + self::assertSame('text', $group['fields'][1]['type']); + } + + /** + * `wp.conditional_logic` is a REAL fallback Migration\AcfJsonReader + * emits (see VisibleWhenMapper::map()) whenever raw ACF + * conditional_logic is too complex to reduce to the abstract + * `visible_when` vocabulary (2+ AND conditions, 2+ OR groups, or an + * unmapped operator) — it is not forbidden like the structural/ + * identity props above. Instead its references must RESOLVE: a + * `conditional_logic` entry pointing at a key that exists nowhere in + * the generated tree is a dangling reference ACF's editor would show + * as broken with no diagnostic from this tool. This is the positive + * (still works) half of that contract. + */ + public function test_wp_conditional_logic_fallback_with_resolvable_reference_still_works(): void + { + $group = $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'key' => 'field_test_toggle_key'], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => [[ + ['field' => 'field_test_toggle_key', 'operator' => '==', 'value' => '1'], + ['field' => 'field_test_toggle_key', 'operator' => '!=', 'value' => '2'], + ]]], + ], + ], ['name' => 'Test']), 'test', 1700000000); + + self::assertIsArray($group['fields'][1]['conditional_logic']); + } + + /** Negative half — a `wp.conditional_logic` fallback referencing a key that doesn't exist anywhere must be rejected loudly. */ + public function test_wp_conditional_logic_fallback_with_dangling_reference_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/conditional_logic/'); + $this->expectExceptionMessageMatches('/field_does_not_exist/'); + + $this->generator->generate($this->tree([ + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => [[ + ['field' => 'field_does_not_exist', 'operator' => '==', 'value' => '1'], + ]]], + ], + ], ['name' => 'Test']), 'test', 1700000000); + } } diff --git a/tests/Schema/FieldsSchemaValidatorTest.php b/tests/Schema/FieldsSchemaValidatorTest.php index f34f17f..6c539ea 100644 --- a/tests/Schema/FieldsSchemaValidatorTest.php +++ b/tests/Schema/FieldsSchemaValidatorTest.php @@ -125,7 +125,19 @@ public function test_wp_type_on_a_field_is_rejected_by_schema(): void self::assertFalse($result->valid); } - /** Sanity control — every OTHER prop inside `wp:` stays fully open. */ + /** + * Sanity control — every OTHER prop inside `wp:` stays fully open. + * + * Round 7 — this control test used to exercise ONLY benign + * presentation props (`toolbar`, `acf_type`), which is exactly why + * the structural/cross-reference bypasses (findings [1]-[4]) went + * unnoticed for a whole review round: nothing here proved the deny- + * list actually covered anything beyond the original three scalars. + * Strengthened to assert BOTH directions — legitimate installed-base + * props (`toolbar`, `new_lines`, `collapsed`, `ui`) still validate, + * AND every newly reserved structural/cross-reference prop is + * independently rejected — on the same field shape. + */ public function test_wp_non_identity_props_on_a_field_still_validate(): void { $tree = Yaml::parse(<<validateData($tree); self::assertTrue($result->valid, print_r($result->errors, true)); + + foreach (['key', 'name', 'type', 'fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + $rejectedTree = Yaml::parse(<<validateData($rejectedTree); + self::assertFalse($rejectedResult->valid, "wp.{$prop} on a field was expected to fail schema validation."); + } } /** Same deny-list, exercised on a flexible_content layout's own `wp:` bag. */ @@ -398,6 +424,124 @@ public function test_usage_rejects_an_empty_list(): void self::assertFalse($result->valid); } + /** + * Round 7 — the reserved deny-list widened beyond `key`/`name`/`type` + * to cover structural containers (`fields`/`sub_fields`/`layouts`, + * which carry the identity of NESTED nodes) and a cross-reference + * (`parent_repeater`, ACF-computed structural metadata with no + * legitimate authoring path). Exercised on a field's own `wp:` bag. + */ + public function test_wp_reserved_structural_props_on_a_field_are_rejected_by_schema(): void + { + foreach (['fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid, "wp.{$prop} on a field was expected to fail schema validation."); + } + } + + /** Same reserved set, exercised on a flexible_content layout's own `wp:` bag. */ + public function test_wp_reserved_structural_props_on_a_layout_are_rejected_by_schema(): void + { + foreach (['fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid, "wp.{$prop} on a layout was expected to fail schema validation."); + } + } + + /** + * Round 7, Finding [1]/[2] — the ROOT-level `wp:` bag used to be a + * bare `{"type":"object"}`, with no deny-list at all, while field- + * and layout-level `wp:` already `$ref`'d `wpOverlay`. `wp.key` + * (repointing the whole field group's key) and `wp.fields` (silently + * zeroing the assembled `fields` list — the most severe finding of + * the whole series) both validated successfully. Root `wp:` must now + * `$ref` the SAME `wpOverlay` def. + */ + public function test_wp_reserved_props_on_root_are_rejected_by_schema(): void + { + foreach (['key', 'name', 'type', 'fields', 'sub_fields', 'layouts', 'parent_repeater'] as $prop) { + $tree = Yaml::parse(<<validateData($tree); + self::assertFalse($result->valid, "wp.{$prop} on root was expected to fail schema validation."); + } + } + + /** + * Sanity control — root's own sanctioned `wp:` uses (e.g. + * `wp.accordions`, appears 38 times in the installed base) must keep + * validating once root `wp:` gains the `wpOverlay` `$ref`. + */ + public function test_wp_accordions_on_root_still_validates(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertTrue($result->valid, print_r($result->errors, true)); + } + + /** + * `wp.conditional_logic` is deliberately NOT part of the schema-level + * deny-list — Migration\AcfJsonReader emits it as a real fallback for + * ACF conditional logic too complex to reduce to `visible_when` (see + * VisibleWhenMapper). Its correctness is enforced at the GENERATOR + * level (dangling-reference resolution against the assembled tree), + * not the schema level, since the schema cannot see across fields. + */ + public function test_wp_conditional_logic_is_not_rejected_by_schema(): void + { + $tree = Yaml::parse(<<validateData($tree); + self::assertTrue($result->valid, print_r($result->errors, true)); + } + public function test_existing_dávka_1_fixtures_still_validate(): void { foreach (['valid-flat', 'valid-nested', 'valid-empty-wp'] as $name) { From d92840121c83b7600d9ee38f472e8febc37d7602 Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Sat, 25 Jul 2026 08:21:26 +0200 Subject: [PATCH 09/10] Round 8: validate wp.conditional_logic shape; align fields-validate with fields-generate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- bin/fields-validate | 32 +++++ src/Generator/FieldsGenerator.php | 70 +++++++++- tests/Generator/FieldsGeneratorTest.php | 86 ++++++++++++ tests/Schema/FieldsValidateCliTest.php | 167 ++++++++++++++++++++++++ 4 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 tests/Schema/FieldsValidateCliTest.php diff --git a/bin/fields-validate b/bin/fields-validate index 5787bb4..4b10ef1 100755 --- a/bin/fields-validate +++ b/bin/fields-validate @@ -3,6 +3,8 @@ declare(strict_types=1); +use Parisek\DefinitionKit\Generator\FieldsGenerator; +use Parisek\DefinitionKit\Generator\GenerationValidationException; use Parisek\DefinitionKit\Lint\KindLinter; use Parisek\DefinitionKit\Schema\FieldsSchemaValidator; use Symfony\Component\Yaml\Yaml; @@ -50,6 +52,36 @@ foreach ($paths as $path) { continue; } + // `wp.conditional_logic` parity with `fields-generate` — see the + // `component.fields.schema.json` `wpOverlay` $def for the reserved-props + // half of this precedent (a pure JSON-Schema shape check both tools share + // via FieldsSchemaValidator above) and FieldsGenerator's own + // `assertConditionalLogicReferencesResolve()` docblock for why + // conditional_logic gets a validation gate instead of an outright ban. + // + // Shape validation (malformed conditional_logic) is a static check with + // no dependency on the assembled field tree. Reference RESOLUTION (does + // `field: X` name a key that actually exists) is NOT static — ACF field + // keys are derived through FieldsGenerator::deriveOrPinKey()'s full + // precedence chain (authored `key:`, the `wp.key` escape hatch, or a + // convention-derived dotted name-chain join), the exact same computation + // `siblingKeyMap()` / `buildField()` perform while assembling the tree. + // There is no lighter-weight way to know "what keys exist" than building + // that tree. Rather than re-implement key derivation a second time here + // (which is exactly the kind of split-brain this fix exists to close), + // this calls FieldsGenerator::generate() directly — a pure, side-effect- + // free computation (nothing is written to disk; only `fields-generate` + // persists the result via AcfJsonWriter/BlockJsonWriter). Both the shape + // check and the reference-resolution check therefore ride the same call. + try { + (new FieldsGenerator())->generate($definition, basename($path, '.yaml'), 0); + } catch (GenerationValidationException $e) { + $failed = true; + echo "FAIL {$path}\n"; + echo " {$e->getMessage()}\n"; + continue; + } + echo "OK {$path}\n"; foreach ($findings as $finding) { // Warnings are reported but never flip the exit code. diff --git a/src/Generator/FieldsGenerator.php b/src/Generator/FieldsGenerator.php index b989eb7..91d1579 100644 --- a/src/Generator/FieldsGenerator.php +++ b/src/Generator/FieldsGenerator.php @@ -281,6 +281,66 @@ private function assertConditionalLogicReferencesResolve(array $builtFields): vo } } + /** + * Round 8 — shape validation. Before this, {@see collectKeysAndConditionalLogicRefs()} + * only inspected `conditional_logic` when it happened to already be + * `is_array()`: a scalar fallback (`wp.conditional_logic: 'bogus'`) or an + * AND-rule missing its `field` key entirely (as opposed to carrying an + * empty one, which round 7 already caught) skipped every check silently + * and would reach `acf.json` verbatim — ACF's editor either ignores it + * or errors opaquely, with zero diagnostic from this tool. + * + * Canonical ACF shape: an array of OR-groups, each an array of AND-rules, + * each rule a map carrying a non-empty string `field` key (`operator` / + * `value` and any other keys are ACF's concern, not validated here — this + * gate only guards the ONE thing this tool itself later reads: the + * `field` reference {@see assertConditionalLogicReferencesResolve()} + * resolves). Throws the same `GenerationValidationException` and matches + * the dangling-reference message's voice: state the problem, name the + * consequence, offer the sanctioned alternative. + */ + private function assertConditionalLogicShapeIsValid(mixed $conditionalLogic, string $fieldPath): void + { + if (!is_array($conditionalLogic)) { + throw new GenerationValidationException(sprintf( + "Field '%s' carries a `wp.conditional_logic` fallback that is not an array — got %s. " + . 'ACF conditional_logic must be an array of OR-groups, each an array of AND-rules, each ' + . 'rule a map with a `field` key — a malformed shape ACF\'s editor would show as broken. ' + . 'Fix the shape, or express the condition through `visible_when:` if it fits the ' + . 'single-condition vocabulary.', + $fieldPath, + get_debug_type($conditionalLogic), + )); + } + + foreach ($conditionalLogic as $orGroup) { + if (!is_array($orGroup)) { + throw new GenerationValidationException(sprintf( + "Field '%s' carries a `wp.conditional_logic` OR-group that is not an array — got %s. " + . 'ACF conditional_logic must be an array of OR-groups, each an array of AND-rules, each ' + . 'rule a map with a `field` key — a malformed shape ACF\'s editor would show as broken. ' + . 'Fix the shape, or express the condition through `visible_when:` if it fits the ' + . 'single-condition vocabulary.', + $fieldPath, + get_debug_type($orGroup), + )); + } + + foreach ($orGroup as $rule) { + if (!is_array($rule) || !isset($rule['field']) || !is_string($rule['field']) || '' === $rule['field']) { + throw new GenerationValidationException(sprintf( + "Field '%s' carries a `wp.conditional_logic` rule with no non-empty string `field` " + . 'key — ACF conditional_logic requires every AND-rule to be a map naming the field ' + . 'it depends on, or ACF\'s editor would show it as broken. Fix the rule\'s `field` ' + . 'key, or express the condition through `visible_when:` if it fits the ' + . 'single-condition vocabulary.', + $fieldPath, + )); + } + } + } + } + /** * @param list> $fields * @param list $allKeys @@ -292,7 +352,15 @@ private function collectKeysAndConditionalLogicRefs(array $fields, array &$allKe $allKeys[] = (string) $field['key']; $conditionalLogic = $field['conditional_logic'] ?? false; - if (is_array($conditionalLogic)) { + // A falsy `conditional_logic` (`false` — FieldReconstructor's default + // for a field with no `visible_when:`; `0` — RootFieldGroupBuilder's + // ACF-canonical "off" marker on accordion pseudo-fields) means "no + // conditional logic", not a malformed shape — leave it unvalidated. + // Anything ELSE that isn't a well-formed array (a truthy scalar, a + // malformed nested shape) IS a validation target. + if (false !== $conditionalLogic && 0 !== $conditionalLogic) { + $this->assertConditionalLogicShapeIsValid($conditionalLogic, (string) $field['key']); + foreach ($conditionalLogic as $orGroup) { foreach ((array) $orGroup as $cond) { if (isset($cond['field'])) { diff --git a/tests/Generator/FieldsGeneratorTest.php b/tests/Generator/FieldsGeneratorTest.php index f3d93a9..5a7a3f3 100644 --- a/tests/Generator/FieldsGeneratorTest.php +++ b/tests/Generator/FieldsGeneratorTest.php @@ -1173,4 +1173,90 @@ public function test_wp_conditional_logic_fallback_with_dangling_reference_is_re ], ], ['name' => 'Test']), 'test', 1700000000); } + + /** + * Shape validation, round 8 — before round 8, `assertConditionalLogicReferencesResolve()` + * only inspected `conditional_logic` when it happened to already be + * `is_array()`; a scalar fallback (a typo, or a hand-authored YAML + * mistake) skipped every check silently and would reach `acf.json` + * verbatim, where ACF's editor either ignores it or errors opaquely. + * The canonical ACF shape is: array of OR-groups, each an array of + * AND-rules, each rule a map with at least a non-empty string `field` + * key. This is the malformed-shape half of that contract — a scalar. + */ + public function test_wp_conditional_logic_scalar_shape_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/conditional_logic/'); + + $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle'], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => 'bogus'], + ], + ], ['name' => 'Test']), 'test', 1700000000); + } + + /** Same contract, a rule missing the required `field` key entirely (not merely empty). */ + public function test_wp_conditional_logic_rule_missing_field_key_is_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/conditional_logic/'); + + $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle'], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => [[ + ['operator' => '==', 'value' => 1], + ]]], + ], + ], ['name' => 'Test']), 'test', 1700000000); + } + + /** + * The already-correct rejection (empty string `field`) must keep + * working after the shape-validation pass is added — round 8 must + * not regress round 7's dangling-reference guard. + */ + public function test_wp_conditional_logic_rule_with_empty_field_string_is_still_rejected(): void + { + $this->expectException(\Parisek\DefinitionKit\Generator\GenerationValidationException::class); + $this->expectExceptionMessageMatches('/conditional_logic/'); + + $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle'], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => [[ + ['field' => '', 'operator' => '==', 'value' => 1], + ]]], + ], + ], ['name' => 'Test']), 'test', 1700000000); + } + + /** A well-formed, resolvable shape (a single OR-group, two AND-rules) must still pass — shape validation must not be over-eager. */ + public function test_wp_conditional_logic_well_formed_multi_rule_shape_still_works(): void + { + $group = $this->generator->generate($this->tree([ + 'toggle' => ['type' => 'boolean', 'label' => 'Toggle', 'key' => 'field_test_toggle_key'], + 'other' => ['type' => 'boolean', 'label' => 'Other', 'key' => 'field_test_other_key'], + 'conditional_field' => [ + 'type' => 'text', + 'label' => 'Conditional', + 'wp' => ['conditional_logic' => [ + [ + ['field' => 'field_test_toggle_key', 'operator' => '==', 'value' => '1'], + ['field' => 'field_test_other_key', 'operator' => '==', 'value' => '1'], + ], + ]], + ], + ], ['name' => 'Test']), 'test', 1700000000); + + self::assertIsArray($group['fields'][2]['conditional_logic']); + } } diff --git a/tests/Schema/FieldsValidateCliTest.php b/tests/Schema/FieldsValidateCliTest.php new file mode 100644 index 0000000..cb0128f --- /dev/null +++ b/tests/Schema/FieldsValidateCliTest.php @@ -0,0 +1,167 @@ +bin = __DIR__ . '/../../bin/fields-validate'; + $this->root = sys_get_temp_dir() . '/fields-validate-cli-test-' . uniqid('', true); + mkdir($this->root, 0777, true); + } + + protected function tearDown(): void + { + $this->rrmdir($this->root); + } + + private function rrmdir(string $dir): void + { + foreach (glob("{$dir}/*") ?: [] as $entry) { + is_dir($entry) ? $this->rrmdir($entry) : unlink($entry); + } + rmdir($dir); + } + + private function writeYaml(string $slug, string $yaml): string + { + $path = "{$this->root}/{$slug}.yaml"; + file_put_contents($path, $yaml); + return $path; + } + + /** @return array{0: string, 1: int|null} */ + private function runCli(string $path): array + { + $output = []; + $exitCode = null; + exec(escapeshellarg(PHP_BINARY) . ' ' . escapeshellarg($this->bin) . ' ' . escapeshellarg($path) . ' 2>&1', $output, $exitCode); + return [implode("\n", $output), $exitCode]; + } + + public function test_valid_definition_exits_zero(): void + { + $path = $this->writeYaml('demo', "name: Demo\nkind: element\nfields:\n title:\n type: text\n label: Title\n"); + + [$output, $exitCode] = $this->runCli($path); + + self::assertSame(0, $exitCode); + self::assertStringContainsString('OK ', $output); + } + + public function test_schema_invalid_definition_exits_nonzero(): void + { + $path = $this->writeYaml('demo', "name: Demo\nkind: element\nfields:\n title:\n type: not_a_real_type\n label: Title\n"); + + [$output, $exitCode] = $this->runCli($path); + + self::assertNotSame(0, $exitCode); + self::assertStringContainsString('FAIL ', $output); + } + + /** Fix 2 — shape check parity: a scalar `wp.conditional_logic` must FAIL fields-validate, same as fields-generate. */ + public function test_conditional_logic_malformed_shape_is_rejected(): void + { + $path = $this->writeYaml('demo', <<runCli($path); + + self::assertNotSame(0, $exitCode); + self::assertStringContainsString('FAIL ', $output); + self::assertStringContainsString('conditional_logic', $output); + } + + /** Fix 2 — reference-resolution parity: a dangling `wp.conditional_logic` reference must FAIL fields-validate, same as fields-generate. */ + public function test_conditional_logic_dangling_reference_is_rejected(): void + { + $path = $this->writeYaml('demo', <<runCli($path); + + self::assertNotSame(0, $exitCode); + self::assertStringContainsString('FAIL ', $output); + self::assertStringContainsString('conditional_logic', $output); + self::assertStringContainsString('field_test_neexistuje', $output); + } + + /** A resolvable `wp.conditional_logic` reference must still pass — the new check must not be over-eager. */ + public function test_conditional_logic_resolvable_reference_still_passes(): void + { + $path = $this->writeYaml('demo', <<runCli($path); + + self::assertSame(0, $exitCode); + self::assertStringContainsString('OK ', $output); + } +} From ea674ba88089d9f559681758898292aa4294dd0e Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Sat, 25 Jul 2026 10:35:13 +0200 Subject: [PATCH 10/10] docs(changelog): cover the wp: identity reservation and conditional_logic 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. --- CHANGELOG.md | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 690a29b..315ea6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,28 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). nesting machinery as an ordinary `group`/`repeater`, including `flexible_content` nested inside another `flexible_content`'s layout. +### Changed + +- **Breaking.** The `wp:` escape hatch no longer accepts properties that carry + a node's identity or its cross-references. `key`, `name`, `type`, `fields`, + `sub_fields`, `layouts` and `parent_repeater` are rejected inside any `wp:` + block — at the root group, on a field at any nesting depth, and on a + flexible_content layout. Enforced identically by the JSON Schema + (`wpOverlay` `$def`) and by the generator (`RESERVED_WP_PROPS`), since + `fields-lint` and `fields-generate` take different code paths. + + `wp:` is merged last with highest precedence, so any property it could + override became a way to desynchronise the generated tree from what the + generator derived. Identity now has exactly one path. The sanctioned + alternatives are unchanged and 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. + + No migration needed: verified against every committed definition in the + downstream fleet (174 across five projects) — none authors any reserved + property inside `wp:`. `wp.accordions` is deliberately untouched (38 uses in + that fleet); its residual overlay now excludes the same reserved set. + ### Fixed - Generator now enforces GLOBAL key uniqueness across an entire generated @@ -44,6 +66,26 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). against the ecosystem's canonical ACF-shape validator — closing the gap that let the `display`/`location` and schema-parity regressions above ship unnoticed. +- `wp.key` no longer desynchronises `conditional_logic`. The sibling + name-to-key map was built from pre-overlay keys, so a field pinning its key + through `wp:` emitted a `conditional_logic` reference to a key present + nowhere in the output — silent, shipped, and simply never fired in the + editor. +- A root `wp: {fields: []}` no longer silently discards the entire generated + field tree, emitting an empty block with no error. +- `wp: {sub_fields: [...]}` / `wp: {layouts: [...]}` can no longer smuggle + unvalidated nodes one level deeper, and `wp: {parent_repeater: ...}` can no + longer overwrite a derived cross-reference. +- Sibling `name` collisions are caught by field shape rather than by an empty + name, so an ordinary field can no longer take the accordion carve-out. +- `wp.conditional_logic` — the migration reader's documented fallback for ACF + conditional logic too complex to express as `visible_when` — is now + validated rather than trusted: the canonical OR-of-AND-rules shape is + enforced, and every referenced key must resolve in the generated tree. A + dangling reference throws instead of shipping a broken editor condition. +- `fields-validate` and `fields-generate` no longer disagree. The + conditional-logic checks lived only in the generator's semantic pass, so a + definition could validate clean and then fail to generate. ## [0.2.1] - 2026-07-23