[Translation] Add icu message support for the lokalise provider #45368
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
|
I looked up how the extraction command is handling the ICU thing, and there, if i undestand it correctly, all message will extracted as ICU. The only exception is when the translation key is already in a domain which is not ICU. So maybe my approach here is wrong to begin with. So i would really appreciate some help or advices to the right direction! :) |
src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
Outdated
Show resolved
Hide resolved
|
I am still having problems even with these changes and try to figure it out with the Lokalise support. The |
|
So after a longer discussion with the Lokalise support i got to this version of the LokaliseProvider: The Diff
diff --git a/src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php b/src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
index b86d3924ff..b431d263bf 100644
--- a/src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
+++ b/src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
@@ -14,6 +14,7 @@ namespace Symfony\Component\Translation\Bridge\Lokalise;
use Psr\Log\LoggerInterface;
use Symfony\Component\Translation\Exception\ProviderException;
use Symfony\Component\Translation\Loader\LoaderInterface;
+use Symfony\Component\Translation\MessageCatalogue;
use Symfony\Component\Translation\MessageCatalogueInterface;
use Symfony\Component\Translation\Provider\ProviderInterface;
use Symfony\Component\Translation\TranslatorBag;
@@ -37,14 +38,16 @@ final class LokaliseProvider implements ProviderInterface
private $logger;
private $defaultLocale;
private $endpoint;
+ private $intlIcuEnabled;
- public function __construct(HttpClientInterface $client, LoaderInterface $loader, LoggerInterface $logger, string $defaultLocale, string $endpoint)
+ public function __construct(HttpClientInterface $client, LoaderInterface $loader, LoggerInterface $logger, string $defaultLocale, string $endpoint, bool $intlIcuEnabled = false)
{
$this->client = $client;
$this->loader = $loader;
$this->logger = $logger;
$this->defaultLocale = $defaultLocale;
$this->endpoint = $endpoint;
+ $this->intlIcuEnabled = $intlIcuEnabled;
}
public function __toString(): string
@@ -102,7 +105,14 @@ final class LokaliseProvider implements ProviderInterface
foreach ($translations as $locale => $files) {
foreach ($files as $filename => $content) {
- $translatorBag->addCatalogue($this->loader->load($content['content'], $locale, str_replace('.xliff', '', $filename)));
+ $domain = str_replace('.' . $locale . '.xliff', '', $filename);
+
+ if ($this->intlIcuEnabled) {
+ $domain .= MessageCatalogue::INTL_DOMAIN_SUFFIX;
+ }
+
+ $catalogue = $this->loader->load($content['content'], $locale, $domain);
+ $translatorBag->addCatalogue($catalogue);
}
}
@@ -149,6 +159,8 @@ final class LokaliseProvider implements ProviderInterface
'filter_langs' => array_values($locales),
'filter_filenames' => array_map([$this, 'getLokaliseFilenameFromDomain'], $domains),
'export_empty_as' => 'skip',
+ 'placeholder_format' => $this->intlIcuEnabled ? 'icu' : 'symfony',
+ 'include_tags' => ['symfony'],
],
]);
@@ -183,6 +195,7 @@ final class LokaliseProvider implements ProviderInterface
'android' => null,
'other' => null,
],
+ 'tags' => ['symfony'],
];
}
@@ -352,6 +365,6 @@ final class LokaliseProvider implements ProviderInterface
private function getLokaliseFilenameFromDomain(string $domain): string
{
- return sprintf('%s.xliff', $domain);
+ return sprintf('%s.%s.xliff', $domain, '%LANG_ISO%');
}
}In this diff are 3 changes, 2 of them which are imho important, the last one less important but nice to have.
What do you think about these points? I would happy to create these into issues / PR's. |
|
@welcoMattic Can you have a look at the latest comment from @DanielBadura? |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Outdated
Show resolved
Hide resolved
For the first 2 points, I'm ok with it, you did great work, thank you. Just a small comment about parameter naming.
About this, I have been wondering for some time if a particular configuration per provider is something to add. And your comment goes in this direction: for example, we could allow users to define one or more tags to add to their Symfony translations, but without forcing it. We should consider it as a feature to be done in another PR. |
|
@DanielBadura could you have a look to failing checks? Otherwise, I think this PR is ready to merge for me |
|
Yes i will try to look at it the next days @welcoMattic :) |
…ding the intlIcuEnabled parameter
74633ef to
692f87a
Compare
|
So i rebased again and added a default value for the provider creation for the tests. That resolved most of the failing test. Now only my new tests are failing :D. But I'm really not sure why I went through it mutltiple times now and i dont understand why the test is failing.. Maybe someone can also have a look? 😅 |
|
@DanielBadura I'll try to take a look on it this week |
src/Symfony/Component/Translation/Bridge/Lokalise/Tests/LokaliseProviderTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Mathieu Santostefano <msantostefano@protonmail.com>
|
code style checks from fabbot seems unrelated 🤔 |
|
Sorry to come late in the review, but can't we use the intl-icu suffix in the domain to decide for the correct format? |
|
Maybe this is possible for sending the files based on that. But if we have no files local and want to read the files from the provider we need to pass the option to get it in the right format. |
|
Can't we set some metadata on the provider side when pushing to know the format? |
|
Im not sure how this can help us since we need to know the format before the query the provider. The format is part of the inital request to retrieve the data. If i understand correctly what you try to achieve is that lokalise is giving us the data in a specific format if we tell them so in some kind of metadata when uploading. AFAIK that is not a feature lokalise e.g. is offering. Maybe with their automation feature the user can somehow achieve that but Im not sure about that. Or did I misunderstood you? EDIT: |
|
You've got me right. Is lokalize the only provider that allow fetching the messages in various formats? I feel like the other providers don't deal with formats and give back strings as is. Am I right? |
|
I can not answer this question about the other providers since i only worked with Side note: The namings for all the providers can be confusing.
So i assumed you meant |
|
I'm going to delegate to @welcoMattic at this point 😬 |
|
@DanielBadura yes, the Provider names are confusing, but we can not do anything about that except to be rigorous when mentioning them. About the main problem, which I understand is telling Lokalise if we want to download translation under ICU format or "symfony" format, IIRC Lokalise is the only provider (for now) which lets us define the format at download by exposing a parameter in the query, so let's use it. About @nicolas-grekas comment:
No we can't, because at the provider level, we only know the domain itself, not the variant (with or without So, for me the only way to do, is the one has been done here: expose a config option for Lokalise provider, to explicitly ask for ICU format or "symfony" format. Now, one question remains: should we set the config option at |
|
icu if |
I had considered this too in the first place. But imho thats not a good approach since you could have intl extension enabled but still using the "old" / "symfony" format. So from my point of view configuring and let the user decide is the best option. Also as i said before, i think it would be good to configure it for the whole component. Then this config could also be used to determine in which format the translations should be created. But this is another topic but maybe relevant. |
|
But if the provider is in control, we don't care about the exact format on the app's side, do we? If not, we should fetch the most capable format, which is ICU. Why would the app want to decide what it works with when it can delegate to the provider? |
|
@DanielBadura about Loco (localise.biz), they also expose a parameter to force icu format at export (see https://localise.biz/api/docs/export/exportall). Are you ok to open another PR to add support for icu format on Loco Provider, after this one? |
|
Well since I'm not working with Loco or generally with translations right now, I can not really test it. 😅 |
Ok, no problem, I will take it |
| @@ -149,6 +156,7 @@ private function exportFiles(array $locales, array $domains): array | |||
| 'filter_langs' => array_values($locales), | |||
| 'filter_filenames' => array_map([$this, 'getLokaliseFilenameFromDomain'], $domains), | |||
| 'export_empty_as' => 'skip', | |||
| 'placeholder_format' => $this->intlIcuEnabled ? 'icu' : 'symfony', | |||
There was a problem hiding this comment.
| 'placeholder_format' => $this->intlIcuEnabled ? 'icu' : 'symfony', | |
| 'placeholder_format' => $this->intlIcuEnabled ? 'icu' : 'symfony', | |
| 'plural_format' => $this->intlIcuEnabled ? 'icu' : 'symfony', |
A different parameter is required to ensure that the plurals are also in the expected ICU format. The param is called plural_format which should contain the same value as placeholder_format. See https://developers.lokalise.com/reference/download-files for all params. It would be great if that can be included. Without this option users might get invalid ICU translation files
There was a problem hiding this comment.
Without this change, you will still get plurals in the old format from Lokalise, which the Symfony Translator is not expecting in ICU files
There was a problem hiding this comment.
This would be really weird behaviour IMHO, but better safe then sorry i guess :D
Could you verify this?
There was a problem hiding this comment.
It's weird, but I guess @TwanVermeulen is right. According to the documentation https://developers.lokalise.com/reference/api-plurals-and-placeholders#plural-format-descriptions I guess we have to pass both parameters.
There was a problem hiding this comment.
I can add it, but as i said - cant really verify for now as I'm not working anymore on the project which needed this patch. So is this verified that this is needed and working?
|
So what could be the next steps to finish this PR? IIRC we did not conclude on how the configuration should be done. |
|
@DanielBadura sorry for the delay. To finish this PR I think we need to be sure about the plural parameter value. About the configuration, I think we are ok. We need to enable the ICU format per Provider. As I mentioned earlier, I will take care of Loco Provider once this PR will be merged. |
|
I'm not sure if it helps, but I did verify this myself + I got the confirmation from the Lokalise support desk. If you want to receive the plurals in ICU format, you need to specify this via plural_format configuration. |
|
I'd like to have an answer to my previous comment:
WDYT? I'm not happy with adding a config option... |
|
IMHO the app is in control in which format it is using the translations. I'm not sure how we want to delegate this to the provider to decide 🤔. |
Some apps are still working with the old plural format, not ICU. For those apps, we must keep an option to not fetch translations in ICU format, otherwise I guess the fetched translations will be in a weird format (old plural format treated as ICU). Did I miss something? |
|
How can we move forward here? |
| @@ -857,6 +857,7 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode, callable $e | |||
| ->fixXmlConfig('locale') | |||
| ->children() | |||
| ->scalarNode('dsn')->end() | |||
| ->booleanNode('intl_icu_enabled')->defaultFalse()->end() | |||
There was a problem hiding this comment.
| ->booleanNode('intl_icu_enabled')->defaultFalse()->end() | |
| ->booleanNode('intl_icu')->defaultFalse()->end() |
I think it's better to name it short.
I'm still convinced that we must leave the control of ICU or not to the app. For example, an app with the special The configuration option seems to be our only solution here. It's totally acceptable for me. @nicolas-grekas @DanielBadura WYDT? To move forward, this branch needs to be rebased. @DanielBadura, are the functional tests still relevant? (I mean, if you test this branch with real API calls, Lokalise responds as expected?) |
I think the same, hence why i proposed the configuration option.
I could rebase this branch. About the testing part: I'm no longer working on the project which had this implementation, so i cant really test it right now with real API calls to Lokalise. |
This PR would add the support for the ICU message format when using the lokalise provider. I saw that the unit tests are written as it would support the ICU message format so therefore the PR aims the 5.4 branch as a bugfix.
The API from lokalise needs a flag for the format to be downloaded, otherwise it return the file in the "classic" format with
%as the delimiter for the placeholders. Also the file was always saved without the suffix. For this i looked up how the ICU support was implemented in symfony and tried to adapt this approach.I think i would need some assitance how to write tests with and without the intl extension. Right now, some tests fail since the intl extension in enabled and i did not adjust them yet.