fix: verify option writes before treating update_option() false as failure

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7

src/Integrations/WordPress/Settings/SettingsWhitelist.php

@@ -101,7 +101,9 @@ final class SettingsWhitelist
 
     private function checkLanguage(mixed $value): ?string
     {
-        if ($value === 'en_US' || $value === '') {
+        // Clients say en_US for English; '' is WordPress' internal storage form for it
+        // (see normalize()) and is not accepted as input.
+        if ($value === 'en_US') {
             return null;
         }
 

src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php

@@ -138,7 +138,7 @@ final class WpSettingsUpdateAbility implements Ability
         try {
             foreach ($settings as $key => $value) {
                 $key = (string) $key;
-                if ($previous[$key] === $value) {
+                if ($this->isStored($key, $this->whitelist->normalize($key, $value))) {
                     continue;
                 }
 
@@ -168,17 +168,34 @@ final class WpSettingsUpdateAbility implements Ability
 
     private function writeOne(string $key, mixed $value): void
     {
+        $normalized = $this->whitelist->normalize($key, $value);
+
         if ($key === 'permalink_structure') {
-            $this->setPermalinkStructure((string) $value);
+            $this->setPermalinkStructure((string) $normalized);
 
             return;
         }
 
-        if (update_option($key, $this->whitelist->normalize($key, $value)) === false) {
+        // update_option() returns false both for a genuine failure and for a value
-            throw new RuntimeException(sprintf('update_option(%s) returned false', $key));
+        // that already matches the stored option (WordPress core short-circuits the
+        // write in that case, see wp-includes/option.php) – get_option() afterwards
+        // tells the two apart, the same way MetaFieldSource::holds() does for meta.
+        if (update_option($key, $normalized) === false && ! $this->isStored($key, $normalized)) {
+            throw new RuntimeException(sprintf('update_option(%s) failed', $key));
         }
     }
 
+    /** Whether option $key already holds $normalized, compared like WordPress compares scalars. */
+    private function isStored(string $key, mixed $normalized): bool
+    {
+        $stored = get_option($key);
+        if (is_scalar($stored) && is_scalar($normalized)) {
+            return (string) $stored === (string) $normalized;
+        }
+
+        return $stored == $normalized; // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual
+    }
+
     /**
      * A write failed; restores the keys already written to their previous values and
      * reports write_failed (fully restored) or partial_write (some keys could not be
@@ -223,5 +240,11 @@ final class WpSettingsUpdateAbility implements Ability
         global $wp_rewrite;
         $wp_rewrite->set_permalink_structure($structure);
         flush_rewrite_rules();
+
+        // set_permalink_structure() has no return value to check; verify the option
+        // itself so a failure here is caught and rolled back like any other setting.
+        if (! $this->isStored('permalink_structure', $structure)) {
+            throw new RuntimeException('set_permalink_structure() did not update the stored value.');
+        }
     }
 }

tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php

@@ -71,6 +71,17 @@ final class SettingsWhitelistTest extends TestCase
         self::assertSame('', $whitelist->normalize('WPLANG', 'en_US'));
     }
 
+    public function testWplangAcceptsOnlyTheEnUsSentinelNotTheRawEmptyString(): void
+    {
+        // '' is WordPress' internal storage form for English (see current()/normalize());
+        // clients must say en_US, never the internal '', which is also not an installed
+        // language code and would otherwise be rejected as "not installed" anyway.
+        Functions\when('get_available_languages')->justReturn(['de_DE']);
+        $whitelist = new SettingsWhitelist();
+
+        self::assertNotNull($whitelist->validate('WPLANG', '', []));
+    }
+
     public function testCurrentNormalizesLanguageAndNumbers(): void
     {
         Functions\when('get_option')->alias(static fn (string $key) => ['WPLANG' => '', 'page_on_front' => '5'][$key]);

tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php

@@ -41,16 +41,41 @@ final class WpSettingsAbilitiesTest extends TestCase
         self::assertArrayHasKey('admin_email', $result->get_error_data()['fields']);
     }
 
+    public function testRejectsAGenuinelyUnknownKey(): void
+    {
+        Functions\expect('update_option')->never();
+
+        $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), Mockery::mock(ChangeLog::class)))
+            ->execute(['settings' => ['template' => 'twentyten']]);
+
+        self::assertSame('lit_validation_failed', $result->get_error_code());
+        self::assertArrayHasKey('template', $result->get_error_data()['fields']);
+    }
+
     public function testUpdatesOnlyChangedValuesAndFlushesPermalinks(): void
     {
-        Functions\when('get_option')->alias(static fn (string $key) => [
+        // A mutating stub: writeOne() verifies a write via get_option() afterwards, so
+        // the stub must actually reflect what update_option()/set_permalink_structure()
+        // "wrote", not just return a fixed value.
+        $options = [
             'blogname' => 'Alt',
             'blogdescription' => 'Gleich',
             'permalink_structure' => '',
-        ][$key]);
+        ];
-        Functions\expect('update_option')->once()->with('blogname', 'Neu');
+        Functions\when('get_option')->alias(static function (string $key) use (&$options) {
+            return $options[$key];
+        });
+        Functions\expect('update_option')->once()->with('blogname', 'Neu')
+            ->andReturnUsing(static function (string $key, $value) use (&$options): bool {
+                $options[$key] = $value;
+
+                return true;
+            });
         $rewrite = Mockery::mock();
-        $rewrite->expects('set_permalink_structure')->with('/%postname%/');
+        $rewrite->expects('set_permalink_structure')->with('/%postname%/')
+            ->andReturnUsing(static function (string $structure) use (&$options): void {
+                $options['permalink_structure'] = $structure;
+            });
         $GLOBALS['wp_rewrite'] = $rewrite;
         Functions\expect('flush_rewrite_rules')->once();
         $log = Mockery::mock(ChangeLog::class);
@@ -67,6 +92,32 @@ final class WpSettingsAbilitiesTest extends TestCase
         self::assertSame(30, $result['log_id']);
     }
 
+    public function testDoesNotTreatAnAlreadyStoredValueAsAWriteFailure(): void
+    {
+        // update_option() returns false both for a genuine failure and for a value that
+        // already matches the stored option. WPLANG's public value "en_US" normalizes to
+        // WordPress' internal "" – if the "already set?" check compared the raw client
+        // input against SettingsWhitelist::current()'s aliased public form instead of the
+        // normalized value against the actual stored option, this looked like a change,
+        // update_option() returned false because nothing needed to change, and that false
+        // was wrongly read as a failure – rolling back the unrelated blogname change too.
+        Functions\when('get_option')->alias(static fn (string $key) => [
+            'blogname' => 'Alt',
+            'WPLANG' => '',
+        ][$key]);
+        Functions\expect('update_option')->once()->with('blogname', 'Neu')->andReturn(true);
+        $log = Mockery::mock(ChangeLog::class);
+        $log->expects('record')->andReturn(31);
+
+        $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => [
+            'blogname' => 'Neu',
+            'WPLANG' => 'en_US',
+        ]]);
+
+        self::assertSame(['blogname'], array_column($result['changed'], 'id'));
+        self::assertSame(31, $result['log_id']);
+    }
+
     public function testRestoresPreviousValuesWhenAWriteFailsPartway(): void
     {
         Functions\when('get_option')->alias(static fn (string $key) => [
@@ -96,11 +147,21 @@ final class WpSettingsAbilitiesTest extends TestCase
 
     public function testReportsPartialWriteWhenRestoringAlsoFails(): void
     {
-        Functions\when('get_option')->alias(static fn (string $key) => [
+        // Stateful: the restore write must genuinely not take effect (stay "Neu") for
+        // isStored() to correctly report the rollback as incomplete.
+        $options = [
             'blogname' => 'Alt',
             'blogdescription' => 'Vorher',
-        ][$key]);
+        ];
-        Functions\expect('update_option')->once()->with('blogname', 'Neu')->andReturn(true);
+        Functions\when('get_option')->alias(static function (string $key) use (&$options) {
+            return $options[$key];
+        });
+        Functions\expect('update_option')->once()->with('blogname', 'Neu')
+            ->andReturnUsing(static function (string $key, $value) use (&$options): bool {
+                $options[$key] = $value;
+
+                return true;
+            });
         Functions\expect('update_option')->once()->with('blogdescription', 'Auch neu')->andReturn(false);
         Functions\expect('update_option')->once()->with('blogname', 'Alt')->andReturn(false);
         $log = Mockery::mock(ChangeLog::class);