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

Refactoring translation handling into a Translation component #6909

Merged
merged 15 commits into from Jan 5, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Dec 30, 2014

Translations were stored in a $GLOBALS array and handled by a static class Piwik\Translate. Furthermore, handling of translations were spread all over the place, caching logic was mixed with translation logic, …

This refactoring contains:

  • created a Translation component following the naming of Symfony's Translation component ("Translator" class, "Loaders", …)
  • all logic related to loading/handling translations is now encapsulated in that component
  • classes of that component are using dependency injection
  • Piwik\Translate is kept for BC and forwards call to the new Translator class
  • that new component is decoupled from the rest of Piwik (except for a few exceptions to improve later): it doesn't know about the HTTP request, about Plugins, about Piwik directories, etc...
  • translations are lazily loaded, i.e. you ask for a french translation then french translations will be loaded automatically
  • that means that Piwik/plugins don't have to tell the translator to load anything: the plugin manager just configures the directories where to find translations (which are the installed plugins…)
  • there is a LoaderInterface with the following implementations:
    • JsonFileLoader
    • LoaderCache: caches another loader by wrapping it (which would be the JsonFileLoader)
  • that way of caching things brings the following benefits:
  • as said above, there's a new dev environment (= new DI config file) where translation cache is disabled: this environment can be used for other things as well
  • there is now a fallback mechanism (previously all translations were merged together in one array): if a french/… translation is not found, then the english one is returned
  • that fallback mechanism would allow to do Add support for a Translation file to extend another language (eg. Spanish Argentina extends spanish) #4983 (regional overrides) by having multiple fallbacks, for example: fr_CA (canadian french) would fallback to fr which would fallback to en (not implemented)
  • moved Piwik\Translate\Writer and its subclasses to the LanguagesManager plugin because these classes were only used in that plugin

And I was able to unit tests the new classes, which is just an awesome feeling :)

There's still a lot of room for improvement, I'll keep working on that PR. Feedback welcome!

@mnapoli mnapoli added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels Dec 30, 2014
@mnapoli mnapoli self-assigned this Dec 30, 2014
@mattab mattab added this to the Piwik 2.11.0 milestone Jan 5, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 5, 2015

OK to review and merge (once tests are finished and green)

When merged, TODO:

@mnapoli mnapoli removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jan 5, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 5, 2015

OK I need to rebase again… Also this PR is for 2.11 so maybe don't merge yet?

@mattab
Copy link
Member

mattab commented Jan 5, 2015

It looks excellent, a nice clean code change for the new year 👍

mattab pushed a commit that referenced this pull request Jan 5, 2015
Refactoring translation handling into a Translation component
@mattab mattab merged commit 899c37c into master Jan 5, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Jan 5, 2015

\o/

Cool you also updated everything else thanks!

@mnapoli mnapoli deleted the translation branch January 5, 2015 11:19
@mattab mattab added the c: i18n For issues around internationalisation and localisation. label Oct 13, 2015
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. c: i18n For issues around internationalisation and localisation. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants