FIX PHP 8.1 support in MySQLiConnector::query errors #10570
Conversation
The default error reporting mode in PHP 8.1 has changed from using errors reported on the connection handle to throwing mysqli_sql_exception. query() makes no allowance for this, and functions up the call stack expect to catch Silverstripe\ORM\Connect\DatabaseException instead - resulting in the MySQLi exception going all the way up to halt the system. We can use a try, catch, and finally to retain backwards compatibility, no matter which setting (e.g. PHP version default) someone has enabled.
|
This fixes an issue for any site with subsites installed being unable to successfully run a dev/build without an existing database (e.g. setting up for development). |
|
@NightJar I thought we were testing on PHP 8.1 before, do you know if there's a test relating to and/or why it was not failing before? |
There are no existing tests, this change set adds some.
Probably the same reason there's no change to the I presume because the most common code path is to use In the Subsites case this is an issue only because the code may run before the database is ready (e.g. first |
|
|
||
| if (strtolower(substr($config['type'] ?? '', 0, 5)) !== 'mysql') { | ||
| return $this->markTestSkipped('The test only relevant for MySQL'); | ||
| } | ||
|
|
||
| if ($charset) { |
There was a problem hiding this comment.
Moving the duplicate logic to this new private method is OK, except for the bit where the marking test as skipped if it's it's not running mysql - It makes it far less obvious could you please re-duplicate those bits and move those lines back to the individual tests
There was a problem hiding this comment.
I'll put it in setUp instead. This makes it super obvious it runs before each test case.
As review feedback; marking the test as skipped in a private function obfuscated where the call was happening and made it harder to skimread the tests. Moving this into a setUp function makes it obvious the check is run before each test case, and skipped if necessary.
|
preparedQuery actually still has this same problem. There’s a catch in prepareStatement but no catch around $statement->execute() in preparedQuery itself, encountering the same problem as query is having if the sql is syntactically correct but the execution causes an error (for example inserting/updating that causes unique contraint clashes) |
There was a problem hiding this comment.
Looks good. I've re-targeted this PR to 4.13 since it's the oldest currently supported version. I've run a fork on this PR (rebased on 4.13) against silverstripe/installer here which went green
Issue #10613
The default error reporting mode in PHP 8.1 has changed from using errors reported on the connection handle to throwing mysqli_sql_exception. query() makes no allowance for this, and functions up the call stack expect to catch
Silverstripe\ORM\Connect\DatabaseException instead - resulting in the MySQLi exception going all the way up to halt the system.
We can use a try, catch, and finally to retain backwards compatibility, no matter which setting (e.g. PHP version default) someone has enabled.
C.f. https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.mysqli