-
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
Add Forge::processIndexes() to create indexes on existing table #6676
Conversation
@codeigniter4/database-team Please Review |
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.
Some initial thoughts.
d6d7895
to
ebe9086
Compare
44f461f
to
9803845
Compare
Setting $asQuery to true will retrieve a stand alone query rather than the partial statement used with CREATE TABLE.
Makes method return a stand alone query
This will construct the queries used to create indexes and run them.
A bit of code that checks that the field actually exists in the table. Since it now may be called without first declaring fields then it needed moved to after processIndexes() so I moved it to _processForeignKeys().
SQLite needed methods for the Table class to add keys to existing table.
Co-authored-by: Michal Sniatala <[email protected]>
Co-authored-by: Michal Sniatala <[email protected]>
Co-authored-by: Michal Sniatala <[email protected]>
Co-authored-by: Michal Sniatala <[email protected]>
Co-authored-by: kenjis <[email protected]>
9803845
to
bfb90d8
Compare
Can you add a test case for without foreign key name? |
I can, but I don't see it as necessary because _processForeignKeys() is called the same as with create table and there are tests that cover this already. I used names because it simplified the test. Your not going to get anymore code coverage with a test without names. |
As far as coverage is concerned, the coverage value we are getting now is line coverage, which means nothing more than knowing lines that are not covered. Just because the coverage does not increase does not mean that the additional tests are useless or not. At the very least, branch coverage should be compared. Looking at the implementation in this foreign key case, there is certainly no need to add it. |
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.
Anyway, I want to keep a test method short. If a test method is too long, it will be difficult to tell what is being tested. See #6676 (comment)
Other than that, I'm okay.
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.
Looks good to me. Thanks!
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.
Thank you!
🕺🥳🚀 |
Closes #4814
This PR allows creating indexes on an existing table.
It is used following
addKey()
,addPrimaryKey()
,addUniqueKey()
, andaddForeignKey()
to add indexes to an existing table. It is used instead of or in place ofcreateTable()
.This is the last part of #6430
Fixes #6453
Checklist: