Skip to content
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

[NFC] Update System Uft8mb4 check to handle for the fact that MySQL8 … #22221

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

seamuslee001
Copy link
Contributor

…outputs utf8mb3 when the charset has been set to utf8 as utf8mb3 is the underlyling charset for utf8

Overview

This fixes the following test failure api_v3_SystemTest::testSystemUTFMB8Conversion Failed asserting that 'CREATE TABLE civicrm_contact (\n....) ENGINE=InnoDB AUTO_INCREMENT=3 DEFAULT CHARSET=utf8mb3 COLLATE=utf8_unicode_ci ROW_FORMAT=DYNAMIC' ends with "DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci ROW_FORMAT=DYNAMIC".

Before

api_v3_SystemTest::testSystemUTFMB8Conversion fails on MySQL8

After

api_v3_SystemTest::testSystemUTFMB8Conversion passed on MySQL8

Technical Details

As per https://dev.mysql.com/doc/refman/8.0/en/charset-unicode-utf8mb3.html mysql 8 onwards is now outputting utf8mb3 when it used to output utf8 as the default charsets for tables

Comments

ping @eileenmcnaughton @totten @demeritcowboy

…outputs utf8mb3 when the charset has been set to utf8 as utf8mb3 is the underlyling charset for utf8
@civibot
Copy link

civibot bot commented Dec 8, 2021

(Standard links)

@demeritcowboy
Copy link
Contributor

I thought I remembered seeing a utility function you had added to check if db is mysql8 since something else had come up before but can't find it. But whatever.

@seamuslee001 seamuslee001 merged commit 95fb8b2 into civicrm:5.45 Dec 8, 2021
@seamuslee001 seamuslee001 deleted the nfc_fix_system_test_mysql8 branch December 8, 2021 02:27
@demeritcowboy
Copy link
Contributor

This is probably what I was thinking of:

protected function isMySQL8(): bool {

So I'm not crazy, at least not about this...

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

Successfully merging this pull request may close these issues.

2 participants