-
Notifications
You must be signed in to change notification settings - Fork 254
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 PHP error from a failed database connection #7815
Fix PHP error from a failed database connection #7815
Conversation
PHP 8.1+ will generate the following error upon a failed database connection: ``` PHP Fatal error: Uncaught TypeError: mysqli_error(): Argument #1 ($mysql) must be of type mysqli, null given in /Sources/Errors.php:464 ``` To fix this, I built a wrapper to check to ensure that the we don't have a null argument going into the related function. Instead if its null and the primary connection is null, it returns a empty string, which is what the code seems to expect.
I am not sure this helps anything? If db_connection is null that means the connection failed (or was somehow bypassed, which shouldn't happen). If we return an empty string, I believe all we will see in these circumstances is a blank error message in smf_error_log. I think that is actually less helpful than 'argument is of type null'. It would be more helpful to find out why the db connection was lost, if available. Or put a proper "db connection lost" error of some type. |
updated env to 8.2.8 same behavior like 8.1.1 mention before |
I am seeing the fatal php error in a MySQL 8.0 and PHP 8.1 env. |
Here is the full PHP error given.
We could also fix this in Errrors.php by checking that we are not about to use a null, but the wrapper returns a empty string. Which Errors.php will check for. Because its a loose type check, our code would basically make |
Sources/Subs-Db-mysql.php
Outdated
{ | ||
global $db_connection; | ||
|
||
if (is_null($connection) && is_null($db_connection)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (is_null($connection) && is_null($db_connection)) | |
if (is_null($connection) || is_null($db_connection)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do I need to make the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; thinking this over again, it is correct imo because it should check the provided connection before the global one. Also could we explicitly check? Another stupid suggestion, but matches everywhere else; also, I dislike superfluous function calls.
if (is_null($connection) && is_null($db_connection)) | |
if ($connection !== null || $db_connection !== null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it, but it needs to be and, not or. If either one is not null, it has a db connection and would just return a empty string if no error has occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh oh did I really get those backwards? 😮 I accidentally inverted your logic, sorryyy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens to the best of us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the inverted logic is in the commit
@sbulen is the provided proof satisfactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c98729c should be reverted.
Inverted the logic. I don't care whether is_null is used or === null. I doubt any speed differences can be found. |
PHP 8.1+ will generate the following error upon a failed database connection:
To fix this, I built a wrapper to check to ensure that the we don't have a null argument going into the related function. Instead if its null and the primary connection is null, it returns a empty string, which is what the code seems to expect.
@albertlast I don't have PG setup anymore to test. But I believe it will suffer the same problem according to the PHP docs. Have you seen that?