fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kenjis merged 15 commits into from
Apr 27, 2023
Merged

fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly #7371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b4ed61d
fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly
kenjis Mar 23, 2023
3bd122c
docs: add PHPDoc
kenjis Mar 23, 2023
7611d0e
test: add test for NOT NULL columns
kenjis Mar 23, 2023
7b8355a
test: fix tests that depends on table state
kenjis Mar 23, 2023
9b6d970
test: improve test code
kenjis Mar 23, 2023
40d9e8d
fix: Postgres Forge::modifyColumn() changes NULL incorrectly
kenjis Mar 24, 2023
50133d5
fix: default $null value
kenjis Mar 24, 2023
fa8643b
fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly
kenjis Mar 24, 2023
8ad1b8a
docs: add note for modifyColumn()
kenjis Mar 23, 2023
00d9b17
refactor: by rector
kenjis Mar 24, 2023
d88c6c2
fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default
kenjis Mar 24, 2023
6621b5a
docs: update docs
kenjis Mar 24, 2023
d73dc99
docs: add @TODO
kenjis Mar 24, 2023
69a5c29
docs: add upgrade note
kenjis Mar 27, 2023
3b416c8
docs: improve explanation
kenjis Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions system/Database/Forge.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
*/
Expand DownExpand Up@@ -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);
Expand Down
13 changes: 10 additions & 3 deletions system/Database/OCI8/Forge.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
}
}
Expand Down
8 changes: 5 additions & 3 deletions system/Database/Postgre/Forge.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down
8 changes: 5 additions & 3 deletions system/Database/SQLSRV/Forge.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down
12 changes: 6 additions & 6 deletions system/Database/SQLite3/Table.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion ..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down
4 changes: 4 additions & 0 deletions tests/system/Commands/Database/MigrateStatusTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
* @group DatabaseLive
Expand All@@ -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)) {
Expand Down
3 changes: 3 additions & 0 deletions tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down
4 changes: 4 additions & 0 deletions tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down
4 changes: 4 additions & 0 deletions tests/system/Database/DatabaseTestCaseTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down
87 changes: 87 additions & 0 deletions tests/system/Database/Live/ForgeTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down
9 changes: 9 additions & 0 deletions tests/system/Database/Migrations/MigrationRunnerTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down
15 changes: 15 additions & 0 deletions user_guide_src/source/changelogs/v4.3.4.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down
13 changes: 13 additions & 0 deletions user_guide_src/source/dbmgmt/forge.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down
3 changes: 2 additions & 1 deletion user_guide_src/source/dbmgmt/forge/026.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
12 changes: 12 additions & 0 deletions user_guide_src/source/installation/upgrade_434.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
:ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly#7371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kenjis
merged 15 commits into

from

Apr 27, 2023
 Merged


fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly
#7371
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


15 commits


Select commit
Hold shift + click to select a range
b4ed61d

fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 23, 2023
3bd122c

docs: add PHPDoc


kenjis Mar 23, 2023
7611d0e

test: add test for NOT NULL columns


kenjis Mar 23, 2023
7b8355a

test: fix tests that depends on table state


kenjis Mar 23, 2023
9b6d970

test: improve test code


kenjis Mar 23, 2023
40d9e8d

fix: Postgres Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
50133d5

fix: default $null value


kenjis Mar 24, 2023
fa8643b

fix: SQLSRV Forge::modifyColumn() changes NULL incorrectly


kenjis Mar 24, 2023
8ad1b8a

docs: add note for modifyColumn()


kenjis Mar 23, 2023
00d9b17

refactor: by rector


kenjis Mar 24, 2023
d88c6c2

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default


kenjis Mar 24, 2023
6621b5a

docs: update docs


kenjis Mar 24, 2023
d73dc99

docs: add @TODO


kenjis Mar 24, 2023
69a5c29

docs: add upgrade note


kenjis Mar 27, 2023
3b416c8

docs: improve explanation


kenjis Apr 26, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


16 changes: 11 additions & 5 deletions
system/Database/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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.
 */
Expand DownExpand Up@@ -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);
Expand Down

13 changes: 10 additions & 3 deletions
system/Database/OCI8/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'] = '';
 }
 }
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/Postgre/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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'])
Expand Down

8 changes: 5 additions & 3 deletions
system/Database/SQLSRV/Forge.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 '
Expand Down

12 changes: 6 additions & 6 deletions
system/Database/SQLite3/Table.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
 }
Expand Down

2 changes: 1 addition & 1 deletion
..._support/MigrationTestMigrations/Database/Migrations/2018-01-24-102301_Some_migration.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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',
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Commands/Database/MigrateStatusTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -14,6 +14,7 @@
use CodeIgniter\CLI\CLI;
use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\StreamFilterTrait;
use Config\Database;

/**
 * @group DatabaseLive
Expand All@@ -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)) {
Expand Down

3 changes: 3 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce1Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCase/DatabaseTestCaseMigrationOnce2Test.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -13,6 +13,7 @@

use CodeIgniter\Test\CIUnitTestCase;
use CodeIgniter\Test\DatabaseTestTrait;
use Config\Database;
use Config\Services;

/**
Expand DownExpand Up@@ -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();
Expand Down

4 changes: 4 additions & 0 deletions
tests/system/Database/DatabaseTestCaseTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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();
Expand Down

87 changes: 87 additions & 0 deletions
tests/system/Database/Live/ForgeTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;

/**
Expand DownExpand Up@@ -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');
Expand Down

9 changes: 9 additions & 0 deletions
tests/system/Database/Migrations/MigrationRunnerTest.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
Expand All@@ -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');
Expand All@@ -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');
Expand Down

15 changes: 15 additions & 0 deletions
user_guide_src/source/changelogs/v4.3.4.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() <db-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
***************

Expand Down

13 changes: 13 additions & 0 deletions
user_guide_src/source/dbmgmt/forge.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -285,6 +285,8 @@ Used to remove multiple columns from a table.
Modifying a Field in a Table
============================

.. _db-forge-modifyColumn:

$forge->modifyColumn()
----------------------

Expand All@@ -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
Expand Down

3 changes: 2 additions & 1 deletion
user_guide_src/source/dbmgmt/forge/026.php

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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

12 changes: 12 additions & 0 deletions
user_guide_src/source/installation/upgrade_434.rst

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -25,6 +25,18 @@ Redirect Status Code
 :ref:`ChangeLog v4.3.4 <v434-redirect-status-code>` and if the code is not
 what you want, :ref:`specify status codes <response-redirect-status-code>`.

Forge::modifyColumn() and NULL
==============================

A bug fix may have changed the NULL constraint in the result of
:ref:`$forge->modifyColumn() <db-forge-modifyColumn>`. See
:ref:`Change Log <v434-forge-modifycolumn>`.
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
*********************

Expand Down