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

EZP-29573: Legacy installer should allow utf8mb4 charset #1386

Merged
merged 2 commits into from
Jan 11, 2019

Conversation

glye
Copy link
Member

@glye glye commented Aug 28, 2018

Fixes https://jira.ez.no/browse/EZP-29573
Ready for review

When creating a database in utf8mb4, the legacy installer complains that only utf-8 is supported. Since utf8mb4 is recommended now we should support both.

utf8mb4 is not available in every database eZ Publish 5.x supports, but then people won't be able to create such a DB, so it won't be a problem.

NB: Since we have not backported the utf8mb4 schema changes, in order for this to work one has to first create the database, then apply the changes in the advisory, and then run the installer.

@glye glye requested review from alongosz and andrerom August 28, 2018 14:11
@micszo micszo self-assigned this Sep 5, 2018
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Tried with a fresh install of v5.4.12-rc3 on a new utf8mb4 database.
Applied the diff from this PR.
Applied the script from the advisory / EZP-29110.
Updated charset in i18n.ini and ezpublish.yml.
Reloaded all caches.

Having problems with publishing new Folder or Article (504 Gateway Time-out).
Also icons with Content types on Sub items list in Content structure are not displayed correctly.
(provided more details on slack)

@andrerom
Copy link
Contributor

andrerom commented Oct 4, 2018

@glye did you figure out anything on this?

@glye
Copy link
Member Author

glye commented Oct 5, 2018

@andrerom No, delayed by other things. Need to get back to it.

@andrerom
Copy link
Contributor

@vidarl was this one of the issue you where hitting when trying to use setup wizard?

@vidarl
Copy link
Member

vidarl commented Oct 29, 2018

@vidarl was this one of the issue you where hitting when trying to use setup wizard?

@andrerom : no. My problem was that the setup wizard would refuse to use a database which has the 'utf8mb4' character set... So after choosing the database to use (in the drop-down and click 'next'), it would say 'Hey.... this db is not in "utf8"'

@andrerom
Copy link
Contributor

@vidarl that sounds exactly like the description in PR here :)

@vidarl
Copy link
Member

vidarl commented Oct 29, 2018

hehe.. that is right.. sorry. I was a bit quick and only read micszo's comment....

@micszo
Copy link
Member

micszo commented Oct 29, 2018

Co-Authored-By: glye <gunnstein.lye@ez.no>
@glye
Copy link
Member Author

glye commented Jan 10, 2019

@micszo Hi! Could you test this again with the last change from André, please?

@micszo
Copy link
Member

micszo commented Jan 11, 2019

@glye tried again on v5.4.12.1 with latest diff from this PR, followed steps as in my review from Sep 5. Getting the same results, 504 on publish etc.

Are there other places in the system apart from i18n.ini (i18n.ini.append.php) and ezpublish.yml where utf-8 should be changed to utf8mb4? (site.ini is also mentioned in JIRA...)
Maybe we need a more detailed instruction for this.

@micszo
Copy link
Member

micszo commented Jan 11, 2019

OK, breakthrough.
After completing all the steps I dropped the db, created it again, forced the setup wizard again, then applied the script from the advisory. Voilà. All issues gone.
Seems like there is some issue with sequence here.

@glye
Copy link
Member Author

glye commented Jan 11, 2019

@micszo Awesomtastic! So what was the difference in sequence from the previous test?

@micszo
Copy link
Member

micszo commented Jan 11, 2019

What I did.

dropped db, created again
rm -rf * in install dir
cloned meta
composer install tag
setup wizard
diff from PR (to unblock wizard)
script from advisory
updated charset (site.ini, i18n.ini, ezpublish.yml)
reloaded cache (admin and cli)

dropped db, created again
setup wizard
script from advisory
reloaded cache

Dunno why it didn't work without repeating those 4 steps.

@micszo
Copy link
Member

micszo commented Jan 11, 2019

I'm still uncertain about this because second wizard wiped some of those charset changes. Checking.

@micszo
Copy link
Member

micszo commented Jan 11, 2019

OK, I have gained some insight now.
Second wizard wiped my charset changes in i18n.ini and site.ini (.append.php), in ezpublish.yml change stayed.
The problems start occurring when charset is changed in i18n.ini. As long as charset is only changed in ezpublish.yml and site.ini it works fine.
Actually I'm not sure what is the correct setup here.

@glye
Copy link
Member Author

glye commented Jan 11, 2019

I looked through it again, and it seems correct that there should be no change in i18n.ini. Is there any reference to i18n.ini in the PR or in the jira issue, that I missed?

@micszo
Copy link
Member

micszo commented Jan 11, 2019

Nope, can't see it either. Must have looked for all places where charset was present. In that case, if it's not required there, we might be good to go. 😅

@glye
Copy link
Member Author

glye commented Jan 11, 2019

Yep, as I recall the i18n.ini change was a mistake, as that setting is about the output charset. The site.ini setting is for the database charset, which is what we want here. utf8mb4 is a mysql-only charset, for output we use utf-8.

@micszo micszo removed their assignment Jan 11, 2019
@glye glye merged commit 79f5e5a into master Jan 11, 2019
@glye glye deleted the ezp29573_legacy_installer_should_allow_utf8mb4 branch January 11, 2019 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants