Problem

Drupal 8 had a centralized .po file directory to prepare for l10n_update feature inclusion in core. This centralized directory can have any number of files, and when a module is added or removed and when Drupal is installed, all files are imported that are for the languages enabled on the site. That is not very effective, and we should keep track of what did we import, and if anything changed and would need to be reimported again or we should leave the file alone.

Proposed solution

- Introduce a table to track file information (name, langcode, date)
- Fill in this table based on the files in the filesystem at first (when importing translations for example)
- On further automated imports (such as when a module is enabled, or a drush command is run), update the table with new files, and only import the needed files which are changed or new and are needed for the import (ie. in the languages configured on the site)

Parent issue

#1191488: META: Integrate l10n_update functionality in core

CommentFileSizeAuthor
#77 1635084-followup.patch951 bytesxjm
#65 track-imports-1635084-65.patch3.42 KBpenyaskito
#62 track-imports-1635084-62.patch3.42 KBpenyaskito
#62 track-imports-1635084-58-interdiff.txt3.08 KBpenyaskito
#59 interdiff-57-58.txt848 bytespenyaskito
#58 track-imports-1635084-57-interdiff.txt1.15 KBpenyaskito
#58 track-imports-1635084-58.patch5.49 KBpenyaskito
#57 track-imports-1635084-57.patch5.47 KBBerdir
#57 track-imports-1635084-57-interdiff.txt1.15 KBBerdir
#52 track-import-po-files-1635084-42.patch5.41 KBpenyaskito
#42 track-import-po-files-1635084-32.patch3.22 KBwebflo
#40 track-import-po-files-1635084-31-regression-test.patch1.63 KBwebflo
#40 track-import-po-files-1635084-31.patch2.46 KBwebflo
#36 track-import-po-files-1635084-30.patch2.46 KBwebflo
#33 track-import-po-files-1635084-29.patch15.62 KBwebflo
#33 track-import-po-files-1635084-29.interdiff.txt2.46 KBwebflo
#30 track-import-po-files-1635084-28.patch15.58 KBwebflo
#30 track-import-po-files-1635084-28.interdiff.txt547 byteswebflo
#29 move-date-format-translation-functions-from-locale-to-system-1635134-15.patch29.37 KBwebflo
#29 move-date-format-translation-functions-from-locale-to-system-1635134-15.interdiff.txt591 byteswebflo
#27 interdiff_23_27.txt2.7 KBvasi1186
#27 track-import-po-files-1635084-27.patch15.59 KBvasi1186
#23 track-import-po-files-1635084-23.patch15.6 KBvasi1186
#19 track-import-po-files-1635084-7.patch15.56 KBwebflo
#19 track-import-po-files-1635084-7.interdiff.txt936 byteswebflo
#18 track-import-po-files-1635084-6.patch16.29 KBwebflo
#16 track-import-po-files-1635084-5.patch16.29 KBwebflo
#11 track-import-po-files-1635084-4.patch13.04 KBwebflo
#9 track-import-po-files-1635084-3.patch13.01 KBwebflo
#5 track-import-po-files-1635084-2.patch13.05 KBwebflo
#2 track-status-of-po-files-1635084-1.patch5.95 KBwebflo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Title: Maintain a list of already imported po files. » Track import status of files in the locale .po file directory
Issue tags: +language-ui

Better title possibly.

webflo’s picture

Status: Active » Needs work
FileSize
5.95 KB
Gábor Hojtsy’s picture

