fix: Task 6 review round 2 - builder column classes, sum check only for exact rows, unknown path wins, column structure text (S43)

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

src/Integrations/Divi/Compose/AttrWriter.php

@@ -77,8 +77,10 @@ final class AttrWriter
             $path = (string) $path;
             $key = $field . '.' . $path;
             if ($this->isStructure($module, $path)) {
-                $errors[$key] = 'Struktur des Abschnitts bzw. der Spalte; nur im Divi-Builder änderbar '
+                $errors[$key] = $module === 'divi/section'
-                    . '(Abschnittsart beim Einfügen über section_type)';
+                    ? 'Struktur des Abschnitts; nur im Divi-Builder änderbar (Abschnittsart beim Einfügen über '
+                        . 'section_type)'
+                    : 'Spezialspalten sind Struktur; nur im Divi-Builder änderbar';
                 continue;
             }
 

src/Integrations/Divi/Compose/Composer.php

@@ -48,6 +48,12 @@ final class Composer
 
     private const DEFAULT_LEVEL = 'h2';
 
+    /**
+     * The builder's "Column Class" choices (Divi 5.2.1 visual-builder/build/field-library.js):
+     * n_24, the special fractions and "none" (no width class). Ruling S43.
+     */
+    private const COLUMN_CLASSES = ['1_5', '2_5', '3_5', '1_7', '1_9', '1_10', '1_11', 'none'];
+
     /** Advice of the S40 overlap error for new nodes. */
     private const ONE_FORM = 'für diesen Pfad nur Kurzform oder attrs angeben';
 
@@ -467,6 +473,13 @@ final class Composer
         }
 
         $result = $this->attrWriter->apply($name, $attrs, $spec['attrs'], $path . '.attrs');
+        // An unknown path is the more useful message on its key (it decides invalid_attr_path).
+        foreach ($result['invalid_paths'] as $invalid) {
+            $key = $path . '.attrs.' . $invalid;
+            if (isset($result['errors'][$key])) {
+                $this->errors[$key] = $result['errors'][$key];
+            }
+        }
         $this->errors += $result['errors'];
         array_push($this->invalidPaths, ...$result['invalid_paths']);
 
@@ -658,6 +671,17 @@ final class Composer
         return $error;
     }
 
+    /** Ruling S43: whether $width is a column class the Visual Builder offers. */
+    public static function isColumnClass(mixed $width): bool
+    {
+        if (! is_string($width)) {
+            return false;
+        }
+
+        return in_array($width, self::COLUMN_CLASSES, true)
+            || (preg_match('/^([1-9]|1\d|2[0-4])_24$/D', $width) === 1);
+    }
+
     /**
      * Whether the fractions add up to exactly 1: numerators scaled to the least common
      * denominator (denominators are divisors of 24 or 5, 7, 9, 10, 11 – at most 27720).

src/Integrations/Divi/Pages/DiviUpdateAbility.php

@@ -24,6 +24,9 @@ final class DiviUpdateAbility implements Ability
 {
     private const SIZING = 'module.decoration.sizing';
 
+    /** Divi's column default (column/module-default-render-attributes.json). */
+    private const DEFAULT_WIDTH = '24_24';
+
     public function __construct(private DiviWriter $writer)
     {
     }
@@ -182,22 +185,34 @@ final class DiviUpdateAbility implements Ability
     }
 
     /**
-     * Ruling S41: a column whose desktop width changes must leave its row filled exactly – the
+     * Column widths on update (rulings S41, S43). Every new desktop flexType must be one of the
-     * rules of "columns" (Composer::rowWidthsError()), checked over the row after all changes.
+     * builder's column classes (Composer::isColumnClass()) or null (removed: Divi's default
+     * 24_24). The row sum is only checked for rows that are exact today – every column has a
+     * class other than "none", the classes fill the row (Composer::rowWidthsError()), the row
+     * does not wrap or use a grid on desktop – and when no new width is "none". Such a row
+     * must stay exact after all changes; other rows are laid out by Divi as the builder allows.
      *
      * @param list<array{0: Block, 1: array<string, mixed>, 2: string}> $updates
      * @return array<string, string>
      */
     private function rowWidths(DiviPage $page, Composer $composer, array $updates): array
     {
+        $errors = [];
         $new = [];
         $rows = [];
         foreach ($updates as [$block, $attrs, $field]) {
-            if ($block->name() !== 'divi/column' || self::width($attrs) === self::width($block->attrs())) {
+            $width = self::width($attrs);
+            if ($block->name() !== 'divi/column' || $width === self::width($block->attrs())) {
                 continue;
             }
 
-            $new[spl_object_id($block)] = $attrs;
+            if ($width !== null && ! Composer::isColumnClass($width)) {
+                $errors[$field . '.attrs.' . self::SIZING] = 'Spaltenbreite (flexType) wie im Divi-Builder erwartet: '
+                    . '1_24 … 24_24, 1_5, 2_5, 3_5, 1_7, 1_9, 1_10, 1_11 oder none';
+                continue;
+            }
+
+            $new[spl_object_id($block)] = $width ?? self::DEFAULT_WIDTH;
             $row = $page->index->parentOf($block);
             if ($row instanceof Block) {
                 $rows[spl_object_id($row)]['row'] = $row;
@@ -205,14 +220,19 @@ final class DiviUpdateAbility implements Ability
             }
         }
 
-        $errors = [];
         foreach ($rows as ['row' => $row, 'fields' => $fields]) {
-            $widths = [];
+            $before = [];
+            $after = [];
             foreach ($row->children() as $column) {
-                $widths[] = self::width($new[spl_object_id($column)] ?? $column->attrs());
+                $before[] = self::width($column->attrs());
+                $after[] = $new[spl_object_id($column)] ?? self::width($column->attrs());
             }
 
-            $error = $composer->rowWidthsError($widths);
+            if (! self::isExact($row, $before, $composer) || in_array('none', $after, true)) {
+                continue;
+            }
+
+            $error = $composer->rowWidthsError($after);
             if ($error !== null) {
                 foreach ($fields as $field) {
                     $errors[$field . '.attrs.' . self::SIZING] = $error
@@ -224,6 +244,19 @@ final class DiviUpdateAbility implements Ability
         return $errors;
     }
 
+    /** @param list<mixed> $widths Desktop flexTypes of the row's columns today. */
+    private static function isExact(Block $row, array $widths, Composer $composer): bool
+    {
+        $layout = AttrMap::desktop($row->attrs(), 'module.decoration.layout');
+        $wraps = is_array($layout)
+            && (($layout['flexWrap'] ?? null) === 'wrap' || ($layout['display'] ?? null) === 'grid');
+        if ($wraps) {
+            return false;
+        }
+
+        return $composer->rowWidthsError($widths) === null;
+    }
+
     /** @param array<string, mixed> $attrs */
     private static function width(array $attrs): mixed
     {

tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php

@@ -527,6 +527,11 @@ final class AttrWriterTest extends TestCase
             self::assertSame([], $result['attrs'], $path);
         }
 
+        $column = ComposeFixture::attrWriter()->apply('divi/column', [], ['module.advanced.specialtyColumns' => null], 'attrs');
+        self::assertStringNotContainsString('section_type', $column['errors']['attrs.module.advanced.specialtyColumns']);
+        $section = ComposeFixture::attrWriter()->apply('divi/section', [], ['module.advanced.type' => null], 'attrs');
+        self::assertStringContainsString('section_type', $section['errors']['attrs.module.advanced.type']);
+
         $heading = ComposeFixture::attrWriter()->apply('divi/heading', [], ['module.meta.adminLabel' => ['desktop' => 'x']], 'attrs');
         self::assertSame([], $heading['errors'], 'only section type and specialty columns are structure');
     }

tests/Unit/Integrations/Divi/Compose/ComposerTest.php

@@ -261,6 +261,20 @@ final class ComposerTest extends TestCase
         self::assertStringContainsString('label', $update['errors']['changes[0].attrs.module.meta.adminLabel']);
     }
 
+    /** Ruling S43: an unknown path wins over the overlap message on the same key. */
+    public function testUnknownPathWinsOverOverlap(): void
+    {
+        $result = ComposeFixture::composer()->compose([[
+            'type' => 'heading',
+            'text' => 'x',
+            'label' => 'B',
+            'attrs' => ['module.meta' => ['desktop' => 'A']],
+        ]], Block::create('divi/column'), 'nodes');
+
+        self::assertSame(['module.meta'], $result['invalid_paths']);
+        self::assertStringContainsString('unbekannter Attributpfad', $result['errors']['nodes[0].attrs.module.meta']);
+    }
+
     /** Ruling S41: the default heading level survives attrs on the same font value. */
     public function testDefaultHeadingLevelIsMergedIntoAttrs(): void
     {

tests/Unit/Integrations/Divi/Pages/DiviWriteAbilitiesTest.php

@@ -318,6 +318,71 @@ final class DiviWriteAbilitiesTest extends TestCase
         self::assertStringContainsString('"flexType":"12_24"}}}}},"builderVersion":"5.2.1"} --><!-- wp:divi/image', $this->saved[0]);
     }
 
+    /** @param array<string, mixed> $attrs */
+    private function updateOn(string $content, array $changes): mixed
+    {
+        $this->post->post_content = $content;
+
+        return $this->update()->execute(['post_id' => 7, 'tree_rev' => Rev::of($content), 'changes' => $changes]);
+    }
+
+    /** @return array<string, mixed> */
+    private static function sizing(?string $width): array
+    {
+        return ['module.decoration.sizing' => $width === null ? null : ['desktop' => ['flexType' => $width]]];
+    }
+
+    /** Ruling S43: only builder column classes (or null) are accepted as new desktop widths. */
+    public function testUpdateChecksTheColumnClassFormat(): void
+    {
+        foreach (['4_5', '13_25', '12_24,12_24', 'auto'] as $width) {
+            $result = $this->update()->execute(['post_id' => 7, 'tree_rev' => $this->treeRev(), 'changes' => [
+                ['id' => 's0.r0.c0', 'attrs' => self::sizing($width)],
+            ]]);
+            self::assertInstanceOf(WP_Error::class, $result, $width);
+            self::assertStringContainsString('none', self::clientError($result)['fields']['changes[0].attrs.module.decoration.sizing'], $width);
+        }
+
+        self::assertSame([], $this->saved);
+    }
+
+    /** Ruling S43: rows that are no plain exact row are not sum-checked (none, omitted width, wrap). */
+    public function testUpdateAcceptsWidthChangesInLooseRows(): void
+    {
+        $noneRow = str_replace('"flexType":"8_24"', '"flexType":"none"', $this->original);
+        self::assertTrue($this->updateOn($noneRow, [['id' => 's0.r0.c1', 'attrs' => self::sizing('12_24')]])['ok']);
+
+        $omitted = str_replace(
+            '{"module":{"decoration":{"sizing":{"desktop":{"value":{"flexType":"8_24"}}}}},"builderVersion":"5.2.1"}',
+            '{"builderVersion":"5.2.1"}',
+            $this->original
+        );
+        self::assertNotSame($omitted, $this->original);
+        self::assertTrue($this->updateOn($omitted, [['id' => 's0.r0.c1', 'attrs' => self::sizing('12_24')]])['ok']);
+        self::assertTrue($this->updateOn($omitted, [['id' => 's0.r0.c1', 'attrs' => self::sizing(null)]])['ok'], 'removal');
+
+        $wrap = str_replace(
+            '<!-- wp:divi/row {"module":{"meta":',
+            '<!-- wp:divi/row {"module":{"decoration":{"layout":{"desktop":{"value":{"flexWrap":"wrap"}}}},"meta":',
+            $this->original
+        );
+        self::assertNotSame($wrap, $this->original);
+        self::assertTrue($this->updateOn($wrap, [['id' => 's0.r0.c0', 'attrs' => self::sizing('12_24')]])['ok']);
+
+        self::assertTrue($this->updateOn($this->original, [['id' => 's0.r0.c0', 'attrs' => self::sizing('none')]])['ok'], 'none itself');
+        self::assertCount(5, $this->saved);
+    }
+
+    /** Ruling S43: a plain exact row stays strict; removal counts as Divi's default 24_24. */
+    public function testUpdateKeepsExactRowsStrict(): void
+    {
+        $broken = $this->updateOn($this->original, [['id' => 's0.r0.c0', 'attrs' => self::sizing(null)]]);
+        self::assertSame('lit_validation_failed', $broken->get_error_code());
+        $broken = $this->updateOn($this->original, [['id' => 's0.r0.c0', 'attrs' => self::sizing('10_24')]]);
+        self::assertSame('lit_validation_failed', $broken->get_error_code());
+        self::assertSame([], $this->saved);
+    }
+
     /** Ruling S7: builder-only paths are refused for users without unfiltered_html – visible to the client. */
     public function testBuilderOnlyPathsAreRefusedForEditors(): void
     {