From 02ae76d4f32d66346e5b25b6da35c644631a1ea2 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 12:00:19 +0200 Subject: [PATCH] fix: stop guessing a kind for composite Divi attribute paths Review fix round 1 on the lit/divi-catalog task: - AttrPathIndex::collectKinds() no longer records a kind from a component.type === "group" node (a composite of several sub-kinds, e.g. button.innerContent = text + link + target), and kinds() now drops any attrName with more than one distinct candidate kind instead of last-write-wins (e.g. imageIcon.innerContent had onoff + icon + image sub-fields sharing one attrName). Both previously produced a wrong-but-present kind, which is more dangerous for 2b's sanitization routing than "unknown". Verified against the committed blurb/button/fullwidth-header fixture entries and live against the full 100-module catalog. - AttrPathIndex::fromDefaults() now builds on AttrMap::flatten() instead of a parallel walker. - ModuleCatalog::shortName() replaces the duplicated substr($name, strlen('divi/')) in DiviCatalogAbility. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- .../Divi/Schema/AttrPathIndex.php | 59 ++++++++++--------- .../Divi/Schema/DiviCatalogAbility.php | 5 +- .../Divi/Schema/ModuleCatalog.php | 6 ++ .../Divi/Schema/DiviCatalogAbilityTest.php | 4 +- .../Divi/Schema/ModuleCatalogTest.php | 19 +++++- 5 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/Integrations/Divi/Schema/AttrPathIndex.php b/src/Integrations/Divi/Schema/AttrPathIndex.php index 2a33866..896e7a3 100644 --- a/src/Integrations/Divi/Schema/AttrPathIndex.php +++ b/src/Integrations/Divi/Schema/AttrPathIndex.php @@ -93,7 +93,7 @@ final class AttrPathIndex self::attrNames($settings, $paths); } - self::fromDefaults($defaults, '', $paths); + self::fromDefaults($defaults, $paths); $list = array_map('strval', array_keys($paths)); sort($list); @@ -102,18 +102,30 @@ final class AttrPathIndex /** * Known attribute path => value kind (text/richtext/code/url/image/color/enum/onoff/number/ - * list/icon), wherever Divi's metadata names an unambiguous field component for the path. - * Not every known path (see paths()) resolves to a kind; callers default to "unknown". + * list/icon), wherever Divi's metadata names exactly one unambiguous field component for the + * path. A path is left out (callers default to "unknown") when Divi's own component is a + * "group" (a composite of several sub-kinds, e.g. button.innerContent = text + link + target) + * or when several distinct "field" components share the same attrName with different kinds + * (e.g. imageIcon.innerContent = onoff useIcon + icon icon + image src) – a wrong-but-present + * kind would be more dangerous for 2b's sanitization routing than an honest "unknown". * * @param array $attributes Metadata "attributes" of one module. * @return array */ public static function kinds(array $attributes): array { - $kinds = ['module.meta.adminLabel' => 'text']; + $candidates = []; foreach ($attributes as $definition) { $settings = is_array($definition['settings'] ?? null) ? $definition['settings'] : []; - self::collectKinds($settings, $kinds); + self::collectKinds($settings, $candidates); + } + + $kinds = ['module.meta.adminLabel' => 'text']; + foreach ($candidates as $attrName => $found) { + $unique = array_unique($found); + if (count($unique) === 1) { + $kinds[$attrName] = $unique[0]; + } } return $kinds; @@ -170,46 +182,39 @@ final class AttrPathIndex } /** - * @param array $node - * @param array $kinds + * Collects every (attrName => kind) candidate found, without yet resolving conflicts – a + * "group" component (a composite of several sub-kinds) never contributes a candidate. + * + * @param array $node + * @param array> $candidates */ - private static function collectKinds(array $node, array &$kinds): void + private static function collectKinds(array $node, array &$candidates): void { $attrName = $node['attrName'] ?? null; - $componentName = $node['component']['name'] ?? null; + $component = is_array($node['component'] ?? null) ? $node['component'] : []; + $componentName = $component['name'] ?? null; if ( is_string($attrName) && preg_match(self::ATTR_NAME_PATTERN, $attrName) + && ($component['type'] ?? null) === 'field' && is_string($componentName) && isset(self::COMPONENT_KINDS[$componentName]) ) { - $kinds[$attrName] = self::COMPONENT_KINDS[$componentName]; + $candidates[$attrName][] = self::COMPONENT_KINDS[$componentName]; } foreach ($node as $value) { if (is_array($value)) { - self::collectKinds($value, $kinds); + self::collectKinds($value, $candidates); } } } - /** - * @param array $attrs - * @param array $paths - */ - private static function fromDefaults(array $attrs, string $prefix, array &$paths): void + /** @param array $attrs */ + private static function fromDefaults(array $attrs, array &$paths): void { - foreach ($attrs as $key => $value) { - if ($prefix === '' && in_array($key, AttrMap::META_KEYS, true)) { - continue; - } - - $path = $prefix === '' ? (string) $key : $prefix . '.' . $key; - if (AttrMap::isBreakpointMap($value)) { - $paths[$path] = true; - } elseif (is_array($value)) { - self::fromDefaults($value, $path, $paths); - } + foreach (array_keys(AttrMap::flatten($attrs)) as $path) { + $paths[$path] = true; } } } diff --git a/src/Integrations/Divi/Schema/DiviCatalogAbility.php b/src/Integrations/Divi/Schema/DiviCatalogAbility.php index 2451e24..263a725 100644 --- a/src/Integrations/Divi/Schema/DiviCatalogAbility.php +++ b/src/Integrations/Divi/Schema/DiviCatalogAbility.php @@ -119,7 +119,8 @@ final class DiviCatalogAbility implements Ability if (! is_string($input['module'] ?? null) || $input['module'] === '') { $modules = []; foreach ($catalog->summary() as $module) { - $modules[] = $module + ['shortform' => ShortformMap::has(substr($module['name'], strlen('divi/')))]; + $shortName = ModuleCatalog::shortName($module['name']); + $modules[] = $module + ['shortform' => ShortformMap::has($shortName)]; } return ['modules' => $modules, 'notes' => $notes]; @@ -133,7 +134,7 @@ final class DiviCatalogAbility implements Ability ); } - $short = substr($name, strlen('divi/')); + $short = ModuleCatalog::shortName($name); $item = ShortformMap::childType($short); return [ diff --git a/src/Integrations/Divi/Schema/ModuleCatalog.php b/src/Integrations/Divi/Schema/ModuleCatalog.php index 47b7083..d94066b 100644 --- a/src/Integrations/Divi/Schema/ModuleCatalog.php +++ b/src/Integrations/Divi/Schema/ModuleCatalog.php @@ -46,6 +46,12 @@ final class ModuleCatalog return str_contains($type, '/') ? $type : 'divi/' . $type; } + /** "divi/button" → "button" (inverse of normalize(), for ShortformMap's short names). */ + public static function shortName(string $name): string + { + return str_starts_with($name, 'divi/') ? substr($name, strlen('divi/')) : $name; + } + public function has(string $name): bool { return isset($this->modules[$name]); diff --git a/tests/Unit/Integrations/Divi/Schema/DiviCatalogAbilityTest.php b/tests/Unit/Integrations/Divi/Schema/DiviCatalogAbilityTest.php index 5b1b3b6..ebcb333 100644 --- a/tests/Unit/Integrations/Divi/Schema/DiviCatalogAbilityTest.php +++ b/tests/Unit/Integrations/Divi/Schema/DiviCatalogAbilityTest.php @@ -54,8 +54,10 @@ final class DiviCatalogAbilityTest extends TestCase { $button = $this->ability()->execute(['module' => 'divi/button'])['module']; - self::assertSame('url', $button['path_kinds']['button.innerContent']); + self::assertSame('text', $button['path_kinds']['module.meta.adminLabel']); self::assertArrayNotHasKey('no.such.path', $button['path_kinds']); + // Composite ("group") paths are left out rather than guessing a single kind. + self::assertArrayNotHasKey('button.innerContent', $button['path_kinds']); } public function testUnknownModuleAndPermission(): void diff --git a/tests/Unit/Integrations/Divi/Schema/ModuleCatalogTest.php b/tests/Unit/Integrations/Divi/Schema/ModuleCatalogTest.php index cd4fbd3..2e5e2d0 100644 --- a/tests/Unit/Integrations/Divi/Schema/ModuleCatalogTest.php +++ b/tests/Unit/Integrations/Divi/Schema/ModuleCatalogTest.php @@ -136,12 +136,13 @@ final class ModuleCatalogTest extends TestCase self::assertSame('richtext', $catalog->pathKind('divi/text', 'content.innerContent')); self::assertSame('text', $catalog->pathKind('divi/heading', 'title.innerContent')); self::assertSame('code', $catalog->pathKind('divi/code', 'content.innerContent')); - self::assertSame('url', $catalog->pathKind('divi/button', 'button.innerContent')); self::assertSame('text', $catalog->pathKind('divi/button', 'module.meta.adminLabel')); // Structural/style groups (fonts, backgrounds, …) carry no single scalar component in // Divi's metadata; 2b falls back to key-name routing for them (spec §5.6, ruling R3). self::assertSame('unknown', $catalog->pathKind('divi/button', 'button.decoration.background')); self::assertSame('unknown', $catalog->pathKind('divi/button', 'no.such.path')); + // divi/image has no explicit attrName for its innerContent sub-fields at all. + self::assertSame('unknown', $catalog->pathKind('divi/image', 'image.innerContent')); // Every kind in the module's kinds() map is a string, keyed by a known attribute path. $paths = $catalog->paths('divi/heading'); @@ -150,4 +151,20 @@ final class ModuleCatalogTest extends TestCase self::assertContains($path, $paths); } } + + public function testLeavesCompositeAttributePathsUnknownRatherThanGuessing(): void + { + $catalog = self::sampleCatalog(); + + // button.innerContent's only attrName-annotated node is the "link" sub-group itself + // (component.type "group": link text + linkUrl + linkTarget) – not a single scalar kind. + self::assertSame('unknown', $catalog->pathKind('divi/button', 'button.innerContent')); + + // imageIcon.innerContent carries three "field" components under the same attrName with + // different kinds (useIcon: onoff, icon: icon, src: image) – a genuine conflict. + self::assertSame('unknown', $catalog->pathKind('divi/blurb', 'imageIcon.innerContent')); + + // fullwidth-header's scrollDown.decoration.icon is the same pattern (onoff/icon/color/number). + self::assertSame('unknown', $catalog->pathKind('divi/fullwidth-header', 'scrollDown.decoration.icon')); + } }