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

Use new i18n-data component to get country and language name translations #7976

Closed
wants to merge 3 commits into from

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 22, 2015

With this PR the new piwik/i18n-data component is used to get translations for language and country names.

I've extracted those translations to a separate component as it was blowing up our translation files a lot. There will be ~400 translation keys less without those.

The new component is already filled up with a lot of languages piwik is currently not available for. (I've used some other open source libraries holding such data)

To get missing translations for countries and languages I will create a separate transifex project, that will share the translators...

Future vision
After this PR was merged I will check if we also could move the translations for continents, month and day names. Maybe we could also use that component to handle the different date formats. Currently they are handled within the translations which leads to errors in displaying dates in piwik if the format is wrong (which was the case for some languages)

@sgiehl sgiehl added the Needs Review PRs that need a code review label May 22, 2015
@sgiehl sgiehl force-pushed the i18n-data branch 2 times, most recently from 47b8dc9 to 702ad9b Compare May 22, 2015 21:58
@sgiehl sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. Needs Review PRs that need a code review and removed Needs Review PRs that need a code review Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 22, 2015
*/
public function getTranslatedCountry($countryCode, $language=null)
{
$filePattern = PIWIK_INCLUDE_PATH . '/vendor/piwik/i18n-data/countries/%s.json';
Copy link
Member

Choose a reason for hiding this comment

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

I'm just curious. Is it best practice to compose a bath to vendor like this? Should we maybe have it somewhere in the DI container or is there a method that we can use or so? Maybe it is best practice so just ignore :) Maybe @mnapoli knows more?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could indeed put the path into the container (like there is path.tmp, there could be path.vendor for example). Also maybe in a few years: http://puli.io/

@tsteur
Copy link
Member

tsteur commented May 24, 2015

Small comments but otherwise 👍

@mnapoli
Copy link
Contributor

mnapoli commented May 24, 2015

FYI there are already intl data in the internal Intl component in Piwik: https://github.com/piwik/piwik/tree/master/core/Intl/Data We should probably merge this data with the new component.

By the way are we doing anything that could duplicate effort done by any other project? E.g. in the future vision, is there any reason not to want to use something done by Symfony or ZF? Maybe they don't provide translated country names, but for formatting date they probably do? We could maybe get rid of some code.

Also regarding the new intl-data component, I downloaded it and it weights 3.2 Mb of JSON files when unzipped. Is this some data that we already have in Piwik today or will some of it not be used?

@mnapoli
Copy link
Contributor

mnapoli commented May 24, 2015

Absolutely random but the last thing that popped up just now in my GitHub timeline is this: https://github.com/commerceguys/intl

@sgiehl
Copy link
Member Author

sgiehl commented May 24, 2015

Hm. https://github.com/commerceguys/intl seems to hold quite similar data.

The reason why the component already has 3.2 MB is because I've added big bunch of translations to languages Piwik currently doesn't have.
Guess we could also reduce that again unti Piwik is available in a new language.

I'll have some more thoughts on that tomorrow. Maybe there's another (better) solution how to handle those data.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label May 25, 2015
@sgiehl
Copy link
Member Author

sgiehl commented May 25, 2015

I'll close this PR for now and issue another one later. I'll integrate the required data within core/Intl and reduce it on that what is realy required

@sgiehl sgiehl closed this May 25, 2015
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.

None yet

3 participants