Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Translation] Adding Translation Providers#38475

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@welcoMattic
Copy link
Member

@welcoMatticwelcoMattic commentedOct 7, 2020•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#36543
LicenseMIT
Doc PRsymfony/symfony-docs#15310

Follow up of#37462

This PR refers to the early opened RFC#36543 about adding third party translation SaaS into Symfony.
I worked in collaboration with@odolbeau on this first draft.

We have implemented only Loco Provider for now, to validate the main workflow of the feature.
We are not very sure about some naming convention, such as Remote Storage, it might be renamed in Transport, to correspond to the naming of third party services in Mailer and Notifier Components. We use Provider name.

This PR brings 2 new commands in Translation component:translation:push andtranslation:pull
It adds also new configuration entry:

framework:default_locale:frtranslator:default_path:'%kernel.project_dir%/translations'enabled_locales:'%locales%'fallbacks:            -en            -itproviders:loco:dsn:'%env(LOCO_DSN)%'domains:['messages']locales:'%locales%'

To do

  • Implement Provider into Translation component
  • Plug it into FrameworkBundle
  • Implement pull and push commands
  • Implement Loco adapter
  • Tests
  • Documentation
  • Update CHANGELOG.md files in FrameworkBundle and Translation Component

Docs:

Adapt language settings in Lokalise to be sure that Symfony locales match with Lokalise languages

language-setting-lokalise

Todo:

  • Implement POEditor Provider (:warning: there is a trick to do in POEditor Dashboard in order to make XLF export works, it will have to be documented explicitly in the symfony/symfony-docs PR)
  • Implement Lokalise Provider
  • Implement Crowdin Provider

These 3 providers are implemented separately. They are not fully tested yet, it is planned to make it done by the end of April 2021.

bocharsky-bw, althaus, AlexandrePavy, Kocal, renanbr, damienalexandre, terales, matthieuwerner, Kern046, Nek-, and 6 more reacted with thumbs up emojiloic425 and cristoforocervino reacted with heart emojisstok, rvanlaak, Kern046, michaelzangerle, Nek-, franmomu, loic425, and emr reacted with rocket emoji
@lyrixxlyrixx changed the titleFeature/remote storages[Translation] remote storagesOct 7, 2020
@welcoMatticwelcoMattic changed the title[Translation] remote storages[Translation][FrameworkBundle] Adding Translation ProvidersOct 8, 2020
@nicolas-grekasnicolas-grekas added this to the5.x milestoneOct 12, 2020
@welcoMatticwelcoMatticforce-pushed thefeature/remote-storages branch 11 times, most recently from9fe79c7 to6406e7bCompareDecember 17, 2020 13:49
@welcoMattic
Copy link
MemberAuthor

@fabpot@nicolas-grekas@Nyholm

I've continued this PR this week and I've reached a level where I think a first code review seems necessary.

What has been done:

  • Added 2 new commandstranslation:push andtranslation:pull with the options to choose the locales and domains to be processed, and 2 flags--force and--delete-obsolete.
  • 3 Providers:Loco,PoEditor andLokalise.
  • The internal mechanics for Providers creation via a Factory. (thank you@odolbeau)

What I have a doubt about:

  • The relevance of the--force flag for the push command. It forces the update of remote translations from the local translations. However, in my last conference at SymfonyWorld, I recommended never to update local translations, directly in the XLIFF files, but to always use the SaaS UI and pull to update local files. What do you think about removing the flag and therefore not allowing remote translations to be updated from local files?

  • The number of API calls for Loco: the initial creation of all translation keys will make one API call per key. This can quickly lead to a scaling problem.

  • The division of Providers into Bridges, as it is done for Notifier and Mailer. I simply reproduced what was done, but I'm not sure I did everything correctly.

I'm listening to comments and feedback to move this feature forward, in order to make it available for 5.3.

@welcoMatticwelcoMatticforce-pushed thefeature/remote-storages branch 2 times, most recently from7abc84f to2869a15CompareDecember 18, 2020 14:37
@welcoMatticwelcoMatticforce-pushed thefeature/remote-storages branch 3 times, most recently fromf8121cb tof64d8e5CompareJanuary 21, 2021 11:25
Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Thank you. Great work!

I understand you don't want to addsymfony/http-client as a dependency to the Translator component, but we should not be so generous with optional arguments, ie the Factories and providers.

@carsonbotcarsonbot changed the title[Translation][FrameworkBundle] Adding Translation Providers[Translation] Adding Translation ProvidersFeb 25, 2021
@welcoMattic
Copy link
MemberAuthor

@fabpot@Nyholm@nicolas-grekas All comments have been addressed, the CI failures on AppVeyor are unrelated to this PR. Thank you all for your reviews and your help to fix it.

@welcoMattic
Copy link
MemberAuthor

Thank you@fabpot, I've addressed your last comments

Co-authored-by: Olivier Dolbeau <github@a.bbnt.me>
@welcoMattic
Copy link
MemberAuthor

I've just rebased the branch on 5.x

Nyholm reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@welcoMattic.

welcoMattic, derrabus, matthieuwerner, SebLevDev, ternel, lyrixx, and TijmenWierenga reacted with heart emojiderrabus, SebLevDev, and ternel reacted with rocket emoji