Issue tags: +Needs tests
+++ b/core/modules/locale/locale.bulk.incundefined
@@ -234,7 +234,7 @@ function locale_translate_add_language_set_batch($langcode) {
  */
-function locale_translate_batch_import_files($langcode = NULL, $finish_feedback = FALSE) {
+function locale_translate_batch_import_files($langcode = NULL, $finish_feedback = FALSE, $force = FALSE) {
   $directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');

Document $force in phpdoc.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -248,6 +248,19 @@ function locale_translate_batch_import_files($langcode = NULL, $finish_feedback
+        // The file is already imported. Remove it from file list and don't import it again.

Line too long.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -297,8 +310,12 @@ function locale_translate_batch_import($filepath, &$context) {
+    $success = _locale_import_read_po('db-store', $file, array(), $langcode[2]);
+    if ($success == NULL) {
+      $file->langcode = $langcode[2];
+      locale_translate_update_file_history($file);
+    }

This is bad but is being worked on elsewhere. Document that NULL is success. Ha.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +328,41 @@ function locale_translate_batch_finished($success, $results) {
+ * @param $filepath
+ *   An filepath of a file to import.

*The* filepath.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +328,41 @@ function locale_translate_batch_finished($success, $results) {
+function locale_translate_file_entity_create($filepath) {
+  $file = entity_create('file', array('filename' => drupal_basename($filepath), 'uri' => $filepath));
+  $file->timestamp = filemtime($file->uri);
+  return $file;
+}

It is strange we are using an entity for this. I understand it is not saved as a file entity, but it looks strange. *However* this is pre-existent to this patch *and* is not in the scope for the patch.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +328,41 @@ function locale_translate_batch_finished($success, $results) {
+  // Update or write new record

. missing at the end.

+++ b/core/modules/locale/locale.installundefined
@@ -127,6 +127,45 @@ function locale_schema() {
+    'description' => 'File and download information for project translations.',

No download information here, right? Not even "project" tranlations. Its just .po files.

"File import status information for interface translation files"

something like that maybe.

+++ b/core/modules/locale/locale.installundefined
@@ -127,6 +127,45 @@ function locale_schema() {
+      'filename' => array(
+        'description' => 'Link to translation file for download.',
+        'type' => 'varchar',

?

+++ b/core/modules/locale/locale.installundefined
@@ -127,6 +127,45 @@ function locale_schema() {
+      'timestamp' => array(
+        'description' => 'Unix timestamp of the time the file was downloaded or saved to disk. Zero if not yet downloaded',

You are using this to store the timestamp of the file itself from the point when it was last imported. Comment not consistent with usage.

+++ b/core/modules/locale/locale.installundefined
@@ -127,6 +127,45 @@ function locale_schema() {
+        'description' => 'Status flag. TBD',

TBD :)

+++ b/core/modules/locale/locale.installundefined
@@ -552,6 +591,14 @@ function locale_update_8009() {
 /**
+ * Add {locale_file} table.
+ */
+function locale_update_8010() {
+  $schema['locale_file'] = drupal_get_schema_unprocessed('locale', 'locale_file');
+  drupal_install_schema($schema);
+}
+

Update functions should not use API calls. Copy the whole schema definition all over again.

Also needs tests.

webflo’s picture

Assigned: Unassigned » webflo
webflo’s picture

Status: Needs work » Needs review
FileSize
13.05 KB

Fixed the docs and added tests.

Status: Needs review » Needs work
Issue tags: -Needs tests, -D8MI, -sprint, -language-ui

The last submitted patch, track-import-po-files-1635084-2.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +D8MI, +sprint, +language-ui

The last submitted patch, track-import-po-files-1635084-2.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
13.01 KB

Fixed locale_update_8010().

Status: Needs review » Needs work

The last submitted patch, track-import-po-files-1635084-3.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
13.04 KB
Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleBulkImportFunctionalTest.phpundefined
@@ -0,0 +1,115 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\locale\Tests\LocaleBulkImportFunctionalTest.
+ */
+
+namespace Drupal\locale\Tests;
+
+use Drupal\simpletest\WebTestBase;
+
+/**
+ * Functional tests for the import of translation files.
+ */
+class LocaleBulkImportFunctionalTest extends WebTestBase {

Wrong name for class if extending webtestbase.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleBulkImportFunctionalTest.phpundefined
@@ -0,0 +1,115 @@
+  public static function getInfo() {
+    return array(
+      'name' => 'Translation bulk import',
+      'description' => 'Tests the bulk import of locale files.',
+      'group' => 'Locale',
+    );

Woot for having bulk import tests now :) We did not have before. ++

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleBulkImportFunctionalTest.phpundefined
@@ -0,0 +1,115 @@
+    // Add language.
+    $edit = array('predefined_langcode' => $langcode);
+    $this->drupalPost('admin/config/regional/language/add', $edit, t('Add language'));
+
+    $count = db_query('SELECT COUNT(*) FROM {locale_file} WHERE langcode = :langcode', array(':langcode' => $langcode))->fetchField();
+    $this->assertEqual(1, $count, t('%count file registered in {locale_file}.', array('%count' => $count)));
+
+    $result = db_query('SELECT langcode, uri FROM {locale_file}')->fetchAssoc();
+    $this->assertEqual($result['uri'], $testfile_uri, t('%uri is in {locale_file}.', array('%uri' => $result['uri'])));
+
+    $this->assertEqual($result['langcode'], $langcode, t('Langcode is %langcode.', array('%langcode' => $langcode)));

I know you don't like UI testing as much as unit testing, but since this does the import as well, we'd ideally check the locale table for translations imported as well.

Same applies to other places after the file is imported, so maybe make it a reusable method.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleBulkImportFunctionalTest.phpundefined
@@ -0,0 +1,115 @@
+  /**
+    * Don't update a pre-existing file.
+    */
+  function testBulkImportNotUpdateExisting() {

Whitespace bug with the comment leading whitespace.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -228,13 +228,15 @@ function locale_translate_add_language_set_batch($langcode) {
+ * @param $force
+ *   (optional) Import all available files.

... available files, even if they were imported before.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -248,6 +250,20 @@ function locale_translate_batch_import_files($langcode = NULL, $finish_feedback
+        // The file is already imported. Remove it from file list and don't
+        // import it again.

// The file is already imported and it did not change since the import. ...

svendecabooter’s picture

Status: Needs work » Needs review

Patch tested and seems to work as advertised.
One problem I noticed though is when a language gets deleted and later on enabled again, the .po files get no longer imported.
The entries in locales_file probably need to be deleted when a language gets deleted?

svendecabooter’s picture

Setting back to Gabor's status :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Yes, let's delete the file, since we also drop all the strings from the DB when a language is deleted.

webflo’s picture

Status: Needs work » Needs review
FileSize
16.29 KB

Renamed and refactored test coded. Add function to delete po files after a language is deleted.

Status: Needs review » Needs work

The last submitted patch, track-import-po-files-1635084-5.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
16.29 KB
+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,180 @@
+    drupal_static_reset('langauge_list');

Fixed typo in langauge_list.

webflo’s picture

Status: Needs review » Needs work
FileSize
936 bytes
15.56 KB
+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +345,64 @@ function locale_translate_batch_finished($success, $results) {
+      $success = file_unmanaged_delete($files->uri);

Needs to be $file->uri.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +345,64 @@ function locale_translate_batch_finished($success, $results) {
+  // Remove registered translation files.
+  db_delete('locale_file')
+    ->condition('langcode', $langcode)
+    ->execute();

Its more accurate to make one query per file.

webflo’s picture

Status: Needs work » Needs review
Schnitzel’s picture

Status: Needs review » Needs work

Reviewed the patch, looks good for me, just some small tiny things

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,180 @@
+
+    // Fill the {locale_file} table with a newer file.
+    $file = $this->mockImportedPoFile($langcode, 1);

took me some time to find out that we are actually fake the timestamp in the DB instead of changing the MTime of the File (what normally happens). I would keep it that way and maybe just make a more extended Comment.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -246,12 +247,41 @@ function locale_translate_batch_import_files($langcode = NULL, $finish_feedback
+    foreach ($result as $uri => $info) {
+      if (isset($files[$uri]) && filemtime($uri) <= $info->timestamp) {
+        // The file is already imported and it did not change since the import.
+        // Remove it from file list and don't it again.
+        unset($files[$uri]);

"don't it again."
what? :)

vasi1186’s picture

Found some very small, minor, issues:

+    $this->assertEqual(1, $count, t('%count file registered in {locale_file}.', array('%count' => $count)));

Does it make sense here to use format_plural() instead of t()?

+        // Remove it from file list and don't it again.

I think there is something wrong with this sentence... Should it be "Remove it from file list and don't import it again."?

+ * Get a array of available interface translation file.
...
+ *    A array of interface translation files.

I think it should be "an array".

Besides these, I think the code looks pretty good after a first review.

vasi1186’s picture

Attached a patch with these minor changes.

@Schnitzel: regarding the timestamp for the mockup files, the method documentation is like this:

 * @param int $timestamp_difference
   *  (optional) Timestamp offset, used to mock older or newer files.
   *
   * @return \Drupal\entity\EntityInterface
   */
  function mockImportedPoFile($langcode, $timestamp_difference = 0) {

Do you think we should add more explanations here or where the method is called?

vasi1186’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +345,65 @@ function locale_translate_batch_finished($success, $results) {
+  $file = entity_create('file', array('filename' => drupal_basename($filepath), 'uri' => $filepath));

This is IMHO wrong.

Either you base yourself on file entities and only save the fid in your custom table, or you don't but then you shouldn't create a fake entity with entity_create() either.

If you don't, just do a new stdClass() and assign the properties.

Unless it is necessary to pass it to the locale functions, but I think we didn't add type hints there?

Berdir’s picture

Status: Needs work » Needs review
vasi1186’s picture

Attached a patch and an interdiff.

I also rename the function locale_translate_file_entity_create() to locale_translate_file_create().

webflo’s picture

Status: Needs review » Needs work

Just a minor. We don't document the type for stdClass. I looked at file_load().

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +345,67 @@ function locale_translate_batch_finished($success, $results) {
+ * @return stdClass
+ *   A file object of type stdClass.

@return
An object representing the file.

webflo’s picture

webflo’s picture

Sorry. This was the wrong patch.

attiks’s picture

Status: Needs review » Needs work
+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,181 @@
+  function addLanguage($langcode) {
+    // Add language.

duplicate, no need for the comment.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,181 @@
+  /**
+   * Get translations for a array of strings.
+   *

... for an array ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,181 @@
+   *   A array of strings to translate.

An array ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,181 @@
+    // Fill the {locale_file} table with a older file.

... an older file.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -311,3 +345,67 @@ function locale_translate_batch_finished($success, $results) {
+    foreach ($files as $file) {
+      $success = file_unmanaged_delete($file->uri);
+      if (!$success) {
+        return FALSE;
+      }
+      // Remove the registered translation file if any.
+      db_delete('locale_file')
+        ->condition('langcode', $langcode)
+        ->coddition('uri', $file->uri)
+        ->execute();

So if the delete of the first file fails, all other files aren't deleted either?

It's maybe better to track the status and delete the files than can be deleted.

webchick’s picture

Not applying. :(

webflo’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
15.62 KB

Addressed the comments from attiks.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Looking good for me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, track-import-po-files-1635084-29.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

track-import-po-files-1635084-28.patch is already committed. track-import-po-files-1635084-28.interdiff.txt as patch.

webflo’s picture

Status: Needs review » Reviewed & tested by the community

No failing tests :)

Gábor Hojtsy’s picture

Agreed, reviewed these changes in person, should be good to go.

attiks’s picture

Status: Reviewed & tested by the community » Needs work

Thanks to @penyaskito

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -393,18 +393,21 @@ function locale_translate_update_file_history($file) {
+        db_delete('locale_file')
+          ->condition('langcode', $langcode)
+          ->coddition('uri', $file->uri)

coddition --> condition

We probably need a test for this ;p

New issue created #1642966: Typo: coddition instead of condition

webflo’s picture

Added a test for locale_translate_delete_translation_files(). track-import-po-files-1635084-31-regression-test.patch should fail.

attiks’s picture

Status: Needs review » Needs work

can you reroll with the test in the normal patch as well?

webflo’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

Sure. Sorry i missed that.

Status: Needs review » Needs work

The last submitted patch, track-import-po-files-1635084-32.patch, failed testing.

Gábor Hojtsy’s picture

Does not work :(

attiks’s picture

Maybe we can re-order the test as follows, i think it makes it easier to read?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.php
@@ -178,4 +177,19 @@ class LocaleFileImportStatus extends WebTestBase {
+  function testDeleteLanguage() {
+    $dir = conf_path() . '/files/translations';
+    variable_set('locale_translate_file_directory', $dir);
+    $file_uri = tempnam($dir, "po_") . '.' . $langcode . '.po';
+    file_put_contents($file_uri, $this->randomString());
+    // make sure the file exists
+    $this->assertTrue($file_uri);
+    $langcode = 'de';
+    $this->addLanguage($langcode);
+    language_delete($langcode);
+    // make sure the file is deleted
+    $this->assertFalse(is_file($file_uri));
+  }

10 days to next Drupal core point release.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -D8MI, -sprint, -language-ui

Status: Needs review » Needs work
Issue tags: +Needs tests, +D8MI, +sprint, +language-ui

The last submitted patch, track-import-po-files-1635084-32.patch, failed testing.

penyaskito’s picture

Pending:
* Add asserts to first check if the file was created.
* Use public:// instead of creating the path by ourselves.
* Test again and see green :-)

Gábor Hojtsy’s picture

Discussed this more with @rfay to figure out file test failures. Ideally we should use the setting for the directory, instead of just assuming the setting as-is by default, see http://api.drupal.org/api/drupal/core%21modules%21locale%21locale.bulk.i.... So not sure public:// will apply here, since we are handling a directory that might be entirely out of the public directory based on settings. I know the tests can assume some things, but generally, the directory can be a source controlled dir somewhere not inside public or private directories.

penyaskito’s picture

As discussed on IRC between @rfay and @gaborhojtsy, we could use a StreamWrapper for this, ending in something like "translations://".

For a reference, see http://api.drupal.org/api/examples/file_example%21file_example.module/fu... and http://api.drupal.org/api/examples/file_example%21file_example_session_s....

This could make our lifes easier and our tests pass.

Gábor Hojtsy’s picture

I'm not 100% positive it will make our tests pass :) Not sure we want to do that refactoring here, while in fact we are attempting to fix a bug with the deletion code that was committed. If this little refactoring gets us there that would be helpful though, since we should likely do it either way.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.41 KB

Added assert checking that the file was created.

Created Drupal\locale\TranslationsStream (but not used in tests).

Tests are passing locally, let's see the testbot.

Status: Needs review » Needs work

The last submitted patch, track-import-po-files-1635084-42.patch, failed testing.

attiks’s picture

+++ b/core/modules/locale/lib/Drupal/locale/TranslationsStream.phpundefined
@@ -0,0 +1,37 @@
+    return variable_get('locale_translate_file_directory', ¶
+      conf_path() . '/files/translations');

whitespace

penyaskito’s picture

I asked the testbot team for running tests with --verbose for debugging what is going on on testbot.

Watch #1655158: Patch failing in qa.d.org, but not locally

Berdir’s picture

Testbot++

http://php.net/tempnam
"Creates a file with a unique filename, with access permission set to 0600, in the specified directory. If the directory does not exist, tempnam() may generate a file in the system's temporary directory, and return the name of that."

Was able to reproduce this locally, noticed that the file was created in /tmp, a simple file_prepare_directory() fixes this. Patch coming in a second.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
5.47 KB

We might want to consider using $this->randomName() instead of tempfile() because that functions does, as documented, a number of weird things that can be quite confusing. An error because the file can not be written to a non-existing directory would have been much more obvious. Also, use translations:://.

However, this should at least allow the current patch to pass.

penyaskito’s picture

Berdir++

Use $this->randomName() and the translations stream wrapper.

penyaskito’s picture

FileSize
848 bytes

Oops, wrong interdiff

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks to me the streamwrappers are not involved in fixing the bug / adding the test here. Also the stream wrapper is not used widely where the translations directory is used, so if we introduce it we should apply it more widely. I think that should be a followup.

penyaskito’s picture

Assigned: webflo » penyaskito

I plan to work on this.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
3.42 KB

Removed references to StreamWrapper. A followup will come.

This should be now ready.

penyaskito’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks good, minus one mistake :)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -178,4 +177,21 @@ class LocaleFileImportStatus extends WebTestBase {
+    $file_uri = $dir. '/po_'. $this->randomString() .'.'. $langcode .'.po';

Minor code style problem: dots should have spaces both before/after.

penyaskito’s picture

I was wrong about that, thought that was only when there were literal strings.

penyaskito’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Title: Track import status of files in the locale .po file directory » Fatal error when deleting language due to DBTNG typo
Category: feature » bug
Status: Needs review » Reviewed & tested by the community

Good to go, fixes the bug (explained in http://drupal.org/node/1635084#comment-6128806), adds tests to prove it works. Retitled for current fix, should be titled / categorized back when committed.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

If this is just fixing the bug, let's not introduce a code style regression at the same time.

$files = locale_translate_get_interface_translation_files($langcode);
+  $return = TRUE;
   if (!empty($files)) {
     foreach ($files as $file) {
       $success = file_unmanaged_delete($file->uri);
       if (!$success) {
-        return FALSE;
+        $return = FALSE;
+      }
+      else {
+        // Remove the registered translation file if any.
+        db_delete('locale_file')
+          ->condition('langcode', $langcode)
+          ->condition('uri', $file->uri)
+          ->execute();
       }
-      // Remove the registered translation file if any.
-      db_delete('locale_file')
-        ->condition('langcode', $langcode)
-        ->coddition('uri', $file->uri)
-        ->execute();
     }
   }
-  return TRUE;
+  return $return;

The only change necessary here is coddition() >> condition(), the rest is just unnecessary noise. The $return just makes the code unnecessarily convoluted.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Damien: well, no. The previous code stopped deleting files from both the file system and DB once a file was found that was impossible to delete. The new code will delete all files and their records in the DB that are deletable, and will return FALSE if there was at least one file that was not deletable. The meaning of the return is the same, but the deletion is more accurate, less garbage leaved behind if a file was not deletable.

Damien Tournoud’s picture

My bad.

Gábor Hojtsy’s picture

Issue tags: -Needs tests
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay! Committed and pushed to 8.x. Thanks for the expanded test coverage! :)

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks.

Gábor Hojtsy’s picture

Title: Fatal error when deleting language due to DBTNG typo » Track import status of files in the locale .po file directory
Category: bug » feature
xjm’s picture

Assigned: penyaskito » xjm
Priority: Normal » Critical
Status: Fixed » Active

This is causing testbot fails, probably due to randomSting() use. Rolling a followup now.

xjm’s picture

Title: Track import status of files in the locale .po file directory » [HEAD BROKEN] Track import status of files in the locale .po file directory
Category: feature » bug
xjm’s picture

Assigned: xjm » Unassigned
Status: Active » Needs review
FileSize
951 bytes

I suspect this should fix the problem.

penyaskito’s picture

Had the same issue locally. If randomeString() returns anything with a slash (/), the path is invalid and it fails. Thanks for catching it, Jess.

webchick’s picture

Title: [HEAD BROKEN] Track import status of files in the locale .po file directory » Track import status of files in the locale .po file directory
Category: bug » feature
Priority: Critical » Normal
Status: Needs review » Fixed

Grumble grumble! Thanks. :)

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +Random test failure
xjm’s picture

Issue summary: View changes

updating summary