From 5ffe2847ac5c5bbc708a75e28a12d1f99c86950c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maur=C3=ADcio=20Meneghini=20Fauth?= Date: Sat, 29 Apr 2023 14:50:18 -0300 Subject: [PATCH] Remove Config::checkServers() method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the Config::checkServers() method as it's duplicated in Config\Settings class. Also uses the Settings class to filter invalid config keys when loading a config file. Signed-off-by: Maurício Meneghini Fauth --- libraries/classes/Common.php | 1 - libraries/classes/Config.php | 75 +----------- libraries/classes/Config/Settings.php | 21 +++- phpstan-baseline.neon | 22 +--- psalm-baseline.xml | 3 - test/classes/AbstractTestCase.php | 1 - test/classes/Config/SettingsTest.php | 5 + test/classes/ConfigTest.php | 158 +++----------------------- 8 files changed, 45 insertions(+), 241 deletions(-) diff --git a/libraries/classes/Common.php b/libraries/classes/Common.php index 742095788b..ddee4ea67e 100644 --- a/libraries/classes/Common.php +++ b/libraries/classes/Common.php @@ -633,7 +633,6 @@ final class Common private static function setCurrentServerGlobal(ContainerInterface $container, Config $config): void { - $config->checkServers(); $server = $config->selectServer(); $GLOBALS['server'] = $server; $GLOBALS['urlParams']['server'] = $server; diff --git a/libraries/classes/Config.php b/libraries/classes/Config.php index 794ea4fa00..b50633d565 100644 --- a/libraries/classes/Config.php +++ b/libraries/classes/Config.php @@ -11,9 +11,7 @@ use PhpMyAdmin\Theme\ThemeManager; use Throwable; use function __; -use function array_filter; use function array_key_last; -use function array_merge; use function array_replace_recursive; use function array_slice; use function count; @@ -34,7 +32,6 @@ use function intval; use function is_array; use function is_bool; use function is_dir; -use function is_int; use function is_numeric; use function is_readable; use function is_string; @@ -53,7 +50,6 @@ use function realpath; use function rtrim; use function setcookie; use function sprintf; -use function str_contains; use function stripos; use function strtolower; use function substr; @@ -61,7 +57,6 @@ use function sys_get_temp_dir; use function time; use function trim; -use const ARRAY_FILTER_USE_KEY; use const DIRECTORY_SEPARATOR; use const PHP_OS; use const PHP_URL_PATH; @@ -103,7 +98,6 @@ class Config $this->config = new Settings([]); $this->defaultServer = $this->config->Servers[1]->asArray(); $config = $this->config->asArray(); - unset($config['Servers']); $this->default = $config; $this->settings = $config; $this->baseSettings = $config; @@ -346,6 +340,7 @@ class Config return false; } + /** @var mixed $cfg */ $cfg = []; /** @@ -379,25 +374,11 @@ class Config $this->sourceMtime = (int) filemtime($this->getSource()); } - /** - * Ignore keys with / as we do not use these - * - * These can be confusing for user configuration layer as it - * flatten array using / and thus don't see difference between - * $cfg['Export/method'] and $cfg['Export']['method'], while rest - * of the code uses the setting only in latter form. - * - * This could be removed once we consistently handle both values - * in the functional code as well. - */ - $cfg = array_filter( - $cfg, - static fn (string $key): bool => ! str_contains($key, '/'), - ARRAY_FILTER_USE_KEY, - ); + if (is_array($cfg)) { + $this->config = new Settings($cfg); + } - $this->settings = array_replace_recursive($this->settings, $cfg); - $this->config = new Settings($cfg); + $this->settings = array_replace_recursive($this->settings, $this->config->asArray()); return true; } @@ -1138,52 +1119,6 @@ class Config return (int) $server; } - /** - * Checks whether Servers configuration is valid and possibly apply fixups. - */ - public function checkServers(): void - { - // Do we have some server? - if (! isset($this->settings['Servers']) || count($this->settings['Servers']) === 0) { - // No server => create one with defaults - $this->settings['Servers'] = [1 => $this->defaultServer]; - $this->config = new Settings($this->settings); - - return; - } - - // We have server(s) => apply default configuration - $newServers = []; - - foreach ($this->settings['Servers'] as $serverIndex => $server) { - // Detect wrong configuration - if (! is_int($serverIndex) || $serverIndex < 1 || ! is_array($server)) { - continue; - } - - $server = array_merge($this->defaultServer, $server); - - // Final solution to bug #582890 - // If we are using a socket connection - // and there is nothing in the verbose server name - // or the host field, then generate a name for the server - // in the form of "Server 2", localized of course! - if (empty($server['host']) && empty($server['verbose'])) { - $server['verbose'] = sprintf(__('Server %d'), $serverIndex); - } - - $newServers[$serverIndex] = $server; - } - - if ($newServers === []) { - // Ensures it has at least one valid server config. - $newServers = [1 => $this->defaultServer]; - } - - $this->settings['Servers'] = $newServers; - $this->config = new Settings($this->settings); - } - /** * Return connection parameters for the database server * diff --git a/libraries/classes/Config/Settings.php b/libraries/classes/Config/Settings.php index f836394d23..f2cf8d0575 100644 --- a/libraries/classes/Config/Settings.php +++ b/libraries/classes/Config/Settings.php @@ -13,6 +13,7 @@ use PhpMyAdmin\Config\Settings\Server; use PhpMyAdmin\Config\Settings\SqlQueryBox; use PhpMyAdmin\Config\Settings\Transformations; +use function __; use function array_map; use function count; use function defined; @@ -21,6 +22,7 @@ use function is_array; use function is_int; use function is_string; use function min; +use function sprintf; use function strlen; use const DIRECTORY_SEPARATOR; @@ -2929,19 +2931,28 @@ final class Settings } $servers = []; - /** - * @var int|string $key - * @var mixed $server - */ foreach ($settings['Servers'] as $key => $server) { if (! is_int($key) || $key < 1 || ! is_array($server)) { continue; } $servers[$key] = new Server($server); + if ($servers[$key]->host !== '' || $servers[$key]->verbose !== '') { + continue; + } + + /** + * Ensures that the database server has a name. + * + * @link https://github.com/phpmyadmin/phpmyadmin/issues/6878 + * + * @psalm-suppress ImpureFunctionCall + */ + $server['verbose'] = sprintf(__('Server %d'), $key); + $servers[$key] = new Server($server); } - if (count($servers) === 0) { + if ($servers === []) { return [1 => new Server()]; } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 7f98021322..29498e2935 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -162,7 +162,7 @@ parameters: - message: "#^Argument of an invalid type mixed supplied for foreach, only iterables are supported\\.$#" - count: 2 + count: 1 path: libraries/classes/Config.php - @@ -215,11 +215,6 @@ parameters: count: 2 path: libraries/classes/Config.php - - - message: "#^Parameter \\#1 \\$value of function count expects array\\|Countable, mixed given\\.$#" - count: 1 - path: libraries/classes/Config.php - - message: "#^Parameter \\#3 \\$default of method PhpMyAdmin\\\\Config\\:\\:setCookie\\(\\) expects string\\|null, mixed given\\.$#" count: 1 @@ -9185,26 +9180,11 @@ parameters: count: 1 path: test/classes/Config/SettingsTest.php - - - message: "#^Cannot access offset 1 on mixed\\.$#" - count: 1 - path: test/classes/ConfigTest.php - - message: "#^Parameter \\#1 \\$haystack of function mb_strstr expects string, array\\ given\\.$#" count: 1 path: test/classes/ConfigTest.php - - - message: "#^Parameter \\#2 \\$array of method PHPUnit\\\\Framework\\\\Assert\\:\\:assertArrayHasKey\\(\\) expects array\\|ArrayAccess, mixed given\\.$#" - count: 3 - path: test/classes/ConfigTest.php - - - - message: "#^Parameter \\#2 \\$array of method PHPUnit\\\\Framework\\\\Assert\\:\\:assertArrayNotHasKey\\(\\) expects array\\|ArrayAccess, mixed given\\.$#" - count: 7 - path: test/classes/ConfigTest.php - - message: "#^Cannot access offset 'favoriteTables' on mixed\\.$#" count: 1 diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 6cdd67441b..c11729a067 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -261,7 +261,6 @@ $path - settings['Servers']]]> settings['ThemeDefault']]]> settings['ThemeDefault']]]> $url @@ -14157,8 +14156,6 @@ mixed[] mixed[] mixed[] - mixed[] - mixed[] diff --git a/test/classes/AbstractTestCase.php b/test/classes/AbstractTestCase.php index 09c6e8e7ec..6971b8581b 100644 --- a/test/classes/AbstractTestCase.php +++ b/test/classes/AbstractTestCase.php @@ -167,7 +167,6 @@ abstract class AbstractTestCase extends TestCase protected function setGlobalConfig(): void { $GLOBALS['config'] = $this->createConfig(); - $GLOBALS['config']->checkServers(); $GLOBALS['config']->set('environment', 'development'); $GLOBALS['cfg'] = $GLOBALS['config']->settings; } diff --git a/test/classes/Config/SettingsTest.php b/test/classes/Config/SettingsTest.php index 600b45e200..8d2b00bb01 100644 --- a/test/classes/Config/SettingsTest.php +++ b/test/classes/Config/SettingsTest.php @@ -1172,6 +1172,11 @@ class SettingsTest extends TestCase yield 'null value' => [null, [1 => $server]]; yield 'valid value' => [[1 => [], 2 => []], [1 => $server, 2 => $server]]; yield 'valid value 2' => [[2 => ['host' => 'test']], [2 => new Server(['host' => 'test'])]]; + yield 'valid value 3' => [ + [4 => ['host' => '', 'verbose' => '']], + [4 => new Server(['host' => '', 'verbose' => 'Server 4'])], + ]; + yield 'invalid value' => ['invalid', [1 => $server]]; yield 'invalid value 2' => [[0 => [], 2 => 'invalid', 'invalid' => [], 4 => []], [4 => $server]]; yield 'invalid value 3' => [[0 => []], [1 => $server]]; diff --git a/test/classes/ConfigTest.php b/test/classes/ConfigTest.php index 40149b7d95..e46a231c80 100644 --- a/test/classes/ConfigTest.php +++ b/test/classes/ConfigTest.php @@ -8,7 +8,6 @@ use PhpMyAdmin\Config; use PhpMyAdmin\Config\Settings; use PhpMyAdmin\Dbal\Connection; -use function array_merge; use function define; use function defined; use function file_exists; @@ -17,6 +16,7 @@ use function fileperms; use function function_exists; use function gd_info; use function mb_strstr; +use function md5; use function ob_end_clean; use function ob_get_contents; use function ob_start; @@ -83,9 +83,6 @@ class ConfigTest extends AbstractTestCase unset($this->permTestObj); } - /** - * Test for load - */ public function testLoadConfigs(): void { $defaultConfig = $this->createConfig(); @@ -102,70 +99,24 @@ class ConfigTest extends AbstractTestCase $config = new Config(); $config->loadAndCheck($tmpConfig); $this->assertSame($defaultConfig->settings, $config->settings); + $this->assertEquals($defaultConfig->getSettings(), $config->getSettings()); - $contents = 'loadAndCheck($tmpConfig); - $defaultConfig->settings['ProtectBinary'] = true; + $defaultConfig->set('environment', 'development'); $this->assertSame($defaultConfig->settings, $config->settings); - $defaultConfig->settings['ProtectBinary'] = 'blob'; - - // Teardown - unlink($tmpConfig); - $this->assertFalse(file_exists($tmpConfig)); - } - - /** - * Test for load - */ - public function testLoadInvalidConfigs(): void - { - $defaultConfig = $this->createConfig(); - $tmpConfig = tempnam('./', 'config.test.inc.php'); - if ($tmpConfig === false) { - $this->markTestSkipped('Creating a temporary file does not work'); - } - - $this->assertFileExists($tmpConfig); - - // end of setup - - // Test loading an empty file does not change the default config - $config = new Config(); - $config->loadAndCheck($tmpConfig); - $this->assertSame($defaultConfig->settings, $config->settings); - - $contents = 'loadAndCheck($tmpConfig); - $defaultConfig->settings['fooBar'] = true; - // Equals because of the key sorting - $this->assertEquals($defaultConfig->settings, $config->settings); - unset($defaultConfig->settings['fooBar']); - - $contents = 'loadAndCheck($tmpConfig); - $defaultConfig->settings['ValidKey'] = true; - // Equals because of the key sorting - $this->assertEquals($defaultConfig->settings, $config->settings); - unset($defaultConfig->settings['ValidKey']); + $this->assertArrayHasKey('environment', $config->settings); + $this->assertSame($config->settings['environment'], 'development'); + $this->assertArrayNotHasKey('UnknownKey', $config->settings); + $this->assertEquals($defaultConfig->getSettings(), $config->getSettings()); // Teardown unlink($tmpConfig); @@ -427,8 +378,7 @@ class ConfigTest extends AbstractTestCase $this->assertIsArray($config['Servers']); $this->assertEquals($settings, $object->getSettings()); $this->assertSame($config['Servers'][1], $object->defaultServer); - unset($config['Servers']); - $this->assertSame($config, $object->default); + $this->assertEquals($config, $object->default); $this->assertSame($config, $object->settings); $this->assertSame($config, $object->baseSettings); } @@ -767,77 +717,6 @@ class ConfigTest extends AbstractTestCase ); } - /** - * Test for checkServers - * - * @param mixed[] $settings settings array - * @param mixed[] $expected expected result - * - * @dataProvider serverSettingsProvider - */ - public function testCheckServers(array $settings, array $expected): void - { - $this->object->settings['Servers'] = $settings; - $this->object->checkServers(); - $expected = array_merge($this->object->defaultServer, $expected); - - $this->assertEquals($expected, $this->object->settings['Servers'][1]); - } - - /** - * Data provider for checkServers test - * - * @return mixed[] - */ - public static function serverSettingsProvider(): array - { - return [ - 'empty' => [[], []], - 'only_host' => [[1 => ['host' => '127.0.0.1']], ['host' => '127.0.0.1']], - 'empty_host' => [[1 => ['host' => '']], ['verbose' => 'Server 1', 'host' => '']], - ]; - } - - public function testCheckServersWithInvalidServer(): void - { - $server = ['host' => '127.0.0.1']; - $this->object->settings['Servers'] = ['invalid' => $server, 1 => $server, 0 => $server, 2 => 'invalid']; - $this->object->checkServers(); - $expected = array_merge($this->object->defaultServer, $server); - - $this->assertArrayNotHasKey('invalid', $this->object->settings['Servers']); - $this->assertArrayNotHasKey(0, $this->object->settings['Servers']); - $this->assertArrayNotHasKey(2, $this->object->settings['Servers']); - $this->assertArrayHasKey(1, $this->object->settings['Servers']); - $this->assertEquals($expected, $this->object->settings['Servers'][1]); - } - - public function testCheckServersWithOnlyInvalidServers(): void - { - $server = ['host' => '127.0.0.1']; - $this->object->settings['Servers'] = ['invalid' => $server, -1 => $server, 0 => $server]; - $this->object->checkServers(); - - $this->assertArrayNotHasKey('invalid', $this->object->settings['Servers']); - $this->assertArrayNotHasKey(0, $this->object->settings['Servers']); - $this->assertArrayNotHasKey(-1, $this->object->settings['Servers']); - $this->assertArrayHasKey(1, $this->object->settings['Servers']); - /** @psalm-suppress InvalidArrayOffset */ - $this->assertEquals($this->object->defaultServer, $this->object->settings['Servers'][1]); - } - - public function testCheckServersWithServerKeysGreaterThanOne(): void - { - $server = ['host' => '127.0.0.1']; - $this->object->settings['Servers'] = [2 => $server]; - $this->object->checkServers(); - $expected = array_merge($this->object->defaultServer, $server); - - $this->assertArrayNotHasKey(1, $this->object->settings['Servers']); - $this->assertArrayHasKey(2, $this->object->settings['Servers']); - $this->assertEquals($expected, $this->object->settings['Servers'][2]); - } - /** * Test for selectServer * @@ -846,20 +725,19 @@ class ConfigTest extends AbstractTestCase * @param int $expected expected result * * @dataProvider selectServerProvider - * @depends testCheckServers */ public function testSelectServer(array $settings, string $request, int $expected): void { - $this->object->settings['Servers'] = $settings; - $this->object->checkServers(); + $object = new Config(); + $object->settings = (new Settings(['Servers' => $settings]))->asArray(); $_REQUEST['server'] = $request; - $this->assertEquals($expected, $this->object->selectServer()); + $this->assertEquals($expected, $object->selectServer()); } /** * Data provider for selectServer test * - * @return mixed[] + * @return array */ public static function selectServerProvider(): array { @@ -868,7 +746,7 @@ class ConfigTest extends AbstractTestCase 'number' => [[1 => []], '1', 1], 'host' => [[2 => ['host' => '127.0.0.1']], '127.0.0.1', 2], 'verbose' => [[1 => ['verbose' => 'Server 1', 'host' => '']], 'Server 1', 1], - 'md5' => [[66 => ['verbose' => 'Server 1', 'host' => '']], '753f173bd4ac8a45eae0fe9a4fbe0fc0', 66], + 'md5' => [[66 => ['verbose' => 'Server 66', 'host' => '']], md5('server 66'), 66], 'nonexisting_string' => [[1 => []], 'invalid', 1], 'nonexisting' => [[1 => []], '100', 1], ];