From ecfece719da6396f27be54e206e27230b690c6c0 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 18:22:51 +0200 Subject: [PATCH] refactor: DiviWriteAbility base and shared DiviErrors (H4) The five Divi write tools extend DiviWriteAbility (final permission and execute, change() per tool, shared output schema). DiviErrors holds the unknown-node, position and compose-error mapping; DiviContentComposer uses the same mapping (ledger T9). ToolNamingTest skips abstract classes. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- .../Divi/Pages/DiviContentComposer.php | 4 +- .../Divi/Pages/DiviDeleteAbility.php | 38 ++--------- .../Divi/Pages/DiviDuplicateNodeAbility.php | 36 ++--------- src/Integrations/Divi/Pages/DiviErrors.php | 41 ++++++++++++ .../Divi/Pages/DiviInsertAbility.php | 41 ++---------- .../Divi/Pages/DiviMoveAbility.php | 42 ++----------- .../Divi/Pages/DiviNodeGetAbility.php | 2 +- .../Divi/Pages/DiviOutlineAbility.php | 2 +- .../Divi/Pages/DiviPageLoader.php | 9 --- .../Divi/Pages/DiviUpdateAbility.php | 48 ++++---------- .../Divi/Pages/DiviWriteAbility.php | 63 +++++++++++++++++++ tests/Unit/Guides/ToolNamingTest.php | 2 +- 12 files changed, 141 insertions(+), 187 deletions(-) create mode 100644 src/Integrations/Divi/Pages/DiviErrors.php create mode 100644 src/Integrations/Divi/Pages/DiviWriteAbility.php diff --git a/src/Integrations/Divi/Pages/DiviContentComposer.php b/src/Integrations/Divi/Pages/DiviContentComposer.php index fe17976..9136eea 100644 --- a/src/Integrations/Divi/Pages/DiviContentComposer.php +++ b/src/Integrations/Divi/Pages/DiviContentComposer.php @@ -98,9 +98,7 @@ final class DiviContentComposer implements ContentComposer $composed = $composition->composer()->compose(array_values($sections), null, 'divi'); if ($composed['errors'] !== []) { - return $composed['invalid_paths'] !== [] - ? Errors::invalidAttrPath($composed['errors']) - : Errors::invalidInput('Divi-Inhalt ist ungültig; nichts wurde angelegt.', $composed['errors']); + return DiviErrors::composed($composed, 'Divi-Inhalt ist ungültig; nichts wurde angelegt.'); } $document = new Document(); diff --git a/src/Integrations/Divi/Pages/DiviDeleteAbility.php b/src/Integrations/Divi/Pages/DiviDeleteAbility.php index cf69bec..f4395a6 100644 --- a/src/Integrations/Divi/Pages/DiviDeleteAbility.php +++ b/src/Integrations/Divi/Pages/DiviDeleteAbility.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Pages; -use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Errors; -use LIT\MCP\Abilities\ToolResult; use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\BlockTree\NodeSummary; use WP_Error; @@ -17,12 +15,8 @@ use WP_Error; * nodes inside another node that is deleted anyway do not count twice. The revision in the * result allows undoing it (lit/revision-restore). */ -final class DiviDeleteAbility implements Ability +final class DiviDeleteAbility extends DiviWriteAbility { - public function __construct(private DiviWriter $writer) - { - } - public function name(): string { return 'lit/divi-delete'; @@ -59,41 +53,19 @@ final class DiviDeleteAbility implements Ability ]; } - public function outputSchema(): ?array - { - return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); - } - public function annotations(): array { return ['readonly' => false, 'destructive' => true, 'idempotent' => false]; } - public function isDirect(): bool + protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error { - return true; - } - - public function permission(array $input): bool|WP_Error - { - $page = $this->writer->page($input); - - return $page instanceof WP_Error ? $page : true; - } - - public function execute(array $input): mixed - { - $page = $this->writer->page($input); - if ($page instanceof WP_Error) { - return $page; - } - /** @var array $nodes */ $nodes = []; foreach (is_array($input['ids'] ?? null) ? $input['ids'] : [] as $id) { $node = is_string($id) ? $page->index->find($id) : null; if ($node === null) { - return DiviPageLoader::unknownNode((string) $id, $page->post); + return DiviErrors::unknownNode((string) $id, $page->post); } $nodes[$id] = $node; } @@ -115,14 +87,14 @@ final class DiviDeleteAbility implements Ability ); } - $editor = $this->writer->composition($page)->editor(); + $editor = $composition->editor(); $change = new DiviChange(sprintf('%d Divi-Knoten gelöscht', count($nodes))); foreach ($nodes as $id => $node) { $change->removed((string) $id, $node); $editor->remove($page->index, $node); } - return $this->writer->save($this->name(), $page, $change, $input); + return $change; } /** @param array $nodes */ diff --git a/src/Integrations/Divi/Pages/DiviDuplicateNodeAbility.php b/src/Integrations/Divi/Pages/DiviDuplicateNodeAbility.php index 88aa8cc..2c74f5d 100644 --- a/src/Integrations/Divi/Pages/DiviDuplicateNodeAbility.php +++ b/src/Integrations/Divi/Pages/DiviDuplicateNodeAbility.php @@ -4,8 +4,6 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Pages; -use LIT\MCP\Abilities\Ability; -use LIT\MCP\Abilities\ToolResult; use LIT\MCP\Integrations\Divi\BlockTree\AttrMap; use WP_Error; @@ -14,12 +12,8 @@ use WP_Error; * (spec §5.7). The copy's admin label gets " (Kopie)"; read-only nodes (global modules, * foreign blocks) are copied unchanged, as a whole, byte-identical reference. */ -final class DiviDuplicateNodeAbility implements Ability +final class DiviDuplicateNodeAbility extends DiviWriteAbility { - public function __construct(private DiviWriter $writer) - { - } - public function name(): string { return 'lit/divi-duplicate-node'; @@ -48,11 +42,6 @@ final class DiviDuplicateNodeAbility implements Ability ]; } - public function outputSchema(): ?array - { - return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); - } - public function annotations(): array { return ['readonly' => false, 'destructive' => false, 'idempotent' => false]; @@ -63,31 +52,20 @@ final class DiviDuplicateNodeAbility implements Ability return false; } - public function permission(array $input): bool|WP_Error + protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error { - $page = $this->writer->page($input); - - return $page instanceof WP_Error ? $page : true; - } - - public function execute(array $input): mixed - { - $page = $this->writer->page($input); - if ($page instanceof WP_Error) { - return $page; - } - $id = is_string($input['id'] ?? null) ? $input['id'] : ''; $node = $page->index->find($id); if ($node === null) { - return DiviPageLoader::unknownNode($id, $page->post); + return DiviErrors::unknownNode($id, $page->post); } - $copy = $this->writer->composition($page)->editor()->duplicate($page->index, $node); + $copy = $composition->editor()->duplicate($page->index, $node); if ($copy->isDivi() && ! $copy->isOpaque()) { // The raw stored desktop value (never NodeSummary::label()'s display-shortened // text): a long or multi-line label must be copied verbatim, only " (Kopie)" added. // Setting only the ".desktop.value" leaf keeps every other breakpoint untouched. + // No client input: the stored label plus a constant suffix, so no choke point (S39). $raw = AttrMap::desktop($node->attrs(), 'module.meta.adminLabel'); $label = is_string($raw) ? $raw : ''; $copy->setAttrs(AttrMap::set( @@ -97,8 +75,6 @@ final class DiviDuplicateNodeAbility implements Ability )); } - $change = (new DiviChange(sprintf('Knoten %s dupliziert', $id)))->changed($copy, 'duplicated'); - - return $this->writer->save($this->name(), $page, $change, $input); + return (new DiviChange(sprintf('Knoten %s dupliziert', $id)))->changed($copy, 'duplicated'); } } diff --git a/src/Integrations/Divi/Pages/DiviErrors.php b/src/Integrations/Divi/Pages/DiviErrors.php new file mode 100644 index 0000000..4fd626f --- /dev/null +++ b/src/Integrations/Divi/Pages/DiviErrors.php @@ -0,0 +1,41 @@ +ID), + DiviPageLoader::OUTLINE_HINT + ); + } + + /** A parent / before / after / index the tree has no place for (TreeEditor::position()). */ + public static function position(string $problem): WP_Error + { + return Errors::invalidInput('Ungültige Position; nichts wurde gespeichert.', ['position' => $problem]); + } + + /** + * The errors of Composer::compose()/update(): invalid_attr_path when an attribute path is + * unknown (the more useful hint), else invalid_input with $message. + * + * @param array{errors: array, invalid_paths: list} $result + */ + public static function composed(array $result, string $message): WP_Error + { + return $result['invalid_paths'] !== [] + ? Errors::invalidAttrPath($result['errors']) + : Errors::invalidInput($message, $result['errors']); + } +} diff --git a/src/Integrations/Divi/Pages/DiviInsertAbility.php b/src/Integrations/Divi/Pages/DiviInsertAbility.php index 9474f3e..4c6b992 100644 --- a/src/Integrations/Divi/Pages/DiviInsertAbility.php +++ b/src/Integrations/Divi/Pages/DiviInsertAbility.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Pages; -use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Errors; -use LIT\MCP\Abilities\ToolResult; use WP_Error; /** @@ -16,12 +14,8 @@ use WP_Error; * (client values pass its choke point) and enter the tree only through TreeEditor::insert(), * which also wraps the first section of an empty page in divi/placeholder (ruling S9). */ -final class DiviInsertAbility implements Ability +final class DiviInsertAbility extends DiviWriteAbility { - public function __construct(private DiviWriter $writer) - { - } - public function name(): string { return 'lit/divi-insert'; @@ -60,41 +54,18 @@ final class DiviInsertAbility implements Ability ]; } - public function outputSchema(): ?array - { - return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); - } - public function annotations(): array { return ['readonly' => false, 'destructive' => false, 'idempotent' => false]; } - public function isDirect(): bool + protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error { - return true; - } - - public function permission(array $input): bool|WP_Error - { - $page = $this->writer->page($input); - - return $page instanceof WP_Error ? $page : true; - } - - public function execute(array $input): mixed - { - $page = $this->writer->page($input); - if ($page instanceof WP_Error) { - return $page; - } - - $composition = $this->writer->composition($page); $editor = $composition->editor(); $position = $editor->position($page->index, $input); if (is_string($position)) { // The nodes are checked against their parent – without a valid position there is none. - return Errors::invalidInput('Ungültige Position; nichts wurde gespeichert.', ['position' => $position]); + return DiviErrors::position($position); } $context = DiviWriter::sectionLabel($page->index, $position['parent']); @@ -107,9 +78,7 @@ final class DiviInsertAbility implements Ability $composed = $composition->composer()->compose($nodes, $position['parent'], 'nodes', $context); if ($composed['errors'] !== []) { - return $composed['invalid_paths'] !== [] - ? Errors::invalidAttrPath($composed['errors']) - : Errors::invalidInput('Knoten sind ungültig; nichts wurde gespeichert.', $composed['errors']); + return DiviErrors::composed($composed, 'Knoten sind ungültig; nichts wurde gespeichert.'); } $violation = $editor->insert($position, $composed['blocks']); @@ -125,6 +94,6 @@ final class DiviInsertAbility implements Ability $change->changed($block, 'inserted'); } - return $this->writer->save($this->name(), $page, $change, $input); + return $change; } } diff --git a/src/Integrations/Divi/Pages/DiviMoveAbility.php b/src/Integrations/Divi/Pages/DiviMoveAbility.php index 5bec051..d18004c 100644 --- a/src/Integrations/Divi/Pages/DiviMoveAbility.php +++ b/src/Integrations/Divi/Pages/DiviMoveAbility.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Pages; -use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Errors; -use LIT\MCP\Abilities\ToolResult; use WP_Error; /** @@ -14,12 +12,8 @@ use WP_Error; * another row or section, as long as the structure rules allow it (spec §5.7). Read-only * nodes (global modules, foreign blocks) move as a whole, unchanged. */ -final class DiviMoveAbility implements Ability +final class DiviMoveAbility extends DiviWriteAbility { - public function __construct(private DiviWriter $writer) - { - } - public function name(): string { return 'lit/divi-move'; @@ -50,45 +44,23 @@ final class DiviMoveAbility implements Ability ]; } - public function outputSchema(): ?array - { - return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); - } - public function annotations(): array { return ['readonly' => false, 'destructive' => false, 'idempotent' => false]; } - public function isDirect(): bool + protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error { - return true; - } - - public function permission(array $input): bool|WP_Error - { - $page = $this->writer->page($input); - - return $page instanceof WP_Error ? $page : true; - } - - public function execute(array $input): mixed - { - $page = $this->writer->page($input); - if ($page instanceof WP_Error) { - return $page; - } - $id = is_string($input['id'] ?? null) ? $input['id'] : ''; $node = $page->index->find($id); if ($node === null) { - return DiviPageLoader::unknownNode($id, $page->post); + return DiviErrors::unknownNode($id, $page->post); } - $editor = $this->writer->composition($page)->editor(); + $editor = $composition->editor(); $position = $editor->position($page->index, $input); if (is_string($position)) { - return Errors::invalidInput('Ungültige Position; nichts wurde gespeichert.', ['position' => $position]); + return DiviErrors::position($position); } $problem = $editor->move($page->index, $node, $position); @@ -99,8 +71,6 @@ final class DiviMoveAbility implements Ability ); } - $change = (new DiviChange(sprintf('Knoten %s verschoben', $id)))->changed($node, 'moved'); - - return $this->writer->save($this->name(), $page, $change, $input); + return (new DiviChange(sprintf('Knoten %s verschoben', $id)))->changed($node, 'moved'); } } diff --git a/src/Integrations/Divi/Pages/DiviNodeGetAbility.php b/src/Integrations/Divi/Pages/DiviNodeGetAbility.php index 20c3c9d..442d5d4 100644 --- a/src/Integrations/Divi/Pages/DiviNodeGetAbility.php +++ b/src/Integrations/Divi/Pages/DiviNodeGetAbility.php @@ -118,7 +118,7 @@ final class DiviNodeGetAbility implements Ability $block = $page->index->find($id); if ($block === null) { - return DiviPageLoader::unknownNode($id, $page->post); + return DiviErrors::unknownNode($id, $page->post); } $names = $this->design->read()->presetNames(); diff --git a/src/Integrations/Divi/Pages/DiviOutlineAbility.php b/src/Integrations/Divi/Pages/DiviOutlineAbility.php index 7635a46..bdb0b4d 100644 --- a/src/Integrations/Divi/Pages/DiviOutlineAbility.php +++ b/src/Integrations/Divi/Pages/DiviOutlineAbility.php @@ -143,7 +143,7 @@ final class DiviOutlineAbility implements Ability return Errors::invalidInput('id ist keine Pfad-ID.', ['id' => 'z. B. "s0.r1.c0.m2"']); } if ($rootId !== null && $page->index->find($rootId) === null) { - return DiviPageLoader::unknownNode($rootId, $page->post); + return DiviErrors::unknownNode($rootId, $page->post); } $outline = new Outline($this->design->read()->presetNames()); diff --git a/src/Integrations/Divi/Pages/DiviPageLoader.php b/src/Integrations/Divi/Pages/DiviPageLoader.php index 08d06f1..0d45ee7 100644 --- a/src/Integrations/Divi/Pages/DiviPageLoader.php +++ b/src/Integrations/Divi/Pages/DiviPageLoader.php @@ -12,7 +12,6 @@ use LIT\MCP\Integrations\WordPress\PostGuard; use LIT\MCP\Integrations\WordPress\Schema\ContentSchema; use LIT\MCP\Log; use WP_Error; -use WP_Post; /** * Loads an entry for the Divi tools: rights and status via PostGuard (spec §4.5), then the @@ -91,12 +90,4 @@ class DiviPageLoader return PostGuard::statusStable($post); } - - public static function unknownNode(string $id, WP_Post $post): WP_Error - { - return Errors::notFound( - sprintf('Knoten "%s" gibt es in Eintrag %d nicht.', $id, $post->ID), - self::OUTLINE_HINT - ); - } } diff --git a/src/Integrations/Divi/Pages/DiviUpdateAbility.php b/src/Integrations/Divi/Pages/DiviUpdateAbility.php index 52ded7a..8a1facb 100644 --- a/src/Integrations/Divi/Pages/DiviUpdateAbility.php +++ b/src/Integrations/Divi/Pages/DiviUpdateAbility.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Pages; -use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Errors; -use LIT\MCP\Abilities\ToolResult; use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\Compose\ColumnWidths; use WP_Error; @@ -19,12 +17,8 @@ use WP_Error; * appear once per call (ruling S14); set and attrs of one change must not reach the same path * (ruling S38, checked by Composer::update()). */ -final class DiviUpdateAbility implements Ability +final class DiviUpdateAbility extends DiviWriteAbility { - public function __construct(private DiviWriter $writer) - { - } - public function name(): string { return 'lit/divi-update'; @@ -77,35 +71,13 @@ final class DiviUpdateAbility implements Ability ]; } - public function outputSchema(): ?array - { - return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); - } - public function annotations(): array { return ['readonly' => false, 'destructive' => false, 'idempotent' => true]; } - public function isDirect(): bool + protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error { - return true; - } - - public function permission(array $input): bool|WP_Error - { - $page = $this->writer->page($input); - - return $page instanceof WP_Error ? $page : true; - } - - public function execute(array $input): mixed - { - $page = $this->writer->page($input); - if ($page instanceof WP_Error) { - return $page; - } - $changes = is_array($input['changes'] ?? null) ? array_values($input['changes']) : []; if ($changes === []) { return Errors::invalidInput('Keine Änderungen angegeben; nichts wurde gespeichert.', [ @@ -113,9 +85,10 @@ final class DiviUpdateAbility implements Ability ]); } - $composer = $this->writer->composition($page)->composer(); + $composer = $composition->composer(); $errors = []; - $invalidPaths = false; + /** @var list $invalidPaths */ + $invalidPaths = []; /** @var array $seen id => position of its first change */ $seen = []; /** @var list, 2: string}> $updates */ @@ -150,7 +123,7 @@ final class DiviUpdateAbility implements Ability $result = $composer->update($block, $change, $field); $errors += $result['errors']; - $invalidPaths = $invalidPaths || $result['invalid_paths'] !== []; + array_push($invalidPaths, ...$result['invalid_paths']); $updates[] = [$block, $result['attrs'], $field]; } @@ -159,9 +132,10 @@ final class DiviUpdateAbility implements Ability } if ($errors !== []) { - return $invalidPaths - ? Errors::invalidAttrPath($errors) - : Errors::invalidInput('Änderungen sind ungültig; nichts wurde gespeichert.', $errors); + return DiviErrors::composed( + ['errors' => $errors, 'invalid_paths' => $invalidPaths], + 'Änderungen sind ungültig; nichts wurde gespeichert.' + ); } $updates = array_values(array_filter( @@ -175,6 +149,6 @@ final class DiviUpdateAbility implements Ability $change->changed($block, 'updated'); } - return $this->writer->save($this->name(), $page, $change, $input); + return $change; } } diff --git a/src/Integrations/Divi/Pages/DiviWriteAbility.php b/src/Integrations/Divi/Pages/DiviWriteAbility.php new file mode 100644 index 0000000..f33584f --- /dev/null +++ b/src/Integrations/Divi/Pages/DiviWriteAbility.php @@ -0,0 +1,63 @@ + $input + */ + abstract protected function change(DiviPage $page, array $input, DiviComposition $composition): DiviChange|WP_Error; + + public function outputSchema(): ?array + { + return ToolResult::schema(['tree_rev' => ['type' => 'string']], ['tree_rev']); + } + + public function isDirect(): bool + { + return true; + } + + final public function permission(array $input): bool|WP_Error + { + $page = $this->writer->page($input); + + return $page instanceof WP_Error ? $page : true; + } + + final public function execute(array $input): mixed + { + $page = $this->writer->page($input); + if ($page instanceof WP_Error) { + return $page; + } + + $change = $this->change($page, $input, $this->writer->composition($page)); + if ($change instanceof WP_Error) { + return $change; + } + + return $this->writer->save($this->name(), $page, $change, $input); + } +} diff --git a/tests/Unit/Guides/ToolNamingTest.php b/tests/Unit/Guides/ToolNamingTest.php index 19c5f0a..cd737ec 100644 --- a/tests/Unit/Guides/ToolNamingTest.php +++ b/tests/Unit/Guides/ToolNamingTest.php @@ -30,7 +30,7 @@ final class ToolNamingTest extends TestCase $class = 'LIT\\MCP\\' . str_replace(['/', '.php'], ['\\', ''], substr((string) $file, strlen($root . '/src/'))); $reflection = new ReflectionClass($class); - if ($reflection->isInterface() || ! $reflection->implementsInterface(Ability::class)) { + if (! $reflection->isInstantiable() || ! $reflection->implementsInterface(Ability::class)) { continue; }