[Translation] Phrase translation provider

[wickedOne]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #49142
License	MIT
Doc PR	symfony/symfony-docs#17861
Recipe PR	symfony/recipes#1172

[wickedOne]

not sure what's wrong with the license file... and the possible typo according to fabbot.io appears to be an unrelated false positive

[wickedOne]

thanks @xabbuh for the hint: fixed the licence problem
nevertheless now some unit tests fail which appear to be flaky / unrelated

[wickedOne]

out of curiosity: why is the declare(strict_types=1); enforced out of the classes, is that a compatibility thing or am i missing something obvious?

[chr-hertel]

Nice one @wickedOne!

Just tested it with the Symfony demo and initial push and pull works quite well with the recipe & docs, but when I update something in Phrase and do a pull again, there is no change in local files.

do i miss something?

btw, see #28423 for strict_type

[wickedOne]

hmmm, @chr-hertel that's weird,
with a push it basically uploads a xliff file with stuff to be updated so perhaps you were a bit too eager (i.e. the upload was still polling) but i'll have a look into that.

and thanks for linking the stric types discussion, revealed some things i wasn't aware of. nevertheless i think this mr has things covered regardless

[chr-hertel]

Okay, tried it again. what I did:

1. Fresh Phrase project
2. This changes in demo: symfony/demo@9132ce9 (DSN in .env.local)
3. manually created the tags validators, messages and security
4. push - works well 👍
   
5. pull
   

With explicit locales/messages on pull it works, at least for other locales than tr. don't get it. it's available in phrase...

[wickedOne]

yes, looks like the --force is needed to do the intesect

not sure whether this is intended behaviour, @welcoMattic any insights?

[wickedOne]

@chr-hertel sorry, completely misread your initial question where you were talking about a pull rather than a push.
it seems that the same issue applies for both: nothing happens without the --force option...

[wickedOne]

[deleted some of my comments to remove the noise]

@chr-hertel it appears this is intended behaviour:
for messages which don't exist in your application, but do exist in phrase (make sure they're tagged with the corresponding translation domain), one can use the bin/console translation:pull phrase command to synchronise to your application.

in order to overwrite your application's messages with changes done in phrase the --force option has to be used (also described in the command's option description):

--force Override existing translations with provider ones (it will delete not synchronized messages).

similar behaviour for the push command where the --force option updates messages on phrase with modifications done locally.

checking this behaviour however, i did notice a bug in the delete routine (--delete-missing option). i will update this pull request with a fix for that

[wickedOne]

ok, added fix for the --delete-missing option.
keep in mind that this option deletes the translation keys on phrase so it will be deleted for all locales.

[wickedOne]

fixed the locale initialisation which caused the error message mentioned in #issuecomment-1416735752

[chr-hertel]

Nice, yes, now it works for me with the demo! 👍

[wickedOne]

@chr-hertel resolved the event setter discussion and the readme one, also added a bit about locale matching in phrase (which indeed is intended behaviour from phrase's side, also see their updated documentation on that)

still like to have your thoughts about the concurrency bit...

[chr-hertel]

LGTM and works with Symfony Demo. Haven't tested the event system, but looks good as well!

[wickedOne]

I feel like we don't really need classes for PhraseCachedResponse, Read+WriteConfig and that simple arrays would do it.

not sure whether i agree for the read & write config as the read config contains logic regarding for phrase fallback locales as well removing the class would result in array operations in the provider. for consistency i'd keep the write config around.

for the cached response an array might suffice

[fabpot]

@wickedOne I'd love to be able to merge this one in 6.3. We're closing 6.3 for new features in about a week from now, will you be able to finish the work here (at least, make sure that all comments are taken into account)?

[wickedOne]

@fabpot that would be great and yes i'm able to allocate some time to it this week.

this probably would exclude #49520 which i willingly put on the backburner until this one got resolved so have to look into it and a week might be a little short for that

however, i think some discussions are still unresolved (and the reason there has been little to no activity):

• i'm fine with moving the cached response to an array, but still unconvinved doing the same for the config classes as i fail to see added benefits, but do see added complexity especially for testing purposes
• i think the biggest unresolved issue is caching for this provider. i do acknowledge the added complexity, nevertheless i think that feature is essential for this provider. so even though the impact might seem minimal and the added complexity might seem unproportional, removing it renders this provider, i'd say not useless, but should come with a warning sign

github comments might not be the most efficient way to get these resolved in a week so feel free propose an alternative; i'm open to it

[wickedOne]

replaced PhraseCachedResponse with an array...

[welcoMattic]

As this class (and the other one for write) is only used in PhraseProvider, I'm not convinced there is any advantage to have it standalone. Could you consider integrate it in PhraseProvider file?

[wickedOne]

my main motivation was not to clutter the provider itself with configuration logic, bit similar to configuring the http client. also probably a bit easier for those who want to alter the config behaviour?

nevertheless, i could move all that logic to the provider class, but that would be quite a bit of a refactor for which i'm not sure i'll have time before the week's end. perhaps on friday which might not be in time...

[welcoMattic]

We don't need to make the Provider fully extensible. The main goal is to allow users to push and pull translations from Phrase, with the same common internal API as other Translation Providers (Loco, Lokalise and Crowdin).

If there is something to be configured by the user for the Provider, it should pass via the DSN.

If I understand correctly the code, WriteConfig is just an array in which you adapt values like the domain or the locale. I think we must simplify it with something like a private property with default config (one for read and one for write if needed) to be tweaked right before each Phrase API call with the right values.

[wickedOne]

true, it's open source so thre's no "need" for anything to be extensible, but i know from experience you'll make a bunch of devs happy when they are 😄

correct, the config classes are not too complex and only the read config contains a bit of logic for the provider (the fallback_locale_enabled one), it's the unsetting of values to basically enforce some values which might not fit into someone's workflow

anyway, i'll have some time today to try and move the logic to the provider so will give it a go. think most time will be spent on re-writing the tests tbh

[wickedOne]

thanks for the review @welcoMattic!
i've applied most of the requested changes, for others please see my comments

[welcoMattic]

@wickedOne We would love to merge this feature for 6.3. The last point to fix is the removal of ReadConfig and WriteConfig classes, and re-integrate those into the Provider class itself. Do you think you will be able to achieve it soon?

[wickedOne]

@wickedOne We would love to merge this feature for 6.3. The last point to fix is the removal of ReadConfig and WriteConfig classes, and re-integrate those into the Provider class itself. Do you think you will be able to achieve it soon?

yes sorry, had some other priorities and wasn't able to get back to this. should be able to give this a go over the weekend and if it turns out to be too much work in regards to time at least to state so

[fabpot]

@welcoMattic Can we merge it as is? Or do you think that your last change suggestion is a blocker?

[welcoMattic]

@fabpot It is not a blocker, as it works, but it not follows the same way to do as all other providers.
I guess we can merge it and refactor the 2 config classes later (I could do it)

[fabpot]

Thank you @wickedOne.

[fabpot]

@welcoMattic If you can take care of the refactoring, I would be happy to merge a PR on this :)

[welcoMattic]

I'm on holidays until next monday, I can do it during the next week

[fabpot]

No worries, we have time ;)