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.

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

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented May 2, 2022

As of MySQL 8.0.28, the utf8mb3 charset is deprecated, so we should
avoid trying to use it.

As of MySQL 8.0.28, the `utf8mb3` charset is deprecated, so we should
avoid trying to use it.
@cmb69 cmb69 requested a review from kamil-tekiela May 2, 2022 11:34
@cmb69 cmb69 linked an issue May 2, 2022 that may be closed by this pull request
@@ -44,7 +44,7 @@ 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']) {
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.

@cmb69 cmb69 closed this May 2, 2022
@cmb69
Copy link
Member Author

cmb69 commented May 6, 2022

Given that several CI jobs are now failing due to this issue, I suggest to merge the PR as stop-gap measure, before we find a proper solution.

@kamil-tekiela, what do you think?

@cmb69 cmb69 reopened this May 6, 2022
@kamil-tekiela
Copy link
Member

Or maybe mark the tests as XFAIL?

@cmb69
Copy link
Member Author

cmb69 commented May 6, 2022

Or maybe mark the tests as XFAIL?

I'd rather ignore utf8mb3 than to basically ignore these three tests. If we were good at fixing XFAILs, I'd think otherwise.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Maybe open up a new issue stating that mysqlnd does not support utf8mb3 so that we can track it.

@cmb69 cmb69 closed this in a7a5902 May 9, 2022
@cmb69 cmb69 deleted the cmb/gh8478 branch May 9, 2022 09:02
@cmb69
Copy link
Member Author

cmb69 commented May 9, 2022

Maybe open up a new issue stating that mysqlnd does not support utf8mb3 so that we can track it.

I have repurposed #8480.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mysqlnd does not support utf8mb3 charset
3 participants