Skip to content
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 ability to set index names #6552

Merged
merged 20 commits into from
Oct 10, 2022

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Sep 18, 2022

Closes #5075

This PR allows setting the index name.

While testing dropping by the same name I discovered that dropKey() was generating errors when dropping a unique index be several of the DBMS. The reason is that some DBMS require DROP CONSTRAINT command rather than DROP INDEX. I did some extra work to get dropKey() to work properly.

MySql and SQLite don't really require a name for Primary Key - though Mysql allows setting the name. When dropping its as simple as DROP PRIMARY KEY.

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

@sclubricants sclubricants added database Issues or pull requests that affect the database layer docs needed Pull requests needing documentation write-ups and/or revisions. 4.3 and removed docs needed Pull requests needing documentation write-ups and/or revisions. labels Sep 18, 2022
@kenjis kenjis added enhancement PRs that improve existing functionalities breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Sep 21, 2022
system/Database/Forge.php Outdated Show resolved Hide resolved
system/Database/Forge.php Outdated Show resolved Hide resolved
system/Database/Forge.php Outdated Show resolved Hide resolved
system/Database/Forge.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Sep 21, 2022

While testing dropping by the same name I discovered that dropKey() was generating errors when dropping a unique index be several of the DBMS. The reason is that some DBMS require DROP CONSTRAINT command rather than DROP INDEX. I did some extra work to get dropKey() to work properly.

This is a bug fix.
If you don't send a PR to fix it, at lease please add it in the changelog.

@sclubricants
Copy link
Member Author

sclubricants commented Sep 22, 2022

@phpstan-var array|array{fields: string[], keyName: string}

@sclubricants sclubricants force-pushed the SetManualIndexNames branch 2 times, most recently from ec2e48d to dae7496 Compare September 25, 2022 17:43
@sclubricants sclubricants mentioned this pull request Sep 25, 2022
5 tasks
@sclubricants sclubricants requested a review from kenjis September 27, 2022 05:25
system/Database/Forge.php Outdated Show resolved Hide resolved
system/Database/Forge.php Outdated Show resolved Hide resolved
@kenjis kenjis added the stale Pull requests with conflicts label Sep 28, 2022
@sclubricants
Copy link
Member Author

I think I have it straightened out now.

@kenjis
Copy link
Member

kenjis commented Oct 7, 2022

Not the fault of this PR, but there are PHPStan errors.
Please rebase.

 Error: Result of && is always false.
 Error: Strict comparison using !== between 0 and 0 will always evaluate to false.
 ------ --------------------------------------------------------------------- 
  Line   system/Helpers/array_helper.php                                      
 ------ --------------------------------------------------------------------- 
  51     Result of && is always false.                                        
  51     Strict comparison using !== between 0 and 0 will always evaluate to  
         false.                                                               
 ------ --------------------------------------------------------------------- 

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

sclubricants and others added 20 commits October 10, 2022 11:39
…nd Sqlite

Forgot to commit this in smaller steps..
Some keys such as unique indexes are created as constraints. This looks to see if there is a constraint by the index name. If there is then we will drop constraint else we drop index.
Same as oracle - this looks up if key is a constraint
Have to lookup if key is constraint.
This test adding custom names but also test removing them by custom names.
Get rid of CPD error
@kenjis
Copy link
Member

kenjis commented Oct 10, 2022

@codeigniter4/database-team
I'm merging this for now.
If you find something wrong, please comment.

@kenjis kenjis merged commit 1269565 into codeigniter4:4.3 Oct 10, 2022
@kenjis
Copy link
Member

kenjis commented Oct 10, 2022

@sclubricants Thank you!

@sclubricants sclubricants deleted the SetManualIndexNames branch October 10, 2022 22:37
@MGatner
Copy link
Member

MGatner commented Oct 11, 2022

🏆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants