-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[RFC] Translator tweaks #14265
Conversation
@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) { |
There was a problem hiding this comment.
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()
?
👍 |
@mpdude any chance to make an update ? |
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 Also note that the 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 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. |
@@ -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);
+ }
+ }
+
for BC break reason it should be moved into V3.0 also we need add some test cases that cover these change. |
@aitboudad @mpdude What's the status of this PR? |
As there is no more activity here, I'm closing this PR. Feel free to reopen a new one if that's still relevant. |
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 ofloadCatalogue
) 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?