diff --git a/src/Integrations/WordPress/Settings/SettingsWhitelist.php b/src/Integrations/WordPress/Settings/SettingsWhitelist.php index 6639939..1a239b8 100644 --- a/src/Integrations/WordPress/Settings/SettingsWhitelist.php +++ b/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; } diff --git a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php b/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php index 7ba3072..26451c4 100644 --- a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php +++ b/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) { - throw new RuntimeException(sprintf('update_option(%s) returned false', $key)); + // update_option() returns false both for a genuine failure and for a value + // 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.'); + } } } diff --git a/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php b/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php index ea11f92..e1f55f2 100644 --- a/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php +++ b/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]); diff --git a/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php b/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php index 2225125..f250978 100644 --- a/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php +++ b/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);