From 9e19f79c41b8c967a0d8b684e5512f5ac15b9cbb Mon Sep 17 00:00:00 2001 From: michalsn Date: Wed, 13 Aug 2025 17:15:40 +0200 Subject: [PATCH 1/4] fix: Connection::getFieldData() default value convention for SQLSRV and OCI8 --- system/Database/OCI8/Connection.php | 2 +- system/Database/SQLSRV/Connection.php | 32 ++++++++- tests/system/Database/Live/ForgeTest.php | 4 +- .../Live/SQLSRV/GetFieldDataTestCase.php | 65 +++++++++++++++++-- user_guide_src/source/changelogs/v4.6.4.rst | 1 + 5 files changed, 96 insertions(+), 8 deletions(-) diff --git a/system/Database/OCI8/Connection.php b/system/Database/OCI8/Connection.php index 67921d2c13f8..d4b55fbc7264 100644 --- a/system/Database/OCI8/Connection.php +++ b/system/Database/OCI8/Connection.php @@ -349,7 +349,7 @@ protected function _fieldData(string $table): array $retval[$i]->max_length = $length; $retval[$i]->nullable = $query[$i]->NULLABLE === 'Y'; - $retval[$i]->default = $query[$i]->DATA_DEFAULT; + $retval[$i]->default = rtrim($query[$i]->DATA_DEFAULT); } return $retval; diff --git a/system/Database/SQLSRV/Connection.php b/system/Database/SQLSRV/Connection.php index 7ef4f31901d1..53927e036f59 100644 --- a/system/Database/SQLSRV/Connection.php +++ b/system/Database/SQLSRV/Connection.php @@ -384,12 +384,42 @@ protected function _fieldData(string $table): array ); $retVal[$i]->nullable = $query[$i]->IS_NULLABLE !== 'NO'; - $retVal[$i]->default = $query[$i]->COLUMN_DEFAULT; + $retVal[$i]->default = $this->normalizeDefault($query[$i]->COLUMN_DEFAULT); } return $retVal; } + /** + * Normalizes SQL Server COLUMN_DEFAULT values. + * Removes wrapping parentheses and handles basic conversions. + */ + private function normalizeDefault(?string $default): ?string + { + if ($default === null) { + return null; + } + + $default = trim($default); + + // Remove outer parentheses (handles both single and double wrapping) + while (preg_match('/^\((.*)\)$/', $default, $matches)) { + $default = trim($matches[1]); + } + + // Handle NULL literal + if (strcasecmp($default, 'NULL') === 0) { + return null; + } + + // Handle string literals - remove quotes and unescape + if (preg_match("/^'(.*)'$/s", $default, $matches)) { + return str_replace("''", "'", $matches[1]); + } + + return $default; + } + /** * Begin Transaction */ diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index d36a0e558685..c2e1e92b0357 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -1093,7 +1093,7 @@ public function testAddFields(): void 'type' => 'int', 'max_length' => 10, 'nullable' => false, - 'default' => '((0))', // Why? + 'default' => '0', ], ]; } elseif ($this->db->DBDriver === 'OCI8') { @@ -1124,7 +1124,7 @@ public function testAddFields(): void 'type' => 'NUMBER', 'max_length' => '11', 'nullable' => false, - 'default' => '0 ', // Why? + 'default' => '0', ], ]; diff --git a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php index 47aa8d108972..68123eca7003 100644 --- a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php +++ b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php @@ -15,7 +15,9 @@ use CodeIgniter\Database\Live\AbstractGetFieldDataTestCase; use Config\Database; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; +use ReflectionClass; /** * @internal @@ -68,7 +70,7 @@ public function testGetFieldDataDefault(): void 'type' => 'int', 'max_length' => 10, 'nullable' => false, - 'default' => '((0))', // int 0 + 'default' => '0', // 'primary_key' => 0, ], (object) [ @@ -76,7 +78,7 @@ public function testGetFieldDataDefault(): void 'type' => 'varchar', 'max_length' => 64, 'nullable' => true, - 'default' => '(NULL)', // NULL value + 'default' => null, // 'primary_key' => 0, ], (object) [ @@ -84,7 +86,7 @@ public function testGetFieldDataDefault(): void 'type' => 'varchar', 'max_length' => 64, 'nullable' => false, - 'default' => "('null')", // string "null" + 'default' => 'null', // string "null" // 'primary_key' => 0, ], (object) [ @@ -92,7 +94,7 @@ public function testGetFieldDataDefault(): void 'type' => 'varchar', 'max_length' => 64, 'nullable' => false, - 'default' => "('abc')", // string "abc" + 'default' => 'abc', // 'primary_key' => 0, ], ]; @@ -235,4 +237,59 @@ public function testGetFieldDataType(): void ]; $this->assertSameFieldData($expected, $fields); } + + #[DataProvider('provideNormalizeDefault')] + public function testNormalizeDefault(?string $input, ?string $expected, string $description): void + { + $reflection = new ReflectionClass($this->db); + $method = $reflection->getMethod('normalizeDefault'); + + $result = $method->invoke($this->db, $input); + + $this->assertSame($expected, $result, "Failed test: {$description}"); + } + + /** + * @return iterable + */ + public static function provideNormalizeDefault(): iterable + { + return [ + // [input, expected_output, description] + + // Null cases + [null, null, 'null input'], + ['(NULL)', null, 'NULL literal wrapped in parentheses'], + ['(null)', null, 'null literal lowercase'], + ['(Null)', null, 'null literal mixed case'], + ['(nULL)', null, 'null literal random case'], + + // String literal cases + ["('hello')", 'hello', 'simple string'], + ["('hello world')", 'hello world', 'string with space'], + ["('')", '', 'empty string literal'], + ["('can''t')", "can't", 'string with escaped quote'], + ["('it''s a ''test''')", "it's a 'test'", 'string with multiple escaped quotes'], + ["('line1'+char(10)+'line2')", "line1'+char(10)+'line2", 'concatenated multiline expression'], + + // Numeric cases + ['((0))', '0', 'zero with double parentheses'], + ['((123))', '123', 'positive integer with double parentheses'], + ['((-456))', '-456', 'negative integer with double parentheses'], + ['((3.14))', '3.14', 'float with double parentheses'], + + // Function/expression cases + ['(getdate())', 'getdate()', 'function call'], + ['(newid())', 'newid()', 'newid function'], + ['(user_name())', 'user_name()', 'user_name function'], + ['(current_timestamp)', 'current_timestamp', 'current_timestamp'], + ['((1+1))', '1+1', 'mathematical expression'], + ['((100*2))', '100*2', 'multiplication expression'], + + // Edge cases + ["((('nested')))", 'nested', 'multiple nested parentheses'], + ['plain_value', 'plain_value', 'value without parentheses'], + ['(complex_func(1, 2))', 'complex_func(1, 2)', 'function with parameters'], + ]; + } } diff --git a/user_guide_src/source/changelogs/v4.6.4.rst b/user_guide_src/source/changelogs/v4.6.4.rst index 8ef80eabbfcf..705901e231af 100644 --- a/user_guide_src/source/changelogs/v4.6.4.rst +++ b/user_guide_src/source/changelogs/v4.6.4.rst @@ -31,6 +31,7 @@ Bugs Fixed ********** - **Database:** Fixed a bug in ``Database::connect()`` which was causing to store non-shared connection instances in shared cache. +- **Database:** Fixed a bug in ``Connection::getFieldData()`` for ``SQLSRV`` and ``OCI8`` where extra characters were returned in column default values (specific to those handlers), instead of following the convention used by other drivers. See the repo's `CHANGELOG.md `_ From 450bf3876e5930c33c44337fe927039a25348526 Mon Sep 17 00:00:00 2001 From: michalsn Date: Wed, 13 Aug 2025 17:44:10 +0200 Subject: [PATCH 2/4] add normalizeDefault() method for OCI8 --- system/Database/OCI8/Connection.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/system/Database/OCI8/Connection.php b/system/Database/OCI8/Connection.php index d4b55fbc7264..796b412982a9 100644 --- a/system/Database/OCI8/Connection.php +++ b/system/Database/OCI8/Connection.php @@ -349,12 +349,25 @@ protected function _fieldData(string $table): array $retval[$i]->max_length = $length; $retval[$i]->nullable = $query[$i]->NULLABLE === 'Y'; - $retval[$i]->default = rtrim($query[$i]->DATA_DEFAULT); + $retval[$i]->default = $this->normalizeDefault($query[$i]->DATA_DEFAULT); } return $retval; } + /** + * Removes trailing whitespace from default values + * returned in database column metadata queries. + */ + private function normalizeDefault(?string $default): ?string + { + if ($default === null) { + return $default; + } + + return rtrim($default); + } + /** * Returns an array of objects with index data * From d1c7370f056a828f86a03b03eb644071ecdec6d9 Mon Sep 17 00:00:00 2001 From: michalsn Date: Wed, 13 Aug 2025 19:34:03 +0200 Subject: [PATCH 3/4] apply code review suggestions --- .../Live/SQLSRV/GetFieldDataTestCase.php | 65 +++++++++---------- .../deadCode.unreachable.neon | 7 +- utils/phpstan-baseline/loader.neon | 2 +- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php index 68123eca7003..9d12f29f6d26 100644 --- a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php +++ b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php @@ -17,7 +17,6 @@ use Config\Database; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; -use ReflectionClass; /** * @internal @@ -239,57 +238,53 @@ public function testGetFieldDataType(): void } #[DataProvider('provideNormalizeDefault')] - public function testNormalizeDefault(?string $input, ?string $expected, string $description): void + public function testNormalizeDefault(?string $input, ?string $expected): void { - $reflection = new ReflectionClass($this->db); - $method = $reflection->getMethod('normalizeDefault'); - - $result = $method->invoke($this->db, $input); - - $this->assertSame($expected, $result, "Failed test: {$description}"); + $normalizeDefault = self::getPrivateMethodInvoker($this->db, 'normalizeDefault'); + $this->assertSame($expected, $normalizeDefault($input)); } /** - * @return iterable + * @return iterable */ public static function provideNormalizeDefault(): iterable { return [ - // [input, expected_output, description] - // Null cases - [null, null, 'null input'], - ['(NULL)', null, 'NULL literal wrapped in parentheses'], - ['(null)', null, 'null literal lowercase'], - ['(Null)', null, 'null literal mixed case'], - ['(nULL)', null, 'null literal random case'], + 'null input' => [null, null], + 'NULL literal wrapped in parentheses' => ['(NULL)', null], + 'null literal lowercase' => ['(null)', null], + 'null literal mixed case' => ['(Null)', null], + 'null literal random case' => ['(nULL)', null], + 'null string' => ["('null')", 'null'], // String literal cases - ["('hello')", 'hello', 'simple string'], - ["('hello world')", 'hello world', 'string with space'], - ["('')", '', 'empty string literal'], - ["('can''t')", "can't", 'string with escaped quote'], - ["('it''s a ''test''')", "it's a 'test'", 'string with multiple escaped quotes'], - ["('line1'+char(10)+'line2')", "line1'+char(10)+'line2", 'concatenated multiline expression'], + 'simple string' => ["('hello')", 'hello'], + 'empty string' => ['(())', ''], + 'string with space' => ["('hello world')", 'hello world'], + 'empty string literal' => ["('')", ''], + 'string with escaped quote' => ["('can''t')", "can't"], + 'string with multiple escaped quotes' => ["('it''s a ''test''')", "it's a 'test'"], + 'concatenated multiline expression' => ["('line1'+char(10)+'line2')", "line1'+char(10)+'line2"], // Numeric cases - ['((0))', '0', 'zero with double parentheses'], - ['((123))', '123', 'positive integer with double parentheses'], - ['((-456))', '-456', 'negative integer with double parentheses'], - ['((3.14))', '3.14', 'float with double parentheses'], + 'zero with double parentheses' => ['((0))', '0'], + 'positive integer with double parentheses' => ['((123))', '123'], + 'negative integer with double parentheses' => ['((-456))', '-456'], + 'float with double parentheses' => ['((3.14))', '3.14'], // Function/expression cases - ['(getdate())', 'getdate()', 'function call'], - ['(newid())', 'newid()', 'newid function'], - ['(user_name())', 'user_name()', 'user_name function'], - ['(current_timestamp)', 'current_timestamp', 'current_timestamp'], - ['((1+1))', '1+1', 'mathematical expression'], - ['((100*2))', '100*2', 'multiplication expression'], + 'function call' => ['(getdate())', 'getdate()'], + 'newid function' => ['(newid())', 'newid()'], + 'user_name function' => ['(user_name())', 'user_name()'], + 'current_timestamp' => ['(current_timestamp)', 'current_timestamp'], + 'mathematical expression' => ['((1+1))', '1+1'], + 'multiplication expression' => ['((100*2))', '100*2'], // Edge cases - ["((('nested')))", 'nested', 'multiple nested parentheses'], - ['plain_value', 'plain_value', 'value without parentheses'], - ['(complex_func(1, 2))', 'complex_func(1, 2)', 'function with parameters'], + 'multiple nested parentheses' => ["((('nested')))", 'nested'], + 'value without parentheses' => ['plain_value', 'plain_value'], + 'function with parameters' => ['(complex_func(1, 2))', 'complex_func(1, 2)'], ]; } } diff --git a/utils/phpstan-baseline/deadCode.unreachable.neon b/utils/phpstan-baseline/deadCode.unreachable.neon index fb91b000b9c4..147f5acd4c43 100644 --- a/utils/phpstan-baseline/deadCode.unreachable.neon +++ b/utils/phpstan-baseline/deadCode.unreachable.neon @@ -1,7 +1,12 @@ -# total 1 error +# total 2 errors parameters: ignoreErrors: + - + message: '#^Unreachable statement \- code above always terminates\.$#' + count: 1 + path: ../../tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php + - message: '#^Unreachable statement \- code above always terminates\.$#' count: 1 diff --git a/utils/phpstan-baseline/loader.neon b/utils/phpstan-baseline/loader.neon index 08c8f5c3fcef..a92562caf39d 100644 --- a/utils/phpstan-baseline/loader.neon +++ b/utils/phpstan-baseline/loader.neon @@ -1,4 +1,4 @@ -# total 2836 errors +# total 2837 errors includes: - argument.type.neon - assign.propertyType.neon From 364ea9708a125264f2543b2255165a4c0532132a Mon Sep 17 00:00:00 2001 From: michalsn Date: Thu, 14 Aug 2025 07:14:01 +0200 Subject: [PATCH 4/4] apply code review suggestions --- tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php | 4 ++++ utils/phpstan-baseline/deadCode.unreachable.neon | 7 +------ utils/phpstan-baseline/loader.neon | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php index 9d12f29f6d26..c07566e187df 100644 --- a/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php +++ b/tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php @@ -14,6 +14,7 @@ namespace CodeIgniter\Database\Live\SQLSRV; use CodeIgniter\Database\Live\AbstractGetFieldDataTestCase; +use CodeIgniter\Database\SQLSRV\Connection; use Config\Database; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; @@ -240,6 +241,8 @@ public function testGetFieldDataType(): void #[DataProvider('provideNormalizeDefault')] public function testNormalizeDefault(?string $input, ?string $expected): void { + $this->assertInstanceOf(Connection::class, $this->db); + $normalizeDefault = self::getPrivateMethodInvoker($this->db, 'normalizeDefault'); $this->assertSame($expected, $normalizeDefault($input)); } @@ -284,6 +287,7 @@ public static function provideNormalizeDefault(): iterable // Edge cases 'multiple nested parentheses' => ["((('nested')))", 'nested'], 'value without parentheses' => ['plain_value', 'plain_value'], + 'value with parentheses' => ['( plain_value )', 'plain_value'], 'function with parameters' => ['(complex_func(1, 2))', 'complex_func(1, 2)'], ]; } diff --git a/utils/phpstan-baseline/deadCode.unreachable.neon b/utils/phpstan-baseline/deadCode.unreachable.neon index 147f5acd4c43..fb91b000b9c4 100644 --- a/utils/phpstan-baseline/deadCode.unreachable.neon +++ b/utils/phpstan-baseline/deadCode.unreachable.neon @@ -1,12 +1,7 @@ -# total 2 errors +# total 1 error parameters: ignoreErrors: - - - message: '#^Unreachable statement \- code above always terminates\.$#' - count: 1 - path: ../../tests/system/Database/Live/SQLSRV/GetFieldDataTestCase.php - - message: '#^Unreachable statement \- code above always terminates\.$#' count: 1 diff --git a/utils/phpstan-baseline/loader.neon b/utils/phpstan-baseline/loader.neon index a92562caf39d..08c8f5c3fcef 100644 --- a/utils/phpstan-baseline/loader.neon +++ b/utils/phpstan-baseline/loader.neon @@ -1,4 +1,4 @@ -# total 2837 errors +# total 2836 errors includes: - argument.type.neon - assign.propertyType.neon