-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Postgre & SQLSRV - Should Never Have A Field Length For TEXT #6405
Conversation
Thank you for sending PR. If this PR is a bug fix, it should go to |
Its not a bug per se, You can assign the constraint as null and it will not cause a problem. This PR just allows using the same table definition to work between databases. Put another way, Forge allows you to assign an illegal constraint. This PR will just ignore such illegal constraint if exists. SQLSRV has same issue. I can add here. |
Okay, this PR is an enhancement. Can you add a test to prove this enhancement? |
Added test and another fix for SQLSRV. SQLSRV adds a constraint whenever a column is defined with a default value. Whenever dropColumn is called the current code drops all indexes associated with the column but it doesn't drop the default constraints. These use a slightly different command as well: Instead of |
The test demonstrates that these two simple commands can now execute successfully on all DBMS $result = $this->forge->addColumn('user', [
'text_with_constraint' => ['type' => 'text', 'constraint' => 255, 'default' => ''],
]);
$result = $this->forge->dropColumn('user', 'text_with_constraint'); Without this PR Postgre and SQLSRV would fail. |
Shows what I know, I didn't think MySQL could have a length spec for TEXT 🤦♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a short description about this enhancement in changelog?
297bd3b
to
27ba993
Compare
@@ -70,6 +70,7 @@ Others | |||
- Added new Form helper function :php:func:`validation_errors()`, :php:func:`validation_list_errors()` and :php:func:`validation_show_error()` to display Validation Errors. | |||
- Now you can autoload helpers by **app/Config/Autoload.php**. | |||
- ``BaseConnection::escape()`` now excludes the ``RawSql`` data type. This allows passing SQL strings into data. | |||
- SQLSRV now automatically drops ``DEFAULT`` constraint when using ``Forge::dropColumn()``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, this commit is not good. Because the document change has nothing to do with the code change.
We added the explanation for commits and commit messages:
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#committing
It seems you used |
This PR fixes an issue where a table definition in Forge has a length value for type TEXT. Postgre does not support a length for the TEXT type.
Its probably possible for a developer just to set length null when developing on Postgre. This PR is useful when using the same table definition across different DBMS.
The following query works fine for Mysql, Sqlite but fails on Postgre & SQLSRV:
https://www.postgresql.org/docs/current/datatype-character.html
Checklist: