NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Translation] Make http requests synchronous when reading the Loco API#44416

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:5.3fromKocal:fix/translation-loco-read-sync-http-requests

Dec 6, 2021

Merged

[Translation] Make http requests synchronous when reading the Loco API#44416

fabpot merged 1 commit intosymfony:5.3fromKocal:fix/translation-loco-read-sync-http-requests

Dec 6, 2021

Conversation

Copy link

Member

Kocal commentedDec 2, 2021•
edited
Loading

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

Hi!

Since we migrated from thePHP Translation's Loco Adapter to theSymfony's Loco Translation Provider, we started to have 429 errors from the Loco API when running our CI or deploying our code on review apps/staging/production environments:

Some screenshots

We have asked to the Loco support and this is their response:

Hello.
You will never get a 429 if you make single threaded requests. i.e. waiting for a response before dispatching another requests. As noted in the API usage limits: parallel requests burst the response speed limit. This is currently the only thing that would invoke a 429. I can provide more detail on the implementation if you would like.
This measure is in lieu of quota based rate limiting which will one day be added, but currently no other limits are in place in terms of number of requests.

Which makes sense. AFAIK, the PHP Translation's Loco Adapter makes synchronous requests to get translations from Loco.

Some Blackfire traces, it does not impact performances:

I consider this PR as a bug-fix and not a feature, that's why I've targeted 5.3.

WDYT?

ping@welcoMattic

carsonbot added Status: Needs Review Bug Translation labels

Dec 2, 2021

carsonbot added this to the5.3 milestone

Dec 2, 2021

carsonbot changed the title~~[Translation][Loco] Make http requests synchronous when reading the Loco API~~[Translation] [Loco] Make http requests synchronous when reading the Loco API

Dec 2, 2021

Copy link

MemberAuthor

Kocal commentedDec 2, 2021

Checks failures are unrelated I think.

fabpot requested review fromnicolas-grekas andwelcoMattic

December 2, 2021 16:31

nicolas-grekas reviewed

Dec 2, 2021

View reviewed changes

src/Symfony/Component/Translation/Bridge/Loco/LocoProvider.php OutdatedShow resolvedHide resolved

welcoMattic approved these changes

Dec 2, 2021

View reviewed changes

Copy link

Member

welcoMattic left a comment

Choose a reason for hiding this comment

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

It is legitimate, as you profiled it and there is no big difference I'm ok to merge it. Thank you@Kocal

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

Dec 2, 2021

Copy link

Member

nicolas-grekas commentedDec 2, 2021•
edited
Loading

BTW, is anyone in touch with Loco, because so far they didn't answer to our ping to back the bridge.

(Crowdin and Lokalise did, so you know which bridge you'd better use ;) )

Copy link

MemberAuthor

Kocal commentedDec 2, 2021

The two foreach have been merged!

nicolas-grekas approved these changes

Dec 2, 2021

View reviewed changes

nicolas-grekas changed the title~~[Translation] [Loco] Make http requests synchronous when reading the Loco API~~[Translation] Make http requests synchronous when reading the Loco API

Dec 2, 2021

Copy link

Member

stof commentedDec 2, 2021

I suggest adding a comment in the code mentioning that the Loco API forbids concurrent requests, to explain why this provider does not take advantage of the request concurrency allowed by http-client (so that it does not get refactored in the opposite way by someone not knowing about this API limitation).