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-28950 Correct legacy instructions, site.ini is the best place for DB charset #2337

Merged
merged 1 commit into from May 24, 2018

Conversation

4 participants
@glye
Copy link
Member

commented May 23, 2018

Question Answer
JIRA issue EZP-28950
Bug/Improvement bug
New feature no
Target version 7.2
BC breaks no
Tests pass doc only
Doc needed yes

The original PR #2277 had this change in i18n.ini, for reasons unknown/forgotten. It should work fine according to docs, and no problem was found in testing. But @emodric reported failures due to the mysql utf8mb4 charset being expected in ini-files and templates. As he pointed out, site.ini is the right/best place for the DB charset, since you may want to have different charset in db and elsewhere.

Refs:
https://github.com/ezsystems/ezpublish-legacy/blob/master/settings/site.ini#L82
https://github.com/ezsystems/ezpublish-legacy/blob/master/settings/i18n.ini#L23
https://doc.ez.no/eZ-Publish/Technical-manual/4.x/Reference/Configuration-files/site.ini/DatabaseSettings/Charset
https://doc.ez.no/eZ-Publish/Technical-manual/4.x/Reference/Configuration-files/i18n.ini/CharacterSettings/Charset

@glye

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

Seems that for legacy to fully work, we have to add utf8mb4 to the encoding table, here: https://github.com/ezsystems/ezpublish-legacy/blob/d76ce9824dabaceeefa77530c8c611e447d1b109/lib/ezi18n/classes/ezcharsetinfo.php#L153

@gggeek

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

I'd say that db charset and app charset are two separate things (not recommended of course, but it could happen a lot in far-east countries). So keep separate settings in i18n and Database confi8g.

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

@gggeek We have to keep separate config anyway, because config from i18n.ini is used for templates, INI files and what not.

@emodric

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

@glye glye requested review from alongosz and andrerom May 23, 2018

@glye

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

@gggeek Yes, we'll keep the settings separate for sure. This PR is just about fixing upgrade instructions.

@glye glye removed the request for review from alongosz May 24, 2018

@glye glye merged commit b2a1ffd into master May 24, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot/phpcsfixer Code review by ezrobot
Details

@glye glye deleted the ezp28950_correct_legacy_doc branch May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.