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

Added support for utf8mb4 (emoji support) #798

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bertrandgorge
Copy link

Fixes issue #791

@@ -67,6 +67,11 @@ function qa_db_table_definitions()
require_once QA_INCLUDE_DIR . 'db/maxima.php';
require_once QA_INCLUDE_DIR . 'app/users.php';

if (defined('QA_USE_UTF8MB4') && QA_USE_UTF8MB4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this same check is used many times, I'd suggest extracting it into a function e.g. qa_db_get_mysql_collation()

@svivian
Copy link
Collaborator

svivian commented May 6, 2020

Hi, sorry for the late reply. Thanks for the PR, from a first glance it looks great! Would you be able to make the PR against the dev branch, as that is the one for the next major version?

At the moment it appears we can only set utf8mb4 prior to starting a new Q2A instance. One thing to consider is whether to add an upgrade path for existing sites. However it may be complex, as we need to detect the MySQL version and only upgrade those on 5.5.3+. But then the upgrade would need to be done on future versions in case they upgraded their MySQL version later. We'd also need people to manually change QA_USE_UTF8MB4 after the upgrade to avoid problems.

@bertrandgorge
Copy link
Author

Hello Scott, yes, I thought about the upgrade path, and I believe it's a lot of work for something people should do manually if they wish to. If you want I can make a "how to upgrade" for people who'd want to do that.

As for the dev branch, I'm not sure I know how to do that ? Should I create another PR ?

@rklec
Copy link

rklec commented Sep 15, 2020

So is there any news here? Emoji support would be a really nice feature by default for q2a.

@svivian
Copy link
Collaborator

svivian commented Sep 17, 2020

@rklec Yes I plan on merging it when I get a free moment.

@@ -107,7 +112,7 @@ function qa_db_table_definitions()
'avatarheight' => 'SMALLINT UNSIGNED', // pixel height of stored avatar
'passsalt' => 'BINARY(16)', // salt used to calculate passcheck - null if no password set for direct login
'passcheck' => 'BINARY(20)', // checksum from password and passsalt - null if no password set for direct login
'passhash' => 'VARCHAR(255) CHARACTER SET utf8 COLLATE utf8_bin DEFAULT NULL', // password_hash
'passhash' => 'VARCHAR(255) CHARACTER SET utf8 COLLATE '.$collation.' DEFAULT NULL', // password_hash
Copy link

Choose a reason for hiding this comment

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

You also need to change the charset if you change the collation, apparently...

Otherwise you may get an error like:

COLLATION 'utf8_bin' is not valid for CHARACTER SET 'utf8mb4'

(At least I got it in MariaDB)

Suggested change
'passhash' => 'VARCHAR(255) CHARACTER SET utf8 COLLATE '.$collation.' DEFAULT NULL', // password_hash
'passhash' => 'VARCHAR(255) CHARACTER SET utf8mb4 COLLATE '.$collation.' DEFAULT NULL', // password_hash

(Of course may be adjusted to work with the defined QA_USE_UTF8MB4 option.)

@svivian
Copy link
Collaborator

svivian commented Nov 12, 2020

Hi @bertrandgorge sorry I never got around to answering you. If you're able to create a new PR on the dev branch that would be great, thanks. Not sure how easy it would be... I think you may need to check out a fresh copy of the dev branch and re-apply the changes. (If that's too much hassle let me know and I'll try and sort it.)

@rklec
Copy link

rklec commented Nov 13, 2020

Actually, you just need to click "edit" at the top right of this PR and change the branch you want to merge this in. Depending on the changes you may of course need to merge in the dev branch afterwards.

@bertrandgorge
Copy link
Author

Hello @rklec I don't see that option.... @svivian if you can manage it, it would be great as I don't work on Question2Answer anymore unfortunately.

@rklec
Copy link

rklec commented Nov 13, 2020

You should be able to click "edit" and then change the branch below the title after the text "1 commit into".

@bertrandgorge bertrandgorge changed the base branch from master to dev November 13, 2020 13:54
@bertrandgorge
Copy link
Author

Got it. There's a conflict of course, coming from a refactorisation of qa-db.php...

@svivian
Copy link
Collaborator

svivian commented Nov 14, 2020

Yeah I’ve made a lot of changes in 1.9. Don’t worry about it, I’ll try and cherry pick what I can.

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.

3 participants