fix: Insert entscheidet fail closed auf den komponierten Blöcken statt auf Client-Text; Update warnt bei Wechsel der Schleifen-Art unter unveränderten Kindern (X3)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
2026-10-19 20:58:48 +02:00
parent dadc1f3ec4
commit 5b52e18a31
6 changed files with 197 additions and 64 deletions
@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace LIT\MCP\Integrations\Divi\Compose;

 use LIT\MCP\Integrations\Divi\BlockTree\AttrMap;
+use LIT\MCP\Integrations\Divi\BlockTree\Block;
 use LIT\MCP\Integrations\Divi\BlockTree\NodeIndex;

 defined('ABSPATH') || exit;
@@ -63,6 +64,42 @@ final class LoopContext
        return LoopScope::none();
    }

+    /**
+     * The loop_* options $block and its descendants reference in their (canonical) attrs. Opaque blocks are not
+     * entered; $stopAtLoops leaves out descendants that run a loop of their own (their content has its own scope).
+     * A reference that is no clean whole value but names a loop_ option counts as "loop_?" (fail closed).
+     *
+     * @return list<string>
+     */
+    public static function optionsIn(Block $block, bool $stopAtLoops): array
+    {
+        if ($block->isOpaque()) {
+            return [];
+        }
+
+        $names = [];
+        $attrs = $block->attrs();
+        array_walk_recursive($attrs, static function (mixed $value) use (&$names): void {
+            if (! is_string($value)) {
+                return;
+            }
+            $name = VariableRef::nameOf($value);
+            if ($name === null && VariableRef::contains($value) && str_contains($value, 'loop_')) {
+                $name = 'loop_?';
+            }
+            if ($name !== null && str_starts_with($name, 'loop_')) {
+                $names[] = $name;
+            }
+        });
+        foreach ($block->children() as $child) {
+            if (! ($stopAtLoops && self::enabled($child->attrs()))) {
+                array_push($names, ...self::optionsIn($child, $stopAtLoops));
+            }
+        }
+
+        return array_values(array_unique($names));
+    }
+
    /** The loop of the node $nodeId (path ID) or its nearest ancestor; none() for an unknown ID. */
    public static function of(NodeIndex $index, string $nodeId): LoopScope
    {
@@ -8,7 +8,6 @@ use LIT\MCP\Abilities\Annotations;
 use LIT\MCP\Abilities\Errors;
 use LIT\MCP\Integrations\Divi\BlockTree\Block;
 use LIT\MCP\Integrations\Divi\Compose\LoopContext;
-use LIT\MCP\Integrations\Divi\Compose\VariableRef;
 use LIT\MCP\Integrations\Divi\Release\DiviFeature;
 use WP_Error;

@@ -105,7 +104,7 @@ final class DiviInsertAbility extends DiviWriteAbility

        $scope = self::loopAt($page->index, $position['parent']);
        $composed = $composition->inLoop($scope)->composer()->compose($nodes, $position['parent'], 'nodes', $context);
-        if (self::startsLoop($composed['blocks']) && self::hasLoopContent($nodes)) {
+        if (self::startsLoop($composed['blocks']) && self::hasLoopContent($composed['blocks'])) {
            return Errors::invalidInput(
                'Schleifen-Inhalte unter einer neuen Divi-Schleife gehen nicht in einem Aufruf; nichts wurde '
                    . 'gespeichert.',
@@ -152,20 +151,19 @@ final class DiviInsertAbility extends DiviWriteAbility
    }

    /**
-     * Whether the input names loop content: a {{loop:…}} placeholder or a reference to a loop_* option.
+     * Whether the composed (canonical, what would be saved) blocks reference loop_* options anywhere – the
+     * client's raw text is never looked at: the writers strip control characters and trim values (S35).
     *
-     * @param array<array-key, mixed> $nodes
+     * @param list<Block> $blocks
     */
-    private static function hasLoopContent(array $nodes): bool
+    private static function hasLoopContent(array $blocks): bool
    {
-        $found = false;
-        array_walk_recursive($nodes, static function (mixed $value) use (&$found): void {
-            $found = $found || (is_string($value) && (
-                preg_match('/^\s*\{\{\s*loop\s*:/', $value) === 1
-                || str_starts_with((string) VariableRef::nameOf($value), 'loop_')
-            ));
-        });
+        foreach ($blocks as $block) {
+            if (LoopContext::optionsIn($block, false) !== []) {
+                return true;
+            }
+        }

-        return $found;
+        return false;
    }
 }
@@ -6,10 +6,7 @@ namespace LIT\MCP\Integrations\Divi\Pages;

 use LIT\MCP\Abilities\Annotations;
 use LIT\MCP\Abilities\Errors;
-use LIT\MCP\Integrations\Divi\BlockTree\Block;
 use LIT\MCP\Integrations\Divi\Compose\LoopContext;
-use LIT\MCP\Integrations\Divi\Compose\LoopScope;
-use LIT\MCP\Integrations\Divi\Compose\VariableRef;
 use LIT\MCP\Integrations\Divi\Release\DiviFeature;
 use WP_Error;

@@ -93,57 +90,11 @@ final class DiviMoveAbility extends DiviWriteAbility
        }

        $change = (new DiviChange(sprintf('Knoten %s verschoben', $id)))->record($node, 'moved');
-        $names = self::loopContent($node);
+        $names = LoopContext::enabled($node->attrs()) ? [] : LoopContext::optionsIn($node, true);
        if ($names !== [] && $from->kind() !== $to->kind()) {
            $change->warning(self::loopWarning($names, $from, $to));
        }

        return $change;
    }
-
-    /**
-     * The loop_* options $block and its descendants show in the scope around $block; a block that runs its own
-     * loop keeps its content's scope, read-only blocks are not entered.
-     *
-     * @return list<string>
-     */
-    private static function loopContent(Block $block): array
-    {
-        if ($block->isOpaque() || LoopContext::enabled($block->attrs())) {
-            return [];
-        }
-
-        $names = [];
-        $attrs = $block->attrs();
-        array_walk_recursive($attrs, static function (mixed $value) use (&$names): void {
-            $name = is_string($value) ? VariableRef::nameOf($value) : null;
-            if ($name !== null && str_starts_with($name, 'loop_')) {
-                $names[] = $name;
-            }
-        });
-        foreach ($block->children() as $child) {
-            array_push($names, ...self::loopContent($child));
-        }
-
-        return array_values(array_unique($names));
-    }
-
-    /** @param list<string> $names */
-    private static function loopWarning(array $names, LoopScope $from, LoopScope $to): string
-    {
-        $where = $to->active()
-            ? sprintf(
-                'jetzt in einer Divi-Schleife vom Typ: %s (vorher: %s); passen sie nicht zu dieser Art, zeigt Divi '
-                    . 'sie leer oder mit Daten eines anderen Objekts mit derselben ID',
-                $to->label(),
-                $from->active() ? $from->label() : 'keine Schleife'
-            )
-            : 'jetzt außerhalb jeder Divi-Schleife; Divi zeigt sie dort leer';
-
-        return sprintf(
-            'Schleifen-Inhalte (%s) liegen %s. Zurückverschieben oder die Inhalte anpassen.',
-            implode(', ', $names),
-            $where
-        );
-    }
 }
@@ -10,6 +10,7 @@ use LIT\MCP\Integrations\Divi\BlockTree\Block;
 use LIT\MCP\Integrations\Divi\BlockTree\NodeIndex;
 use LIT\MCP\Integrations\Divi\Compose\ColumnWidths;
 use LIT\MCP\Integrations\Divi\Compose\Composer;
+use LIT\MCP\Integrations\Divi\Compose\LoopContext;
 use LIT\MCP\Integrations\Divi\Compose\LoopScope;
 use LIT\MCP\Integrations\Divi\Design\PresetAssignment;
 use LIT\MCP\Integrations\Divi\Release\DiviFeature;