@fabpotfabpot merged commit6ff9c97 intosymfony:5.xApr 21, 2021
@fabpot
Copy link
Member

I will create the subtree split for the Loco provider after having merged the PRs that will add the other providers.

Nyholm, welcoMattic, SebLevDev, and OskarStark reacted with thumbs up emoji

@Nyholm
Copy link
Member

Wohoo. Thank you@welcoMattic,@odolbeau and others for all the work you've put in.

welcoMattic, derrabus, matthieuwerner, SebLevDev, damienalexandre, and OskarStark reacted with heart emoji

@Kocal
Copy link
Member

Thanks@welcoMattic, great job!!

welcoMattic and OskarStark reacted with heart emoji

@welcoMatticwelcoMattic deleted the feature/remote-storages branchApril 21, 2021 15:22
}

$endpoint =sprintf('%s%s','default' ===$dsn->getHost() ?self::HOST :$dsn->getHost(),$dsn->getPort() ?':'.$dsn->getPort() :'');
$client =$this->client->withOptions([
Copy link
Contributor

Choose a reason for hiding this comment

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

In notifier bridges we do this in the transport. Imho the Factory does not need to know the endpoint.

plus we are consistent for mailer notifier and translation afterwards and the mailer and notifier bridges are quite bulletproof

@fabpotfabpot mentioned this pull requestMay 1, 2021
fabpot added a commit that referenced this pull requestMay 9, 2021
…ii-bodnar)This PR was squashed before being merged into the 5.3-dev branch.Discussion----------[Translation] Added Crowdin Translation Provider| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       |  <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License       | MIT| Doc PR        | <!-- required for new features -->To follow up on#38475, this PR adds [Crowdin](https://crowdin.com/) Provider.This provider was removed a few weeks ago from the Translation Providers feature by `@welcoMattic`.We discussed all the recent changes made on `ProviderInterface`, `TranslatorBagInterface`, and others and I already applied these changes to Crowdin Provider.Also, this Provider is adapted to work with both [Crowdin](https://crowdin.com/) and [Crowdin Enterprise](https://crowdin.com/enterprise).The todo list to make it ready is:- [x] Write integration tests by mocking HTTP ResponsesI will make it done before the beginning of May.Commits-------d7fda16 [Translation] Added Crowdin Translation Provider
fabpot added a commit that referenced this pull requestMay 10, 2021
This PR was merged into the 5.3-dev branch.Discussion----------[Translation] Added PoEditor Provider| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        |symfony/symfony-docs#15310To follow up on#38475, this PR adds [PoEditor](https://poeditor.com/) Provider.The todo list to make it ready is:- [x] Apply recent changes that have been made on `ProviderInterface` and `TranslatorBagInterface` (we removed the `all()` and `getDomains()` method from TranslatorBagInterface)- [x] Add PoEditorProvider to `src/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php` file- [x] Add PoEditor case to `Symfony\Component\Translation\Exception\UnsupportedSchemeException`- [x] Write integration tests by mocking HTTP ResponsesThe major part of the remaining work concerns tests, I will make it done before the beginning of May.Commits-------240ac22 Added PoEditor Provider
fabpot added a commit that referenced this pull requestMay 10, 2021
This PR was merged into the 5.3-dev branch.Discussion----------[Translation] Added Lokalise Provider| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        |symfony/symfony-docs#15310To follow up on#38475, this PR adds [Lokalise](https://lokalise.com/) Provider.The todo list to make it ready is:- [x] Apply recent changes that have been made on `ProviderInterface` and `TranslatorBagInterface` (we removed the `all()` and `getDomains()` method from TranslatorBagInterface)- [x] Add LokaliseProvider to `src/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php` file- [x] Add Lokalise case to `Symfony\Component\Translation\Exception\UnsupportedSchemeException`- [x] Move `LokaliseProvider` and `LokaliseProviderFactory` from `Symfony\Component\Translation\Bridge\Lokalise\Provider` to `Symfony\Component\Translation\Bridge\Lokalise` namespace- [x] Write integration tests by mocking HTTP ResponsesThe major part of the remaining work concerns tests, I will make it done before the beginning of May.Commits-------022d828 Added Lokalise Provider
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestMay 19, 2021
This PR was submitted for the 5.4 branch but it was squashed and merged into the 5.3 branch instead.Discussion----------[Translation] Introduce Translation ProvidersDocs forsymfony/symfony#38475,symfony/symfony#40926,symfony/symfony#40927, andsymfony/symfony#40947Ready for first review, but I'm not sure that I've written documentation in the right and all required places.ATM, Translation Providers Bridges packages doesn't exists, so Flex recipes are not created yet.Commits-------943a63f [Translation] Introduce Translation Providers
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@NyholmNyholmNyholm approved these changes

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@odolbeauodolbeauodolbeau left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.3

Development

Successfully merging this pull request may close these issues.

[RFC] [Translation] Remote Storages

10 participants

@welcoMattic@Nyholm@nicolas-grekas@fabpot@Kocal@stof@odolbeau@OskarStark@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp