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 test for Mysqli\Result #5917

Merged
merged 1 commit into from Jun 7, 2023
Merged

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Feb 10, 2023

Q A
Type improvement
Fixed issues n/a

Summary

This test is required in order to confirm cases like Result::rowCount() returning -1.

Related to #5915, #4880, #4870.

To-Do

  • Find a query or a command sequence capable to reproduce the -1 result.

@phansys phansys force-pushed the mysqli_result_test_3 branch 15 times, most recently from a653fa0 to 9be0b17 Compare February 13, 2023 17:48
@phansys phansys marked this pull request as ready for review February 13, 2023 18:01
@phansys phansys force-pushed the mysqli_result_test_3 branch 2 times, most recently from 3a90891 to 366bdfe Compare February 14, 2023 16:29
@derrabus derrabus changed the base branch from 3.5.x to 3.6.x February 14, 2023 21:09
@derrabus
Copy link
Member

I'm not really sure what is the purpose of that test. To me, it looks like we're asserting internal behavior of the MySQLi extension.

@phansys
Copy link
Contributor Author

phansys commented Feb 15, 2023

IMO, the purpose of this test is to ensure the behavior of the Result class (more precisely, of the Result::rowCount() method) regarding the different behaviors it adopts based on the value present at mysqli_driver::$report_mode.
As in #5915 I'm proposing changes that may affect these behaviors, I think is better to have the current expectations covered first.

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label May 16, 2023
@phansys
Copy link
Contributor Author

phansys commented May 18, 2023

IMO, this is still relevant.

@github-actions github-actions bot removed the Stale label May 19, 2023
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

So mysqli has the behaviour to have a rowcount of -1 when something went wrong and this is a first step to cover such a case for the future of letting $result->rowCount() return only 0 or a positive integer in the future? I'm not sure I understand why this is in a separate PR.

tests/Functional/Driver/Mysqli/ResultTest.php Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
@phansys
Copy link
Contributor Author

phansys commented May 22, 2023

So mysqli has the behaviour to have a rowcount of -1 when something went wrong and this is a first step to cover such a case for the future of letting $result->rowCount() return only 0 or a positive integer in the future? I'm not sure I understand why this is in a separate PR.

This test is provided in a separate PR because the behavior was not previously covered, and I think this is relevant regardless the merge of #5915. See #5915 (comment).

@phansys phansys changed the base branch from 3.6.x to 3.7.x May 22, 2023 23:36
@SenseException
Copy link
Member

@derrabus This is the behavior that currently is with mysqli and it's returning whatever mysqli does. #5915 is going to break it, but for now these tests prevent a bc break before 4.0.x. WDYT?

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

I'm still not comfortable with that test tbh. But I'm fine merging it after some changes.

tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
@phansys phansys force-pushed the mysqli_result_test_3 branch 2 times, most recently from bad8621 to 224ec4e Compare June 5, 2023 23:17
@phansys phansys force-pushed the mysqli_result_test_3 branch 10 times, most recently from e92916e to dee5100 Compare June 7, 2023 01:34
tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
tests/Functional/Driver/Mysqli/ResultTest.php Outdated Show resolved Hide resolved
@derrabus derrabus added this to the 3.7.0 milestone Jun 7, 2023
@derrabus derrabus merged commit e0870f1 into doctrine:3.7.x Jun 7, 2023
72 checks passed
@phansys phansys deleted the mysqli_result_test_3 branch June 7, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants