From 048cd9353d8ae83d1ad1ae2e762a81e17288bab4 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Fri, 16 Oct 2026 18:06:52 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20DesignLock=20gegen=20parallele=20Best?= =?UTF-8?q?=C3=A4tigungen;=20confirm=20ohne=20design=5Frev=20ist=20ein=20E?= =?UTF-8?q?ingabefehler?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 --- src/Integrations/Divi/Design/DesignChange.php | 70 ++++++++- src/Integrations/Divi/Design/DesignLock.php | 52 +++++++ src/Uninstaller.php | 14 +- .../Divi/Design/DesignChangeTest.php | 137 ++++++++++++++++++ .../Divi/Design/DesignLockTest.php | 73 ++++++++++ .../Design/DiviDesignRestoreAbilityTest.php | 8 + .../Design/DiviPresetsDeleteAbilityTest.php | 8 + .../Divi/Design/DiviPresetsSetAbilityTest.php | 3 + tests/Unit/UninstallTest.php | 3 + tests/Unit/UninstallerTest.php | 3 + 10 files changed, 357 insertions(+), 14 deletions(-) create mode 100644 src/Integrations/Divi/Design/DesignLock.php create mode 100644 tests/Unit/Integrations/Divi/Design/DesignLockTest.php diff --git a/src/Integrations/Divi/Design/DesignChange.php b/src/Integrations/Divi/Design/DesignChange.php index 73122a8..c9c3eb0 100644 --- a/src/Integrations/Divi/Design/DesignChange.php +++ b/src/Integrations/Divi/Design/DesignChange.php @@ -19,10 +19,12 @@ defined('ABSPATH') || exit; /** * The change pipeline of every design system write (spec §4, G2, G6): plan on the live inventory → without - * confirm a dry run (diff, affected posts, design_rev; nothing saved) → with confirm the design_rev must still match - * → no live post may block (P8/P8a) → release lock → snapshot of the area → write through Divi → read back and - * verify → change log. A failed write or read-back writes the snapshot back (Rollback); when that fails too the - * error is partial_write with the snapshot to restore by hand. The rights are the tool's (spec §10). + * confirm a dry run (diff, affected posts, design_rev; nothing saved) → with confirm the design_rev is required, the + * area's DesignLock is taken and the plan made again on the inventory read under the lock, whose design_rev must + * still match → no live post may block (P8/P8a) → Divi release gate → snapshot of the area → write through Divi → + * read back and verify → change log; the lock is released at the end, also after a failure. A failed write or + * read-back writes the snapshot back (DesignArea::restore(), Rollback); when that fails too the error is + * partial_write with the snapshot to restore by hand. The rights are the tool's (spec §10). */ final class DesignChange { @@ -30,8 +32,11 @@ final class DesignChange private const LISTED_IDS = 10; - public function __construct(private SnapshotStore $snapshots, private ChangeLog $log) - { + public function __construct( + private SnapshotStore $snapshots, + private ChangeLog $log, + private DesignLock $lock = new DesignLock() + ) { } /** @return array> Input properties every design write tool takes. */ @@ -66,6 +71,44 @@ final class DesignChange * @throws DiviLocked */ public function run(string $ability, DesignArea $area, array $input, callable $plan): array|WP_Error + { + if (($input['confirm'] ?? false) !== true) { + return $this->attempt($ability, $area, null, $plan); + } + + $rev = $input['design_rev'] ?? null; + if (! is_string($rev) || $rev === '') { + return Errors::invalidInput( + 'design_rev fehlt; nichts wurde gespeichert.', + ['design_rev' => 'aus dem Probelauf übernehmen'] + ); + } + + if (! $this->lock->acquire($area->name())) { + return Errors::make( + 'conflict', + sprintf('Gerade läuft eine andere Änderung an den %s; nichts wurde gespeichert.', $area->label()), + 'Kurz warten und den Probelauf wiederholen.', + 409 + ); + } + try { + return $this->attempt($ability, $area, $rev, $plan); + } finally { + $this->lock->release($area->name()); + } + } + + /** + * Plans on the inventory read now – under the lock when confirmed ($confirmedRev) – and runs the dry run or + * the confirmed change. + * + * @param callable(array): (DesignPlan|WP_Error) $plan + * @return array|WP_Error + * @throws WriteFailed + * @throws DiviLocked + */ + private function attempt(string $ability, DesignArea $area, ?string $confirmedRev, callable $plan): array|WP_Error { $current = $area->raw(); $rev = $area->rev($current); @@ -82,7 +125,7 @@ final class DesignChange ->toArray(); } - if (($input['confirm'] ?? false) !== true) { + if ($confirmedRev === null) { return Errors::confirmRequired( $planned->summary . ' Nichts wurde gespeichert (Probelauf).', ['diff' => $planned->diff, 'design_rev' => $rev, 'warnings' => $planned->warnings] @@ -92,7 +135,7 @@ final class DesignChange ); } - if (($input['design_rev'] ?? null) !== $rev) { + if ($confirmedRev !== $rev) { return Errors::revMismatch( sprintf('Die %s wurden seit dem Probelauf geändert; nichts wurde gespeichert.', $area->label()), 'Erneut ohne confirm aufrufen (Probelauf), dem Nutzer die Änderung zeigen und mit der neuen ' @@ -100,6 +143,17 @@ final class DesignChange ); } + return $this->apply($ability, $area, $planned, $current); + } + + /** + * @param array $current + * @return array|WP_Error + * @throws WriteFailed + * @throws DiviLocked + */ + private function apply(string $ability, DesignArea $area, DesignPlan $planned, array $current): array|WP_Error + { $blocked = $this->blocked($planned); if ($blocked !== null) { return $blocked; diff --git a/src/Integrations/Divi/Design/DesignLock.php b/src/Integrations/Divi/Design/DesignLock.php new file mode 100644 index 0000000..aaaf926 --- /dev/null +++ b/src/Integrations/Divi/Design/DesignLock.php @@ -0,0 +1,52 @@ + (never + * autoloaded) holds the time the lock was taken. add_option() is atomic – it fails while the option exists. A lock + * older than STALE_AFTER seconds (a request that died) is removed and taken once more. + */ +final class DesignLock +{ + public const STALE_AFTER = 120; + + private const PREFIX = 'lit_mcp_lock_'; + + /** @throws InvalidArgumentException For an area outside SnapshotStore::AREAS. */ + public static function option(string $area): string + { + if (! in_array($area, SnapshotStore::AREAS, true)) { + throw new InvalidArgumentException(sprintf('Unknown design area "%s".', $area)); + } + + return self::PREFIX . $area; + } + + public function acquire(string $area): bool + { + $option = self::option($area); + if (add_option($option, time(), '', false)) { + return true; + } + + $since = get_option($option, 0); + if (is_numeric($since) && time() - (int) $since <= self::STALE_AFTER) { + return false; + } + delete_option($option); + + return add_option($option, time(), '', false); + } + + public function release(string $area): void + { + delete_option(self::option($area)); + } +} diff --git a/src/Uninstaller.php b/src/Uninstaller.php index 5af05b9..18b73f8 100644 --- a/src/Uninstaller.php +++ b/src/Uninstaller.php @@ -8,9 +8,9 @@ defined('ABSPATH') || exit; /** * Removes WP-LIT-MCP's own data (uninstall.php, Task 12): options (incl. the Snapshots des - * Designsystems, Etappe 4), the endpoint-check and Divi-release transients, the log-cleanup - * cron hook and the change log table – on every site of a network – and the Protokoll tab's - * per-user screen option. + * Designsystems and their change locks, Etappe 4), the endpoint-check and Divi-release + * transients, the log-cleanup cron hook and the change log table – on every site of a + * network – and the Protokoll tab's per-user screen option. * * Deliberately self-contained (no reference to any other LIT\MCP\* class): uninstall.php * is executed by WordPress core standalone, without loading the plugin's main file first, @@ -18,9 +18,10 @@ defined('ABSPATH') || exit; * names below are therefore duplicated from Access::OPTION, Abilities\ExposurePolicy::OPTION, * Limits::OPTION, ChangeLog\ChangeLogInstaller::VERSION_OPTION, Integrations\Divi\Release\ * ReleaseStore::OPTION, Integrations\Divi\Release\CheckReportStore::OPTION, Integrations\ - * Divi\Release\DiviCheckRoute::LOCK, Admin\ConnectionStatus::ENDPOINT_TRANSIENT, - * Integrations\Divi\Release\ReleaseResolver::CACHE, ChangeLog\ChangeLogCleanup::HOOK, - * ChangeLog\ChangeLog::TABLE and Admin\LogTab::PER_PAGE_OPTION on purpose; + * Divi\Release\DiviCheckRoute::LOCK, Integrations\Divi\Design\DesignLock::option(), + * Admin\ConnectionStatus::ENDPOINT_TRANSIENT, Integrations\Divi\Release\ReleaseResolver::CACHE, + * ChangeLog\ChangeLogCleanup::HOOK, ChangeLog\ChangeLog::TABLE and Admin\LogTab::PER_PAGE_OPTION + * on purpose; * tests/Unit/UninstallerTest.php and tests/Unit/UninstallTest.php keep them in sync with * those constants. * @@ -36,6 +37,7 @@ final class Uninstaller 'lit_mcp_divi_releases', 'lit_mcp_divi_check_report', 'lit_mcp_divi_check_lock', 'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets', 'lit_mcp_snapshots_theme_options', + 'lit_mcp_lock_colors', 'lit_mcp_lock_variables', 'lit_mcp_lock_presets', 'lit_mcp_lock_theme_options', ]; private const TRANSIENTS = ['lit_mcp_endpoint_check', 'lit_mcp_divi_state']; private const CRON_HOOK = 'lit_mcp_log_cleanup'; diff --git a/tests/Unit/Integrations/Divi/Design/DesignChangeTest.php b/tests/Unit/Integrations/Divi/Design/DesignChangeTest.php index 0ec9ae1..69bdbd4 100644 --- a/tests/Unit/Integrations/Divi/Design/DesignChangeTest.php +++ b/tests/Unit/Integrations/Divi/Design/DesignChangeTest.php @@ -6,6 +6,7 @@ namespace LIT\MCP\Tests\Unit\Integrations\Divi\Design; use LIT\MCP\Integrations\Divi\Design\DesignArea; use LIT\MCP\Integrations\Divi\Design\DesignChange; +use LIT\MCP\Integrations\Divi\Design\DesignLock; use LIT\MCP\Integrations\Divi\Design\DesignPlan; use LIT\MCP\Integrations\Divi\Design\SnapshotStore; use LIT\MCP\Integrations\Divi\Design\SnapshotTooLarge; @@ -17,11 +18,35 @@ use LIT\MCP\Integrations\Divi\Release\ReleaseStatus; use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\Tests\Support\FakeChangeLog; use LIT\MCP\Tests\Unit\TestCase; +use Brain\Monkey\Functions; use Mockery; use RuntimeException; final class DesignChangeTest extends TestCase { + /** @var array Options as add_option()/delete_option() see them (DesignLock). */ + private array $options = []; + + protected function setUp(): void + { + parent::setUp(); + $this->options = []; + Functions\when('add_option')->alias(function (string $name, mixed $value = ''): bool { + if (array_key_exists($name, $this->options)) { + return false; + } + $this->options[$name] = $value; + + return true; + }); + Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default); + Functions\when('delete_option')->alias(function (string $name): bool { + unset($this->options[$name]); + + return true; + }); + } + /** * A DesignArea over an array; the first $failWrites calls of write()/restore() throw, $differences are the * value warnings of differences(). @@ -164,6 +189,118 @@ final class DesignChangeTest extends TestCase self::assertSame([], $area->writes); } + /** Final review M-9: confirm without design_rev is a missing input, not a stale one. */ + public function testConfirmWithoutDesignRevIsInvalidInput(): void + { + $stored = ['v' => 1]; + $area = self::area($stored); + $snapshots = Mockery::mock(SnapshotStore::class); + $snapshots->shouldNotReceive('save'); + + $result = (new DesignChange($snapshots, FakeChangeLog::forbidden())) + ->run('lit/divi-presets-set', $area, ['confirm' => true], self::plan(['v' => 2])); + + $details = self::assertToolError($result, 'validation_failed'); + self::assertSame('design_rev fehlt; nichts wurde gespeichert.', $details['message']); + self::assertSame('aus dem Probelauf übernehmen', $details['fields']['design_rev']); + self::assertSame([], $area->writes); + } + + /** Final review I-5: a confirm while another change of the area runs writes nothing. */ + public function testAHeldLockRefusesTheConfirm(): void + { + $this->options['lit_mcp_lock_presets'] = time(); + $stored = ['v' => 1]; + $area = self::area($stored); + $snapshots = Mockery::mock(SnapshotStore::class); + $snapshots->shouldNotReceive('save'); + + $result = (new DesignChange($snapshots, FakeChangeLog::forbidden(), new DesignLock()))->run( + 'lit/divi-presets-set', + $area, + ['confirm' => true, 'design_rev' => $area->rev($stored)], + self::plan(['v' => 2]) + ); + + self::assertToolError($result, 'conflict'); + self::assertSame(409, $result->get_error_data()['status']); + self::assertStringContainsString('andere Änderung an den Presets', $result->get_error_message()); + self::assertSame([], $area->writes); + self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'a foreign lock is not released'); + } + + public function testTheLockIsHeldWhileWritingAndReleasedAfterSuccess(): void + { + $stored = ['v' => 1]; + $area = self::area($stored); + $snapshots = Mockery::mock(SnapshotStore::class); + $snapshots->expects('save')->andReturnUsing(function (): string { + self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'snapshot under the lock'); + + return 'presets-1'; + }); + $snapshots->expects('attachLog')->andReturnUsing(function (): void { + self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'log under the lock'); + }); + + $result = (new DesignChange($snapshots, new FakeChangeLog(9)))->run( + 'lit/divi-presets-set', + $area, + ['confirm' => true, 'design_rev' => $area->rev($stored)], + self::plan(['v' => 2]) + ); + + self::assertTrue($result['ok']); + self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options); + } + + public function testTheLockIsReleasedAfterAFailedWrite(): void + { + $stored = ['v' => 1]; + $area = self::area($stored, 1); + $snapshots = Mockery::mock(SnapshotStore::class); + $snapshots->expects('save')->andReturn('presets-1'); + + try { + (new DesignChange($snapshots, FakeChangeLog::forbidden()))->run( + 'lit/divi-presets-set', + $area, + ['confirm' => true, 'design_rev' => $area->rev($stored)], + self::plan(['v' => 2]) + ); + self::fail('expected WriteFailed'); + } catch (WriteFailed) { + self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options); + } + } + + /** The design_rev is checked against the inventory read under the lock, not before it. */ + public function testTheRevIsCheckedOnTheInventoryReadUnderTheLock(): void + { + $stored = ['v' => 1]; + $area = self::area($stored); + $rev = $area->rev($stored); + $snapshots = Mockery::mock(SnapshotStore::class); + $snapshots->shouldNotReceive('save'); + Functions\when('add_option')->alias(function (string $name, mixed $value = '') use (&$stored): bool { + $stored = ['v' => 'parallel']; + $this->options[$name] = $value; + + return true; + }); + + $result = (new DesignChange($snapshots, FakeChangeLog::forbidden()))->run( + 'lit/divi-presets-set', + $area, + ['confirm' => true, 'design_rev' => $rev], + self::plan(['v' => 2]) + ); + + self::assertToolError($result, 'rev_mismatch'); + self::assertSame([], $area->writes); + self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options); + } + public function testBlockedUsagesRefuseTheConfirm(): void { $stored = ['v' => 1]; diff --git a/tests/Unit/Integrations/Divi/Design/DesignLockTest.php b/tests/Unit/Integrations/Divi/Design/DesignLockTest.php new file mode 100644 index 0000000..7e63891 --- /dev/null +++ b/tests/Unit/Integrations/Divi/Design/DesignLockTest.php @@ -0,0 +1,73 @@ + */ + private array $options = []; + + protected function setUp(): void + { + parent::setUp(); + $this->options = []; + Functions\when('add_option')->alias(function (string $name, mixed $value = '', string $unused = '', mixed $autoload = null): bool { + self::assertFalse($autoload, 'locks are never autoloaded'); + if (array_key_exists($name, $this->options)) { + return false; + } + $this->options[$name] = $value; + + return true; + }); + Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default); + Functions\when('delete_option')->alias(function (string $name): bool { + unset($this->options[$name]); + + return true; + }); + } + + public function testAcquireIsExclusiveUntilReleased(): void + { + $lock = new DesignLock(); + + self::assertTrue($lock->acquire('presets')); + self::assertIsInt($this->options['lit_mcp_lock_presets']); + self::assertFalse($lock->acquire('presets')); + self::assertTrue($lock->acquire('colors'), 'areas lock independently'); + + $lock->release('presets'); + self::assertTrue($lock->acquire('presets')); + } + + public function testAStaleLockIsTakenOver(): void + { + $this->options['lit_mcp_lock_presets'] = time() - DesignLock::STALE_AFTER - 1; + + self::assertTrue((new DesignLock())->acquire('presets')); + self::assertGreaterThan(time() - 5, $this->options['lit_mcp_lock_presets']); + } + + public function testAFreshLockIsNotTakenOver(): void + { + $this->options['lit_mcp_lock_presets'] = time() - 10; + + self::assertFalse((new DesignLock())->acquire('presets')); + } + + public function testOptionNamesAndUnknownAreas(): void + { + self::assertSame('lit_mcp_lock_theme_options', DesignLock::option('theme_options')); + + $this->expectException(InvalidArgumentException::class); + DesignLock::option('pages'); + } +} diff --git a/tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php b/tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php index db24ad3..81bb7be 100644 --- a/tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php +++ b/tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php @@ -15,6 +15,14 @@ use Mockery; final class DiviDesignRestoreAbilityTest extends TestCase { + protected function setUp(): void + { + parent::setUp(); + // DesignLock (final review I-5): the lock is always free here. + Functions\when('add_option')->justReturn(true); + Functions\when('delete_option')->justReturn(true); + } + public function testRestoresWithDryRunAndASnapshotOfTheCurrentState(): void { Functions\when('current_user_can')->justReturn(true); diff --git a/tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php b/tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php index b3eb004..af4dc39 100644 --- a/tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php +++ b/tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php @@ -17,6 +17,14 @@ use Mockery; final class DiviPresetsDeleteAbilityTest extends TestCase { + protected function setUp(): void + { + parent::setUp(); + // DesignLock (final review I-5): the lock is always free here. + Functions\when('add_option')->justReturn(true); + Functions\when('delete_option')->justReturn(true); + } + private function ability(UsageReport $report, ?DesignSystemWriter $writer = null, ?SnapshotStore $snapshots = null): DiviPresetsDeleteAbility { Functions\when('current_user_can')->justReturn(true); diff --git a/tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php b/tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php index b668551..e1b7692 100644 --- a/tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php +++ b/tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php @@ -36,6 +36,9 @@ final class DiviPresetsSetAbilityTest extends TestCase $this->stored = PresetBookTest::stored(); $this->snapshots = Mockery::mock(SnapshotStore::class); Functions\when('current_user_can')->justReturn(true); + // DesignLock (final review I-5): the lock is always free here. + Functions\when('add_option')->justReturn(true); + Functions\when('delete_option')->justReturn(true); } private function ability(bool $unfiltered = true, ?GlobalUsages $usages = null, ?DiviPageLoader $loader = null, ?DesignSystemWriter $writer = null): DiviPresetsSetAbility diff --git a/tests/Unit/UninstallTest.php b/tests/Unit/UninstallTest.php index 8a1774e..118f445 100644 --- a/tests/Unit/UninstallTest.php +++ b/tests/Unit/UninstallTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Tests\Unit; +use LIT\MCP\Integrations\Divi\Design\DesignLock; use Brain\Monkey\Functions; use LIT\MCP\Abilities\ExposurePolicy; use LIT\MCP\Access; @@ -57,6 +58,8 @@ final class UninstallTest extends TestCase ReleaseStore::OPTION, CheckReportStore::OPTION, DiviCheckRoute::LOCK, 'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets', 'lit_mcp_snapshots_theme_options', + DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'), + DesignLock::option('theme_options'), ]; self::assertSame(array_merge($own, $own), $deleted); self::assertNotContains('mcp_jwt_secret', $deleted); diff --git a/tests/Unit/UninstallerTest.php b/tests/Unit/UninstallerTest.php index 3fc8622..2570e3d 100644 --- a/tests/Unit/UninstallerTest.php +++ b/tests/Unit/UninstallerTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Tests\Unit; +use LIT\MCP\Integrations\Divi\Design\DesignLock; use Brain\Monkey\Functions; use LIT\MCP\Abilities\ExposurePolicy; use LIT\MCP\Access; @@ -63,6 +64,8 @@ final class UninstallerTest extends TestCase ReleaseStore::OPTION, CheckReportStore::OPTION, DiviCheckRoute::LOCK, 'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets', 'lit_mcp_snapshots_theme_options', + DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'), + DesignLock::option('theme_options'), ], $deleted );