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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan created an issue. See original summary.

Sutharsan’s picture

Sutharsan’s picture

The attached test reproduces the bug.

Sutharsan’s picture

This patch includes the fix and above test.

Status: Needs review » Needs work

The last submitted patch, 4: locale-translation-download-2949825-4.patch, failed testing. View results

Sutharsan’s picture

Gábor Hojtsy’s picture

Yay, looks great. I agree it was not intended that multiple copies of the file would stay around.

idebr’s picture

The 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.

andypost’s picture

+++ b/core/modules/locale/locale.batch.inc
@@ -292,7 +292,7 @@ function locale_translation_http_check($uri) {
-  if ($uri = system_retrieve_file($source_file->uri, $directory)) {
+  if ($uri = system_retrieve_file($source_file->uri, $directory, FALSE, FILE_EXISTS_REPLACE)) {

Probably better to add a setting to configure behavior to make it more inline with update_manager_file_get()

Sutharsan’s picture

@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.

Sutharsan’s picture

Update 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.

Sutharsan’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

I see nothing is blocking it

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/locale/tests/src/Functional/LocaleTranslationDownloadTest.php
@@ -0,0 +1,72 @@
+    $admin_user = $this->drupalCreateUser([
+      'administer modules',
+      'administer site configuration',
+      'administer languages',
+      'access administration pages',
+      'translate interface',
+    ]);
+    $this->drupalLogin($admin_user);
...
+  public function testUpdateImportSourceRemote() {

Can'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.

Sutharsan’s picture

tldr
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.

alexpott’s picture

+++ b/core/modules/locale/tests/src/Functional/LocaleTranslationDownloadTest.php
@@ -0,0 +1,72 @@
+    $admin_user = $this->drupalCreateUser([
+      'administer modules',
+      'administer site configuration',
+      'administer languages',
+      'access administration pages',
+      'translate interface',
+    ]);
+    $this->drupalLogin($admin_user);
+    $this->addLanguage('de');

This can be replaced with:

    $language = ConfigurableLanguage::create([
      'id' => 'de',
      'label' => LanguageManager::getStandardLanguageList()['de'][1],
    ]);
    $language->save();

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.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
1.41 KB

Tests modified.

Sutharsan’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Tests improvement is out of scope

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Adding credit for @andypost and @alexpott for patch review.

Committed and pushed 3e3b6687eb to 8.6.x and ca5e5feab4 to 8.5.x. Thanks!

  • alexpott committed 3e3b668 on 8.6.x
    Issue #2949825 by Sutharsan, andypost, alexpott: Downloaded translation...

  • alexpott committed ca5e5fe on 8.5.x
    Issue #2949825 by Sutharsan, andypost, alexpott: Downloaded translation...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.