Problem/Motivation
Translation files downloaded by Locale module are stored in the local file system. During the download the translation file is remaned (default behaviour of system_retrieve_file()). A previously downloaded file is not overwritten. After time will will lead to a build-up of old translation files as newer files
from the same version gets downloaded and renamed with a unique name. Only the latest downloaded file will be used, the older versions will never be used again by Locale module.
This will results in files like:
- token-8.x-1.0.nl.po
- token-8.x-1.0.nl_0.po
- token-8.x-1.0.nl_1.po
This behavior was never intended during original development (in which I was heavily involved).
Proposed resolution
Replace existing file during download using FILE_EXISTS_REPLACE.
Remaining tasks
t.b.d.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff-2949825-7-19.txt | 1.41 KB | Sutharsan |
#19 | locale-translation-download-2949825-19.patch | 3.19 KB | Sutharsan |
Comments
Comment #2
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #3
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThe attached test reproduces the bug.
Comment #4
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedThis patch includes the fix and above test.
Comment #7
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedComment #9
Gábor HojtsyYay, looks great. I agree it was not intended that multiple copies of the file would stay around.
Comment #10
idebr CreditAttribution: idebr at ezCompany commentedThe 7.x version of the Localization update module had a feature request to clean up previous versions of the translation file, since only one translation file per project is actually used. This seems like a more structural approach for keeping a single po file per project, since a new release tag would just add a new file and leave the previous one in place.
Comment #11
andypostProbably better to add a setting to configure behavior to make it more inline with
update_manager_file_get()
Comment #12
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented@idebr, I agree that we should also remove translation files from older versions. But I'd rather have that done in a separate issue. This issue adds a test which is a very good basis to add the tests for the scenario you mention.
Comment #13
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedUpdate module has a mechanism to determine if a file is stale before replacing. Locale module does this based on file timestamp. Once it gets to
locale_translation_http_check()
that decision has already been made. Therefore, I see no need for a second mechanism.I did discuss the cleaning up of old po files with @andypost in IRC. I will open a new issue for that, as it may prove to be more complex than I initially thought. This issue is a good step into the right direction, other issue can build on top of it.
Comment #14
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedCreated #2950294: Clean-up translations directory
Comment #15
andypostI see nothing is blocking it
Comment #16
alexpottCan't this test be a kernel test - I see no reason for this to be a BrowserTest and can't see why we're logging in.
Comment #17
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedtldr
A browser test is by far the easiest way to get the test done. The alternative is a significant refactoring.
Details
Log-in is needed because of the way the language gets created in LocaleUpdateBase. This can be avoided by modifying LocaleUpdateBase::addLanguage. However this test also uses other methods from LocaleUpdateBase (::setTranslationFiles; and indirectly ::makePoFile, ::setTranslationsDirectory)
Currently LocaleUpdateBase, is a browser test because all tests that extend LocaleUpdateBase all do need to be browser tests for the actions performed in those test.
Refactoring is needed to change the situation. Move large parts from LocaleUpdateBase to a trait. Use the trait in LocaleUpdateBase and LocaleTranslationDownloadTest. The trait can be used in both kernel and browser tests.
Comment #18
alexpottThis can be replaced with:
At least it saves us from testing in both the sub-site and the runner. With this all the test is in the runner. We should file a followup to refactor bits of \Drupal\Tests\locale\Functional\LocaleUpdateBase into a trait so we can write kernel tests for this as with this change there is no need for a working site anymore.
Comment #19
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedTests modified.
Comment #20
Sutharsan CreditAttribution: Sutharsan at LimoenGroen commentedFollow-up created #2965866: Refactor LocaleUpdateBase to allow locale kernel tests
Comment #21
andypostTests improvement is out of scope
Comment #22
alexpottAdding credit for @andypost and @alexpott for patch review.
Committed and pushed 3e3b6687eb to 8.6.x and ca5e5feab4 to 8.5.x. Thanks!