[Translation] Crowdin Bridge: Fix locale vs LanguageId #50040

pculka · 2023-04-17T13:16:52Z

Q	A
Branch?	6.2
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #...
License	MIT
Doc PR	symfony/symfony-docs#...

carsonbot · 2023-04-17T13:16:54Z

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

andrii-bodnar · 2023-04-18T13:23:20Z

src/Symfony/Component/Translation/Bridge/Crowdin/CrowdinProvider.php

@@ -109,6 +109,25 @@ public function read(array $domains, array $locales): TranslatorBag
        $translatorBag = new TranslatorBag();
        $responses = [];

+        $localeLanguageMap = [];
+        foreach ($this->listLanguages($locales) as $language) {


this method doesn't accept any arguments

Probably it would be better to extract this to a separate method, e.g. mapLocaleToLanguageId or something like this

andrii-bodnar · 2023-04-18T13:24:55Z

src/Symfony/Component/Translation/Bridge/Crowdin/CrowdinProvider.php

@@ -118,7 +137,7 @@ public function read(array $domains, array $locales): TranslatorBag

            foreach ($locales as $locale) {
                if ($locale !== $this->defaultLocale) {
-                    $response = $this->exportProjectTranslations($locale, $fileId);
+                    $response = $this->exportProjectTranslations($localeLanguageMap[$locale], $fileId);


The same issue probably is also relevant for the uploadTranslations method

Actually from my observation the mapping is done on Crowdin side and works (i.e. de-DE source gets properly mapped to de)

andrii-bodnar · 2023-04-18T13:31:24Z

Hi @pculka, could you please provide locale examples than currently can't be synced with Crowdin?

andrii-bodnar · 2023-04-18T13:33:02Z

The failed Unit tests do not expect the GET languages call, you should include the mock for this call in the tests

pculka · 2023-04-18T13:42:46Z

Hi @andrii-bodnar - In our project we use full locale codes, not language codes. For example for Germany the full locale is de-DE. This is in languages endpoint of Crowdin is stated as locale attribute:

"data": {
  "id": "de",
  "name": "German",
  "editorCode": "de",
  "twoLettersCode": "de",
  "threeLettersCode": "deu",
  "locale": "de-DE",
  "androidCode": "de-rDE",
  "osxCode": "de.lproj",
  "osxLocale": "de",
  "pluralCategoryNames": [
      "one",
      "other"
  ],
  "pluralRules": "(n != 1)",
  "pluralExamples": [
      "1",
      "0, 2-999; 1.2, 2.07..."
  ],
  "textDirection": "ltr",
  "dialectOf": null
}

When downloading translations in your code - you also properly annotated that the requested parameter has to be LanguageID, but you pass in locale and that fails. If I pass locale, I have to find the correct language ID by the locale and then the languages will be downloaded.

pculka · 2023-04-18T13:47:32Z

I'll try to add GET for the mock

andrii-bodnar · 2023-04-18T13:59:18Z

@pculka got it, the current approach might be unreliable since a user can use a different language code format and in this case it's hard to guess the right language ID. I would suggest encouraging users to use Language Mapping in this case.

The flow:

a user has some locale that is different from the default Crowdin language ID
a user set up the Language mapping for this locale in the Crowdin project
then, on the Provider side you can use the Get Project to get the languageMapping property and find the language ID for the mapped locale.
use this language ID for requests.

This will not break any existing integrations and will allow customizing the used language codes.

By the way, something similar is suggested for the other translation provider here - https://symfony.com/doc/current/translation.html#installing-and-configuring-a-third-party-provider

pculka · 2023-04-18T14:35:53Z

Well I think the core problem lies in misunderstanding and misdocumentation of how to use locales should be used within symfony itself. It tends to be easy to confuse code vs locale vs osxlocale, etc. That's why I ended up here, as I was using proper i18n locale string, but in communication with CrowdIn the expected parameters are Language IDs.
Hence in your suggestion you suggest MIX UP between locale and language id once again. I mean - it is doable, but not sure if that's the right thing to do.

Anyway I've added the mapping, in get project info I now have:

        "languageMapping": {
            "de": {
                "locale": "de-DE"
            },
            "hu": {
                "locale": "hu-HU"
            }
        },

You suggest I change this code I provided to check for the project languageMapping setting and just fix it that way, right?

andrii-bodnar · 2023-04-18T14:44:52Z

@pculka does Symfony has some language codes list that is recommended to use for translation catalogs?

pculka · 2023-04-18T19:03:02Z

I doubt it. both 'de' and 'de-DE' are valid locale strings, both referencing german language, but -DE specifies also the region which is optional. I mean. Meaning de-DE should try automatically a fallback maybe? Not sure how to tackle this situation properly.

andrii-bodnar · 2023-04-19T06:32:49Z

Well, the Language Mapping approach might be more complicated. Probably we can keep the suggested approach in this case and not overcomplicate this for now

pculka · 2023-04-19T12:26:24Z

added one more fallback for underscore locale (you had that in one of your tests), and tests are passing.
Hopefully someone merges this

pculka · 2023-04-19T12:34:21Z

and refactored - extracted the mapping function out

pculka · 2023-04-20T06:39:34Z

@nicolas-grekas please can you check this PR? I also noticed a failed test in appveyor, 1) Symfony\Component\Messenger\Bridge\Redis\Tests\Transport\RedisExtIntegrationTest::testGetAfterReject

something in terms of redis. Maybe there is a problem with redis.dll for windows, which is not compiled for 8.2 - If it is needed, I compiled a working redis.dll for 8.2.5 for x64 both ts and nts versions: https://github.com/pculka/phpredis/releases/tag/5.3.7

But if it is possible to merge without passing appveyor, it would be great to get this done (and if @andrii-bodnar agrees)

nicolas-grekas

I'm going to trust you on this one :)

src/Symfony/Component/Translation/Bridge/Crowdin/CrowdinProvider.php

nicolas-grekas · 2023-04-20T12:36:58Z

Thank you @pculka.

…i-bodnar) This PR was submitted for the 6.4 branch but it was merged into the 6.3 branch instead. Discussion ---------- [Translation][Crowdin] Use project language mapping | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Sometimes the locale codes may be different from the [codes](https://developer.crowdin.com/language-codes/) used in the Crowdin API. This will cause API errors because the wrong code is passed in the upload and download translation requests. There was already a fix (#50040), but it only works partially. The suggested approach uses the [language mapping](https://support.crowdin.com/project-settings/#languages) configured in the Crowdin project. If the locale code and the crowdin code are different, the user needs to configure the language mapping in the Crowdin project. Documentation - symfony/symfony-docs#19325 Commits ------- a1d2f67 [Translation] Crowdin Bridge: use the project language mapping

pculka marked this pull request as ready for review April 17, 2023 13:17

carsonbot added Status: Needs Review Bug Translation labels Apr 17, 2023

carsonbot added this to the 6.2 milestone Apr 17, 2023

nicolas-grekas requested a review from welcoMattic April 17, 2023 13:30

andrii-bodnar reviewed Apr 18, 2023

View reviewed changes

[Translation] Crowdin Bridge: Fix locale vs LanguageId

38ce6c3

nicolas-grekas force-pushed the fix-locale-vs-languageid branch from b4f4695 to 38ce6c3 Compare April 20, 2023 12:34

nicolas-grekas approved these changes Apr 20, 2023

View reviewed changes

src/Symfony/Component/Translation/Bridge/Crowdin/CrowdinProvider.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 20, 2023

nicolas-grekas merged commit 5269b70 into symfony:6.2 Apr 20, 2023
7 checks passed

fabpot mentioned this pull request Apr 28, 2023

Release v6.2.10 #50189

Merged

andrii-bodnar mentioned this pull request Dec 21, 2023

[Translation][Crowdin] Use project language mapping #53178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Translation] Crowdin Bridge: Fix locale vs LanguageId #50040

[Translation] Crowdin Bridge: Fix locale vs LanguageId #50040

pculka commented Apr 17, 2023

carsonbot commented Apr 17, 2023

andrii-bodnar Apr 18, 2023

andrii-bodnar Apr 18, 2023

andrii-bodnar Apr 18, 2023

pculka Apr 19, 2023

andrii-bodnar commented Apr 18, 2023

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

pculka commented Apr 18, 2023 •

edited

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

andrii-bodnar commented Apr 19, 2023

pculka commented Apr 19, 2023

pculka commented Apr 19, 2023

pculka commented Apr 20, 2023

nicolas-grekas left a comment

nicolas-grekas commented Apr 20, 2023

[Translation] Crowdin Bridge: Fix locale vs LanguageId #50040

[Translation] Crowdin Bridge: Fix locale vs LanguageId #50040

Conversation

pculka commented Apr 17, 2023

carsonbot commented Apr 17, 2023

andrii-bodnar Apr 18, 2023

Choose a reason for hiding this comment

andrii-bodnar Apr 18, 2023

Choose a reason for hiding this comment

andrii-bodnar Apr 18, 2023

Choose a reason for hiding this comment

pculka Apr 19, 2023

Choose a reason for hiding this comment

andrii-bodnar commented Apr 18, 2023

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

pculka commented Apr 18, 2023 • edited

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

andrii-bodnar commented Apr 18, 2023

pculka commented Apr 18, 2023

andrii-bodnar commented Apr 19, 2023

pculka commented Apr 19, 2023

pculka commented Apr 19, 2023

pculka commented Apr 20, 2023

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 20, 2023

pculka commented Apr 18, 2023 •

edited