diff --git a/system/Database/Forge.php b/system/Database/Forge.php index a33deb3c1c4d..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 @@ -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. */ @@ -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/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'] = ''; } } diff --git a/system/Database/Postgre/Forge.php b/system/Database/Postgre/Forge.php index 662fbe666595..47be1b063c8d 100644 --- a/system/Database/Postgre/Forge.php +++ b/system/Database/Postgre/Forge.php @@ -109,10 +109,12 @@ protected function _alterTable(string $alterType, string $table, $field) . " SET DEFAULT {$data['default']}"; } - if (isset($data['null'])) { - $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) - . ($data['null'] === true ? ' DROP' : ' SET') . ' NOT NULL'; + $nullable = true; // Nullable by default. + 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'; if (! empty($data['new_name'])) { $sqls[] = $sql . ' RENAME COLUMN ' . $this->db->escapeIdentifiers($data['name']) diff --git a/system/Database/SQLSRV/Forge.php b/system/Database/SQLSRV/Forge.php index 8d7a720313f7..e44624761850 100755 --- a/system/Database/SQLSRV/Forge.php +++ b/system/Database/SQLSRV/Forge.php @@ -203,10 +203,12 @@ protected function _alterTable(string $alterType, string $table, $field) . " DEFAULT {$data['default']} FOR " . $this->db->escapeIdentifiers($data['name']); } - if (isset($data['null'])) { - $sqls[] = $sql . ' ALTER COLUMN ' . $this->db->escapeIdentifiers($data['name']) - . ($data['null'] === true ? ' DROP' : '') . " {$data['type']}{$data['length']} NOT NULL"; + $nullable = true; // Nullable by default. + 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'; if (! empty($data['comment'])) { $sqls[] = 'EXEC sys.sp_addextendedproperty ' 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/_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/Live/ForgeTest.php b/tests/system/Database/Live/ForgeTest.php index d859bc93833c..54704a79e4c1 100644 --- a/tests/system/Database/Live/ForgeTest.php +++ b/tests/system/Database/Live/ForgeTest.php @@ -16,7 +16,9 @@ use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; use Config\Database; +use LogicException; use RuntimeException; +use stdClass; use Tests\Support\Database\Seeds\CITestSeeder; /** @@ -1276,6 +1278,91 @@ public function testModifyColumnRename() $this->forge->dropTable('forge_test_three', true); } + public function testModifyColumnNullTrue() + { + // @TODO remove this in `4.4` branch + 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); + } + + public function testModifyColumnNullFalse() + { + // @TODO remove this in `4.4` branch + 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); + + $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() { $group = config('Database'); 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'); diff --git a/user_guide_src/source/changelogs/v4.3.4.rst b/user_guide_src/source/changelogs/v4.3.4.rst index ec02c35e64df..efa598d221b5 100644 --- a/user_guide_src/source/changelogs/v4.3.4.rst +++ b/user_guide_src/source/changelogs/v4.3.4.rst @@ -35,6 +35,21 @@ 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() +--------------------- + +- 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 the ``null`` key. +- Now in all database drivers ``$forge->modifyColumn()`` always sets ``NULL`` + when you don't specify the ``null`` key. +- The ``NULL``/``NOT NULL`` change may still be unexpectedly, it is recommended + 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 bfb3bca9aea1..fd7c3e280b44 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() ---------------------- @@ -294,6 +296,17 @@ 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 the value for ``null`` key. 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``. + +.. 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 diff --git a/user_guide_src/source/installation/upgrade_434.rst b/user_guide_src/source/installation/upgrade_434.rst index 38f3305ca212..c83ae19c251e 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`` key. + +Note that the bug may have changed unexpected NULL constraints in previous +versions. + Breaking Enhancements *********************