fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectly by kenjis · Pull Request #7371 · codeigniter4/CodeIgniter4

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis · 2023-03-23T05:11:33Z

Description
See #7302

The $forge->modifyColumn() has been fixed. Due to a bug, in previous versions, SQLite3/Postgres/SQLSRV might change NULL/NOT NULL unpredictably.
In previous versions, the OCI8 driver did not change NULL/NOT NULL when you don’t specify null value.
Now in all database drivers $forge->modifyColumn() always sets NULL when you don’t specify null value.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2023-03-23T05:44:25Z

Because sanity-tests fail, we cannot run database-live-tests...

kenjis · 2023-03-23T07:00:08Z

Okay. SQLite3 database-live-tests passed.
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4497811722

kenjis · 2023-03-23T07:23:42Z

+ $this->db->resetDataCache();
+
+ $col1 = $this->getMetaData('col1', 'forge_test_modify');
+ $this->assertTrue($col1->nullable); // Nullable if not specified.


@codeigniter4/database-team Should this be false? What do you think?
Maybe different databases have different defaults when not specifying null or not null?

I have also changed the behavior of other drivers to match the behavior of MySQL.

kenjis · 2023-03-23T08:20:32Z

I didn't know but the following test passes on MySQL.

$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' => ['name' => 'new_name', 'type' => 'VARCHAR', 'constraint' => 1],
'col2' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => true],
'col3' => ['type' => 'VARCHAR', 'constraint' => 1, 'null' => false],
]);
$this->db->resetDataCache();
$col1 = $this->getMetaData('new_name', 'forge_test_modify');
$this->assertTrue($col1->nullable); // Nullable if not specified.$col2 = $this->getMetaData('col2', 'forge_test_modify');
$this->assertTrue($col2->nullable);
$col3 = $this->getMetaData('col3', 'forge_test_modify');
$this->assertFalse($col3->nullable);

Added notes in the docs: 3b8a428

kenjis · 2023-03-27T00:38:24Z

Rebased and add upgrade note.

kenjis · 2023-04-08T02:36:03Z

@codeigniter4/database-team Please review.

kenjis · 2023-04-25T00:38:30Z

Any comment?

MGatner

I would prefer someone DB to take a look too but the code makes sense and tests look good so I trust this is appropriate.

kenjis · 2023-04-26T10:48:10Z

I believe this code is okay. The issue is that this behavior is okay.

michalsn

I think that using:

null value

Could be a bit misleading, and some users may think they should define their table fields as null (at least for me). So, we could use something more specific.

It should be `NULL`.

For consistency.

Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis · 2023-04-26T23:04:04Z

@michalsn Exactly! Applied your suggestions.

kenjis changed the title ~~fix: SQLite3 Forge::modifyColumn() changes NULL incorrectly~~fix: [SQLite3] Forge::modifyColumn() changes NULL incorrectlyMar 23, 2023

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 23, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch 2 times, most recently from 6338612 to 156419aCompare March 23, 2023 06:02

kenjis marked this pull request as draft March 23, 2023 06:04

kenjis force-pushed the fix-modifyColumn-SQLite3 branch 2 times, most recently from 6d2bfe1 to 89c4bc8Compare March 23, 2023 06:41

kenjis mentioned this pull request Mar 23, 2023
test: Forge::modifyColumn() and null #7302
Merged
5 tasks

kenjis commented Mar 23, 2023
View reviewed changes

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from 89c4bc8 to 7480fd5Compare March 23, 2023 08:11

kenjis force-pushed the fix-modifyColumn-SQLite3 branch 3 times, most recently from 8068beb to 20cfb13Compare March 23, 2023 08:34

kenjis added the database Issues or pull requests that affect the database layer label Mar 24, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from 20cfb13 to 16177e7Compare March 24, 2023 02:24

kenjis changed the title ~~fix: [SQLite3] Forge::modifyColumn() changes NULL incorrectly~~fix: [SQLite3][Postgres] Forge::modifyColumn() changes NULL incorrectlyMar 24, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from 07d4457 to 7e13c3aCompare March 24, 2023 02:55

kenjis changed the title ~~fix: [SQLite3][Postgres] Forge::modifyColumn() changes NULL incorrectly~~fix: [SQLite3][Postgres][SQLSRV] Forge::modifyColumn() changes NULL incorrectlyMar 24, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch 4 times, most recently from 9c05b29 to e8e7a6bCompare March 24, 2023 04:20

kenjis changed the title ~~fix: [SQLite3][Postgres][SQLSRV] Forge::modifyColumn() changes NULL incorrectly~~fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL incorrectlyMar 24, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from 5c2f42f to 88c087cCompare March 24, 2023 06:52

kenjis added the breaking change Pull requests that may break existing functionalities label Mar 24, 2023

kenjis changed the title ~~fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL incorrectly~~fix: [SQLite3][Postgres][SQLSRV][OCI8] Forge::modifyColumn() changes NULL constraint incorrectlyMar 24, 2023

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from e4be766 to 4653dffCompare March 24, 2023 07:45

kenjis marked this pull request as ready for review March 27, 2023 00:38

samsonasik requested a review from michalsn April 25, 2023 01:40

MGatner approved these changes Apr 26, 2023
View reviewed changes

michalsn reviewed Apr 26, 2023
View reviewed changes

kenjisand others added 15 commits April 27, 2023 08:01

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

docs: add PHPDoc
3bd122c

test: add test for NOT NULL columns
7611d0e

test: fix tests that depends on table state
7b8355a

test: improve test code
9b6d970

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

fix: default $null value
50133d5
It should be `NULL`.

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

docs: add note for modifyColumn()
8ad1b8a

refactor: by rector
00d9b17

fix: OCI8 Forge::modifyColumn() changes behavior to NULL by default
d88c6c2
For consistency.

docs: update docs
6621b5a

docs: add@todo
d73dc99

docs: add upgrade note
69a5c29

docs: improve explanation
3b416c8
Apply suggestions from code review Co-authored-by: Michal Sniatala <michal@sniatala.pl>

kenjis force-pushed the fix-modifyColumn-SQLite3 branch from bbbebf7 to 3b416c8Compare April 26, 2023 23:03

kenjis requested a review from michalsn April 26, 2023 23:04

michalsn approved these changes Apr 27, 2023
View reviewed changes

kenjis merged commit 90762f3 into codeigniter4:developApr 27, 2023

kenjis deleted the fix-modifyColumn-SQLite3 branch April 27, 2023 09:18

kenjis mentioned this pull request Jan 21, 2024
docs: fix incorrect descriptions in Forge::modifyColumn() notes #8441
Merged
2 tasks