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

Files: 
CommentFileSizeAuthor
#77 1635084-followup.patch951 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1635084-followup.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#65 track-imports-1635084-65.patch3.42 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 36,991 pass(es).
[ View ]
#62 track-imports-1635084-62.patch3.42 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 36,986 pass(es).
[ View ]
#57 track-imports-1635084-57.patch5.47 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 36,996 pass(es).
[ View ]
#57 track-imports-1635084-57-interdiff.txt1.15 KBBerdir
#52 track-import-po-files-1635084-42.patch5.41 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 37,027 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#42 track-import-po-files-1635084-32.patch3.22 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 37,042 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#40 track-import-po-files-1635084-31-regression-test.patch1.63 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch track-import-po-files-1635084-31-regression-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 track-import-po-files-1635084-31.patch2.46 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 37,002 pass(es).
[ View ]
#36 track-import-po-files-1635084-30.patch2.46 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 37,015 pass(es).
[ View ]
#33 track-import-po-files-1635084-29.patch15.62 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch track-import-po-files-1635084-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#33 track-import-po-files-1635084-29.interdiff.txt2.46 KBwebflo
#30 track-import-po-files-1635084-28.patch15.58 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 37,012 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch move-date-format-translation-functions-from-locale-to-system-1635134-15_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 36,964 pass(es).
[ View ]
#23 track-import-po-files-1635084-23.patch15.6 KBvasi1186
PASSED: [[SimpleTest]]: [MySQL] 36,972 pass(es).
[ View ]
#19 track-import-po-files-1635084-7.patch15.56 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,962 pass(es).
[ View ]
#19 track-import-po-files-1635084-7.interdiff.txt936 byteswebflo
#18 track-import-po-files-1635084-6.patch16.29 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,982 pass(es).
[ View ]
#16 track-import-po-files-1635084-5.patch16.29 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 36,961 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#11 track-import-po-files-1635084-4.patch13.04 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]
#9 track-import-po-files-1635084-3.patch13.01 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 36,893 pass(es), 0 fail(s), and 30 exception(s).
[ View ]
#5 track-import-po-files-1635084-2.patch13.05 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 36,909 pass(es), 0 fail(s), and 30 exception(s).
[ View ]
#2 track-status-of-po-files-1635084-1.patch5.95 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 36,835 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

Comments

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.

Status:Active» Needs work
StatusFileSize
new5.95 KB
FAILED: [[SimpleTest]]: [MySQL] 36,835 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

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.

Assigned:Unassigned» webflo

Status:Needs work» Needs review
StatusFileSize
new13.05 KB
FAILED: [[SimpleTest]]: [MySQL] 36,909 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

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.

Status:Needs work» Needs review

#5: track-import-po-files-1635084-2.patch queued for re-testing.

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.

Status:Needs work» Needs review
StatusFileSize
new13.01 KB
FAILED: [[SimpleTest]]: [MySQL] 36,893 pass(es), 0 fail(s), and 30 exception(s).
[ View ]

Fixed locale_update_8010().

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new13.04 KB
PASSED: [[SimpleTest]]: [MySQL] 36,888 pass(es).
[ View ]

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

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?

Setting back to Gabor's status :)

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.

Status:Needs work» Needs review
StatusFileSize
new16.29 KB
FAILED: [[SimpleTest]]: [MySQL] 36,961 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new16.29 KB
PASSED: [[SimpleTest]]: [MySQL] 36,982 pass(es).
[ View ]

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -0,0 +1,180 @@
+    drupal_static_reset('langauge_list');

Fixed typo in langauge_list.

Status:Needs review» Needs work
StatusFileSize
new936 bytes
new15.56 KB
PASSED: [[SimpleTest]]: [MySQL] 36,962 pass(es).
[ View ]

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

Status:Needs work» Needs review

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? :)

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.

StatusFileSize
new15.6 KB
PASSED: [[SimpleTest]]: [MySQL] 36,972 pass(es).
[ View ]

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?

Status:Needs work» Needs review

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?

Status:Needs work» Needs review

StatusFileSize
new15.59 KB
PASSED: [[SimpleTest]]: [MySQL] 36,964 pass(es).
[ View ]
new2.7 KB

Attached a patch and an interdiff.

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

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.

Status:Needs work» Needs review
StatusFileSize
new591 bytes
new29.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch move-date-format-translation-functions-from-locale-to-system-1635134-15_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

StatusFileSize
new547 bytes
new15.58 KB
PASSED: [[SimpleTest]]: [MySQL] 37,012 pass(es).
[ View ]

Sorry. This was the wrong patch.

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.

Not applying. :(

Status:Needs work» Needs review
StatusFileSize
new2.46 KB
new15.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch track-import-po-files-1635084-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Addressed the comments from attiks.

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.

Status:Needs work» Needs review
StatusFileSize
new2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 37,015 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

No failing tests :)

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

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

Status:Needs work» Needs review
StatusFileSize
new2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 37,002 pass(es).
[ View ]
new1.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch track-import-po-files-1635084-31-regression-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.22 KB
FAILED: [[SimpleTest]]: [MySQL] 37,042 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Sure. Sorry i missed that.

Status:Needs review» Needs work

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

Does not work :(

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.

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.

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 :-)

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new5.41 KB
FAILED: [[SimpleTest]]: [MySQL] 37,027 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

+++ 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

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

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.

Status:Needs work» Needs review
StatusFileSize
new1.15 KB
new5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 36,996 pass(es).
[ View ]

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.

StatusFileSize
new5.49 KB
PASSED: [[SimpleTest]]: [MySQL] 36,986 pass(es).
[ View ]
new1.15 KB

Berdir++

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

StatusFileSize
new848 bytes

Oops, wrong interdiff

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.

Assigned:webflo» penyaskito

I plan to work on this.

Status:Needs work» Needs review
StatusFileSize
new3.08 KB
new3.42 KB
PASSED: [[SimpleTest]]: [MySQL] 37,003 pass(es).
[ View ]

Removed references to StreamWrapper. A followup will come.

This should be now ready.

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.

StatusFileSize
new3.42 KB
PASSED: [[SimpleTest]]: [MySQL] 36,991 pass(es).
[ View ]

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

Status:Needs work» Needs review

Title:Track import status of files in the locale .po file directoryFatal 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.

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.

<?php
$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.

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.

My bad.

Issue tags:-Needs tests

Status:Reviewed & tested by the community» Fixed

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

Issue tags:-sprint

Yay, thanks.

Title:Fatal error when deleting language due to DBTNG typoTrack import status of files in the locale .po file directory
Category:bug» feature

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

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

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

Assigned:xjm» Unassigned
Status:Active» Needs review
StatusFileSize
new951 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1635084-followup.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I suspect this should fix the problem.

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

Title:[HEAD BROKEN] Track import status of files in the locale .po file directoryTrack 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.

Issue tags:+Random test failure

Issue summary:View changes

updating summary