test: Forge::addColumn() and null #7319
Conversation
kenjis
force-pushed
the
test-Forge-addColumn-null
branch
from
8f00d43 to
8633a82
Compare
| $col3 = $this->getMetaData('col3', 'forge_test_table'); | ||
| $this->assertFalse($col3->nullable); | ||
| $col4 = $this->getMetaData('col4', 'forge_test_table'); | ||
| $this->assertTrue($col4->nullable); |
There was a problem hiding this comment.
Since $col1 is false from a logical point of view, I think $col4 should be false too. Thoughts?
There was a problem hiding this comment.
See #7235
I thought that, but if we change it, it is a breaking change.
There was a problem hiding this comment.
Yes, fair point but in this case, I would treat it as a bug.
API should be predictable and having a different behavior for the "same thing" is something we should avoid.
There was a problem hiding this comment.
Okay, I'll be fine with documenting this and leaving it to be fixed in the next major version.
@codeigniter4/database-team any thoughts on this one?
There was a problem hiding this comment.
It is inconsistent to have different behavior when creating and adding, but I am a little doubtful that it is worth changing in v5.
However, if we decide to change it, I would like to include that in the documentation.
@codeigniter4/database-team Opinions?
There was a problem hiding this comment.
It seems nobody has strong opinion.
kenjis
force-pushed
the
test-Forge-addColumn-null
branch
from
8633a82 to
f251ae2
Compare
|
Rebased to resolve conflicts. |
|
This PR just adds test for the current implementation. |
Needs #7301Description
See #7235
Forge::addColumn()and nullChecklist: