Skip to content

Fix GH-8478: PHP 8.0 tests must be compatible /w MySQL 8.0 #8480

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

Closed
wants to merge 3 commits into from
Closed

Fix GH-8478: PHP 8.0 tests must be compatible /w MySQL 8.0 #8480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d99f3
Fix GH-8478: PHP 8.0 tests must be compatible /w MySQL 8.0
cmb69 May 2, 2022
dacd085
Add comment
cmb69 May 2, 2022
0b4a3df
Fix comment
cmb69 May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ext/mysqli/tests/mysqli_character_set.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ if (!function_exists('mysqli_set_charset')) {
/* The server currently 17.07.2007 can't handle data sent in ucs2 */
/* The server currently 16.08.2010 can't handle data sent in utf16 and utf32 */
/* The server currently 02.09.2011 can't handle data sent in utf16le */
if ($charset['Charset'] == 'ucs2' || $charset['Charset'] == 'utf16' || $charset['Charset'] == 'utf32' || 'utf16le' == $charset['Charset']) {
/* As of MySQL 8.0.28, `SHOW CHARACTER SET` contains utf8mb3, but that is not yet supported by mysqlnd */
if ($charset['Charset'] == 'ucs2' || $charset['Charset'] == 'utf16' || $charset['Charset'] == 'utf32' || 'utf16le' == $charset['Charset'] || 'utf8mb3' == $charset['Charset']) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that say utf8 instead? Also, please add a comment on why we are doing that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that utf8mb3 is reported by SHOW CHARACTER SET, but no longer allowed to be used as character set. Looks like upstream screwed that up, but somehow we need to cater to that.

I've added respective comments.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I am investigating this now and here is why I found out so far.

MySQL 8.0.29 changed the output of SHOW CHARACTER SET. This change is consistent with changes they have been making since at least 8.0.26 to the output to slowly phase out utf8 as an alias of utf8mb3. It future, they have plans to swap utf8 to be an alias of utf8mb4.

Here's the problem now. Our mysqlnd doesn't support utf8mb3 charset... This would mean that the test failures are valid.

According to MySQL docs the only values we should skip in the tests are:

  • ucs2
  • utf16
  • utf16le
  • utf32

There's no mention of utf8mb3 being invalid connection character set. There's also no mention of utf8 being invalid either. I am not sure what to do with this information. We should probably patch mysqlnd up to allow for these charsets. They have no future, but until these charsets are phased out and companies upgrade to this MySQL version might take 10 or 20 years. If clients are allowed to use utf8mb3 connection charset now, we should allow it in PHP.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the problem now. Our mysqlnd doesn't support utf8mb3 charset...

Ah, so this is a mysqlnd problem only!

We should probably patch mysqlnd up to allow for these charsets.

Yeah, that likely makes the most sense.

Anyhow, since this patch is not a proper solution, I'm closing the PR.

continue;
}

Expand Down
3 changes: 2 additions & 1 deletion ext/mysqli/tests/mysqli_options.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ require_once('skipifconnectfailure.inc');
$k = $charset['Charset'];
/* The server currently 17.07.2007 can't handle data sent in ucs2 */
/* The server currently 16.08.2010 can't handle data sent in utf16 and utf32 */
if ($charset['Charset'] == 'ucs2' || $charset['Charset'] == 'utf16' || $charset['Charset'] == 'utf32') {
/* As of MySQL 8.0.28, `SHOW CHARACTER SET` contains utf8mb3, but that is not yet supported by mysqlnd */
if ($charset['Charset'] == 'ucs2' || $charset['Charset'] == 'utf16' || $charset['Charset'] == 'utf32' || $charset['Charset'] == 'utf8mb3') {
continue;
}
if (true !== mysqli_options($link, MYSQLI_SET_CHARSET_NAME, $charset['Charset'])) {
Expand Down
3 changes: 2 additions & 1 deletion ext/mysqli/tests/mysqli_set_charset.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ if ((($res = mysqli_query($link, 'SHOW CHARACTER SET LIKE "latin1"', MYSQLI_STOR
printf("[016] Cannot get list of character sets\n");

while ($tmp = mysqli_fetch_assoc($res)) {
if ('ucs2' == $tmp['Charset'] || 'utf16' == $tmp['Charset'] || 'utf32' == $tmp['Charset'] || 'utf16le' == $tmp['Charset'])
/* As of MySQL 8.0.28, `SHOW CHARACTER SET` contains utf8mb3, but that is not yet supported by mysqlnd */
if ('ucs2' == $tmp['Charset'] || 'utf16' == $tmp['Charset'] || 'utf32' == $tmp['Charset'] || 'utf16le' == $tmp['Charset'] || 'utf8mb3' == $tmp['Charset'])
continue;

/* Uncomment to see where it hangs - var_dump($tmp); flush(); */
Expand Down