From 62f8465e60daae73eb3bb955a6a10e23daa1d606 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Mon, 19 Oct 2026 14:19:41 +0200 Subject: [PATCH] fix: Variablen-Detach meldet verbleibende eingebettete Verweise und lehnt Werte mit Verweis, Markup oder leerem Inhalt ab Co-Authored-By: Claude Sonnet 5.5 --- src/Integrations/Divi/Design/DetachTarget.php | 7 +- .../Divi/Design/DiviDesignDetachAbility.php | 54 +++++++++++-- .../Divi/Design/ReportsLeftovers.php | 19 +++++ src/Integrations/Divi/Design/TreeRewrite.php | 19 +++-- .../Divi/Design/VariableDetachRewrite.php | 74 ++++++++++++++++-- .../Design/DiviDesignDetachAbilityTest.php | 76 ++++++++++++++++++- .../Divi/Design/PageRewriterTest.php | 29 +++++++ .../Divi/Design/TreeRewriteTest.php | 19 +++++ .../Divi/Design/VariableDetachRewriteTest.php | 41 +++++++--- 9 files changed, 303 insertions(+), 35 deletions(-) create mode 100644 src/Integrations/Divi/Design/ReportsLeftovers.php diff --git a/src/Integrations/Divi/Design/DetachTarget.php b/src/Integrations/Divi/Design/DetachTarget.php index 22c9b26..fa1899f 100644 --- a/src/Integrations/Divi/Design/DetachTarget.php +++ b/src/Integrations/Divi/Design/DetachTarget.php @@ -24,6 +24,8 @@ final class DetachTarget * @param string $nestedIn Where the preset step removes references (messages). * @param list $notes Warnings of every answer. * @param string $doneHint The next step once nothing references the target. + * @param bool $complete False when references stay that this call cannot + * remove (embedded ones in presets): no done hint. */ public function __construct( public readonly string $subject, @@ -33,7 +35,8 @@ final class DetachTarget public readonly ?Closure $presets, public readonly string $nestedIn, public readonly array $notes, - public readonly string $doneHint + public readonly string $doneHint, + public readonly bool $complete = true ) { } @@ -50,6 +53,6 @@ final class DetachTarget */ public function finished(array $skipped, int $hidden): bool { - return $this->postIds === null && $skipped === [] && $hidden === 0; + return $this->complete && $this->postIds === null && $skipped === [] && $hidden === 0; } } diff --git a/src/Integrations/Divi/Design/DiviDesignDetachAbility.php b/src/Integrations/Divi/Design/DiviDesignDetachAbility.php index f4075d6..c5f970a 100644 --- a/src/Integrations/Divi/Design/DiviDesignDetachAbility.php +++ b/src/Integrations/Divi/Design/DiviDesignDetachAbility.php @@ -235,20 +235,58 @@ final class DiviDesignDetachAbility implements Ability, RequiresDiviFeatures 'Verweise auf diese Variable im Divi-Builder lösen.' ); } - $nested = $postIds === null && PresetBook::fromDivi($this->writer->presets())->referencing($id) !== []; + $refusal = $this->unsafeValue($value, $label, $type); + if ($refusal !== null) { + return $refusal; + } + $presets = PresetBook::fromDivi($this->writer->presets()); + $nested = $postIds === null && $presets->referencing($id) !== []; $affixes = $type === 'strings'; + $notes = []; + if ($nested) { + $left = $presets->withVariableResolved($id, $value, $affixes)->referencing($id); + if ($left !== []) { + $notes[] = sprintf( + 'In %d Presets steht die Variable eingebettet in einem längeren Text (%s); diese Verweise bleiben ' + . 'unverändert – im Divi-Builder prüfen.', + count($left), + implode(', ', array_column($left, 'name')) + ); + } + } return new DetachTarget( sprintf('Variable „%s“', $label), $postIds, new ReferenceUsageQuery([$id]), - static fn (): NodeRewrite => new VariableDetachRewrite($id, $value, $type), + fn (): NodeRewrite => new VariableDetachRewrite($id, $value, $type, ($this->unfiltered)()), $nested ? static fn (PresetBook $presets): PresetBook => $presets->withVariableResolved($id, $value, $affixes) : null, 'Presets', - [], - self::ARCHIVE_HINT + $notes, + self::ARCHIVE_HINT, + $notes === [] + ); + } + + /** + * Refuses a value that must not be inlined into content (S1, D7): empty (it would blank every reference), a + * reference marker, or markup without unfiltered_html. DiviSaver writes without kses, so nothing else checks. + */ + private function unsafeValue(string $value, string $label, string $type): ?WP_Error + { + $problem = match (true) { + $value === '' => 'ist leer', + VariableDetachRewrite::unsafe($value, true) => 'enthält einen Verweis', + VariableDetachRewrite::unsafe($value, ($this->unfiltered)()) => 'enthält Markup (< oder >), das dieses ' + . 'Konto nicht speichern darf (unfiltered_html)', + default => null, + }; + + return $problem === null ? null : Errors::unsupported( + sprintf('Der Wert der Variablen „%s“ (Typ %s) %s; nichts wurde geändert.', $label, $type, $problem), + 'Verweise auf diese Variable im Divi-Builder lösen.' ); } @@ -328,7 +366,13 @@ final class DiviDesignDetachAbility implements Ability, RequiresDiviFeatures } if ($presets === [] && PageRewriteReport::isEmpty($preview)) { - $summary = sprintf('Keine Verweise auf %s gefunden – nichts zu tun.', $target->subject); + $summary = $preview['skipped'] === [] && $target->complete + ? sprintf('Keine Verweise auf %s gefunden – nichts zu tun.', $target->subject) + : sprintf( + 'Verweise auf %s lassen sich nicht automatisch lösen und bleiben unverändert (siehe skipped und ' + . 'warnings) – nichts zu tun.', + $target->subject + ); $done = $target->finished($preview['skipped'], $preview['hidden_count']); return PageRewriteReport::nothingToDo( diff --git a/src/Integrations/Divi/Design/ReportsLeftovers.php b/src/Integrations/Divi/Design/ReportsLeftovers.php new file mode 100644 index 0000000..f68ae1a --- /dev/null +++ b/src/Integrations/Divi/Design/ReportsLeftovers.php @@ -0,0 +1,19 @@ +rewrite->rewrite($block); - if ($attrs === null) { - continue; + if ($attrs !== null) { + $block->setAttrs($attrs); + $changed[] = ['id' => $id, 'module' => $block->name(), 'label' => NodeSummary::label($block)]; + array_push($warnings, ...$this->rewrite->notes($block, $id)); + } + $leftover = $this->rewrite instanceof ReportsLeftovers ? $this->rewrite->leftover($block) : null; + if ($leftover !== null) { + $skipped[] = ['id' => $id, 'reason' => $leftover]; } - - $block->setAttrs($attrs); - $changed[] = ['id' => $id, 'module' => $block->name(), 'label' => NodeSummary::label($block)]; - array_push($warnings, ...$this->rewrite->notes($block, $id)); } return ['changed' => $changed, 'warnings' => $warnings, 'skipped' => $skipped]; diff --git a/src/Integrations/Divi/Design/VariableDetachRewrite.php b/src/Integrations/Divi/Design/VariableDetachRewrite.php index 9b988c0..44c49a9 100644 --- a/src/Integrations/Divi/Design/VariableDetachRewrite.php +++ b/src/Integrations/Divi/Design/VariableDetachRewrite.php @@ -17,13 +17,31 @@ defined('ABSPATH') || exit; * change the text) and are named in notes(). Other references – colors, other variables, dynamic content – stay * untouched. Pure PHP. */ -final class VariableDetachRewrite implements NodeRewrite +final class VariableDetachRewrite implements NodeRewrite, ReportsLeftovers { - private const EMBEDDED = '%s: Die Variable steht hier eingebettet in einem längeren Text; dieser Verweis bleibt ' - . 'stehen – im Divi-Builder ersetzen.'; + private const EMBEDDED = 'Die Variable steht hier eingebettet in einem längeren Text; dieser Verweis bleibt ' + . 'unverändert – im Divi-Builder prüfen.'; - public function __construct(private string $id, private string $value, private string $variableType) + private const UNSAFE = 'Vor- oder Nachtext des Verweises enthält Markup oder einen Verweis; der Verweis bleibt ' + . 'unverändert – im Divi-Builder prüfen.'; + + /** @param bool $markupAllowed Whether "<" and ">" may enter the content (unfiltered_html); markers never may. */ + public function __construct( + private string $id, + private string $value, + private string $variableType, + private bool $markupAllowed = false + ) { + } + + /** + * Whether $text must not be inlined into content: it holds a reference marker (it never went through + * VariableRef::canonical()) or – without unfiltered_html – markup (D7, S1). + */ + public static function unsafe(string $text, bool $markupAllowed): bool { + return str_contains($text, VariableRef::MARKER) || str_contains($text, VariableRef::END) + || (! $markupAllowed && (str_contains($text, '<') || str_contains($text, '>'))); } /** @@ -44,8 +62,8 @@ final class VariableDetachRewrite implements NodeRewrite public function rewrite(Block $block): ?array { - if (! $block->isDivi()) { - return null; + if (! $block->isDivi() || $this->hasUnsafeAffix($block->attrs())) { + return null; // fail closed: the block stays whole and leftover() reports it } $changed = false; @@ -56,7 +74,19 @@ final class VariableDetachRewrite implements NodeRewrite public function notes(Block $block, string $nodeId): array { - return $this->embeds($block->attrs()) ? [sprintf(self::EMBEDDED, $nodeId)] : []; + return []; // leftovers go through leftover(): they also matter for blocks rewrite() did not change + } + + public function leftover(Block $block): ?string + { + if (! $block->isDivi()) { + return null; + } + if ($this->hasUnsafeAffix($block->attrs())) { + return self::UNSAFE; + } + + return $this->embeds($block->attrs()) ? self::EMBEDDED : null; } public function concernsInstance(Block $instance): bool @@ -86,6 +116,36 @@ final class VariableDetachRewrite implements NodeRewrite return $value; } + /** Whether a whole reference below $value carries before/after text that must not be inlined. */ + private function hasUnsafeAffix(mixed $value): bool + { + if (is_string($value)) { + if ($this->variableType !== 'strings' || VariableRef::nameOf($value) !== $this->id) { + return false; + } + $settings = VariableRef::settingsOf($value); + + foreach (['before', 'after'] as $key) { + $text = is_string($settings[$key] ?? null) ? $settings[$key] : ''; + if (self::unsafe($text, $this->markupAllowed)) { + return true; + } + } + + return false; + } + if (! is_array($value)) { + return false; + } + foreach ($value as $item) { + if ($this->hasUnsafeAffix($item)) { + return true; + } + } + + return false; + } + /** Whether a string below $value names the variable inside a longer text. */ private function embeds(mixed $value): bool { diff --git a/tests/Unit/Integrations/Divi/Design/DiviDesignDetachAbilityTest.php b/tests/Unit/Integrations/Divi/Design/DiviDesignDetachAbilityTest.php index 0cb23bd..8cbe831 100644 --- a/tests/Unit/Integrations/Divi/Design/DiviDesignDetachAbilityTest.php +++ b/tests/Unit/Integrations/Divi/Design/DiviDesignDetachAbilityTest.php @@ -88,10 +88,10 @@ final class DiviDesignDetachAbilityTest extends TestCase } /** Reads variables() (the Variablen-Verwaltung is allowed unless $manage is false), never saves. */ - private static function variableAdapter(bool $manage = true): GlobalVariablesAdapter&MockInterface + private static function variableAdapter(bool $manage = true, string $radius = '8px'): GlobalVariablesAdapter&MockInterface { $variables = Mockery::mock(GlobalVariablesAdapter::class); - $variables->allows('read')->andReturn(self::variables()); + $variables->allows('read')->andReturn(self::variables($radius)); $variables->allows('canManage')->andReturn($manage); $variables->expects('save')->never(); @@ -99,11 +99,11 @@ final class DiviDesignDetachAbilityTest extends TestCase } /** @return array>> Global variables in Divi's stored form. */ - private static function variables(): array + private static function variables(string $radius = '8px'): array { return [ 'numbers' => [self::RADIUS => [ - 'id' => self::RADIUS, 'label' => 'Eckenradius', 'value' => '8px', 'order' => '1', + 'id' => self::RADIUS, 'label' => 'Eckenradius', 'value' => $radius, 'order' => '1', 'status' => 'active', 'lastUpdated' => '2026-10-01T10:00:00.000Z', 'variableType' => 'numbers', ]], // Archived or not does not matter for a detach (V9). @@ -294,6 +294,74 @@ final class DiviDesignDetachAbilityTest extends TestCase ); } + /** @return array */ + public static function unsafeValues(): array + { + return [ + 'empty' => ['', true], + 'marker' => ['$variable({"type":"content"})$', true], + 'end marker' => ['x)$', true], + 'markup without unfiltered_html' => ['8px', false], + ]; + } + + /** @dataProvider unsafeValues */ + public function testAValueThatMustNotBeInlinedRefusesTheCall(string $value, bool $unfiltered): void + { + $ability = $this->ability( + self::untouched(), + null, + null, + null, + null, + $unfiltered, + self::variableAdapter(true, $value) + ); + + $error = self::assertToolError($ability->execute(['variable' => 'Eckenradius']), 'unsupported'); + + self::assertStringContainsString('Divi-Builder', $error['hint']); + } + + public function testMarkupInTheValueIsAllowedWithUnfilteredHtml(): void + { + $pages = Mockery::mock(PageRewriter::class); + $pages->expects('preview')->andReturn(PageRewriteReportTest::preview(1)); + $ability = $this->ability($pages, null, null, null, null, true, self::variableAdapter(true, '8px')); + + self::assertToolError($ability->execute(['variable' => 'Eckenradius']), 'confirm_required'); + } + + public function testReferencesLeftOnPagesNeverGetTheDoneHint(): void + { + $preview = PageRewriteReportTest::preview(0); + $preview['skipped'] = [['post_id' => 12, 'reason' => 's0.r0.c0.m0: Die Variable steht hier eingebettet']]; + $pages = Mockery::mock(PageRewriter::class); + $pages->expects('preview')->andReturn($preview); + + $result = $this->ability($pages)->execute(['variable' => 'Eckenradius']); + + self::assertStringNotContainsString('lit/divi-variables-set', $result['summary']); + self::assertStringNotContainsString('Keine Verweise', $result['summary']); + self::assertSame($preview['skipped'], $result['skipped']); + } + + public function testEmbeddedReferencesInPresetsAreReportedAndWithholdTheDoneHint(): void + { + $stored = PresetBookTest::stored(); + $stored['module']['divi/button']['items']['aaaaaaaaaa']['attrs'] = ['button' => ['decoration' => [ + 'border' => ['desktop' => ['value' => ['radius' => 'Ab ' . VariableDetachRewriteTest::ref(self::RADIUS) . ' x']]], + ]]]; + $pages = Mockery::mock(PageRewriter::class); + $pages->expects('preview')->andReturn(PageRewriteReportTest::preview(0)); + + $result = $this->ability($pages, self::readOnlyWriter($stored))->execute(['variable' => 'Eckenradius']); + + self::assertStringNotContainsString('lit/divi-variables-set', $result['summary']); + self::assertStringContainsString('In 1 Presets steht die Variable eingebettet', $result['warnings'][0]); + self::assertStringContainsString('CTA primär', $result['warnings'][0]); + } + public function testVariablePostIdsSkipThePresetStep(): void { $pages = Mockery::mock(PageRewriter::class); diff --git a/tests/Unit/Integrations/Divi/Design/PageRewriterTest.php b/tests/Unit/Integrations/Divi/Design/PageRewriterTest.php index 1326eba..1075b97 100644 --- a/tests/Unit/Integrations/Divi/Design/PageRewriterTest.php +++ b/tests/Unit/Integrations/Divi/Design/PageRewriterTest.php @@ -9,6 +9,8 @@ use Closure; use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\BlockTree\GlobalInstance; +use LIT\MCP\Integrations\Divi\BlockTree\Json; +use LIT\MCP\Integrations\Divi\Design\VariableDetachRewrite; use LIT\MCP\Integrations\Divi\Design\NodeRewrite; use LIT\MCP\Integrations\Divi\Design\PageRewriter; use LIT\MCP\Integrations\Divi\Library\GlobalUsages; @@ -368,6 +370,33 @@ final class PageRewriterTest extends TestCase self::assertSame(['102'], array_column($this->log->records, 'object_id')); } + /** V9: a page that only mentions the variable inside a longer text is not saved, but reported as skipped. */ + public function testVariableReferencesLeftBehindAreSkippedEvenOnAPageThatIsNotSaved(): void + { + $embedded = ''; + $whole = ''; + $rewriter = $this->rewriter([ + self::page(101, self::layout($embedded)), + self::page(102, self::layout($whole, $embedded)), + ]); + $rewrite = static fn (): NodeRewrite => new VariableDetachRewrite('gvid-radius0001', '8px', 'numbers'); + + $preview = $rewriter->preview(self::query(), $rewrite, null); + $result = $rewriter->apply(self::ABILITY, self::query(), $rewrite, null, self::SUMMARY); + + self::assertSame(1, $preview['pages_count']); + self::assertSame( + [101, 102], + array_column($preview['skipped'], 'post_id') + ); + self::assertStringStartsWith('s0.r0.c0.m0: Die Variable steht hier eingebettet', $preview['skipped'][0]['reason']); + self::assertStringStartsWith('s0.r0.c0.m1: ', $preview['skipped'][1]['reason']); + self::assertSame([102], array_column($result['saved'], 'post_id')); + self::assertSame([101, 102], array_column($result['skipped'], 'post_id')); + } + /** R8: pages loadWritable() refuses (Divi 4, unparsable – fail-closed candidates) are skipped, not remaining. */ public function testRefusedPagesAreSkippedWithTheLoaderMessage(): void { diff --git a/tests/Unit/Integrations/Divi/Design/TreeRewriteTest.php b/tests/Unit/Integrations/Divi/Design/TreeRewriteTest.php index e2e1809..5469cfb 100644 --- a/tests/Unit/Integrations/Divi/Design/TreeRewriteTest.php +++ b/tests/Unit/Integrations/Divi/Design/TreeRewriteTest.php @@ -12,6 +12,7 @@ use LIT\MCP\Integrations\Divi\BlockTree\Parser; use LIT\MCP\Integrations\Divi\BlockTree\Serializer; use LIT\MCP\Integrations\Divi\Design\NodeRewrite; use LIT\MCP\Integrations\Divi\Design\TreeRewrite; +use LIT\MCP\Integrations\Divi\Design\VariableDetachRewrite; use LIT\MCP\Tests\Unit\TestCase; final class TreeRewriteTest extends TestCase @@ -134,4 +135,22 @@ final class TreeRewriteTest extends TestCase $result['skipped'] ); } + + public function testAReportsLeftoversStrategyListsUnchangedBlocksAsSkipped(): void + { + $embedded = 'Ab ' . VariableDetachRewriteTest::ref('gvid-radius0001') . ' Rundung'; + $text = static fn (string $value): string => ''; + $document = self::document( + $text($embedded) . $text(VariableDetachRewriteTest::ref('gvid-radius0001')) . '' + ); + + $result = (new TreeRewrite(new VariableDetachRewrite('gvid-radius0001', '8px', 'numbers'))) + ->apply(NodeIndex::build($document)); + + self::assertSame(['s0.r0.c0.m1'], array_column($result['changed'], 'id')); + self::assertSame(['s0.r0.c0.m0'], array_column($result['skipped'], 'id'), 'embedded only: reported'); + self::assertSame([], $result['warnings']); + } } diff --git a/tests/Unit/Integrations/Divi/Design/VariableDetachRewriteTest.php b/tests/Unit/Integrations/Divi/Design/VariableDetachRewriteTest.php index 5ed55e9..793d0cf 100644 --- a/tests/Unit/Integrations/Divi/Design/VariableDetachRewriteTest.php +++ b/tests/Unit/Integrations/Divi/Design/VariableDetachRewriteTest.php @@ -148,19 +148,42 @@ final class VariableDetachRewriteTest extends TestCase $attrs = $rewrite->rewrite($first); self::assertSame($embedded, $attrs['content']['innerContent']['desktop']['value'] ?? null); $first->setAttrs((array) $attrs); - self::assertSame( - ['s0.r0.c0.m0: Die Variable steht hier eingebettet in einem längeren Text; dieser Verweis bleibt stehen – ' - . 'im Divi-Builder ersetzen.'], - $rewrite->notes($first, 's0.r0.c0.m0') - ); + self::assertStringContainsString('eingebettet', (string) $rewrite->leftover($first)); + self::assertStringContainsString('bleibt unverändert', (string) $rewrite->leftover($first)); + self::assertNotNull($rewrite->leftover(self::node($index, 'm1')), 'embedded only: still reported'); self::assertNull($rewrite->rewrite(self::node($index, 'm1')), 'only embedded: nothing to replace'); - self::assertSame( - [], - (new VariableDetachRewrite(self::CLAIM, 'x', 'strings'))->notes($first, 's0.r0.c0.m0'), - 'another variable is not noted' + self::assertNull( + (new VariableDetachRewrite(self::CLAIM, 'x', 'strings'))->leftover($first), + 'another variable is not reported' ); } + public function testUnsafeBeforeAndAfterLeaveTheBlockWholeAndReportIt(): void + { + $index = self::index([ + ['divi/text', ['content' => ['innerContent' => self::desktop(self::ref(self::CLAIM, ['before' => '']))]]], + ['divi/text', ['content' => ['innerContent' => self::desktop( + // Built by hand: VariableRef::serialize() itself refuses a marker in the settings. + '$variable(' . Json::encode(['type' => 'content', 'value' => [ + 'name' => self::CLAIM, 'settings' => ['after' => 'a $variable(b'] + ]]) . ')$' + )]]], + ['divi/text', ['content' => ['innerContent' => self::desktop(self::ref(self::CLAIM, ['before' => '']))]]], + ]); + $strict = new VariableDetachRewrite(self::CLAIM, 'x', 'strings'); + $allowed = new VariableDetachRewrite(self::CLAIM, 'x', 'strings', true); + + self::assertNull($strict->rewrite(self::node($index, 'm0'))); + self::assertStringContainsString('Markup', (string) $strict->leftover(self::node($index, 'm0'))); + self::assertNull($allowed->rewrite(self::node($index, 'm1')), 'a marker is never inlined'); + self::assertNotNull($allowed->leftover(self::node($index, 'm1'))); + $third = self::node($index, 'm2'); + $attrs = (array) $allowed->rewrite($third); + self::assertSame('x', $attrs['content']['innerContent']['desktop']['value'] ?? null); + $third->setAttrs($attrs); + self::assertNull($allowed->leftover($third)); + } + public function testInstancesThatMentionTheIdConcernTheRewrite(): void { $index = self::index([