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

Update temp table handler to support utf8mb4 if that is the db collation #15992

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Update temp table handler to support utf8mb4 if that is the db collation

Before

When temp tables are created using utf8 - the default & always the case AFAIK it is always utf8

After

If the DB is set to a default of UTF8MB4 (e.g as a result of running #15969 ) then that is respected

Technical Details

This is an alternate approach to @mfb's approach of trying to move the hard-coding to utf8mb4. When I looked at @mfb's excellent conversion script I realised he was changing the DB default & I started to think the right way to handle charset was to ensure the DB defaults were being set and only override them when we need to. This will only override if the default is not set

Comments

@civibot civibot bot added the master label Nov 29, 2019
@civibot
Copy link

civibot bot commented Nov 29, 2019

(Standard links)

@mattwire
Copy link
Contributor

@eileenmcnaughton Can you please rebase?

@eileenmcnaughton
Copy link
Contributor Author

@mattwire done

}
$dbUTF = CRM_Core_BAO_SchemaHandler::getDBCollation();
if (in_array($dbUTF, ['utf8_unicode_ci', 'utf8mb4_unicode_ci'])
&&in_array($dbUTF, ['utf8', 'utf8mb4'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton Please add a space after the &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattwire done

@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Jan 13, 2020
@mattwire
Copy link
Contributor

@eileenmcnaughton I've tested this and it's ok to merge once the very minor typo is addressed above.

@eileenmcnaughton
Copy link
Contributor Author

@mattwire so OK to merge now?

@mattwire mattwire merged commit 488fc5e into civicrm:master Jan 17, 2020
@eileenmcnaughton eileenmcnaughton deleted the utf_temp branch January 17, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants