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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7
This commit is contained in:
2026-09-26 12:00:35 +02:00
co-authored by Claude Opus 5.5
parent 2f4dcf9ce4
commit 02ae76d4f3
5 changed files with 62 additions and 31 deletions
+32 -27
View File
@@ -93,7 +93,7 @@ final class AttrPathIndex
self::attrNames($settings, $paths); self::attrNames($settings, $paths);
} }
self::fromDefaults($defaults, '', $paths); self::fromDefaults($defaults, $paths);
$list = array_map('strval', array_keys($paths)); $list = array_map('strval', array_keys($paths));
sort($list); sort($list);
@@ -102,18 +102,30 @@ final class AttrPathIndex
/** /**
* Known attribute path => value kind (text/richtext/code/url/image/color/enum/onoff/number/ * 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. * list/icon), wherever Divi's metadata names exactly one unambiguous field component for the
* Not every known path (see paths()) resolves to a kind; callers default to "unknown". * 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<string, mixed> $attributes Metadata "attributes" of one module. * @param array<string, mixed> $attributes Metadata "attributes" of one module.
* @return array<string, string> * @return array<string, string>
*/ */
public static function kinds(array $attributes): array public static function kinds(array $attributes): array
{ {
$kinds = ['module.meta.adminLabel' => 'text']; $candidates = [];
foreach ($attributes as $definition) { foreach ($attributes as $definition) {
$settings = is_array($definition['settings'] ?? null) ? $definition['settings'] : []; $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; return $kinds;
@@ -170,46 +182,39 @@ final class AttrPathIndex
} }
/** /**
* @param array<array-key, mixed> $node * Collects every (attrName => kind) candidate found, without yet resolving conflicts – a
* @param array<string, string> $kinds * "group" component (a composite of several sub-kinds) never contributes a candidate.
*
* @param array<array-key, mixed> $node
* @param array<string, list<string>> $candidates
*/ */
private static function collectKinds(array $node, array &$kinds): void private static function collectKinds(array $node, array &$candidates): void
{ {
$attrName = $node['attrName'] ?? null; $attrName = $node['attrName'] ?? null;
$componentName = $node['component']['name'] ?? null; $component = is_array($node['component'] ?? null) ? $node['component'] : [];
$componentName = $component['name'] ?? null;
if ( if (
is_string($attrName) is_string($attrName)
&& preg_match(self::ATTR_NAME_PATTERN, $attrName) && preg_match(self::ATTR_NAME_PATTERN, $attrName)
&& ($component['type'] ?? null) === 'field'
&& is_string($componentName) && is_string($componentName)
&& isset(self::COMPONENT_KINDS[$componentName]) && isset(self::COMPONENT_KINDS[$componentName])
) { ) {
$kinds[$attrName] = self::COMPONENT_KINDS[$componentName]; $candidates[$attrName][] = self::COMPONENT_KINDS[$componentName];
} }
foreach ($node as $value) { foreach ($node as $value) {
if (is_array($value)) { if (is_array($value)) {
self::collectKinds($value, $kinds); self::collectKinds($value, $candidates);
} }
} }
} }
/** /** @param array<string, mixed> $attrs */
* @param array<array-key, mixed> $attrs private static function fromDefaults(array $attrs, array &$paths): void
* @param array<string, bool> $paths
*/
private static function fromDefaults(array $attrs, string $prefix, array &$paths): void
{ {
foreach ($attrs as $key => $value) { foreach (array_keys(AttrMap::flatten($attrs)) as $path) {
if ($prefix === '' && in_array($key, AttrMap::META_KEYS, true)) { $paths[$path] = true;
continue;
}
$path = $prefix === '' ? (string) $key : $prefix . '.' . $key;
if (AttrMap::isBreakpointMap($value)) {
$paths[$path] = true;
} elseif (is_array($value)) {
self::fromDefaults($value, $path, $paths);
}
} }
} }
} }
@@ -119,7 +119,8 @@ final class DiviCatalogAbility implements Ability
if (! is_string($input['module'] ?? null) || $input['module'] === '') { if (! is_string($input['module'] ?? null) || $input['module'] === '') {
$modules = []; $modules = [];
foreach ($catalog->summary() as $module) { 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]; 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); $item = ShortformMap::childType($short);
return [ return [
@@ -46,6 +46,12 @@ final class ModuleCatalog
return str_contains($type, '/') ? $type : 'divi/' . $type; 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 public function has(string $name): bool
{ {
return isset($this->modules[$name]); return isset($this->modules[$name]);
@@ -54,8 +54,10 @@ final class DiviCatalogAbilityTest extends TestCase
{ {
$button = $this->ability()->execute(['module' => 'divi/button'])['module']; $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']); 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 public function testUnknownModuleAndPermission(): void
@@ -136,12 +136,13 @@ final class ModuleCatalogTest extends TestCase
self::assertSame('richtext', $catalog->pathKind('divi/text', 'content.innerContent')); self::assertSame('richtext', $catalog->pathKind('divi/text', 'content.innerContent'));
self::assertSame('text', $catalog->pathKind('divi/heading', 'title.innerContent')); self::assertSame('text', $catalog->pathKind('divi/heading', 'title.innerContent'));
self::assertSame('code', $catalog->pathKind('divi/code', 'content.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')); self::assertSame('text', $catalog->pathKind('divi/button', 'module.meta.adminLabel'));
// Structural/style groups (fonts, backgrounds, …) carry no single scalar component in // 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). // 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', 'button.decoration.background'));
self::assertSame('unknown', $catalog->pathKind('divi/button', 'no.such.path')); 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. // Every kind in the module's kinds() map is a string, keyed by a known attribute path.
$paths = $catalog->paths('divi/heading'); $paths = $catalog->paths('divi/heading');
@@ -150,4 +151,20 @@ final class ModuleCatalogTest extends TestCase
self::assertContains($path, $paths); 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'));
}
} }