Skip to content

[RFC] Translator tweaks #14265

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

Closed
wants to merge 1 commit into from
Closed

[RFC] Translator tweaks #14265

wants to merge 1 commit into from

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Apr 7, 2015

While playing around with the Translator component, I noticed that when a Translator for {A, B} (primary, fallback) locales is used and writes the Catalogue into the cache, there will be no cached Catalogue usable for a {B}-only Translator.

At first, I thought this was only because a wrong method (initCatalogue instead of loadCatalogue) was called.

But as the cached catalogue also embeds the "fallback" chain of catalogues, it was (before this PR) also not possible to simply re-use a catalogue because the re-using Translator might have a different set or order of fallback locales.

This PR changes the way catalogues are cached - they will be cached stand-alone (per locale). The "fallback locale" wiring takes place once the catalogue has been loaded, depending on the Translator's settings.

I have no figures, but this might help to improve cache efficiency and resource usage, in particular because catalogues for fallback locales and not "blended" (possibly several times) into different cache files.

Thoughts?

@aitboudad
Copy link
Contributor

@mpdude this increases readability but I'm not sure it can improve performance, can you give us some stats(prod/dev) ?

@@ -486,19 +445,36 @@ private function loadFallbackCatalogues($locale)
protected function computeFallbackLocales($locale)
{
$locales = array();
$candidateLocales = array();

if (strrchr($this->locale, '_') !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using here strpos()?

@aitboudad
Copy link
Contributor

👍

@aitboudad
Copy link
Contributor

@mpdude any chance to make an update ?

@mpdude
Copy link
Contributor Author

mpdude commented Apr 8, 2015

First, I don't have any numbers to show whether this improves or degrades performance; see #13948 for a discussion how such a benchmark should look like.

Then, this PR is about "wiring" up fallback MessageCatalogues at runtime. It would make the fix from #14268 unnecessary because the problem described there does not exist in the first place. But it also means that the change from #13981 (in fact, the entire getFallbackContent method) would be dropped because there is no more "fallback content" to dump into the ConfigCache.

Also note that the computeFallbackLocales method is changed slightly. It would previously return all configured fallback locales, excluding the "current" $locale, but including fallback variants from that (en for en_GB, for example). With this PR, it will return all locales that need to be tried should $locale fail, including fallback variants from each of these. Honestly, I don't know if that might be a problem.

Altogether I'm not really sure if this takes us somewhere or what the final destination might be. It's just a feeling that putting smaller and independent MessageCatalogues (i. e. only for one locale; possibly only for one domain) into the cache might be a prerequisite for further deferring catalogue loading (for example, load only catalogue parts for the domain needed in the current trans() request; load catalogues for fallback locales step by step when no translation is found).

That's why it is an RFC :-). I'd be glad to fine-tune the details once we agree that it is worth the effort.

@aitboudad
Copy link
Contributor

@@ -358,6 +358,20 @@ class Translator implements TranslatorInterface, TranslatorBagInterface
         $cache = new ConfigCache($this->cacheDir.'/catalogue.'.$locale.'.php', $this->debug);
         if ($forceRefresh || !$cache->isFresh()) {
             $this->initializeCatalogue($locale);
+            if (!$this->debug) {
+                $catalogue = $this->catalogues[$locale];
+                // merge all fallback catalogues messages into $catalogue
+                $fallbackCatalogue = $catalogue->getFallbackCatalogue();
+                $messages = $catalogue->all();
+                while ($fallbackCatalogue) {
+                    $messages = array_replace_recursive($fallbackCatalogue->all(), $messages);
+                    $fallbackCatalogue = $fallbackCatalogue->getFallbackCatalogue();
+                }
+                foreach ($messages as $domain => $domainMessages) {
+                    $catalogue->add($domainMessages, $domain);
+                }
+            }
+
  • computeFallbackLocales

for BC break reason it should be moved into V3.0 also we need add some test cases that cover these change.

@fabpot
Copy link
Member

fabpot commented Jan 27, 2016

@aitboudad @mpdude What's the status of this PR?

@fabpot
Copy link
Member

fabpot commented Jun 9, 2016

As there is no more activity here, I'm closing this PR. Feel free to reopen a new one if that's still relevant.

@fabpot fabpot closed this Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants