Skip to content

feat: [MySQLi] add config to use MYSQLI_OPT_INT_AND_FLOAT_NATIVE - #7265

Merged
kenjis merged 14 commits into
codeigniter4:4.4from
kai890707:4.4
Feb 22, 2023
Merged

feat: [MySQLi] add config to use MYSQLI_OPT_INT_AND_FLOAT_NATIVE#7265
kenjis merged 14 commits into
codeigniter4:4.4from
kai890707:4.4

Conversation

@kai890707

@kai890707 kai890707 commented Feb 16, 2023 •
edited by kenjis
Loading

Copy link
Copy Markdown
Contributor

Supersedes #7260

Description
See #7259

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

@michalsn michalsn added enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer 4.4 labels Feb 16, 2023
@michalsn

michalsn commented Feb 16, 2023

Copy link
Copy Markdown
Member

As @kenjis mentioned in #7260 - this should be configurable. Otherwise, we will break other people's code.

@kai890707

kai890707 commented Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

As @kenjis mentioned in #7260 - this should be configurable. Otherwise, we will break other people's code.

Okay, I will try to handle this part.

@kenjis kenjis added tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. labels Feb 16, 2023
@kai890707

kai890707 commented Feb 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@michalsn
Are the current changes in this area acceptable?
Currently only enhancements are made for the MYSQLI_OPT_INT_AND_FLOAT_NATIVE section.

michalsn
michalsn requested changes Feb 18, 2023
View reviewed changes

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general, it looks good, thanks.

We would need some tests for this feature to ensure it works. You can see how you can change the database config when you write your tests here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/tests/system/Database/Live/ConnectTest.php#L33 We should have some sample data in the database already that you can use for the sample query - you don't need to create a new table etc.

Finally, we need the user guide to be updated - other users have to know there is this new feature that they can use. Please add the description for this config option here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/database/configuration.rst (a table with all option is at the end).

Comment thread app/Config/Database.php Outdated
Comment thread system/Database/BaseConnection.php Outdated
Comment thread system/Database/MySQLi/Connection.php Outdated
@kenjis

kenjis commented Feb 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

About the user guide. Please add this enhancement in the v4.4.0 changelog, too.
https://github.com/codeigniter4/CodeIgniter4/blob/4.4/user_guide_src/source/changelogs/v4.4.0.rst#database

michalsn
michalsn reviewed Feb 18, 2023
View reviewed changes

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding markTestSkipped() should cover all failed database tests.

Please check out the other failed tests - the descriptions are quite self-explanatory:

Comment thread tests/system/Database/Live/MySQLi/NumberNativeTest.php Outdated
Comment thread tests/system/Database/Live/MySQLi/NumberNativeTest.php
kenjis
kenjis reviewed Feb 19, 2023
View reviewed changes
Comment thread user_guide_src/source/database/configuration.rst Outdated
@kai890707

kai890707 commented Feb 19, 2023

Copy link
Copy Markdown
Contributor Author

Sorry, @michalsn , I don't quite understand how to fix this error.
Do you have any suggestions to help me clear this error?
https://github.com/codeigniter4/CodeIgniter4/actions/runs/4211879196/jobs/7311399553#step:8:28

@kenjis

kenjis commented Feb 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@kai890707 Please do not use 4.4 as your PR branch in the next PR.
It breaks my local repository's 4.4 branch when I checkout your PR branch.

@kenjis

kenjis commented Feb 19, 2023

Copy link
Copy Markdown
Member

@kai890707 Just run composer cs-fix in your project root folder.

@kai890707

kai890707 commented Feb 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@kai890707 Please do not use 4.4 as your PR branch in the next PR. It breaks my local repository's 4.4 branch when I checkout your PR branch.

Oh, I'm sorry.
This is my first contribution to an open source project and I will improve it in the next PR.

@kai890707 Just run composer cs-fix in your project root folder.

Okay, thanks for the answer.

@kai890707
fix:Run composer cs-fix and modify the Enhancements/DataBase descript…
c18581d 
…ion in user_guide_src/source/changelogs/v4.4.0.rst.
kenjis
kenjis reviewed Feb 19, 2023
View reviewed changes
Comment thread user_guide_src/source/changelogs/v4.4.0.rst Outdated
kenjis
kenjis reviewed Feb 19, 2023
View reviewed changes
Comment thread user_guide_src/source/changelogs/v4.4.0.rst Outdated
kenjis
kenjis reviewed Feb 19, 2023
View reviewed changes
Comment thread app/Config/Database.php Outdated
@kenjis kenjis removed tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. labels Feb 19, 2023
kenjis
kenjis reviewed Feb 19, 2023
View reviewed changes
Comment thread user_guide_src/source/changelogs/v4.4.0.rst Outdated
@kai890707
fix:Modify changelogs/v4.4.0.rst description and remove the numberNat…
0a8c971 
…ive attribute from the $test array in app/Config/Database.php
@kai890707

kai890707 commented Feb 19, 2023

Copy link
Copy Markdown
Contributor Author

@kenjis @michalsn
All changes have been modified, please review and I will fill in any omissions.
Thank you both for your help! 😄

michalsn
michalsn approved these changes Feb 19, 2023
View reviewed changes

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good job. Thanks!

kenjis
kenjis approved these changes Feb 19, 2023
View reviewed changes

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@kenjis
kenjis merged commit 51ad7e6 into codeigniter4:4.4 Feb 22, 2023
@kenjis

kenjis commented Feb 22, 2023

Copy link
Copy Markdown
Member

@kai890707 Thank you!

@kenjis kenjis mentioned this pull request Feb 22, 2023
Closed
@kenjis kenjis changed the title Enhance: [MySQLi] use MYSQLI_OPT_INT_AND_FLOAT_NATIVE. feat: [MySQLi] add option to use MYSQLI_OPT_INT_AND_FLOAT_NATIVE Feb 24, 2023
@kenjis kenjis changed the title feat: [MySQLi] add option to use MYSQLI_OPT_INT_AND_FLOAT_NATIVE feat: [MySQLi] add config to use MYSQLI_OPT_INT_AND_FLOAT_NATIVE Feb 24, 2023
@kenjis kenjis mentioned this pull request Sep 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenjis kenjis kenjis approved these changes
@michalsn michalsn michalsn approved these changes

Assignees

No one assigned

Labels

database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kai890707 @michalsn @kenjis