Mysqli: fix missing error for invalid mysqli_options() option #20971
Conversation
…cting report_mode
…cting report_mode
…cting report_mode
…cting report_mode
…cting report_mode
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
From what I can see (as somebody who doesn't maintain this code), most arguments are validated without the MYSQLI_REPORT_ERROR guard, and throw a zend_argument_value_error().
But somebody who does maintain this should confirm.
Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
Co-authored-by: Ilija Tovilo <ilija.tovilo@me.com>
…cting report_mode
- if (MyG(report_mode) & MYSQLI_REPORT_ERROR) {
zend_value_error("mysqli_options(): Invalid option %d", (int)mysql_option);Do we need to remove |
|
Somebody else should review this. /cc @Girgias @SakiTakamachi |
|
First of all, I don't know if this should be an error condition. I guess it kind of makes sense. But as you can see in the test, no message was the behaviour by design. The error should be without the guard and without the function name in the message. It seems to me like you have also put it in the wrong place. Judging by the message and the test, you wanted to put it in the switch statement after that. The message isn't very descriptive either. What does it mean "invalid option"? What should the user do differently? |
…cting report_mode
…cting report_mode
…cting report_mode
|
Changed the error message to |
Co-authored-by: Kamil Tekiela <tekiela246@gmail.com>
Co-authored-by: Kamil Tekiela <tekiela246@gmail.com>
…cting report_mode
Co-authored-by: Kamil Tekiela <tekiela246@gmail.com>
|
Thank you @kamil-tekiela kamil-tekiela |
|
@iluuu1994 Are you ok with how this looks now? |
…cting report_mode
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
I'm not a codeowner, but yes, LGTM now. Thanks @arshidkv12!
…cting report_mode
ext/mysqli/tests/gh20968.phpt
Outdated
| @@ -0,0 +1,27 @@ | |||
| --TEST-- | |||
| Bug #20968 mysqli_options() with invalid option triggers ValueError | |||
There was a problem hiding this comment.
| Bug #20968 mysqli_options() with invalid option triggers ValueError | |
| GH-20968 mysqli_options() with invalid option triggers ValueError |
We use Bug #xxxxx notation to refer to old bugsnet bugs.
ext/mysqli/tests/gh20968.phpt
Outdated
|
|
||
| $mysqli = new mysqli($host, $user, $passwd, $db, $port, $socket); | ||
|
|
||
| $value = $mysqli->options(10, 'invalid_option'); |
There was a problem hiding this comment.
Add a try catch here and check the exception message, we don't care about the stack trace.
ext/mysqli/tests/gh20968.phpt
Outdated
| @@ -0,0 +1,27 @@ | |||
| --TEST-- | |||
| Bug #20968 mysqli_options() with invalid option triggers ValueError | |||
There was a problem hiding this comment.
| Bug #20968 mysqli_options() with invalid option triggers ValueError | |
| Bug #20968 mysqli_options() with invalid option should triggers ValueError |
The phrasing of the bug title seems to imply that an invalid option MUST NOT thow a ValueError, which is the complete opposite of what is being done.
…cting report_mode
|
Please check the new commit @Girgias |
Co-authored-by: Gina Peter Banyard <girgias@php.net>
…cting report_mode
4 participants
#20968