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

M3: Change UTF8 to utf8mb4 for new installation #8349

Merged
merged 9 commits into from Jan 29, 2020

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Jan 23, 2020

Please be sure you are submitting this against the staging branch.

Q A
Bug fix?
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Description:

This PR is follow up of this PR #6554

Based on this discussion #6490 and this article https://mathiasbynens.be/notes/mysql-utf8mb4
This PR add as default charset utf8mb4 which allow use all unicode symbols (like emoji)

Conclusion from article

Never use utf8 in MySQL — always use utf8mb4 instead. Updating your databases and code might take some time, but it’s definitely worth the effort. Why would you arbitrarily limit the set of symbols that can be used in your database? Why would you lose data every time a user enters an astral symbol as part of a comment or message or whatever it is you store in your database? There’s no reason not to strive for full Unicode support everywhere. Do the right thing, and use utf8mb4. 🍻

Steps to test this PR:

  1. Create new installation
  2. Go to emails, create email with 🥐 emoji in subject
  3. Should works properly.
  4. The old installation with this code applied should work as before.

List backwards compatibility breaks:

Hard to say. We have to setup VARCHAR to 191 lenght. More in FriendsOfSymfony/FOSUserBundle#1919 (comment)

@kuzmany kuzmany changed the title Change UTF8 to utf8mb4 for new installation M3: Change UTF8 to utf8mb4 for new installation Jan 23, 2020
@escopecz escopecz added this to the 3.0.0 milestone Jan 23, 2020
@escopecz
Copy link
Sponsor Member

escopecz commented Jan 23, 2020

I fixed other places where Mautic was using 255 instead of 191 length for varchars. Now the tests are passing and Mautic runs properly with the database built on previous Mautic 2 version. The only difference is that the bin/console doctrine:schema:update --dump-sql command outputs queries like this one:

ALTER TABLE example_table CHANGE date_added date_added DATETIME DEFAULT NULL COMMENT '(DC2Type:datetime)', CHANGE created_by created_by INT DEFAULT NULL, CHANGE created_by_user created_by_user VARCHAR(191) DEFAULT NULL, CHANGE date_modified date_modified DATETIME DEFAULT NULL COMMENT '(DC2Type:datetime)', CHANGE modified_by modified_by INT DEFAULT NULL, CHANGE modified_by_user modified_by_user VARCHAR(191) DEFAULT NULL, CHANGE checked_out checked_out DATETIME DEFAULT NULL COMMENT '(DC2Type:datetime)', CHANGE checked_out_by checked_out_by INT DEFAULT NULL, CHANGE checked_out_by_user checked_out_by_user VARCHAR(191) DEFAULT NULL, CHANGE name name VARCHAR(191) NOT NULL;

So the migration is not necessary if it's not a pain point for the Mautic user. It will run the same as before. The goal is for the new Mautic installations to use the right database encoding.

@escopecz escopecz added this to Needs code review and/or test in Mautic 3 Jan 23, 2020
@escopecz escopecz added essential This must be done to close the milestone ready-to-test PR's that are ready to test labels Jan 29, 2020
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I tested this PR on an existing Mautic database with no migration and on new installation. I did not notice any issue. The newly created database has the correct encoding and varchar lengths.

Screenshot 2020-01-29 at 14 16 07

Screenshot 2020-01-29 at 14 16 31

@escopecz escopecz merged commit a522fa3 into mautic:3.x Jan 29, 2020
@escopecz escopecz removed essential This must be done to close the milestone ready-to-test PR's that are ready to test labels Jan 29, 2020
@escopecz escopecz moved this from Needs code review and/or test to Done in Mautic 3 Jan 29, 2020
@simcen
Copy link
Contributor

simcen commented Feb 24, 2021

Hello @escopecz and @kuzmany
I came across this PR investigating an issue when Mautic crashed when trying to save emails with newer emojis in the subject. Turns out that the EmojiHelper doesn't support newer ones and doesn't turn them into short coes. Thanks for this PR, new installations don't have that issue anymore.

Two questions:

  • Is there any idea how to migrate existing installations to the newer charsets?
  • Would this PR make the EmojiHelper not needed anymore as emojis can be saved as unicode?

Thanks
Simon

@escopecz
Copy link
Sponsor Member

Thanks!

Is there any idea how to migrate existing installations to the newer charsets?

It's complicated as the migration takes forever on bigger databases. MySql 8 solves this problem but not all Mautic instances are on MySql 8, so we cannot fix it systematically. the other problem is with foreign keys. We'd have to drop foreign keys, run the charset migration and enable the foreign keys again. But that would mean to suspend the instance for the time of the migration so the database won't get into an inconstant state.

Would this PR make the EmojiHelper not needed anymore as emojis can be saved as unicode?

Yes, once the encoding will be changed from UTF8 to UTF8MB4 then we can delete EmojiHelper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Mautic 3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants