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

Fix bug with DB-Forge composite foreign keys #4932

Merged
merged 4 commits into from
Aug 23, 2021

Conversation

monkenWu
Copy link
Contributor

Description
Fixes #4310
Original PR #4931

Change the branch base to 4.2.

I edited the addForeignKey method and _processForeignKeys method of CodeIgniter\Database\Forge so that they can define composite foreign keys in an array.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan changed the title Fix bug with DB-Forge composite foreign keys - Change branch base Fix bug with DB-Forge composite foreign keys Jul 13, 2021
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

This looks good.

I was wondering if it would be better to leave the string casting in addForeignKey() method and parse the variables, but using arrays is more natural in this case. I don't think many developers have extended this method so chances that this will break someone's code are low.

I'm fine with these changes, but since we're removing casting in the public method for some variables, please mention this in the changelog (https://github.com/codeigniter4/CodeIgniter4/tree/4.2/user_guide_src/source/changelogs - you will have to create a new file v4.2.0.rst). We don't wanna surprise anyone.

@monkenWu
Copy link
Contributor Author

@michalsn Thanks for the review.
In fact, I refer to the method addKey(), which accepts both string and array. Therefore, I also continued this pattern in addForeignKey().
擷取

I will create a new v4.2.0.rst file soon and mention this change in Enhancements.

@paulbalandan
Copy link
Member

Please rename your last commit to something else. We're not releasing yet.

@monkenWu monkenWu force-pushed the forge-composite-foreign-key branch from f335cf2 to c829d40 Compare July 23, 2021 11:59
@monkenWu
Copy link
Contributor Author

@paulbalandan I fixed the composition foreign keys exception and added a new test case

@lonnieezell
Copy link
Member

@michalsn are you able to review these changes?

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thanks @monkenWu it looks good!

One last thing - you have to rebase to resolve conflicts. If you have never done this before, here is a nice instruction: https://samsonasik.wordpress.com/2015/09/16/practical-git-4-rebasing-conflicted-task-branch-against-primary-branch/

tests/system/Database/Live/ForgeTest.php Outdated Show resolved Hide resolved
tests/system/Database/Live/ForgeTest.php Outdated Show resolved Hide resolved
tests/system/Database/Live/ForgeTest.php Outdated Show resolved Hide resolved
tests/system/Database/Live/ForgeTest.php Outdated Show resolved Hide resolved
@paulbalandan
Copy link
Member

@monkenWu can you do a last minute rebase to fix the merge conflicts before we can merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants