From b4ed61d4444c3978ead134084980bf7d11f86b69 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 14:08:09 +0900 Subject: [PATCH 01/15] fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly --- system/Database/Forge.php | 12 +++++-- system/Database/SQLite3/Table.php | 12 +++---- tests/system/Database/Live/ForgeTest.php | 45 ++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/system/Database/Forge.php b/system/Database/Forge.php index a33deb3c1c4d..307c339c8b3e 100644 --- a/system/Database/Forge.php +++ b/system/Database/Forge.php @@ -897,13 +897,19 @@ protected function _processFields(bool $createTable = false): array $this->_attributeDefault($attributes, $field); if (isset($attributes['NULL'])) { + $nullString = ' ' . $this->null; + if ($attributes['NULL'] === true) { - $field['null'] = empty($this->null) ? '' : ' ' . $this->null; + $field['null'] = empty($this->null) ? '' : $nullString; + } elseif ($attributes['NULL'] === $nullString) { + $field['null'] = $nullString; + } elseif ($attributes['NULL'] === '') { + $field['null'] = ''; } else { - $field['null'] = ' NOT NULL'; + $field['null'] = ' NOT ' . $this->null; } } elseif ($createTable === true) { - $field['null'] = ' NOT NULL'; + $field['null'] = ' NOT ' . $this->null; } $this->_attributeAutoIncrement($attributes, $field); diff --git a/system/Database/SQLite3/Table.php b/system/Database/SQLite3/Table.php index e0367f16e49d..4ed9b5152eea 100644 --- a/system/Database/SQLite3/Table.php +++ b/system/Database/SQLite3/Table.php @@ -183,14 +183,14 @@ public function dropColumn($columns) * * @return Table */ - public function modifyColumn(array $field) + public function modifyColumn(array $fields) { - $field = $field[0]; - - $oldName = $field['name']; - unset($field['name']); + foreach ($fields as $field) { + $oldName = $field['name']; + unset($field['name']); - $this->fields[$oldName] = $field; + $this->fields[$oldName] = $field; + } return $this; } diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index d859bc93833c..6821136c3fae 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -17,6 +17,7 @@ use CodeIgniter\Test\DatabaseTestTrait; use Config\Database; use RuntimeException; +use stdClass; use Tests\Support\Database\Seeds\CITestSeeder; /** @@ -1276,6 +1277,50 @@ public function testModifyColumnRename() $this->forge->dropTable('forge_test_three', true); } + public function testModifyColumnNull() + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.'); + } + + $this->forge->dropTable('forge_test_modify', true); + + $this->forge->addField([ + 'col1' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true], + 'col2' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true], + 'col3' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => true], + ]); + $this->forge->createTable('forge_test_modify'); + + $this->forge->modifyColumn('forge_test_modify', [ + 'col1' => ['type' => 'VARCHAR', 'constraint' => 1], + 'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true], + 'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false], + ]); + + $this->db->resetDataCache(); + + $col1 = $this->getMetaData('col1', 'forge_test_modify'); + $this->assertTrue($col1->nullable); + $col2 = $this->getMetaData('col2', 'forge_test_modify'); + $this->assertTrue($col2->nullable); + $col3 = $this->getMetaData('col3', 'forge_test_modify'); + $this->assertFalse($col3->nullable); + + $this->forge->dropTable('forge_test_modify', true); + } + + private function getMetaData(string $column, string $table): stdClass + { + $fields = $this->db->getFieldData($table); + + return $fields[array_search( + $column, + array_column($fields, 'name'), + true + )]; + } + public function testConnectWithArrayGroup() { $group = config('Database'); From 3bd122c805e07144a7e5d2c3b1f0ea61f600d9b0 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 14:09:00 +0900 Subject: [PATCH 02/15] docs: add PHPDoc --- system/Database/Forge.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Database/Forge.php b/system/Database/Forge.php index 307c339c8b3e..5c27a4348ed4 100644 --- a/system/Database/Forge.php +++ b/system/Database/Forge.php @@ -562,7 +562,7 @@ public function createTable(string $table, bool $ifNotExists = false, array $att } /** - * @return string + * @return string SQL string * * @deprecated $ifNotExists is no longer used, and will be removed. */ From 7611d0e525eb10243e7cebe7836233f7c51b1e44 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 14:37:49 +0900 Subject: [PATCH 03/15] test: add test for NOT NULL columns --- tests/system/Database/Live/ForgeTest.php | 35 +++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index 6821136c3fae..185f3a261703 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -1277,7 +1277,7 @@ public function testModifyColumnRename() $this->forge->dropTable('forge_test_three', true); } - public function testModifyColumnNull() + public function testModifyColumnNullTrue() { if ($this->db->DBDriver === 'SQLSRV') { $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.'); @@ -1310,6 +1310,39 @@ public function testModifyColumnNull() $this->forge->dropTable('forge_test_modify', true); } + public function testModifyColumnNullFalse() + { + if ($this->db->DBDriver === 'SQLSRV') { + $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.'); + } + + $this->forge->dropTable('forge_test_modify', true); + + $this->forge->addField([ + 'col1' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false], + 'col2' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false], + 'col3' => ['type' => 'VARCHAR', 'constraint' => 255, 'null' => false], + ]); + $this->forge->createTable('forge_test_modify'); + + $this->forge->modifyColumn('forge_test_modify', [ + 'col1' => ['type' => 'VARCHAR', 'constraint' => 1], + 'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true], + 'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false], + ]); + + $this->db->resetDataCache(); + + $col1 = $this->getMetaData('col1', 'forge_test_modify'); + $this->assertTrue($col1->nullable); // Nullable by default. + $col2 = $this->getMetaData('col2', 'forge_test_modify'); + $this->assertTrue($col2->nullable); + $col3 = $this->getMetaData('col3', 'forge_test_modify'); + $this->assertFalse($col3->nullable); + + $this->forge->dropTable('forge_test_modify', true); + } + private function getMetaData(string $column, string $table): stdClass { $fields = $this->db->getFieldData($table); From 7b8355a7df8dc157d66e6e7087cbd77d3d8f892a Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 14:09:35 +0900 Subject: [PATCH 04/15] test: fix tests that depends on table state --- .../Migrations/2018-01-24-102301_Some_migration.php | 2 +- tests/system/Commands/Database/MigrateStatusTest.php | 4 ++++ .../DatabaseTestCaseMigrationOnce1Test.php | 3 +++ .../DatabaseTestCaseMigrationOnce2Test.php | 4 ++++ tests/system/Database/DatabaseTestCaseTest.php | 4 ++++ tests/system/Database/Migrations/MigrationRunnerTest.php | 9 +++++++++ 6 files changed, 25 insertions(+), 1 deletion(-) diff --git a/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php b/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php index a6eb0b54ab62..39eb6e0879cc 100644 --- a/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php +++ b/tests/_support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php @@ -21,7 +21,7 @@ public function up() 'constraint' => 255, ], ]); - $this->forge->createTable('foo', true); + $this->forge->createTable('foo'); $this->db->table('foo')->insert([ 'key' => 'foobar', diff --git a/tests/system/Commands/Database/MigrateStatusTest.php b/tests/system/Commands/Database/MigrateStatusTest.php index 10db52e8c6a8..be55a2e2e064 100644 --- a/tests/system/Commands/Database/MigrateStatusTest.php +++ b/tests/system/Commands/Database/MigrateStatusTest.php @@ -14,6 +14,7 @@ use CodeIgniter\CLI\CLI; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\StreamFilterTrait; +use Config\Database; /** * @group DatabaseLive @@ -29,6 +30,9 @@ final class MigrateStatusTest extends CIUnitTestCase protected function setUp(): void { + $forge = Database::forge(); + $forge->dropTable('foo', true); + parent::setUp(); if (! is_file($this->migrationFileFrom)) { diff --git a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php index 77c10b4933b1..7177c0e5da37 100644 --- a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php +++ b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php @@ -56,6 +56,9 @@ final class DatabaseTestCaseMigrationOnce1Test extends CIUnitTestCase protected function setUp(): void { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $this->setUpMethods[] = 'setUpAddNamespace'; parent::setUp(); diff --git a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php index ec24ffe03eea..6c8f2f4e3504 100644 --- a/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php +++ b/tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php @@ -13,6 +13,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; use Config\Services; /** @@ -55,6 +56,9 @@ final class DatabaseTestCaseMigrationOnce2Test extends CIUnitTestCase protected function setUp(): void { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $this->setUpMethods[] = 'setUpAddNamespace'; parent::setUp(); diff --git a/tests/system/Database/DatabaseTestCaseTest.php b/tests/system/Database/DatabaseTestCaseTest.php index 5d779727cc1e..0a1c164b3066 100644 --- a/tests/system/Database/DatabaseTestCaseTest.php +++ b/tests/system/Database/DatabaseTestCaseTest.php @@ -13,6 +13,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; use Config\Services; use Tests\Support\Database\Seeds\AnotherSeeder; use Tests\Support\Database\Seeds\CITestSeeder; @@ -60,6 +61,9 @@ final class DatabaseTestCaseTest extends CIUnitTestCase protected function setUp(): void { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $this->setUpMethods[] = 'setUpAddNamespace'; parent::setUp(); diff --git a/tests/system/Database/Migrations/MigrationRunnerTest.php b/tests/system/Database/Migrations/MigrationRunnerTest.php index 9872080b8719..a9fa0171c104 100644 --- a/tests/system/Database/Migrations/MigrationRunnerTest.php +++ b/tests/system/Database/Migrations/MigrationRunnerTest.php @@ -349,6 +349,9 @@ public function testLatestSuccess() public function testRegressSuccess() { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $runner = new MigrationRunner($this->config); $runner->setSilent(false) ->setNamespace('Tests\Support\MigrationTestMigrations') @@ -368,6 +371,9 @@ public function testRegressSuccess() public function testLatestTriggersEvent() { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $runner = new MigrationRunner($this->config); $runner->setSilent(false) ->setNamespace('Tests\Support\MigrationTestMigrations'); @@ -387,6 +393,9 @@ public function testLatestTriggersEvent() public function testRegressTriggersEvent() { + $forge = Database::forge(); + $forge->dropTable('foo', true); + $runner = new MigrationRunner($this->config); $runner->setSilent(false) ->setNamespace('Tests\Support\MigrationTestMigrations'); From 9b6d97034ea494d0f140808a58adc139d30e316e Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 17:21:19 +0900 Subject: [PATCH 05/15] test: improve test code --- tests/system/Database/Live/ForgeTest.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index 185f3a261703..2e9dd4d0abed 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -16,6 +16,7 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; use Config\Database; +use LogicException; use RuntimeException; use stdClass; use Tests\Support\Database\Seeds\CITestSeeder; @@ -1347,11 +1348,17 @@ private function getMetaData(string $column, string $table): stdClass { $fields = $this->db->getFieldData($table); - return $fields[array_search( + $name = array_search( $column, array_column($fields, 'name'), true - )]; + ); + + if ($name === false) { + throw new LogicException('Column not found: ' . $column); + } + + return $fields[$name]; } public function testConnectWithArrayGroup() From 40d9e8d257a276fea89b51decfc7da268c6f7df0 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 11:22:14 +0900 Subject: [PATCH 06/15] fix: Postgres Forge::modifyColumn() changes NULL incorrectly --- system/Database/Postgre/Forge.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/system/Database/Postgre/Forge.php b/system/Database/Postgre/Forge.php index 662fbe666595..2a935e2393bd 100644 --- a/system/Database/Postgre/Forge.php +++ b/system/Database/Postgre/Forge.php @@ -109,10 +109,14 @@ protected function _alterTable(string $alterType, string $table, $field) . " SET DEFAULT {$data['default']}"; } + $nullable = true; // Nullable by default. if (isset($data['null'])) { - $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) - . ($data['null'] === true ? ' DROP' : ' SET') . ' NOT NULL'; + if ($data['null'] === false || $data['null'] === ' NOT ' . $this->null) { + $nullable = false; + } } + $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) + . ($nullable === true ? ' DROP' : ' SET') . ' NOT NULL'; if (! empty($data['new_name'])) { $sqls[] = $sql . ' RENAME COLUMN ' . $this->db->escapeIdentifiers($data['name']) From 50133d5374d5593ffc4763ba2ac12bb6766507a6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 11:34:12 +0900 Subject: [PATCH 07/15] fix: default $null value It should be `NULL`. --- system/Database/Forge.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Database/Forge.php b/system/Database/Forge.php index 5c27a4348ed4..f83a9acf94a6 100644 --- a/system/Database/Forge.php +++ b/system/Database/Forge.php @@ -153,7 +153,7 @@ class Forge * * @internal Used for marking nullable fields. Not covered by BC promise. */ - protected $null = ''; + protected $null = 'NULL'; /** * DEFAULT value representation in CREATE/ALTER TABLE statements From fa8643bd5f0be14903fc9be3bb700e964202e66e Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 11:54:23 +0900 Subject: [PATCH 08/15] fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly --- system/Database/SQLSRV/Forge.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/system/Database/SQLSRV/Forge.php b/system/Database/SQLSRV/Forge.php index 8d7a720313f7..837f6be49d50 100755 --- a/system/Database/SQLSRV/Forge.php +++ b/system/Database/SQLSRV/Forge.php @@ -203,10 +203,14 @@ protected function _alterTable(string $alterType, string $table, $field) . " DEFAULT {$data['default']} FOR " . $this->db->escapeIdentifiers($data['name']); } + $nullable = true; // Nullable by default. if (isset($data['null'])) { - $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) - . ($data['null'] === true ? ' DROP' : '') . " {$data['type']}{$data['length']} NOT NULL"; + if ($data['null'] === false || $data['null'] === ' NOT ' . $this->null) { + $nullable = false; + } } + $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) + . " {$data['type']}{$data['length']} " . ($nullable === true ? '' : 'NOT') . ' NULL'; if (! empty($data['comment'])) { $sqls[] = 'EXEC sys.sp_addextendedproperty ' From 8ad1b8a76448b29eeb8d764905190b2640eda59b Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 23 Mar 2023 17:10:56 +0900 Subject: [PATCH 09/15] docs: add note for modifyColumn() --- user_guide_src/source/dbmgmt/forge.rst | 9 +++++++++ user_guide_src/source/dbmgmt/forge/026.php | 3 ++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/user_guide_src/source/dbmgmt/forge.rst b/user_guide_src/source/dbmgmt/forge.rst index bfb3bca9aea1..17a376b77f0d 100644 --- a/user_guide_src/source/dbmgmt/forge.rst +++ b/user_guide_src/source/dbmgmt/forge.rst @@ -294,6 +294,15 @@ change the name, you can add a "name" key into the field defining array. .. literalinclude:: forge/026.php +.. note:: The ``modifyColumn()`` may unexpectedly change ``NULL``/``NOT NULL``. + So it is recommended to always specify ``null`` value. + +.. note:: Due to a bug, prior v4.3.3, SQLite3 may not set ``NOT NULL`` even if you + specify ``'null' => false``. + +.. note:: Due to a bug, prior v4.3.3, Postgres and SQLSRV set ``NOT NULL`` even + if you specify ``'null' => false``. + .. _db-forge-adding-keys-to-a-table: Adding Keys to a Table diff --git a/user_guide_src/source/dbmgmt/forge/026.php b/user_guide_src/source/dbmgmt/forge/026.php index bd923d8a6ec0..be597f058b0b 100644 --- a/user_guide_src/source/dbmgmt/forge/026.php +++ b/user_guide_src/source/dbmgmt/forge/026.php @@ -4,7 +4,8 @@ 'old_name' => [ 'name' => 'new_name', 'type' => 'TEXT', + 'null' => false, ], ]; $forge->modifyColumn('table_name', $fields); -// gives ALTER TABLE `table_name` CHANGE `old_name` `new_name` TEXT +// gives ALTER TABLE `table_name` CHANGE `old_name` `new_name` TEXT NOT NULL From 00d9b17ec43dd8beaf3d51e8edf31c16e57266b7 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 12:43:22 +0900 Subject: [PATCH 10/15] refactor: by rector --- system/Database/Postgre/Forge.php | 6 ++---- system/Database/SQLSRV/Forge.php | 6 ++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/system/Database/Postgre/Forge.php b/system/Database/Postgre/Forge.php index 2a935e2393bd..47be1b063c8d 100644 --- a/system/Database/Postgre/Forge.php +++ b/system/Database/Postgre/Forge.php @@ -110,10 +110,8 @@ protected function _alterTable(string $alterType, string $table, $field) } $nullable = true; // Nullable by default. - if (isset($data['null'])) { - if ($data['null'] === false || $data['null'] === ' NOT ' . $this->null) { - $nullable = false; - } + if (isset($data['null']) && ($data['null'] === false || $data['null'] === ' NOT ' . $this->null)) { + $nullable = false; } $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) . ($nullable === true ? ' DROP' : ' SET') . ' NOT NULL'; diff --git a/system/Database/SQLSRV/Forge.php b/system/Database/SQLSRV/Forge.php index 837f6be49d50..e44624761850 100755 --- a/system/Database/SQLSRV/Forge.php +++ b/system/Database/SQLSRV/Forge.php @@ -204,10 +204,8 @@ protected function _alterTable(string $alterType, string $table, $field) } $nullable = true; // Nullable by default. - if (isset($data['null'])) { - if ($data['null'] === false || $data['null'] === ' NOT ' . $this->null) { - $nullable = false; - } + if (isset($data['null']) && ($data['null'] === false || $data['null'] === ' NOT ' . $this->null)) { + $nullable = false; } $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) . " {$data['type']}{$data['length']} " . ($nullable === true ? '' : 'NOT') . ' NULL'; From d88c6c22acd77ca6c578db8ba01a7d0b04c23e20 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 15:23:53 +0900 Subject: [PATCH 11/15] fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default For consistency. --- system/Database/OCI8/Forge.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/system/Database/OCI8/Forge.php b/system/Database/OCI8/Forge.php index 514cf99f41f4..31b20584fe74 100644 --- a/system/Database/OCI8/Forge.php +++ b/system/Database/OCI8/Forge.php @@ -120,10 +120,17 @@ protected function _alterTable(string $alterType, string $table, $field) // If a null constraint is added to a column with a null constraint, // ORA-01451 will occur, // so add null constraint is used only when it is different from the current null constraint. - $isWantToAddNull = strpos($field[$i]['null'], ' NOT') === false; - $currentNullAddable = $nullableMap[$field[$i]['name']]; + // If a not null constraint is added to a column with a not null constraint, + // ORA-01442 will occur. + $wantToAddNull = strpos($field[$i]['null'], ' NOT') === false; + $currentNullable = $nullableMap[$field[$i]['name']]; - if ($isWantToAddNull === $currentNullAddable) { + if ($wantToAddNull === true && $currentNullable === true) { + $field[$i]['null'] = ''; + } elseif ($field[$i]['null'] === '' && $currentNullable === false) { + // Nullable by default + $field[$i]['null'] = ' NULL'; + } elseif ($wantToAddNull === false && $currentNullable === false) { $field[$i]['null'] = ''; } } From 6621b5ae0737d7398853bdebfcc0622a0b3de953 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 16:32:22 +0900 Subject: [PATCH 12/15] docs: update docs --- user_guide_src/source/changelogs/v4.3.4.rst | 13 +++++++++++++ user_guide_src/source/dbmgmt/forge.rst | 6 +++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/user_guide_src/source/changelogs/v4.3.4.rst b/user_guide_src/source/changelogs/v4.3.4.rst index ec02c35e64df..187b36d2b0ba 100644 --- a/user_guide_src/source/changelogs/v4.3.4.rst +++ b/user_guide_src/source/changelogs/v4.3.4.rst @@ -35,6 +35,19 @@ Redirect Status Code always be used when you don't specify a status code. In previous versions, 302 might be changed. +Forge::modifyColumn() +--------------------- + +- The :ref:`$forge->modifyColumn() ` has been fixed. Due + to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change + ``NULL``/``NOT NULL`` unpredictably. +- In previous versions, the OCI8 driver did not change ``NULL``/``NOT NULL`` + when you don't specify ``null`` value. +- Now in all database drivers ``$forge->modifyColumn()`` always sets ``NULL`` + when you don't specify ``null`` value. +- The ``NULL``/``NOT NULL`` change may still be unexpectedly, it is recommended + to always specifiy ``null`` value. + Message Changes *************** diff --git a/user_guide_src/source/dbmgmt/forge.rst b/user_guide_src/source/dbmgmt/forge.rst index 17a376b77f0d..bc8f1c13bfc0 100644 --- a/user_guide_src/source/dbmgmt/forge.rst +++ b/user_guide_src/source/dbmgmt/forge.rst @@ -285,6 +285,8 @@ Used to remove multiple columns from a table. Modifying a Field in a Table ============================ +.. _db-forge-modifyColumn: + $forge->modifyColumn() ---------------------- @@ -295,7 +297,9 @@ change the name, you can add a "name" key into the field defining array. .. literalinclude:: forge/026.php .. note:: The ``modifyColumn()`` may unexpectedly change ``NULL``/``NOT NULL``. - So it is recommended to always specify ``null`` value. + So it is recommended to always specify ``null`` value. Unlike when creating + a table, if ``null`` is not specified, the column will be ``NULL``, not + ``NOT NULL``. .. note:: Due to a bug, prior v4.3.3, SQLite3 may not set ``NOT NULL`` even if you specify ``'null' => false``. From d73dc997a558b82e9d43036a0d42d61651cd8eb0 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 24 Mar 2023 17:02:48 +0900 Subject: [PATCH 13/15] docs: add @TODO --- tests/system/Database/Live/ForgeTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/system/Database/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index 2e9dd4d0abed..54704a79e4c1 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -1280,6 +1280,7 @@ public function testModifyColumnRename() public function testModifyColumnNullTrue() { + // @TODO remove this in `4.4` branch if ($this->db->DBDriver === 'SQLSRV') { $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.'); } @@ -1313,6 +1314,7 @@ public function testModifyColumnNullTrue() public function testModifyColumnNullFalse() { + // @TODO remove this in `4.4` branch if ($this->db->DBDriver === 'SQLSRV') { $this->markTestSkipped('SQLSRV does not support getFieldData() nullable.'); } From 69a5c290756f7e0801f411e05afcf67fdb3ba9a2 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 27 Mar 2023 09:37:13 +0900 Subject: [PATCH 14/15] docs: add upgrade note --- user_guide_src/source/changelogs/v4.3.4.rst | 2 ++ user_guide_src/source/installation/upgrade_434.rst | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.3.4.rst b/user_guide_src/source/changelogs/v4.3.4.rst index 187b36d2b0ba..8f67346825ea 100644 --- a/user_guide_src/source/changelogs/v4.3.4.rst +++ b/user_guide_src/source/changelogs/v4.3.4.rst @@ -35,6 +35,8 @@ Redirect Status Code always be used when you don't specify a status code. In previous versions, 302 might be changed. +.. _v434-forge-modifycolumn: + Forge::modifyColumn() --------------------- diff --git a/user_guide_src/source/installation/upgrade_434.rst b/user_guide_src/source/installation/upgrade_434.rst index 38f3305ca212..ac19ddc0f5ae 100644 --- a/user_guide_src/source/installation/upgrade_434.rst +++ b/user_guide_src/source/installation/upgrade_434.rst @@ -25,6 +25,18 @@ Redirect Status Code :ref:`ChangeLog v4.3.4 ` and if the code is not what you want, :ref:`specify status codes `. +Forge::modifyColumn() and NULL +============================== + +A bug fix may have changed the NULL constraint in the result of +:ref:`$forge->modifyColumn() `. See +:ref:`Change Log `. +To set the desired NULL constraint, change ``Forge::modifyColumn()`` to always +specify the ``null`` value. + +Note that the bug may have changed unexpected NULL constraints in previous +versions. + Breaking Enhancements ********************* From 3b416c8420f494355b45da0c99834fe299dc1643 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 27 Apr 2023 08:00:36 +0900 Subject: [PATCH 15/15] docs: improve explanation Apply suggestions from code review Co-authored-by: Michal Sniatala --- user_guide_src/source/changelogs/v4.3.4.rst | 6 +++--- user_guide_src/source/dbmgmt/forge.rst | 2 +- user_guide_src/source/installation/upgrade_434.rst | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/user_guide_src/source/changelogs/v4.3.4.rst b/user_guide_src/source/changelogs/v4.3.4.rst index 8f67346825ea..efa598d221b5 100644 --- a/user_guide_src/source/changelogs/v4.3.4.rst +++ b/user_guide_src/source/changelogs/v4.3.4.rst @@ -44,11 +44,11 @@ Forge::modifyColumn() to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change ``NULL``/``NOT NULL`` unpredictably. - In previous versions, the OCI8 driver did not change ``NULL``/``NOT NULL`` - when you don't specify ``null`` value. + when you don't specify the ``null`` key. - Now in all database drivers ``$forge->modifyColumn()`` always sets ``NULL`` - when you don't specify ``null`` value. + when you don't specify the ``null`` key. - The ``NULL``/``NOT NULL`` change may still be unexpectedly, it is recommended - to always specifiy ``null`` value. + to always specify the ``null`` key. Message Changes *************** diff --git a/user_guide_src/source/dbmgmt/forge.rst b/user_guide_src/source/dbmgmt/forge.rst index bc8f1c13bfc0..fd7c3e280b44 100644 --- a/user_guide_src/source/dbmgmt/forge.rst +++ b/user_guide_src/source/dbmgmt/forge.rst @@ -297,7 +297,7 @@ change the name, you can add a "name" key into the field defining array. .. literalinclude:: forge/026.php .. note:: The ``modifyColumn()`` may unexpectedly change ``NULL``/``NOT NULL``. - So it is recommended to always specify ``null`` value. Unlike when creating + So it is recommended to always specify the value for ``null`` key. Unlike when creating a table, if ``null`` is not specified, the column will be ``NULL``, not ``NOT NULL``. diff --git a/user_guide_src/source/installation/upgrade_434.rst b/user_guide_src/source/installation/upgrade_434.rst index ac19ddc0f5ae..c83ae19c251e 100644 --- a/user_guide_src/source/installation/upgrade_434.rst +++ b/user_guide_src/source/installation/upgrade_434.rst @@ -32,7 +32,7 @@ A bug fix may have changed the NULL constraint in the result of :ref:`$forge->modifyColumn() `. See :ref:`Change Log `. To set the desired NULL constraint, change ``Forge::modifyColumn()`` to always -specify the ``null`` value. +specify the ``null`` key. Note that the bug may have changed unexpected NULL constraints in previous versions.