[Translation][Lokalise] fix "Project too big for sync export" #62348
Conversation
|
Hey! Thanks for your PR. You are targeting branch "7.4" but it seems your PR description refers to branch "6.4". Cheers! Carsonbot |
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
3 times, most recently
from
439bb53 to
fee6f0b
Compare
|
Status: needs work Its necessary add new tests for the new request. I tested this solution with a test account and a very simple example. I will wait for @bastien70's confirmation that it works in his environment before add the new tests. |
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
7 times, most recently
from
e984324 to
8ba777f
Compare
|
Hello, it works! Congratulations et thank you! :) |
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
from
8ba777f to
414d6da
Compare
|
Status: needs review Bastien has already checked this solution in his environment (Thanks Bastien for the check!), and the test has been added. Edit: I believe the errors are not relevant in the context of the PR |
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
2 times, most recently
from
604182b to
4f27075
Compare
src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Translation/Bridge/Lokalise/LokaliseProvider.php
Outdated
Show resolved
Hide resolved
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
from
4f27075 to
de16442
Compare
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
2 times, most recently
from
f69efc0 to
6b09c0a
Compare
| { | ||
| private const LOKALISE_GET_KEYS_LIMIT = 5000; | ||
| private const PROJECT_TOO_BIG_STATUS_CODE = 413; | ||
| /** @var list<string> */ |
| } | ||
|
|
||
| if (200 !== $response->getStatusCode()) { | ||
| if (($errorCode = $responseContent['error']['code'] ?? null) && self::PROJECT_TOO_BIG_STATUS_CODE === $errorCode) { |
There was a problem hiding this comment.
| if (($errorCode = $responseContent['error']['code'] ?? null) && self::PROJECT_TOO_BIG_STATUS_CODE === $errorCode) { | |
| if (self::PROJECT_TOO_BIG_STATUS_CODE !== ($responseContent['error']['code'] ?? null)) { | |
| throw ... |
| break; | ||
| } | ||
| ++$attempt; | ||
| usleep(500000 * $attempt); |
There was a problem hiding this comment.
Is this retry strategy recommended anywhere? Why not just hit every 500ms without increasing the delay?
| usleep(500000 * $attempt); | ||
| } | ||
|
|
||
| $response = $this->client->request('GET', $downloadUrl, ['buffer' => true]); |
There was a problem hiding this comment.
no need to buffer since we write by chunk:
| $response = $this->client->request('GET', $downloadUrl, ['buffer' => true]); | |
| $response = $this->client->request('GET', $downloadUrl, ['buffer' => false]); |
(the alternative would be to pass a file handler instead of a boolean, then wait for the last chunk below, but it's also fine as you wrote it)
| if (200 !== $response->getStatusCode()) { | ||
| throw new ProviderException(\sprintf('Unable to download translations file from Lokalise: "%s".', $response->getContent(false)), $response); | ||
| } | ||
| $newfile = \sprintf('%s%s%s.zip', sys_get_temp_dir(), \DIRECTORY_SEPARATOR, uniqid()); |
There was a problem hiding this comment.
| $newfile = \sprintf('%s%s%s.zip', sys_get_temp_dir(), \DIRECTORY_SEPARATOR, uniqid()); | |
| $zipFile = tempnam(sys_get_temp_dir(), 'lokalise'); | |
| $extractPath = $zipFile.'.dir'; |
| fclose($fileHandler); | ||
|
|
||
| $zip = new \ZipArchive(); | ||
| if (!$zip->open($newfile, \ZipArchive::CREATE)) { |
There was a problem hiding this comment.
| if (!$zip->open($newfile, \ZipArchive::CREATE)) { | |
| if (!$zip->open($zipFile)) { |
|
|
||
| $zip = new \ZipArchive(); | ||
| if (!$zip->open($newfile, \ZipArchive::CREATE)) { | ||
| throw new LogicException(\sprintf('failed to open zip file "%s".', $newfile)); |
There was a problem hiding this comment.
same below:
| throw new LogicException(\sprintf('failed to open zip file "%s".', $newfile)); | |
| throw new LogicException(\sprintf('Failed to open zip file "%s".', $zipFile)); |
| if (\in_array($filename, ['.', '..'])) { | ||
| continue; | ||
| } | ||
| $path = \sprintf('%s%s%s', $dir, \DIRECTORY_SEPARATOR, $filename); |
There was a problem hiding this comment.
| $path = \sprintf('%s%s%s', $dir, \DIRECTORY_SEPARATOR, $filename); | |
| $path = $dir.'/'.$filename; |
| { | ||
| $fileContents = []; | ||
| foreach (scandir($dir) as $filename) { | ||
| if (\in_array($filename, ['.', '..'])) { |
There was a problem hiding this comment.
| if (\in_array($filename, ['.', '..'])) { | |
| if (\in_array($filename, ['.', '..'], true)) { |
| } | ||
| $path = \sprintf('%s%s%s', $dir, \DIRECTORY_SEPARATOR, $filename); | ||
| if (is_dir($path)) { | ||
| $fileContents = array_merge($fileContents, $this->getFileContents($path, $filename)); |
There was a problem hiding this comment.
Let's change the implementation of the method and let's not pretend this can handle arbitrary recursive unzipped content
we can do a two levels read of the dir: level 1 = languages, str_replace included, level 2 = content
santysisi
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
from
6b09c0a to
565c8c2
Compare
nicolas-grekas
force-pushed
the
fix/translation-lokalize-project-to-big-for-sync-export
branch
from
565c8c2 to
a79695c
Compare
|
Thank you @santysisi. |
| $downloadUrl = $process['details']['download_url']; | ||
| break; | ||
| } | ||
| usleep(500000); |
There was a problem hiding this comment.
Shouldn’t we have some kind of timeout in case the API never responds with any of the codes we use to detect success and failure?
5 participants
This fix follows this recommendation
Since this fix adds a new request for a different path, I'm not sure whether to use branch 6.4 or perhaps 8.1. Please let me know.
This fix requires the
zipextension because the returned file is a ZIP archive, and this extension is needed to extract the contents. This fix does not add that extension as a dependency; instead, it only usesextension_loadedto check if it is loaded in the project.