Apply recent changes that have been made onProviderInterface andTranslatorBagInterface (we removed theall() andgetDomains() method from TranslatorBagInterface)
Add LokaliseProvider tosrc/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php file
Add Lokalise case toSymfony\Component\Translation\Exception\UnsupportedSchemeException
MoveLokaliseProvider andLokaliseProviderFactory fromSymfony\Component\Translation\Bridge\Lokalise\Provider toSymfony\Component\Translation\Bridge\Lokalise namespace
Write integration tests by mocking HTTP Responses

The major part of the remaining work concerns tests, I will make it done before the beginning of May.

carsonbot added Status: Needs Review Translation labels

Apr 23, 2021

Copy link

carsonbot commentedApr 23, 2021

Please note that you need squash your commits before this PR can be merged. The maintainer can also squash the commits for you, but then you need to “Allow edits from maintainer” (there is a checkbox in the sidebar of the PR).

Cheers!

Carsonbot

Nyholm added this to the5.3 milestone

Apr 23, 2021

Nyholm mentioned this pull request

Apr 23, 2021

[CI] Sort packages by length to match modified package better#40928

Merged

OskarStark reviewed

Apr 24, 2021

View reviewed changes

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

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

Do you need to adjust UnsupoortedSchemeException like we do for the Notifier?

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved

OskarStark added a commit that referenced this pull request

Apr 27, 2021

minor#40928[CI] Sort packages by length to match modified package b…

63496df

…etter (Nyholm)This PR was merged into the 5.3-dev branch.Discussion----------[CI] Sort packages by length to match modified package better| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        |Given the build error in#40927, I saw that we match "modified packages" wrong. The script things we modified `symfony/translation` rather than the new bridge. This is because we are using a simple [string matchning](https://github.com/symfony/symfony/blob/18658a29a37d6de3e73c95f96fb9e51bf1d5f59d/.github/get-modified-packages.php#L24). If we sort the packages by length, we make sure we match the most detailed (longest) string first.Commits-------f7a0bd1 [CI] Sort packages by length to match modified package better

welcoMattic force-pushed thefeature/lokalise-provider branch 2 times, most recently fromc41ea84 to749cca8Compare

April 27, 2021 16:23

OskarStark reviewed

Apr 28, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved

OskarStark reviewed

Apr 28, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

welcoMattic commentedApr 28, 2021

Thanks@OskarStark for the quick review, I have not finish the revamp of Lokalise accordingly to the recent changes 😉

Copy link

MemberAuthor

welcoMattic commentedMay 5, 2021•
edited
Loading

status: needs review

CI failures seems unrelated to this PR.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

May 5, 2021

welcoMattic force-pushed thefeature/lokalise-provider branch 3 times, most recently from906af69 tod7a0ad9Compare

May 5, 2021 13:08

carsonbot added Status: Needs Review and removed Status: Needs Work labels

May 5, 2021

welcoMattic force-pushed thefeature/lokalise-provider branch 2 times, most recently fromd434655 to2399fc7Compare

May 5, 2021 13:18

welcoMattic mentioned this pull request

May 5, 2021

[Translation] Introduce Translation Providerssymfony/symfony-docs#15310

Merged

welcoMattic force-pushed thefeature/lokalise-provider branch from2399fc7 to608685fCompare

May 6, 2021 20:39

nicolas-grekas approved these changes

May 7, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(with easy to fix comments)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.phpShow resolvedHide resolved

src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php OutdatedShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

May 7, 2021

welcoMattic force-pushed thefeature/lokalise-provider branch 3 times, most recently from4fef59d to31df459Compare

May 7, 2021 16:35

nicolas-grekas added the Ready label

May 9, 2021

Copy link

Member

fabpot commentedMay 9, 2021

Tests seem broken.

welcoMattic force-pushed thefeature/lokalise-provider branch from15b6ba2 to6a5d222Compare

May 9, 2021 16:02

Copy link

MemberAuthor

welcoMattic commentedMay 9, 2021

@fabpot I've just pushed a fix (adding symfony/config as dev dependency of the Lokalise Provider Bridge). It fixes tests on Travis. On AppVeyor, failures seems unrelated

fabpot approved these changes

May 10, 2021

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

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

LGTM, to be merged after the HOST constant is fixed.

src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProviderFactory.php OutdatedShow resolvedHide resolved

welcoMattic force-pushed thefeature/lokalise-provider branch 2 times, most recently fromebd9708 tofe0685aCompare

May 10, 2021 09:58

Copy link

MemberAuthor

welcoMattic commentedMay 10, 2021•
edited
Loading

@fabpot Last comment has been addressed, branch has been rebased, it looks ready 👍

fabpot reviewed

May 10, 2021

View reviewed changes

src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProviderFactory.php OutdatedShow resolvedHide resolved

welcoMattic force-pushed thefeature/lokalise-provider branch 2 times, most recently from631b635 to82179a6Compare

May 10, 2021 10:05

fabpot reviewed

May 10, 2021

View reviewed changes

src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProviderFactory.php OutdatedShow resolvedHide resolved

welcoMattic force-pushed thefeature/lokalise-provider branch 2 times, most recently from3b068e8 to89f9e1dCompare

May 10, 2021 10:07

fabpot approved these changes

May 10, 2021

View reviewed changes

Added Lokalise Provider

022d828

welcoMattic force-pushed thefeature/lokalise-provider branch from89f9e1d to022d828Compare

May 10, 2021 10:15

Copy link

Member

fabpot commentedMay 10, 2021

Thank you@welcoMattic.

fabpot merged commit0b66008 intosymfony:5.x

May 10, 2021

Copy link

Member

stof commentedMay 10, 2021

@welcoMattic actually, the symfony/config component is necessary when using the xliff loader (which is why it is only a dev dependency in symfony/translation as you are not forced to use xliff). As the Lokalise provider relies on loading xliff, it should have a non-dev requirement on symfony/config

Copy link

Member

stof commentedMay 10, 2021

@welcoMattic does this provider handle properly the fact that Symfony translation keys are only unique inside a domain, not globally ?

welcoMattic deleted the feature/lokalise-provider branch

May 10, 2021 11:51

Copy link

MemberAuthor

welcoMattic commentedMay 10, 2021

@stof indeed, actually all Translation Providers useXliffFileLoader for loading pulled translations, so I guess I should set symfony/config as hard dependency in Loco, Lokalise, PoEditor and Crowdin Providers?

About handling translation keys by domain and not glabally, I paid attention to handle it in all Providers after your first comment about it.

Copy link

Member

stof commentedMay 10, 2021

so I guess I should set symfony/config as hard dependency in Loco, Lokalise, PoEditor and Crowdin Providers?

yes, indeed.

welcoMattic mentioned this pull request

May 10, 2021

[Translation] Improved Translation Providers#41155

Merged

Copy link

MemberAuthor

welcoMattic commentedMay 10, 2021

@stof fixed here#41155

fabpot added a commit that referenced this pull request

May 10, 2021

bug#41155[Translation] Improved Translation Providers (welcoMattic)

90ab9ed

This PR was merged into the 5.3-dev branch.Discussion----------[Translation] Improved Translation Providers| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        |Some small improvements on Translation Providers:- move symfony/config from dev dependency to hard dependency for all Bridges as all providers use XliffFileLoader to load pulled translations (cf.#40927 (comment))- replace all instances of```'headers' => [    'Authorization' => 'Bearer API_TOKEN',]```with `'auth_bearer' => 'API_TOKEN',` even in tests files.- Fix Lokalise base_uri concatenationCommits-------84fd13c Improved Translation Providers

fabpot mentioned this pull request

May 12, 2021

Release v5.3.0-BETA4#41196

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

May 19, 2021

minor#15310[Translation] Introduce Translation Providers (welcoMattic)

fb1fa0d

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