Get status of local and remote translation files

Rerolled.

+++ b/core/modules/locale/locale.compare.inc
@@ -0,0 +1,135 @@
+  $languages = $languages ? $languages : array_keys(locale_translatable_language_list());
...
+    foreach ($languages as $lang) {
+      $project_clone = clone $project;
+      $source = locale_translation_source_build($project_clone, $lang);
+      $source->url = locale_translation_build_string($source, $source->server_pattern);

Use $language or $langcode instead of $lang

Amazing work, some small comments

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,116 @@
+    $context['results'] ++;

is the space needed in front of ++

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+ *
+ * Eliminates a difference between the download time
+ * (Database: locale_file.timestamp) and the actual .po file timestamp.
+ */

what is the unit: seconds, minutes, ...

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+  }
+  if ($check_local) {

add a blank line to improve reading.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+  else {
+    return $result_remote;

if $check_local is FALSE, does this mean that $check_remote is TRUE? If not add an elseif ($check_remote)

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+ * - translations

is this a root folder?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+  $directory = $source->directory;

this will need to be changed to translations:// once #1658842: Introduce a translations:// stream wrapper to access the .po file directory is committed

The patch looks good. But still a lot of @todos. Here are some minors.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,338 @@
+ *   String containing place holders. Available place holders:
...
+ *   String with replaced place holders.

Typo: placeholders.

+++ b/core/modules/locale/locale.moduleundefined
@@ -65,6 +66,43 @@ const LOCALE_NOT_CUSTOMIZED = 0;
+ * The file name containing replacement patterns which are also used by the
+ * server pattern. See locale_translation_build_string() for supported patterns.

for supported placeholders. I think its better to standardize the wording.

@Sutharsan: can you work on this going forward or should we try and find others to help out?

Add tag.

Elevating to major as it should have been.

Just a quick review. I this this looks good, minus some of the @todo items. I do not agree with this TODO though:

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,181 @@
+// @todo Replace this with a stream wrapper? May use ReadOnlyStreamWrapper:
+//       http://drupal.org/node/1308054

I think this is fine as-is, its a one off check and therefore would have too much of an overhead to get a stream wrapper.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -269,3 +282,220 @@ function locale_translation_build_server_pattern($project, $template) {
+  // If the server_pattern contains a remote file path we check for a
+  // corresponding local po file in the local translation directory. If the server_pattern
+  // is a local po file path we chekc for a po file using this pattern.

+++ b/core/modules/locale/locale.moduleundefined
@@ -66,6 +66,43 @@ const LOCALE_NOT_CUSTOMIZED = 0;
+ * The file name containing placeholders which are also used by the server
+ * pattern. See locale_translation_build_server_pattern() for supported placeholders.

Over 80 chars.

+++ b/core/modules/locale/locale.moduleundefined
@@ -230,6 +267,26 @@ function locale_language_delete($language) {
+// ---------------------------------------------------------------------------------
+// Locale translation core functionality
+
+/**
+ * Returns list of translatable languages.
+ *
+ * @return array
+ *   Array of installed languages keyed by language name. English is omitted
+ *   unless its marked as translatable.
+ */
+function locale_translatable_language_list() {
+//drupal_static_reset('language_list');
+  $languages = language_list();
+//debug($languages);
+//debug(db_query('SELECT * FROM {language} ORDER BY weight ASC, name ASC')->fetchAllAssoc('langcode', PDO::FETCH_ASSOC));
+  if (!locale_translate_english()) {
+    unset($languages['en']);
+  }
+  return $languages;

I think this new function looks like a good wrapper. I'm not sure if you need the debug stuff around for future working patches, it looks pretty straightforward :)

I've also asked webflo and attiks to take a look, don't wait for them with any updates though IMHO.

@Sutharsan: Do you need any help with this? What kind of help is needed?

Reflecting on #21, I think its fine to drop the remote only option. Otherwise trying to get you more reviewers :)

Oh and for the user interface I assume that would come somewhere between #1804688: Download and import interface translations and #1804702: Display interface translation status.

Nice patch, some typos and minor remarks

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -29,6 +36,67 @@ public static function getInfo() {
+    $this->drupalPost('admin/config/regional/language/add', array('predefined_langcode' => 'de'), t('Add language'));

Maybe add a comment that we enable German by default.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -29,6 +36,67 @@ public static function getInfo() {
+"Project-Id-Version: Drupal 7\\n"

I know it doesn't really matter, but Drupal 7?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +137,103 @@ function testLocaleCompare() {
+   * URL. Because drupal does not allow file with a po extension to be accessed

file --> files

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +137,103 @@ function testLocaleCompare() {
+   * (denied in .htaccess) the translation files  get a txt extension. An other

two spaces between files get

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +137,103 @@ function testLocaleCompare() {
+    // translation files and match the projct definitions set in

projct --> project

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +137,103 @@ function testLocaleCompare() {
+    //debug(file_scan_directory(variable_get('file_public_path', conf_path() . '/files') . '/local', '/.*\.*/', array('recurse' => TRUE)));
+    //debug(file_scan_directory(variable_get('file_public_path', conf_path() . '/files') . '/remote', '/.*\.*/', array('recurse' => TRUE)));

may be removed

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +137,103 @@ function testLocaleCompare() {
+    $this->assertEqual($result['custom_module_one']['de']->type, 'local', 'Translation of custom_module_one. found');
+  }

dot after custom_module_one may be removed.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -28,7 +28,18 @@
+ * interface translation server pattern = sites/all/modules/custom/example_module/%project-%version.%language.po

No idea how we should right this, since modules is now in the root folder?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,195 @@
+ *   Batch proces to check the availability of remote Gettext .po files.

proces --> process

should this be remote or local files?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,195 @@
+ * per project and per language. This data is stored a state variable

... stored *in* a state ...

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,195 @@
+    // Update the file object with the result data. In case of a redirect we
+    // we store the resulting url.

one we too many

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,195 @@
+ * and remote files. But also we also check local sources if no remote file is

... But we also ...

+++ b/core/modules/locale/locale.batch.incundefined
@@ -0,0 +1,195 @@
+ * In the preceding batch processes data of remote and local translation sources

Maybe add a @see that references the 'preceding'.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+      $project_clone = clone $project;

Why is this cloned? If it's needed (and I assume it is), add a comment.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+ * Only po files in the local file system and checked. Any remote translation

and --> are

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+    // If the directory contains a streamwrapper, it is converted to a real
+    // path.

Maybe explain why this is needed

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+  $source = clone $project;

another clone

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+ * Determine if the source file is a remote or a local file.

remove 'or a local ', because the function name says that.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+ *   TRUE if the $url is a remote file location.

remove location

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,270 @@ function locale_translation_build_server_pattern($project, $template) {
+// @todo Move to locale.inc?

is the @todo still needed?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,6 +441,43 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+/**
+ * Page callback: Display the current translation status.
+ */

i think you have to add @see locale_menu()

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,6 +441,43 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    drupal_set_message(t('No translatable languages available. <a href="@add_lanuage">Add a language</a> first.', array('@add_lanuage' => url('admin/config/regional/language'))), 'warning');

Try to remove the link out of the message, so it's easier to translate.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,6 +441,43 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+//debug(state()->get('locale_translation_status'));
+//debug(format_date(state()->get('locale_translation_status_last_update'), 'long'));

debug

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,6 +441,43 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+  // @todo Calculate and display the translation status here.
+  return 'TODO: Show the transaltion status here';

i assume this is for a follow up issue, if it exists link to it in a comment.

Type: transaltion --> translation

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,6 +441,43 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+/**
  * Default theme function for translatione edit form.
  */

I know, not part of the patch, but there's a typo translatione --> translation

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -16,12 +16,130 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+      // Make the module appear as unhidden.

This sounds strange: "Don't hide the module' or something like that?

I think this looks good, maybe add the notes from #24 into the issue summary.

Most of these are nit-pickish but this patch introduces some spelling/grammar errors which we need to fix before commit. Also, some work on the naming to make it more obvious what's happening here.

Nice work on the batch API stuff, and the tests! Really exciting to see l10n_update in core pushing forward! :D

+++ b/core/modules/locale/config/locale.settings.ymlundefined
@@ -1,3 +1,5 @@
+  check_mode: 'all'

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,272 @@ function locale_translation_build_server_pattern($project, $template) {
+  if (config('locale.settings')->get('translation.check_mode') == LOCALE_TRANSLATION_CHECK_ALL) {

I don't think this variable is descriptive enough; I had to grep around for context in the rest of the patch to understand. From reading the docs at LOCALE_TRANSLATION_CHECK_ALL and friends, I think it should rather be "update_check_mode" or something (and then those constants named LOCALE_TRANSLATION_UPDATE_CHECK_X)

I would also suggest ALL should be BOTH or REMOTE_AND_LOCAL or something like that. "All" makes it sound like there are multiple choices here, but there are really only two, and REMOTE on its own is not one of them.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -30,6 +37,73 @@ public static function getInfo() {
+    variable_set('locale_translate_file_directory', $path);

I was going to whine here about adding a new variable and not using CMI, but I noticed this variable already exists in 8.x. :) So ignore me. :D

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -30,6 +37,73 @@ public static function getInfo() {
+   * Creates a translation file and test its timestamp.

"tests" its timestamp.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -30,6 +37,73 @@ public static function getInfo() {
+   * @param string $timestamp
+   *   Timestamp to set the file to. Defaults to current time.

String? Not an integer?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +143,89 @@ function testLocaleCompare() {
+   * URL. Because drupal does not allow files with a po extension to be accessed
+   * (denied in .htaccess) the translation files get a txt extension. An other

"Drupal" should be capitalized.

", another" rather than ". An other"

Great job with these comments, btw. Very clearly describe what's going on.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +143,89 @@ function testLocaleCompare() {
+   * respective local and remote translation directory. The test check whether

The test "checks" whether

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +143,89 @@ function testLocaleCompare() {
+   * the most recent files are selected in the different check scenario's: check

No ' in scenarios.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -69,12 +143,89 @@ function testLocaleCompare() {
+    // Setup timestamps to identify old and new translation sources.
...
+    // Setup the environment.

"Set up"

+++ b/core/modules/locale/locale.compare.incundefined
@@ -265,7 +266,272 @@ function locale_translation_build_server_pattern($project, $template) {
+ * @return integer
+ *   - "-1": $source1 < $source2 OR $source1 is missing.
+ *   - "0":  $source1 == $source2 OR both $source1 and $source2 are missing.
+ *   - "1":  $source1 > $source2 OR $source2 is missing.

It probably makes sense to create constants for these so they are self-documenting.

+++ b/core/modules/locale/locale.moduleundefined
@@ -227,7 +275,20 @@ function locale_language_delete($language) {
+ *   unless its marked as translatable.

"it's"

+++ b/core/modules/locale/locale.moduleundefined
@@ -644,6 +705,21 @@ function locale_form_system_file_system_settings_alter(&$form, $form_state) {
+/**
+ * Submit handler for the file system settings form.
+ *
+ * Clears the translation status when the Interface translations directory
+ * changes. Without a translations directory local po files in the directory
+ * should be ignored. The old translation status is no longer valid.
+ */
+function locale_system_file_system_settings_submit(&$form, $form_state) {
+  if ($form['locale_translate_file_directory']['#default_value'] != $form_state['values']['locale_translate_file_directory']) {
+  state()->delete('locale_translation_status');
+  state()->delete('locale_translation_status_last_update');
+  }

Can we pull those two state()->delete() lines into an API function and call it from this form submit handler, rather than embedding the logic in here?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -441,7 +441,44 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    drupal_set_message(t('No translatable languages available.') . ' ' . l(t('Add language'), 'admin/config/regional/language'), 'warning');

Is this legit? I thought you needed to do t('blah blah <a href="@something">blah</a>')?

This looks good to me, great job!

Awesome, thanks a lot! Committed and pushed to 8.x. Not sure if this needs a change notice?

Superb, thanks all! Let's move to #1804688: Download and import interface translations and #1804702: Display interface translation status :)

It did not take the sprint tag removal for some reason.