@@ -191,11 +192,23 @@ final class DiviUpdateAbility extends DiviWriteAbility
            static fn (array $update): bool => $update[1] !== $update[0]->attrs()
        ));
        $change = new DiviChange(sprintf('%d Divi-Knoten geändert', count($updates)));
+        $before = [];
+        foreach ($updates as [$block]) {
+            $before[spl_object_id($block)] = self::loopAt($page->index, $block);
+        }
        foreach ($updates as [$block, $attrs]) {
            // Composer::update() output: every client value already passed the choke point.
            $block->setAttrs($attrs);
            $change->record($block, 'updated');
        }
+        foreach ($updates as [$block]) {
+            $from = $before[spl_object_id($block)];
+            $to = self::loopAt($page->index, $block);
+            $names = $from->kind() === $to->kind() ? [] : LoopContext::optionsIn($block, true);
+            if ($names !== []) {
+                $change->warning(sprintf('%s: %s', $page->index->idOf($block), self::loopWarning($names, $from, $to)));
+            }
+        }

        return $change;
    }
@@ -136,4 +136,23 @@ abstract class DiviWriteAbility implements Ability, RequiresDiviFeatures

        return LoopContext::fromChain($chain);
    }
+
+    /** @param list<string> $names */
+    protected static function loopWarning(array $names, LoopScope $from, LoopScope $to): string
+    {
+        $where = $to->active()
+            ? sprintf(
+                'jetzt in einer Divi-Schleife vom Typ: %s (vorher: %s); passen sie nicht zu dieser Art, zeigt Divi '
+                    . 'sie leer oder mit Daten eines anderen Objekts mit derselben ID',
+                $to->label(),
+                $from->active() ? $from->label() : 'keine Schleife'
+            )
+            : 'jetzt außerhalb jeder Divi-Schleife; Divi zeigt sie dort leer';
+
+        return sprintf(
+            'Schleifen-Inhalte (%s) liegen %s. Zurückverschieben oder die Inhalte anpassen.',
+            implode(', ', $names),
+            $where
+        );
+    }
 }
@@ -163,7 +163,12 @@ final class DiviLoopWriteTest extends DiviWriteAbilityTestCase
            $refused = $this->insertInto($parent, ['nodes' => [
                ['type' => 'text', 'attrs' => $ownLoop, 'content' => $content],
            ]]);
-            self::assertStringContainsString('erst nach dem Speichern', self::problems($refused), $parent);
+            // c2 has no loop: the composer refuses the loop content itself (the call is refused either way).
+            self::assertStringContainsString(
+                $parent === 's0.r0.c0' ? 'erst nach dem Speichern' : 'braucht eine Divi-Schleife',
+                self::problems($refused),
+                $parent
+            );
        }
        self::assertSame([], $this->saved);

@@ -258,4 +263,114 @@ final class DiviLoopWriteTest extends DiviWriteAbilityTestCase
            self::assertStringNotContainsString('Schleife', implode(' | ', $result['warnings']));
        }
    }
+
+    /**
+     * Fix round 1 (critical): the fail-closed decision reads the composed blocks, not the client's text. The writers
+     * strip control characters and trim values (S35), so these forms become canonical loop content.
+     *
+     * @return array<string, array{0: string}>
+     */
+    public static function obfuscatedLoopContent(): array
+    {
+        $ref = VariableRef::serialize('content', 'loop_post_title', ['before' => '', 'after' => '']);
+
+        return [
+            'control char inside the placeholder' => ["{\x01{loop:post_title}}"],
+            'leading space before the full form' => [' ' . $ref],
+            'leading control char before the full form' => ["\x01" . $ref],
+            'plain placeholder' => ['{{loop:post_title}}'],
+        ];
+    }
+
+    /** @dataProvider obfuscatedLoopContent */
+    public function testANewLoopWithObfuscatedLoopContentIsRefused(string $content): void
+    {
+        $refused = $this->insertInto('s0.r0.c0', ['nodes' => [[
+            'type' => 'text',
+            'attrs' => ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]],
+            'content' => $content,
+        ]]]);
+
+        // Whichever check catches it (fail closed on the composed blocks, or the writer's own D9 refusal): refused.
+        self::assertNotSame('', self::problems($refused));
+        self::assertSame([], $this->saved);
+    }
+
+    /** Loop content through attrs and in a sibling of the loop node: the whole call fails closed (ruling X3). */
+    public function testLoopContentViaAttrsOrInASiblingOfANewLoopIsRefused(): void
+    {
+        $ownLoop = ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]];
+        $ref = VariableRef::serialize('content', 'loop_post_title', ['before' => '', 'after' => '']);
+
+        $viaAttrs = $this->insertInto('s0.r0.c0', ['nodes' => [[
+            'type' => 'text',
+            'attrs' => $ownLoop + ['content.innerContent' => ['desktop' => $ref]],
+        ]]]);
+        self::assertStringContainsString('erst nach dem Speichern', self::problems($viaAttrs));
+
+        $sibling = $this->insertInto('s0.r0.c0', ['nodes' => [
+            ['type' => 'text', 'attrs' => $ownLoop, 'content' => '<p>Liste</p>'],
+            ['type' => 'text', 'content' => "{\x01{loop:post_title}}"],
+        ]]);
+        self::assertStringContainsString('erst nach dem Speichern', self::problems($sibling));
+        self::assertSame([], $this->saved);
+    }
+
+    /** Loop content in a descendant of the new loop node (a section with a module) is refused, nothing saved. */
+    public function testLoopContentBelowANewLoopContainerIsRefused(): void
+    {
+        $refused = $this->insert()->execute([
+            'post_id' => 7,
+            'tree_rev' => $this->currentRev(),
+            'nodes' => [[
+                'type' => 'section',
+                'attrs' => ['module.advanced.loop' => ['desktop' => self::POST_LOOP]],
+                'modules' => [['type' => 'text', 'content' => "{\x01{loop:post_title}}"]],
+            ]],
+        ]);
+
+        self::assertInstanceOf(WP_Error::class, $refused);
+        self::assertSame([], $this->saved);
+    }
+
+    /** The update path decides on the composed attrs too: a control-char placeholder gets the scope of its node. */
+    public function testUpdateChecksObfuscatedLoopContentInTheNodesScope(): void
+    {
+        $ok = $this->changeAll([['id' => 's0.r0.c0.m1', 'set' => ['content' => "{\x01{loop:post_date}}"]]]);
+        self::assertTrue($ok['ok'], (string) json_encode($ok));
+
+        $refused = $this->changeAll([['id' => 's0.r0.c2.m0', 'set' => ['content' => "{\x01{loop:post_date}}"]]]);
+        self::assertStringContainsString('braucht eine Divi-Schleife', self::problems($refused));
+    }
+
+    /** Fix round 1 (important): a loop whose kind changes under unchanged loop_* children warns, as move does. */
+    public function testUpdateWarnsWhenTheLoopKindChangesUnderUnchangedChildren(): void
+    {
+        $result = $this->changeAll([
+            ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]]],
+        ]);
+
+        self::assertTrue($result['ok'], (string) json_encode($result));
+        $warnings = implode(' | ', $result['warnings']);
+        self::assertStringContainsString('s0.r0.c0:', $warnings);
+        self::assertStringContainsString('loop_post_title', $warnings);
+        self::assertStringContainsString('Typ: Terme (vorher: Beiträge)', $warnings);
+        self::assertStringNotContainsString('loop_term_name', $warnings, 'a descendant with its own loop keeps it');
+    }
+
+    public function testUpdateSwitchingTheLoopOffWarnsAndAKeptKindDoesNot(): void
+    {
+        $same = $this->changeAll([
+            ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => [
+                'enable' => 'on', 'queryType' => 'current_page',
+            ]]]],
+        ]);
+        self::assertTrue($same['ok'], (string) json_encode($same));
+        self::assertSame([], $same['warnings']);
+
+        $off = $this->changeAll([
+            ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => ['enable' => 'off']]]],
+        ]);
+        self::assertStringContainsString('außerhalb jeder Divi-Schleife', implode(' | ', $off['warnings']));
+    }
 }