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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Feb 22, 2023
Merged

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

merged 14 commits into from
Feb 22, 2023

Conversation

kai890707
Copy link
Contributor

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

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
Copy link
Member

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

@kai890707
Copy link
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
Copy link
Contributor Author

kai890707 commented Feb 17, 2023 •
edited
Loading

@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
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.

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).

app/Config/Database.php Outdated Show resolved Hide resolved
system/Database/BaseConnection.php Outdated Show resolved Hide resolved
system/Database/MySQLi/Connection.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Feb 18, 2023 •
edited
Loading

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
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.

Adding markTestSkipped() should cover all failed database tests.

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

tests/system/Database/Live/MySQLi/NumberNativeTest.php Outdated Show resolved Hide resolved
tests/system/Database/Live/MySQLi/NumberNativeTest.php Show resolved Hide resolved
@kai890707
Copy link
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
Copy link
Member

kenjis commented Feb 19, 2023 •
edited
Loading

@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
Copy link
Member

kenjis commented Feb 19, 2023

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

@kai890707
Copy link
Contributor Author

kai890707 commented Feb 19, 2023 •
edited
Loading

@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
app/Config/Database.php Outdated Show resolved Hide resolved
@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
@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
Copy link
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
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.

Good job. Thanks!

kenjis
kenjis approved these changes Feb 19, 2023
View reviewed changes
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!

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

kenjis commented Feb 22, 2023

@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
4.4 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