From 14bc954eccadc03afd289317c0e17426a558f70d Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sun, 27 Sep 2026 23:12:14 +0200 Subject: [PATCH] fix: Theme Builder minors of the final review (M-4, M-5, M-6, M-9, M-10, M-11, M-12 c) - tb-list applies the detached limit after the edit filter (reads up to 500 IDs) and notes when more are not listed (M-4); an invalid template_id is invalid input, shared with ThemeBuilderAccess::template() (M-5). - Templates carry their status; tb-list notes that a live template outside publish (e.g. trashed) can only be switched off (M-10). - changeableTemplate() refuses without canUse() first, like tb-list and tb-create-template; the layout/template refusal difference is documented (M-9). - tb-create-template reads the default template from editableLiveTemplates() (M-11); the condition-ID character set (role slugs) is documented (M-6); tb-assign documents that a failed reattach keeps Divi's unused marks removed (M-12 c). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- docs/guides/theme-builder.md | 9 ++- .../Divi/ThemeBuilder/TbAssignAbility.php | 4 +- .../ThemeBuilder/TbCreateTemplateAbility.php | 9 ++- .../Divi/ThemeBuilder/TbListAbility.php | 41 ++++++++++-- .../Divi/ThemeBuilder/Template.php | 1 + .../Divi/ThemeBuilder/TemplateConditions.php | 3 + .../Divi/ThemeBuilder/ThemeBuilderAccess.php | 24 +++++-- tests/Support/ThemeBuilderFixtures.php | 6 +- .../Divi/ThemeBuilder/TbListAbilityTest.php | 64 +++++++++++++++++++ .../ThemeBuilder/ThemeBuilderAccessTest.php | 33 +++++++++- 10 files changed, 176 insertions(+), 18 deletions(-) diff --git a/docs/guides/theme-builder.md b/docs/guides/theme-builder.md index 4e86c6e..55b3bd9 100644 --- a/docs/guides/theme-builder.md +++ b/docs/guides/theme-builder.md @@ -23,12 +23,13 @@ Kopf- und Fußzeile heißen „global“, weil andere Vorlagen sie mitbenutzen. ## Werkzeuge (über `mcp-adapter-execute-ability`) -- `lit/tb-list`: alle Vorlagen mit `enabled`, `live` (für Besucher wirksam), Bedingungen (`use_on`, +- `lit/tb-list`: alle Vorlagen mit `enabled`, `live` (für Besucher wirksam), `status`, Bedingungen (`use_on`, `exclude_from` und Beschriftungen), Layouts (`id`, Titel, `shared_with` = andere Vorlagen mit demselben Layout) und `template_rev`. `template_id`: nur diese Vorlage. `conditions: true` liefert alle Bedingungen, die Divi anbietet; eine ID, die auf „:“ endet, braucht ein Objekt am Ende, z. B. `singular:post_type:page:id:42`. `detached` zeigt Vorlagen, die ein Speichern im Theme Builder entfernt hat - (siehe unten); `draft_pending` zeigt ungespeicherte Änderungen im Theme Builder. + (siehe unten, höchstens 50 je Aufruf); `draft_pending` zeigt ungespeicherte Änderungen im Theme Builder. Eine + aktive Vorlage, die nicht veröffentlicht ist (z. B. im Papierkorb), lässt sich nur deaktivieren. - `lit/tb-create-template`: `title`, je Bereich (`header`, `body`, `footer`) `source`: `"new"` (neues Layout; optional `nodes` wie bei `lit-divi-insert` und `title`), `"global"` (Kopf-/Fußzeile der Standardvorlage mitbenutzen; nicht für `body`), `"default"` (nichts Eigenes, Standard bei fehlender @@ -50,7 +51,9 @@ Kopf- und Fußzeile heißen „global“, weil andere Vorlagen sie mitbenutzen. Erlaubt sind nur die Bedingungen, die Divi selbst anbietet (`lit/tb-list` mit `conditions: true`), höchstens 50 je Liste. Ein Objekt am Ende (Seite, Beitrag, Begriff, Person) muss existieren und für das Konto lesbar bzw. zuweisbar sein; sonst lautet die Antwort wie bei einer unbekannten ID. Bedingungen für Autoren -(`archive:user:id:…`) verlangen das Recht, Benutzer aufzulisten (`list_users`) – strenger als Divi. +(`archive:user:id:…`) verlangen das Recht, Benutzer aufzulisten (`list_users`) – strenger als Divi. Bedingungs-IDs +bestehen nur aus Buchstaben, Ziffern, `_`, `:` und `-`; eine Rolle, deren Kennung andere Zeichen enthält (manche +Rollen-Plugins), dann im Divi Theme Builder setzen. ## Felder im Inhalt-Layout (Dynamic Content) diff --git a/src/Integrations/Divi/ThemeBuilder/TbAssignAbility.php b/src/Integrations/Divi/ThemeBuilder/TbAssignAbility.php index b4f57bf..e6d1a07 100644 --- a/src/Integrations/Divi/ThemeBuilder/TbAssignAbility.php +++ b/src/Integrations/Divi/ThemeBuilder/TbAssignAbility.php @@ -26,7 +26,9 @@ defined('ABSPATH') || exit; * live list (only on the user's request, B19; only one no Theme Builder post lists – tb-list's detached, audit L4) * – always disabled, Divi's "unused" marks removed from it and its * layouts, which must still be published (B10). Templates have no revisions: the previous conditions are in the - * answer and the change log; a failed write restores them (and undoes the reattach). + * answer and the change log; a failed write restores them (and undoes the reattach). The undo does not put back + * Divi's "unused" marks a reattach removed (review M-12 c): the template is detached again, but Divi no longer + * trashes it after seven days – harmless, it stays under detached in lit/tb-list. */ final class TbAssignAbility implements Ability { diff --git a/src/Integrations/Divi/ThemeBuilder/TbCreateTemplateAbility.php b/src/Integrations/Divi/ThemeBuilder/TbCreateTemplateAbility.php index 6b6ae33..21e9170 100644 --- a/src/Integrations/Divi/ThemeBuilder/TbCreateTemplateAbility.php +++ b/src/Integrations/Divi/ThemeBuilder/TbCreateTemplateAbility.php @@ -216,11 +216,14 @@ final class TbCreateTemplateAbility implements Ability return $plan; } - /** The default template, only when the user may edit it (ruling Q19, like tb-list Q8/B11). */ + /** + * The default template, only when the user may edit it (ruling Q19, like tb-list Q8/B11) – from the one shared + * split of the live templates (ThemeBuilderAccess::editableLiveTemplates(), ruling Q21, review M-11). + */ private function defaultTemplate(): ?Template { - foreach ($this->access->store()->liveTemplates() as $template) { - if ($template->default && $this->access->canEditTemplate($template->id)) { + foreach ($this->access->editableLiveTemplates()['templates'] as $template) { + if ($template->default) { return $template; } } diff --git a/src/Integrations/Divi/ThemeBuilder/TbListAbility.php b/src/Integrations/Divi/ThemeBuilder/TbListAbility.php index d011d84..63fb4a4 100644 --- a/src/Integrations/Divi/ThemeBuilder/TbListAbility.php +++ b/src/Integrations/Divi/ThemeBuilder/TbListAbility.php @@ -26,6 +26,9 @@ final class TbListAbility implements Ability { public const DETACHED_LIMIT = 50; + /** At most this many detached IDs are read to find DETACHED_LIMIT the user may edit (review M-4). */ + public const DETACHED_SCAN = 500; + public function __construct(private ThemeBuilderAccess $access, private TemplateConditions $conditions) { } @@ -110,18 +113,29 @@ final class TbListAbility implements Ability $split = $this->access->editableLiveTemplates(); $live = $split['templates']; $hiddenLive = count(array_filter($split['hidden'], static fn (Template $t): bool => $t->isLive())); + // Review M-4: the limit counts only templates the user may edit; foreign ones do not crowd them out. $detached = []; - foreach ($store->detachedIds(self::DETACHED_LIMIT) as $id) { + $detachedIds = $store->detachedIds(self::DETACHED_SCAN); + $moreDetached = count($detachedIds) >= self::DETACHED_SCAN; + foreach ($detachedIds as $id) { $template = $store->template($id, false); - if ($template !== null && $this->access->canEditTemplate($template->id)) { - $detached[] = $template; + if ($template === null || ! $this->access->canEditTemplate($template->id)) { + continue; } + if (count($detached) >= self::DETACHED_LIMIT) { + $moreDetached = true; + break; + } + $detached[] = $template; } // Only what the user may see counts for shared_with (B11). $shared = array_merge($live, $detached); if (isset($input['template_id'])) { - $id = (int) PostGuard::id($input['template_id']); + $id = PostGuard::id($input['template_id']); + if ($id === null) { + return ThemeBuilderAccess::invalidTemplateId(); + } $only = static fn (array $list): array => array_values( array_filter($list, static fn (Template $template): bool => $template->id === $id) ); @@ -149,12 +163,31 @@ final class TbListAbility implements Ability $hiddenLive ); } + $trashed = array_values(array_filter( + $live, + static fn (Template $t): bool => $t->isLive() && $t->status !== 'publish' + )); + if ($trashed !== []) { + // Review M-10: tb-assign and switching on read such a template as missing (ThemeBuilderAccess::template()). + $notes[] = sprintf( + 'Aktiv, aber nicht veröffentlicht (z. B. im Papierkorb, siehe status): %s – nur deaktivieren ' + . '(lit/tb-activate enabled: false); Bedingungen ändern oder erneut aktivieren geht erst, wenn ' + . 'die Vorlage in WordPress wiederhergestellt ist.', + implode(', ', array_map(static fn (Template $t): string => (string) $t->id, $trashed)) + ); + } if ($detached !== []) { $notes[] = 'detached: Diese Vorlagen hat ein späteres Speichern im Theme Builder entfernt – oder jemand ' . 'hat sie dort bewusst gelöscht; Divi löscht als ungenutzt markierte Vorlagen und Layouts 7 Tage ' . 'nach der Markierung. Nur auf ausdrücklichen Wunsch wieder zuordnen (bleibt deaktiviert): ' . 'lit/tb-assign mit reattach: true.'; } + if ($moreDetached && ! isset($input['template_id'])) { + $notes[] = sprintf( + 'Weitere entfernte Vorlagen nicht aufgeführt (höchstens %d je Aufruf).', + self::DETACHED_LIMIT + ); + } $result = [ 'theme_builder_id' => $store->liveId(), diff --git a/src/Integrations/Divi/ThemeBuilder/Template.php b/src/Integrations/Divi/ThemeBuilder/Template.php index 82797d2..13cdfbf 100644 --- a/src/Integrations/Divi/ThemeBuilder/Template.php +++ b/src/Integrations/Divi/ThemeBuilder/Template.php @@ -119,6 +119,7 @@ final class Template return [ 'id' => $this->id, 'title' => $this->title, + 'status' => $this->status, 'default' => $this->default, 'enabled' => $this->enabled, 'live' => $this->isLive(), diff --git a/src/Integrations/Divi/ThemeBuilder/TemplateConditions.php b/src/Integrations/Divi/ThemeBuilder/TemplateConditions.php index 7e32d6f..5e5b96d 100644 --- a/src/Integrations/Divi/ThemeBuilder/TemplateConditions.php +++ b/src/Integrations/Divi/ThemeBuilder/TemplateConditions.php @@ -18,6 +18,9 @@ defined('ABSPATH') || exit; * of that type with read_post (not trashed, no auto-draft/attachment status), a term of that taxonomy with * assign_term, a user only with list_users (ruling Q10, stricter than Divi), an existing role. Refusals for * missing and unreadable objects read the same and never name an object's title (P12). Nothing is written here. + * Every condition ID is limited to letters, digits, "_", ":" and "-" (at most 200 characters): a role slug with + * other characters (possible with role plugins) is refused as an invalid condition ID and must be set in Divi's + * Theme Builder (review M-6). */ final class TemplateConditions { diff --git a/src/Integrations/Divi/ThemeBuilder/ThemeBuilderAccess.php b/src/Integrations/Divi/ThemeBuilder/ThemeBuilderAccess.php index 8f75493..67edd3f 100644 --- a/src/Integrations/Divi/ThemeBuilder/ThemeBuilderAccess.php +++ b/src/Integrations/Divi/ThemeBuilder/ThemeBuilderAccess.php @@ -19,7 +19,12 @@ defined('ABSPATH') || exit; * Builder page, theme-builder.php:2059-2063, ruling Q15) and the Divi role option theme_builder (off for * every role but administrator by default, F12); one layout or template additionally edit_post on it. * Without those, or with the group "Divi Theme Builder" disabled, every TB ID answers like a missing ID - * (P12 pattern). The group's "nur lesen" stops writes (E9). Not final so abilities can be tested with a mock. + * (P12 pattern); the Theme Builder tools themselves (tb-list, tb-create-template, tb-assign, tb-activate) refuse + * without canUse() as forbidden, whatever the ID (review M-9). With the Theme Builder rights, a template the user + * may not edit still reads like a missing one (template(), ruling Q18 – templates are listed by tb-list), while a + * layout without edit_post answers forbidden (layout(): only accounts with the Theme Builder rights get there, and + * tb-list names the layout IDs of their templates). The group's "nur lesen" stops writes (E9). Not final so + * abilities can be tested with a mock. */ class ThemeBuilderAccess { @@ -146,10 +151,7 @@ class ThemeBuilderAccess { $id = PostGuard::id($templateId); if ($id === null) { - return Errors::invalidInput( - 'template_id fehlt oder ist keine ID.', - ['template_id' => 'ID aus lit/tb-list'] - ); + return self::invalidTemplateId(); } $post = get_post($id); @@ -168,6 +170,12 @@ class ThemeBuilderAccess return $forWrite ? ($this->writableGate() ?? $post) : $post; } + /** The answer for a template_id that is missing or no ID (template(), lit/tb-list – review M-5). */ + public static function invalidTemplateId(): WP_Error + { + return Errors::invalidInput('template_id fehlt oder ist keine ID.', ['template_id' => 'ID aus lit/tb-list']); + } + /** * A template the Theme Builder tools may change (lit/tb-assign, lit/tb-activate): rights and group gate * (template(), a template the user may not edit reads as missing – ruling Q18), no Theme Builder draft (E7), @@ -180,6 +188,12 @@ class ThemeBuilderAccess */ public function changeableTemplate(array $input, bool $offSwitch = false): Template|WP_Error { + // Review M-9: without the Theme Builder rights refuse like tb-list/tb-create-template – independent of the ID. + $refusal = $this->canUse(); + if ($refusal !== null) { + return $refusal; + } + $post = $this->template($input['template_id'] ?? null, true); if ($post instanceof WP_Error) { $id = $offSwitch && Errors::codeOf($post) === 'not_found' diff --git a/tests/Support/ThemeBuilderFixtures.php b/tests/Support/ThemeBuilderFixtures.php index 1bd9e72..d8edc84 100644 --- a/tests/Support/ThemeBuilderFixtures.php +++ b/tests/Support/ThemeBuilderFixtures.php @@ -127,7 +127,11 @@ final class ThemeBuilderFixtures $store->allows('liveTemplates')->andReturn($templates); $store->allows('templateIds')->andReturn(array_map(static fn (Template $t): int => $t->id, $templates)); $store->allows('template')->andReturnUsing(static fn (int $id): ?Template => $byId[$id] ?? null)->byDefault(); - $store->allows('detachedIds')->andReturn(array_map(static fn (Template $t): int => $t->id, $detached)); + $store->allows('detachedIds')->andReturnUsing(static fn (int $limit): array => array_slice( + array_map(static fn (Template $t): int => $t->id, $detached), + 0, + $limit + ))->byDefault(); $store->allows('isListedAnywhere')->andReturnUsing( static fn (int $id): bool => in_array($id, array_map(static fn (Template $t): int => $t->id, $templates), true) )->byDefault(); diff --git a/tests/Unit/Integrations/Divi/ThemeBuilder/TbListAbilityTest.php b/tests/Unit/Integrations/Divi/ThemeBuilder/TbListAbilityTest.php index dcd4277..8b15a32 100644 --- a/tests/Unit/Integrations/Divi/ThemeBuilder/TbListAbilityTest.php +++ b/tests/Unit/Integrations/Divi/ThemeBuilder/TbListAbilityTest.php @@ -94,6 +94,70 @@ final class TbListAbilityTest extends TestCase self::assertToolError(self::ability($store)->execute(['template_id' => 4711]), 'not_found'); } + /** Review M-5: an invalid template_id is named as invalid, not as template 0. */ + public function testAnInvalidTemplateIdIsInvalidInput(): void + { + $result = self::ability(ThemeBuilderFixtures::store([ThemeBuilderFixtures::template(1065)])) + ->execute(['template_id' => 'abc']); + + self::assertToolError($result, 'validation_failed'); + self::assertSame('template_id fehlt oder ist keine ID.', self::clientError($result)['message']); + } + + /** + * Review M-4: the detached limit applies to the templates the user may edit – foreign detached ones do not + * crowd them out – and a note says when more are not listed. + */ + public function testTheDetachedLimitCountsOnlyEditableTemplates(): void + { + $foreign = range(3000, 3000 + TbListAbility::DETACHED_LIMIT - 1); + $own = range(4000, 4000 + TbListAbility::DETACHED_LIMIT); + $detached = array_map( + static fn (int $id): \LIT\MCP\Integrations\Divi\ThemeBuilder\Template => ThemeBuilderFixtures::template($id, ['attached' => false]), + array_merge($foreign, $own) + ); + $store = ThemeBuilderFixtures::store([], 0, $detached); + $asked = []; + $store->allows('detachedIds')->andReturnUsing(static function (int $limit) use (&$asked, $detached): array { + $asked[] = $limit; + + return array_slice(array_map(static fn ($t): int => $t->id, $detached), 0, $limit); + }); + self::denyEditing($foreign); + + $result = self::ability($store)->execute([]); + + self::assertSame(array_slice($own, 0, TbListAbility::DETACHED_LIMIT), array_column($result['detached'], 'id')); + self::assertStringContainsString('Weitere entfernte Vorlagen nicht aufgeführt', implode(' ', $result['notes'])); + self::assertSame([TbListAbility::DETACHED_SCAN], $asked); + + $few = ThemeBuilderFixtures::store([], 0, [ThemeBuilderFixtures::template(1066, ['attached' => false])]); + self::assertStringNotContainsString( + 'Weitere entfernte Vorlagen', + implode(' ', self::ability($few)->execute([])['notes']) + ); + } + + /** + * Review M-10: a live template in the trash is listed with its status and the note that it can only be switched + * off (tb-assign and switching on read it as missing). + */ + public function testATrashedLiveTemplateSaysItCanOnlyBeSwitchedOff(): void + { + $store = ThemeBuilderFixtures::store([ThemeBuilderFixtures::template(1066, ['enabled' => true, 'status' => 'trash'])]); + + $result = self::ability($store)->execute([]); + + self::assertSame('trash', $result['templates'][0]['status']); + self::assertStringContainsString('lit/tb-activate enabled: false', implode(' ', $result['notes'])); + self::assertStringContainsString('1066', implode(' ', $result['notes'])); + + $published = ThemeBuilderFixtures::store([ThemeBuilderFixtures::template(1066, ['enabled' => true])]); + $plain = self::ability($published)->execute([]); + self::assertSame('publish', $plain['templates'][0]['status']); + self::assertStringNotContainsString('Papierkorb', implode(' ', $plain['notes'])); + } + public function testNeedsTheThemeBuilderRole(): void { $ability = self::ability(ThemeBuilderFixtures::store(), ['theme_builder']); diff --git a/tests/Unit/Integrations/Divi/ThemeBuilder/ThemeBuilderAccessTest.php b/tests/Unit/Integrations/Divi/ThemeBuilder/ThemeBuilderAccessTest.php index cff27d2..29a95b9 100644 --- a/tests/Unit/Integrations/Divi/ThemeBuilder/ThemeBuilderAccessTest.php +++ b/tests/Unit/Integrations/Divi/ThemeBuilder/ThemeBuilderAccessTest.php @@ -208,6 +208,31 @@ final class ThemeBuilderAccessTest extends TestCase ); } + /** + * Review M-9: without the Theme Builder rights tb-assign and tb-activate refuse like tb-list and + * tb-create-template (forbidden, independent of the ID) instead of "Vorlage X gibt es nicht". + */ + public function testChangeableTemplateRefusesWithoutTheThemeBuilderRightsFirst(): void + { + LibraryFixtures::given([ThemeBuilderFixtures::templatePost(1066)]); + $store = ThemeBuilderFixtures::store([ThemeBuilderFixtures::template(1066)]); + $rev = ThemeBuilderFixtures::template(1066)->rev(); + + foreach ([1066, 4711, 'abc'] as $id) { + $refused = ThemeBuilderFixtures::access(['theme_builder'], true, $store) + ->changeableTemplate(['template_id' => $id, 'template_rev' => $rev]); + self::assertToolError($refused, 'forbidden'); + self::assertStringNotContainsString((string) $id, self::clientError($refused)['message']); + } + foreach ([true, false] as $offSwitch) { + self::assertToolError( + ThemeBuilderFixtures::access(['theme_builder'], true, $store) + ->changeableTemplate(['template_id' => 1066, 'template_rev' => $rev], $offSwitch), + 'forbidden' + ); + } + } + /** Ruling Q18: a template the user may not edit answers like a missing one, also for writing. */ public function testChangeableTemplateHidesTemplatesTheUserMayNotEdit(): void { @@ -242,13 +267,19 @@ final class ThemeBuilderAccessTest extends TestCase ThemeBuilderFixtures::access([], true, ThemeBuilderFixtures::store([ ThemeBuilderFixtures::template(1066, ['status' => 'trash']), ])), - ThemeBuilderFixtures::access(['theme_builder'], true, ThemeBuilderFixtures::store([$trashed])), + ThemeBuilderFixtures::access([], true, ThemeBuilderFixtures::store([$trashed]), false), ]; foreach ($notLive as $index => $other) { $refused = $other->changeableTemplate($input, true); self::assertToolError($refused, 'not_found', (string) $index); self::assertSame(self::clientError($missing), self::clientError($refused), (string) $index); } + // Review M-9: without the Theme Builder rights the refusal does not depend on the ID. + self::assertToolError( + ThemeBuilderFixtures::access(['theme_builder'], true, ThemeBuilderFixtures::store([$trashed])) + ->changeableTemplate($input, true), + 'forbidden' + ); self::assertToolError( ThemeBuilderFixtures::access([], false, ThemeBuilderFixtures::store([$trashed]))->changeableTemplate($input, true),