Collect project information for translation update

Looks great :)

A quick review of the current patch:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,72 @@
+
+/**
+ * @file
+ * Definition of Drupal\locale\Tests\LocaleCompareUnitTestCase.
+ */
+

Comment does not match filename.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,72 @@
+    return array(
+      'name' => 'Locale Compare',
+      'description' => 'Test handling of translation comparison.',

Comparison tests? The explanation is not very clear to me here.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,72 @@
+  /**
+   * Functional tests for localizing date formats.
+   */

Certainly not date formats :)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,72 @@
+    module_load_include('compare.inc', 'locale');
+    variable_set('locale_check_disabled', 1);
+
+
+    // Check if hidden modules are not included.

2 empty lines too much?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,72 @@
+    // Make the test module look like a normale custom module. i.e. the module

normale => normal

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,82 @@
+/**
+ * Default update server, filename and URL.
+ * @see locale_default_translation_server().
+ */
+define('LOCALE_DEFAULT_SERVER', 'localize.drupal.org');
+define('LOCALE_DEFAULT_SERVER_URL', 'http://localize.drupal.org/l10n_server.xml');
+define('LOCALE_DEFAULT_PATTERN', 'http://ftp.drupal.org/files/translations/%core/%project/%project-%release.%language.po');

Need phpdoc for each define separately. Also D8 uses the const keyword now instead of define to define constants.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,82 @@
+    // @todo Do we need to add hidden projects for interface translation?
+    //       Hidden projects are mainly used in automated tests.

I don't think we should. Those who use hidden modules to make their modules UI saner should have an alter function for it too :) (like Drupal Gardens). It is not the job of this functionality IMHO. It would be good to be in line with how update module works.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,82 @@
+    // @todo Review the naming!
+    'name' => variable_get('locale_default_server', LOCALE_DEFAULT_SERVER),
+    'server_url' => variable_get('locale_default_server_url', LOCALE_DEFAULT_SERVER_URL),
+    'update_url' => variable_get('locale_default_pattern', LOCALE_DEFAULT_PATTERN),

These might be better named locale_translation_* I think that is the variable space (if not locale_translate_*). Something along those lines, there are existing variables.

+++ b/core/modules/locale/locale.infoundefined
@@ -4,3 +4,4 @@ package = Core
+dependencies[] = update

This is good for now, but I think it should eventually become an optional dependency. So if you have update module enabled, it could do this, but if you disable update module, then locale would not have this feature. :) I think.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.infoundefined
@@ -0,0 +1,12 @@
+interface translation project = locale_test
\ No newline at end of file

Missing newline at end of file.

Yes, I think it would still be possible to have locale module translate the UI and not update anything. As soon as update module is turned on again, its update functionality would start working again. IMHO. If update module is turned on, it cannot work, so it would not do any project/translation updates. It would "gracefully degrade" so to speak to the base functionality of not doing updates.

Changes in this patch:

Commented out definitions for interface translations from in locale_test.info file and moved it into locale_test_locale_translation_additional_project_info();. The assertions for that are passing in the simple test file (= LocaleCompareUnitTest.php).

Furthermore uncommented

$this->assertFalse(isset($projects['locale_test_disabled']), t('Disabled module not found'));

Because there is already

$this->assertTrue(isset($projects['locale_test_disabled']), t('Disabled module found'));

Now all tests are passing on my local machine.

Thank's @ Sutharsan! He helped me to understand how to get into testing of the locale module.

checked the locale-project-info-1627006-10.patch looks good to me, one small thing.

+++ b/core/modules/locale/locale-project-info-1627006-10_0.patchundefined
@@ -0,0 +1,854 @@
++  // Call hook_locale_translation_additional_project_info for modules.
++  if (empty($projects)) {
++    $hook = 'locale_translation_additional_project_info';
++    foreach (module_implements($hook) as $module) {
++      $projects = module_invoke($module, $hook);

This should probably make an "$projects += module_invoke($module, $hook);".

Or why are you not using module_invoke_all() ?

@ Sutharsan:

#14
Could you post me please, in which line of the patch I can relocate your mention with the "added patch file to the patch"?

# 15
Which kind of documentation you imageine?

#17
This patch isn't working for me. I get the following:

Drupal-Icon:locale pm$ git apply locale-project-info-1627006-17.patch
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.php: already exists in working directory
error: core/modules/locale/locale.api.php: already exists in working directory
error: core/modules/locale/locale.compare.inc: already exists in working directory
error: patch failed: core/modules/locale/locale.install:33
error: core/modules/locale/locale.install: patch does not apply
error: core/modules/locale/tests/modules/locale_test/locale_test.info: already exists in working directory
error: core/modules/locale/tests/modules/locale_test/locale_test.module: already exists in working directory
error: core/modules/locale/tests/modules/locale_test_disabled/locale_test_disabled.info: already exists in working directory
error: core/modules/locale/tests/modules/locale_test_disabled/locale_test_disabled.module: already exists in working directory
Drupal-Icon:locale pm$

Just reviewed your patch. Mostly code style and naming comments. The code looks very clean btw, great stuff :)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,76 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\locale\Tests\LocaleCompareUnitTest.
+ */
+
+namespace Drupal\locale\Tests;
+
+use Drupal\simpletest\WebTestBase;
+
+/**
+ * Tests for comparing status of existing project translations with available translations.
+ */
+class LocaleCompareUnitTest extends WebTestBase {

...UnitTest extends WebTestBase; not the right naming standard(?) We don't name web tests as unit tests. Although you don't use the web UI, you need the database.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,76 @@
+    // Create Article node type.
+    $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));
+
+    // Create and login user.
+    $admin_user = $this->drupalCreateUser(array('administer site configuration', 'administer languages', 'access administration pages', 'create article content'));
+    $this->drupalLogin($admin_user);

I don't think you are actually using the article content type here, are you? Also, you don't seem to need a user or permissions either.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareUnitTest.phpundefined
@@ -0,0 +1,76 @@
+  /**
+   * Unit tests for translation status storage and translation status compare.
+   */

Technically these are not unit tests because you depend on the DB setup with the projects.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+ *
+ * @todo more description.

Yup :)

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+ * @param $projects
+ *   Array of project data.
+ *
+ * @see locale_project_list().
+ */
+function hook_locale_translation_projects_alter(&$projects) {
+  // The $projects array contains the project data returned by
+  // update_get_projects(). A number of the array elements are described in
+  // the documentation of hook_update_projects_alter().

Would be good to document the array structure. I see you documented this inside the function, but right here would be better.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+ * Allows modules and themes to define or alter project definitions for
+ * interface translation.

Function comments should be on one line.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+  // translation server (similar to localization.drupal.org).

localize.drupal.org

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+  // Po file located in local file system.

@attiks asked me about the local file system possibilities, great to see this documented.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+  // Themes can implement this hook too in their template.php file (but only
+  // in this file!).

Why only in that file?

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,106 @@
+// @todo Remove this list.
+// Whish list
+// ==========
+// Let the translation server provide the name and pattern details, instead
+//   of defining it in the .info file or in code.
+//   Will this work if an installation profile or a custom module provides
+//   it's own info hook and needs to download translations from a remote server?

Wishlist sounds good. In fact the update URL including pattern is in http://localize.drupal.org/l10n_server.xml, but not as separate pieces of data. This is really the server's property, so I agree it would be good to just get it from the server.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * @file
+ *   The API for comparing project translation status with available translation.
+ */

@file comments are not indented. Yeah.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+ *
+ * Based on l10n_update_build_projects().
+ */
+function locale_translation_build_projects() {

This comment should be removed later.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+  // This function depends on Update module. We degrade gracefully.
+  if (!module_exists('update')) {
+    return array();
+  }

Fantastic.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+  // Mark all previous projects as disabled and store new project data.
+  db_update('locale_project')
+    ->fields(array(
+      'status' => 0,
+    ))
+    ->execute();
+

Hm, why? Are you only getting project data for enabled I assume. Would be good to document IMHO. Any race condition possibilities here?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * Fetch an array of projects for translation update.
+ *
+ * @return
+ *   Array of project data including .info file data.
+ */
+function locale_translation_project_list() {
...
+ * @return
+ *   Array of .info file data.
+ */
+function _locale_translation_prepare_project_list($data, $type) {

Specify the return type as array (@return array).

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+    if (empty($projects)) {
+    $hook = 'locale_translation_additional_project_info';
+

Wrong indent for if().

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+ *   Array of server parameters:
+ *   - "server": Localization server name
+ *   - "server_url": Localization server URL where language list can be
+ *                   retrieved.
+ *   - "server_pattern": URL containing po file pattern.

Not sure if you should indent values for lists like that when wrapping to second line. But in this case, it can even fit on first, as far as I see.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+  // If we still don't have an url, cannot find this server, return false.
...
+        // The name is in our list, it can be full data or just an url.

'an url' => 'a url' (I think, sounds like an 'a' independent of how you pronounce url in English :)

+++ b/core/modules/locale/locale.installundefined
@@ -552,6 +620,77 @@ function locale_update_8009() {
 /**
+ * Add a cache table and locale_project table for the locale module.
+ */
+function locale_update_8010() {

Would be good to document the similarities of this table to the update project tables.

+++ b/core/modules/locale/locale.installundefined
@@ -552,6 +620,77 @@ function locale_update_8009() {
+  // Add a 'locale' cache table.
+  $table = drupal_get_schema_unprocessed('system', 'cache');
+  $table['description'] = 'Cache table for the locale module to store various data.';
+  db_create_table('cache_locale', $table);
+

You are not using this yet, are you? What do you need it for?

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -0,0 +1,44 @@
+  );
+
+	return $projects;
+}

Wrong whitespace.

+++ b/core/modules/locale/tests/modules/locale_test_disabled/locale_test_disabled.moduleundefined
@@ -0,0 +1,7 @@
+<?php
+
+/**
+ * @file
+ *   Simulate a disabled contrib for Locale test scripts.
+ *   Further, nothing to do here.

Do not indent @file comments, also if you need a second line, separate with an empty line.

Quick re-roll, addressed most of the things in Gabor's review.

Fixed the $modules definition.

@Berdir: thanks!

Seems like there are still a couple @todos. I don't think it is realistic to expect any major development on localize.d.o in the timeframe left for Drupal 8, so let's work with what we have there :)

@Berdir: also, can you point out the items you did not cover in your patch? It would be simpler to discuss further :) Thanks!

A deep review once again :)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -0,0 +1,77 @@
+/**
+ * @file
+ * Definition of Drupal\locale\Tests\LocaleCompareUnitTest.
+ */

The test is named LocaleCompareTest, doc does not match name.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -0,0 +1,77 @@
+  /**
+   * Tets for translation status storage and translation status compare.
+   */

- Tests (typo)
- "translation status comparison"

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
diff --git a/core/modules/locale/locale.api.php b/core/modules/locale/locale.api.php
index 0000000..e1ad3a1

index 0000000..e1ad3a1
--- /dev/null

--- /dev/null
+++ b/core/modules/locale/locale.api.phpundefined

+++ b/core/modules/locale/locale.api.phpundefined
+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@

@@ -0,0 +1,104 @@
+<?php
+

Should have @file level comments explaining what this file is.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@
+ * Alter the list of projects to be updated by locale's interface translation.
+ *
+ * @todo more description.
+ *
+ * @param $projects
+ *   Array of project data.
+ *
+ * @see locale_project_list().
+ */
+function hook_locale_translation_projects_alter(&$projects) {
+  // The $projects array contains the project data returned by
+  // update_get_projects(). A number of the array elements are described in
+  // the documentation of hook_update_projects_alter().
+
+  // In the .info file of a project a localization server can be specified.
+  // Using this hook the localization server specification can be altered or
+  // added. The 'interface translation server pattern' element is optional but
+  // can be specified to override the translation download path specified in the
+  // 10n_server.xml file.

I think the first set of comments should be moved to the phpdoc to explain $projects. Also make it "@param array $projects".

Also move the second block of comments to replace the @todo, and explain the keys we add and their roles there. Eg. why do we need the server and server URL separately?

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@
+function hook_locale_translation_additional_project_info() {
+  // If your custom module contains new strings the Locale interface translation
+  // can be configured to recognize and import the translations.
+  // The tanslations can be located in the local file system or remotely in a
+  // translation server (similar to localize.drupal.org).
+  // Required: type. "project" is required if the "example_project" is a custom
+  // module and not a contributed module.

Move this up to the main phpdoc for reference. Also maybe reword a bit to explain the main reason. Something like this to lead it up would be good:

"For modules not hosted on drupal.org, a project definition is not available or discovered by update module. To let translations to be provided for that either on the local file system or in a file structure similar to localize.drupal.org's downloads, we let custom modules provided via this API."

And then explain the specifics of the project key, etc.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@
+  // When multiple custom modules share the same po file, their project
+  // definitions should match. Both "project", "version" and "interface
+  // translation server" definitions are used as part of the po file format and
+  // should match.
+  // In this example the above example module and the other_example_module share
+  // the same po file. "project", "version" and "interface translation server
+  // pattern" are overridden to match the above po file name as provided by the
+  // example_module.

Explain here that this is for the "submodule" use case, as in when multiple modules are under the same project/code tree.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@
+// @todo Remove this list.
+// Whish list
+// ==========
+// Let the translation server provide the name and pattern details, instead
+//   of defining it in the .info file or in code.

We'd need to support module level stuff anyway, so we can add this automation later if we want to. I think this can be a followup easily.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,104 @@
+//   Will this work if an installation profile or a custom module provides
+//   it's own info hook and needs to download translations from a remote server?

Not sure what you mean by this. What needs to be tested here?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * Default URL of the xml file at the translation server containing all
+ * available languages.
...
+/**
+ * Default pattern of path and name of the gettext file at the translation
+ * server.

Try and reword these to fit on one line.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * Build list of projects and stores the result in the database.

Build => Builds

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+function locale_translation_build_projects() {
+  // This function depends on Update module. We degrade gracefully.
+  if (!module_exists('update')) {
+    return array();
+  }
...
+function locale_translation_project_list() {
+  // This function depends on Update module. We degrade gracefully.
+  if (!module_exists('update')) {
+    return array();
+  }

What is the effect of these on the upper layers of the API? What happens if the update module is not enabled? Does the user know they miss functionality? Can we document that?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * Retrieve data for default server.
+ *
+ * @return array
+ *   Array of server parameters:
+ *   - "server": Localization server name
+ *   - "server_url": Localization server URL where language list can be
+ *     retrieved.
+ *   - "server_pattern": URL containing po file pattern.
+ */
+// @todo: Rework the individual vars to a hook_locale_translation_server_info() with sets of server definitions?
+function locale_translation_default_translation_server() {
+  return array(
+    'server' => variable_get('locale_translation_default_server', LOCALE_TRANSLATION_DEFAULT_SERVER),
+    'server_url' => variable_get('locale_translation_default_server_url', LOCALE_TRANSLATION_DEFAULT_SERVER_URL),
+    'server_pattern' => variable_get('locale_translation_default_server_pattern', LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN),
+  );

As said above, the server based sourcing of these values can happen in a followup IMHO. So I'd remove this todo. However, it would be great to get these keys inline with the info file key names. They are totally different now (eg. use underscores).

Same stands above in build projects where these keys are put on projects as well. A little unification would be good.

+++ b/core/modules/locale/locale.installundefined
@@ -32,6 +32,7 @@ function locale_uninstall() {
+  // @todo Delete locale_translation_ variables.

Sounds like this would be pretty simple to do(?).

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +160,73 @@ function locale_schema() {
+    'description' => 'Update information for project translations.',

@@ -623,6 +691,77 @@ function locale_update_8010() {
+    'description' => 'Update information for project translations.',

Maybe explain a bit more, eg. "Translation status information for projects compared to translation server data." or something along those lines.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +160,73 @@ function locale_schema() {
+  // @todo Enable if required.
+  //$schema['cache_locale_update'] = drupal_get_schema_unprocessed('system', 'cache');
+  //$schema['cache_locale_update']['description'] = 'Cache table for the Localization Update module to store information about available releases, fetched from central server.';

Sounds like we can return to this later in a followup.

+++ b/core/modules/locale/locale.installundefined
@@ -586,7 +654,7 @@ function locale_update_8009() {
-function locale_update_8010() {
+function locale_update_8011() {
   $table = array(
     'description' => 'File import status information for interface translation files.',
     'fields' => array(
@@ -623,6 +691,77 @@ function locale_update_8010() {

@@ -623,6 +691,77 @@ function locale_update_8010() {
 }
 
 /**
+ * Add a cache table and locale_project table for the locale module.
+ */
+function locale_update_8010() {

DO NOT reorder update functions!

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -0,0 +1,44 @@
+ * @file
+ *   Simulate a custom module with a local po file.

Text for @file should be on the same column as the @file comment itself (outdent 2 spaces).

This patch clears away some of the minor items from Gábor's review in #25.

It hasn't touched the locale.api.php file, so all of the comments relating to that still stand. Plus these three:

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+function locale_translation_build_projects() {
+  // This function depends on Update module. We degrade gracefully.
+  if (!module_exists('update')) {
+    return array();
+  }
...
+function locale_translation_project_list() {
+  // This function depends on Update module. We degrade gracefully.
+  if (!module_exists('update')) {
+    return array();
+  }

What is the effect of these on the upper layers of the API? What happens if the update module is not enabled? Does the user know they miss functionality? Can we document that?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,401 @@
+/**
+ * Retrieve data for default server.
+ *
+ * @return array
+ *   Array of server parameters:
+ *   - "server": Localization server name
+ *   - "server_url": Localization server URL where language list can be
+ *     retrieved.
+ *   - "server_pattern": URL containing po file pattern.
+ */
+// @todo: Rework the individual vars to a hook_locale_translation_server_info() with sets of server definitions?
+function locale_translation_default_translation_server() {
+  return array(
+    'server' => variable_get('locale_translation_default_server', LOCALE_TRANSLATION_DEFAULT_SERVER),
+    'server_url' => variable_get('locale_translation_default_server_url', LOCALE_TRANSLATION_DEFAULT_SERVER_URL),
+    'server_pattern' => variable_get('locale_translation_default_server_pattern', LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN),
+  );

As said above, the server based sourcing of these values can happen in a followup IMHO. So I'd remove this todo. However, it would be great to get these keys inline with the info file key names. They are totally different now (eg. use underscores).

Same stands above in build projects where these keys are put on projects as well. A little unification would be good.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +160,73 @@ function locale_schema() {
+  // @todo Enable if required.
+  //$schema['cache_locale_update'] = drupal_get_schema_unprocessed('system', 'cache');
+  //$schema['cache_locale_update']['description'] = 'Cache table for the Localization Update module to store information about available releases, fetched from central server.';

Sounds like we can return to this later in a followup.

Here's a diff of the changes between:
locale-project-information-1627006-22.patch
locale-project-information-1627006-26.patch

@Sutharsan - thanks for the clarification. got it.

Reviewed this with @webchick just now. She was ok with introducing these as .info file properties, just asked us to vet the items introduced and only include ones we do need in this patch. So looks like tracking it down:

- we need pattern because we want to allow local file storage (eg. in git) and we want to allow other translation sources to use different patterns (eg. if someone uses pootle or something else)
- we need the server url so we can initialize the pattern from the server if we don't find it locally (and it was not provided); however, this requires #1632384: Import available language list from translation server in fact as far as I'm seeing in the patch, so if we want this convenience, we maybe cannot forgo introducing that either, even though we would not use most of it, we'd just use the update url pattern
- we need the project name for custom projects, especially with submodules, so they can be grouped together
- finally, what do we need the server name for?

So looks like if we forgo the pattern automation, then we can skip the server URL and only use the project and download pattern keys. Unless we need the server name for something. Do we want to display it on a nice UI somewhere?

Yes, sounds like that would both simplify the code, make it easier to get accepted and still keep the automations implementable in contrib if need be :)

working on this

just discussed with Gabor about the gracefully degradation in locale_translation_build_projects() and locale_translation_project_list().

The function locale_translation_get_projects() calls locale_translation_build_projects() (which will return an empty array if update module is disabled) but then still uses the already existing data in the locale_project table, which is good. But maybe we should document this in the locale_translation_build_projects() and locale_translation_project_list() function, just to not confuse devs :)

and maybe we can extend this:

    if ($result->rowCount() == 0) {
      // At least the core project should be in the database, so we build the
      // data if none are found.
      locale_translation_build_projects();
      $result = db_query('SELECT * FROM {locale_project}');
    }

with

    if ($result->rowCount() == 0 && module_exists('update')) {
      // At least the core project should be in the database, so we build the
      // data if none are found.
      locale_translation_build_projects();
      $result = db_query('SELECT * FROM {locale_project}');
    }

because if the update module was never enabled, the $result will be empty all the time =)

unsassigning form me, because sutharsan is working on it.

I think this sounds like a good plan!

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -0,0 +1,77 @@
+    $this->assertFalse(isset($projects['locale_test']), t('Hidden module not found'));
...
+    $this->assertTrue(isset($projects['locale_test_disabled']), t('Disabled module found'));

Remove t() calls from all assertions. see #500866: [META] remove t() from assert message for more information.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleCompareTest.phpundefined
@@ -0,0 +1,77 @@
+    $this->assertEqual($projects['locale_test']['name'] , 'locale_test', t('%key found in project info.', array('%key' => 'interface translation project')));

Use format_string().

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,110 @@
+ * by update_get_projects(). Using this hook the data returned by ¶

Some whitespace.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,110 @@
+function hook_locale_translation_translation_servers() {
+  ¶
+}

A example would be nice.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+        'length' => '50',

{system}.name has a length of 255.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+      'project_type' => array(
+        'description' => 'Project type, may be core, module, theme',
+        'type' => 'varchar',
+        'length' => '50',
+        'not null' => TRUE,

{system}.type is equivalent and has a length of 12. I think its better to standardize the column name.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+      'version' => array(
+        'description' => 'Human readable name for project used on the interface.',

Description is wrong.

applied changes from #43

#44 plus a few grammar and schema description

+++ b/core/modules/locale/locale.install
@@ -159,6 +162,55 @@ function locale_schema() {
+        'length' => '255',

should be int

Size increased to 15 because we have project-type 'module-disabled' it's not a {system}.type is a project type + project status

A fatal error occurred: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'project_type' at row 1: INSERT INTO {locale_project} (name, project_type, core, version, server_pattern, status) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => locale_test_disabled [:db_insert_placeholder_1] => module-disabled [:db_insert_placeholder_2] => 8.x [:db_insert_placeholder_3] => 8.0-dev [:db_insert_placeholder_4] => http://ftp.drupal.org/files/translations/%core/%project/%project-%version.%language.po [:db_insert_placeholder_5] => 0 )

I think it looks good. Just a few minors.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,267 @@
+  $transaction = db_transaction();

$transaction is never used.

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

Typo: placeholders

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

Typo: placeholders

+++ b/core/modules/locale/locale.installundefined
@@ -623,6 +675,63 @@ function locale_update_8010() {
+  // Add a 'locale' cache table.
+  $table = drupal_get_schema_unprocessed('system', 'cache');
+  $table['description'] = 'Cache table for the locale module to store various data.';
+  db_create_table('cache_locale', $table);

Is it save to call drupal_get_schema_unprocessed() for cache tables in hook_update_N()? I am not sure about this.

You are right on all your points. Updated.

Fixed the syntax error.

Update the last patch and added a @defgroup definition. Moved the big dockblock to the top of locale.api.php.

Looks good to me now!

This blocks the following things:

- #1742894: Get status of local and remote translation files
- actually downloading the .po files
- displaying all languages and using the above layers downloading translations in the installer
- #1337628: Enhance language select form with textbox and other tools UX work on the installer so it can make sense of 100 languages displayed
- integrating the downloaded translations with CMI so you can download an English Drupal and can still get your default views, vocabularies, etc. configured in your language when you install it

(Edited first wrong link).

Here is some feedback I found while going through. 95% of it is docs changes, which are fine to handle in a follow-up.

A couple of larger things are:

- This introduces (at least) two new variables which need to be converted to CMI. In general, I *really* dont' want to commit anymore patches that do this, because it conflicts with and holds back a separate effort going on to get the number of variable_get()s in core down to 0. I talked with Gábor about it and his stance is that this is work that could happen after feature freeze, which is fair enough, but it'd still be lovely to have a commitment from someone working on D8MI (who is not key to some of these major features) offer to take this on after the sprint. webflo has offered to do this. Yay!

- The logic to add in the cache tables looks like it's doing more work than it needs to, at least when you compare it to http://api.drupal.org/api/drupal/modules%21update%21update.install/funct... and http://api.drupal.org/api/drupal/modules%21update%21update.install/funct... and the like. I'm concerned that the way it is now could cause the locale module's cache tables to get out of sync with system module's cache tables.

Since we have some verbal commitment for the CMI follow-up, and since the cache table getting out of sync is somewhat an edge case that hopefully we can put away with the docs changes in short order after this patch...

Committed and pushed to 8.x. Thanks!!!

Great to see this feature taking off! :D

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * .info file properties for interface translation settings.

These comments are very thorough (yay), but need a bit of clean-up. I've outlined a few minor grammar problems below, but in general I feel like I'm being explained something but not given some specific details to help me understand the big picture until a lot later in the text.

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * Modules hosted on drupal.org, a project definition is automatically added to

Should be "For" or "On" modules hosted on drupal.org...?

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * the same data to build a list of module to check for new translations.

list of modules

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * Custom module which contain new strings should provide po file(s) containing

Either Custom "modules" or "A" custom module which "contains"

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * by update_get_projects(). Using this hook the data returned by

Let's add a comma after "Using this hook"

+++ b/core/modules/locale/locale.api.phpundefined
@@ -0,0 +1,111 @@
+ * this hook to specify the interface translation server pattern.

Or to add additional custom/non-Drupal.org modules to it, correct? Might want to point this out.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+    $result = db_query('SELECT * FROM {locale_project}');
...
+      $result = db_query('SELECT * FROM {locale_project}');

In general I think we do not use SELECT * because it has bad performance on InnoDB. Can we specify the columns here?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+    if ($result->rowCount() == 0 && module_exists('update')) {

Hm. Is this sufficient to check if the information is out of date? What if update module is enabled, and the row count comes back with 30 rows, but I have 20 more modules I've added? (This is very feasible on sites that aren't running cron.php regularly.)

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+ * The project data is based on the project list supplied by the Update module.

Does it make sense to abstract this out to some other .inc/.module? Or do you think it's better to hinge it off of Update module because our checkbox on the registration form about privacy is tied to that, and this is a similar deal? I know a lot of sites that keep Update module turned off because it can cause problems with cache rebuild race conditions. OTOH, it seems like this functionality could possibly do that too, so maybe best to leave it where it is. Follow-up, if anything, in any case.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+          // The first release with the same major release number which is not a
+          // dev release is the one. Releases are sorted the most recent first.

This logic looks like it'd be really nice to pull out into a helper function, since I've seen similar checks both here and in Update module. (Follow-up)

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+      elseif ($name == "drupal" || preg_match("/HEAD/", $data['info']['version'], $matches)) {

Could we have a comment explaining what preg_match("/HEAD/" is doing?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+      'status' => $data['project_status'] ? 1 : 0,

Should this also be in an isset() check, as the others are?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+  $projects = &drupal_static(__FUNCTION__, array());
...
+    $projects = array();

Won't these two lines cancel each other out? $projects should already have been instantiated as an array(), no?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+ * the 'project' parameter in the .info file data.
+ * Custom modules or themes can bring their own gettext translation file. To

(nitpick) There should be a newline between these two sentences.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+    // Include interface translation projects.
+    // Custom modules can bring their own gettext translation file.
+    // To enable import of this file the module must define
+    // 'interface translation project = myproject' in its .info file.
+    // To allow update_process_info_list() to identify this as a project
+    // the 'project' property is filled with the 'interface translation project'
+    // value.

(nitpick) There's some non-standard wrapping here which we should fix.

Though I wonder if we should simply add a @see to the PHPDoc to point people off to the docs which already explain this, rather than doing so here, since the function logic here is actually fairly simple.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+    'pattern' => variable_get('locale_translation_default_server_pattern', LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN),

This should be moved to CMI; we don't want to introduce more variables to convert, IMO.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+    '%language' => isset($project->language) ? $project->language : '%language',
+    '%filename' => isset($project->filename) ? $project->filename : '%filename',

The rest of these have been instantiated before it gets to this function (or at least there are no isset() checks), can we do the same for these values?

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+        'description' => 'Project type, may be core, module, theme',

Project type":"

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+        'description' => 'Core compatibility string for this project.',
...
+        'description' => 'The version release of the project.',

Could we add a small example of a valid value here? I don't think people will know what this is by reading the schema.

+++ b/core/modules/locale/locale.installundefined
@@ -159,6 +162,55 @@ function locale_schema() {
+        'description' => 'The update status of the project.',

Same here. What possible values are there and what do they mean?

+++ b/core/modules/locale/locale.installundefined
@@ -623,6 +675,113 @@ function locale_update_8010() {
+  // Add a 'locale' cache table.

I think this should be using a method similar to http://api.drupal.org/api/drupal/modules%21update%21update.install/funct... rather than re-defining the schema again here.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -0,0 +1,27 @@
+ * Simulate a custom module with a local po file.

Just in general, we call these gettext files, .po files, and po files. It'd be nice to standardize on one terminology.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -0,0 +1,27 @@
+  if (!variable_get('locale_translation_test_system_info_alter', FALSE)) {

Another variable we need to move to CMI.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -0,0 +1,27 @@
+    // Make the module appear as not-disabled.
+    $info['hidden'] = FALSE;

"unhidden" rather than "not-disabled"

+++ b/core/modules/locale/tests/modules/locale_test_disabled/locale_test_disabled.moduleundefined
@@ -0,0 +1,6 @@
+ * Simulate a disabled contrib for Locale test scripts.

contrib "module"

About the cache table thing: This is a temporary solution, we hope that this can be updated to use a Key/Value storage just like the update "cache". Sutharsan attempted to re-integrate the update cache handling back into the default cache handling in a separate issue, which I sabotaged (sorry!) because that is IMHO the wrong approach.

Maybe open a follow-up for that, or continue using #1547008: Replace Update's cache system with the (expirable) key value store for that, which is the Issue I mentioned...

@Sutharsan, if you want, I can help you resolving some webchicks comments, like CMI integration and typo/language specific error. Please, let me know if I can help.

@rvilar: @webflo is working on a followup as per @webchick's notes above. Please help review those when posted (I heard later today). Thanks everyone for being active!

Update: @webflo did not have time for this, @Sutharsan said he would be able to work on fixes for the points mentioned by @webchick over the weekend.

I had some time to work on coverting the introduced variables to CMI. Here is a first patch.

I forgot the $config->save().

Does $config->save(); need to be called after $config->delete(...); as well?

No it doesn't :)

+ test_system_info_alter: false
That should be part of the test module config.

And
check_disabled
Doesn't say anything about the usage.
Can we rename it to
check_disabled_modules

Or something else that explains what it does.

checking that I've moved + test_system_info_alter: false out correctly

Looking at the usage of the config variable in the test I would say we should revert that to variable_get/variable_del. This is infact just a "sate" and we should handle that with a key value store which isn't in yet...

So revert all test related stuff and fix the comments.

checking that I'm reverting test_system_info_alter: config stuff correctly...

But I figure I should also
do something with the
- variable_del('locale_translation_test_system_info_alter');
that was in /core/modules/locale/locale.install
because it should be in the test and not the locale module

I'm not sure though.

Just leave it like it was before. It will get cleaned up in the future.

I'm not sure about webchick's comment about non Drupal.org modules related to the addtogroup hooks.

Maybe this will do:
* Modules or distributions that use a dedicated translation server should use
* this hook to specify the interface translation server pattern, or to add
* additional custom/non-Drupal.org modules to the distribution.

to the distribution => to the list of modules known to locale.

It is really about which modules locale knows about and where it gets the translation files for them.

There will be changes needed to this. But here is what I have so far.

This patch addresses #56, #66 and #68.

Some items form webchick's comments in #56 are in the patch as @todo's. I'm not sure of the best way to track that. Others are spelled out below as I have questions about them, or I'll list them as needing their own follow up issues that need to be created. Between the patch, the @todo's and the list below, all of webchick's comments are addressed.

#70 and #72 are not taken into consideration yet.

Questions

Question about HEAD

I dont think I have this comment exactly right:
locale.compare.inc
// HEAD versions are for module maintainers and should not be used.
elseif ($name == "drupal" || preg_match("/HEAD/", $data['info']['version'], $matches)) {
// Pick latest available release.
$release = array_shift($project_updates[$name]['releases']);
}

Question about isset() check

#56 comment
+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+ 'status' => $data['project_status'] ? 1 : 0,
Should this also be in an isset() check, as the others are?

Question about lines canceling each other out

#56 comment
+++ b/core/modules/locale/locale.compare.inc
@@ -0,0 +1,265 @@
+ $projects = &drupal_static(__FUNCTION__, array());
...
+ $projects = array();
Won't these two lines cancel each other out? $projects should already have been instantiated as an array(), no?

Question about duplicate comments.

In: locale.compare.inc
// Include interface translation projects. Custom modules can bring their
// own gettext translation file. To enable import of this file the module
// must define 'interface translation project = myproject' in its .info
// file. To allow update_process_info_list() to identify this as a project
// the 'project' property is filled with the 'interface translation project' // value.

this is a lot for inline comments, and is repeating what is in the function header comments.
webchick said: Though I wonder if we should simply add a @see to the PHPDoc to point people off to the docs which already explain this, rather than doing so here, since the function logic here is actually fairly simple.

Follow-up issues Needed

Needs follow-up issue: abstract project data to other module

+++ b/core/modules/locale/locale.compare.inc
@@ -0,0 +1,265 @@
+ * The project data is based on the project list supplied by the Update module.
Does it make sense to abstract this out to some other .inc/.module? Or do you think it's better to hinge it off of Update module because our checkbox on the registration form about privacy is tied to that, and this is a similar deal? I know a lot of sites that keep Update module turned off because it can cause problems with cache rebuild race conditions. OTOH, it seems like this functionality could possibly do that too, so maybe best to leave it where it is. Follow-up, if anything, in any case.

Needs follow-up issue: helper function

+++ b/core/modules/locale/locale.compare.incundefined
@@ -0,0 +1,265 @@
+ // The first release with the same major release number which is not a
+ // dev release is the one. Releases are sorted the most recent first.
This logic looks like it'd be really nice to pull out into a helper function, since I've seen similar checks both here and in Update module. (Follow-up)

Needs follow-up issue: standardize gettext, .po, po terminology

#56 comment
+++ b/core/modules/locale/tests/modules/locale_test/locale_test.module
@@ -0,0 +1,27 @@
+ * Simulate a custom module with a local po file.
Just in general, we call these gettext files, .po files, and po files. It'd be nice to standardize on one terminology.

Test failed because didn't set $config first. Attempts to fix that.

I'm not sure what is the best practice, to do it in both code blocks? Like:

  // Check if disabled modules are detected.
    $config = config('locale.settings');
    $config->set('translation.check_disabled_modules', TRUE);
    $config->save();
    drupal_static_reset('locale_translation_project_list');
    $projects = locale_translation_project_list();
    $this->assertTrue(isset($projects['locale_test_disabled']), 'Disabled module found');

    // Check the fully processed list of project data of both enabled and
    // disabled modules.
    $config = config('locale.settings');
    $config->set('translation.check_disabled_modules', TRUE);
    $config->save();
    drupal_static_reset('locale_translation_project_list');
    $projects = locale_translation_get_projects();
    $this->assertEqual($projects['drupal']->name, 'drupal', 'Core project found');

Or just once before both.

- Question about HEAD: I think "HEAD" is a legacy code piece here, it used to be the "tip of CVS", that is similar to what "master" is now with git; so I'm not sure we need that code even anymore; that is the "|| preg_match("/HEAD/", $data['info']['version'], $matches)" part.

- Question about isset() check: I think it can be az empty() there, that sounds like the best option, so 'status' => !empty($data['project_status']) ? 1 : 0,

- Question about lines canceling each other out: sounds like the second initialization is unneccessary; it would probbaly make sense if someone sets the value to FALSE or '' instead of array() outside this function, but that is broken code, we don't need to necessarily work around it; it would fail here in that case, and that sounds fine

- Question about duplicate comments: if the same concept is explained above in phpdoc (which is preferred to long blocks of inline comments), then we can just remove it; if the phpdoc section is sufficiently far away, we can add a @see *into the phpdoc* of the function housing this code

- Needs follow-up issue: abstract project data to other module: the reason we do tie to update module is not only for the privacy setting checkbox; it is more for the APIs provided by the module to check project versions and get info file data

- Needs follow-up issue: helper function: agree, should be a followup

- Needs follow-up issue: standardize gettext, .po, po terminology: agree, should be followup; this also highly depends on the UX of it, eg. talking about Gettext PO files is simpler for users instead of just Gettext or just PO, however, repeating that in the docs all the time and especially in API function names is painful; anyway, separate issue

The rest are in the code as @todo's as far as I understand, so this should mean we are taking care of everything noted so far somehow :)

Regarding my question in #76 about what pattern to use with possible repeated $config = config(....);

I did:
762 grep -R config\( * | grep Test.php
and picked this one to look at:
763 vi core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php

And the patterns in UserRegistrationTest.php look different like:

function testRegistrationWithEmailVerification() {
    $config = config('user.settings');
    // Require e-mail verification.
    $config->set('verify_mail', TRUE)->save();

    // Set registration to administrator only.
    $config->set('register', USER_REGISTER_ADMINISTRATORS_ONLY)->save();
    $this->drupalGet('user/register');
    $this->assertResponse(403, t('Registration page is inaccessible when only administrators can create accounts.'));

    // Allow registration by site visitors without administrator approval.
    $config->set('register', USER_REGISTER_VISITORS)->save();
    $edit = array();
    $edit['name'] = $name = $this->randomName();
    $edit['mail'] = $mail = $edit['name'] . '@example.com';
    $this->drupalPost('user/register', $edit, t('Create new account'));
    $this->assertText(t('A welcome message with further instructions has been sent to your e-mail address.'), t('User registered successfully.'));
   

It seems there, that the $config = config(...); is just done once. And also that the save is part of the line that does the set.

Ideally you only want to instantiate a config object once per function call, to reduce overhead.

As far as chaining, we've typically been doing it as much as possible, because I believe (personally) it makes the code easier to read, but others might disagree and I don't think there is an established standard. So the example in #79 is fine as far as I can see.

Regarding: Needs follow-up issue: abstract project data to other module,
based on #78, leaving it as is, no follow-up issue is needed.

Regarding: Needs follow-up issue: helper function
#1774024: Create helper function to find release info for a project (follow-up issue to locale collect project information)

Regarding: Needs follow-up issue: standardize gettext, .po, po terminology
#1774032: standardize gettext, .po, po terminology (follow-up issue to locale collect project information)

This patch addresses #78
except, thinking about preg_match("/HEAD/"... still.

Also addresses #76 #79 and #80 about the $config = config(...) pattern.

Does not address #70 which I think is ok. I moved it into an uninstall function in the test module install file before I saw that comment.

Also addresses #71 and #72

Rest of things to address are @todo in the patch.

cleaned up white space.

Image added

Just summarizing all the loose ends and loose thoughts before I give this issue a break.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -37,13 +37,19 @@ function locale_translation_get_projects() {
+    // @todo: http://drupal.org/node/1627006#comment-6396658
+    // Specify the columns because SELECT * has bad performance on InnoDB.
     $result = db_query('SELECT * FROM {locale_project}');

Which columns should be specified?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -37,13 +37,19 @@ function locale_translation_get_projects() {
+    // @todo: http://drupal.org/node/1627006#comment-6396658
+    // This may not be sufficient to check if the information is out of date for
+    // sites that are not running cron.php regularly.
     if ($result->rowCount() == 0 && module_exists('update')) {

What else should be checked?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -37,13 +37,19 @@ function locale_translation_get_projects() {
+      // @todo: http://drupal.org/node/1627006#comment-6396658
+      // Specify the columns because SELECT * has bad performance on InnoDB.
       $result = db_query('SELECT * FROM {locale_project}');

Which columns should be specified?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -107,6 +113,7 @@ function locale_translation_build_projects() {
+          // @todo http://drupal.org/node/1774024 Make a helper function.

Just marking the todo that links to the follow-up issue to make a helper function.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -114,6 +121,7 @@ function locale_translation_build_projects() {
+      // HEAD versions are for module maintainers and should not be used.
       elseif ($name == "drupal" || preg_match("/HEAD/", $data['info']['version'], $matches)) {

Is the HEAD just residual and really can be taken out?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -258,6 +265,8 @@ function locale_translation_build_server_pattern($project, $template) {
+    // @todo: http://drupal.org/node/1627006#comment-6396658
+    // Can these be instantiated before getting to this function?
     '%language' => isset($project->language) ? $project->language : '%language',
     '%filename' => isset($project->filename) ? $project->filename : '%filename',

What should these be changed to?

+++ b/core/modules/locale/locale.installundefined
@@ -172,20 +169,20 @@ function locale_schema() {
+        'description' => 'Core compatibility string for this project, for example: 7.x.',

Is this the right type of compatibility string? Where can I find out?

+++ b/core/modules/locale/locale.installundefined
@@ -172,20 +169,20 @@ function locale_schema() {
+        'description' => 'The version release of the project, for example: 7.x-1.0.',

Is this the right version release example? Where can I find out?

+++ b/core/modules/locale/locale.installundefined
@@ -199,7 +196,9 @@ function locale_schema() {
+        // @todo http://drupal.org/node/1627006#comment-6396658
+        // Check where these status values are defined and reference it here.
+        'description' => 'The update status of the project. Possible values are: deleted, updated, created, rebuilt, error.',

I tried to look these up and found some by doing a grep for status in the local.module. I thought that deleted, etc might be defined as int's maybe as a result of:
use Drupal\locale\LocaleLookup;
use Drupal\locale\LocaleConfigSubscriber;
but I didn't see it defined there. What are the possible values? Where is the reference?

+++ b/core/modules/locale/locale.installundefined
@@ -678,6 +677,9 @@ function locale_update_8010() {
+  // @todo http://drupal.org/node/1627006#comment-6396658
+  // Use a similar method to http://api.drupal.org/api/drupal/modules%21update%21update.install/function/update_update_7001/7
+  // instead of re-defining the schema again.

Just marking this todo.

[edited: dreditor mistake/repeats]

+++ b/core/modules/locale/locale.compare.incundefined
@@ -37,13 +37,19 @@ function locale_translation_get_projects() {
@@ -107,6 +113,7 @@ function locale_translation_build_projects() {

@@ -107,6 +113,7 @@ function locale_translation_build_projects() {
         foreach ($project_updates[$name]['releases'] as $project_release) {
           // The first release with the same major release number which is not a
           // dev release is the one. Releases are sorted the most recent first.
+          // @todo http://drupal.org/node/1774024 Make a helper function.
           if ($project_release['version_major'] == $matches[1] &&
               (!isset($project_release['version_extra']) || $project_release['version_extra'] != 'dev')) {
             $release = $project_release;
@@ -114,6 +121,7 @@ function locale_translation_build_projects() {

@@ -114,6 +121,7 @@ function locale_translation_build_projects() {
           }
         }
       }
+      // HEAD versions are for module maintainers and should not be used.
       elseif ($name == "drupal" || preg_match("/HEAD/", $data['info']['version'], $matches)) {
         // Pick latest available release.
         $release = array_shift($project_updates[$name]['releases']);
@@ -132,7 +140,7 @@ function locale_translation_build_projects() {

@@ -132,7 +140,7 @@ function locale_translation_build_projects() {
       // A project can provide the path and filename pattern to download the
       // gettext file. Use the default if not.
       'server_pattern' => isset($data['info']['interface translation server pattern']) ? $data['info']['interface translation server pattern'] : $default_server['pattern'],
-      'status' => $data['project_status'] ? 1 : 0,
+      'status' => !empty($data['project_status']) ? 1 : 0,
     );
     $project = (object) $data;

I'm mystified as to why all this code is in locale at all. Shouldn't we be using API from the update.module for all this version name parsing? Surely this is done already elsewhere.

Sorry, dreditor.

#1777106: Make check for out of date project update information more robust for sites that are not running cron regularly (follow-up) is the follow up issue about a more robust out-of-date project information check.

#1777126: Have locale and update module share/reuse code for version name parsing is the follow up issue to look into re-using code with the update module.

This patch addresses the examples.

Still left to track down are the source of the states, and the HEAD possible legacy code.

And:
+ // @todo: http://drupal.org/node/1627006#comment-6396658
+ // Can these be instantiated before getting to this function?
'%language' => isset($project->language) ? $project->language : '%language',
'%filename' => isset($project->filename) ? $project->filename : '%filename',

I have no context, but the drupal.org packaging script does not use HEAD when writing out version information http://drupalcode.org/project/drupalorg.git/blob/refs/heads/7.x-3.x:/dru... Does this help anyone?

About the grep for HEAD

I took out the grep for HEAD. Feedback is all supporting that that was legacy code, and we dont have projects with a HEAD version.

While looking into that, I got confused, which might mean that locale_translation_get_projects() needs some better comments, and that I'm not the one to write them, since I couldn't quite sort out what it was trying to accomplish.

(core/modules/locale/locale.compare.inc) locale_translation_get_projects() is only called from the test LocaleCompareTest.php That seemed strange to me.

In function locale_translation_get_projects() it seems to be checking to see if any projects need to be updated: if they have any newer releases. But it seems to be gathering the newer available release info only for projects that have an update status of not-fetched. Within in those not-fetched, it's only gathering the newer release info only for projects that are a dev version, or core. So, why not look at all projects to see if they have a newer recommended version?

status is tricky

. (skip to conclusion if you don't want to see my thought process and investigation)

status investigation

There is a project status which is I think either 1 = enabled or 0 = disabled
see line 84 in core/modules/locale/locale.compare.inc

There is project_status in core/modules/update/update.compare.inc (line 438)
which could be insecure / UPDATE_NOT_SECURE, unpublished, revoked / UPDATE_REVOKED, unsupported / UPDATE_NOT_SUPPORTED, not-fetched / UPDATE_NOT_FETCHED, published

Note core/modules/locale/locale.compare.inc line 107 references the not-fetched project_updates project_status (which is I think different from a project status: enabled or disabled).

core/modules/update/update.compare.inc line 46 defines values for project_status to be TRUE (for enabled projects). This is probably the 1 = enabled.

There are also translation status states with possible values: deleted, updated, created, rebuilt, error. Used in core/modules/locale/locale.module

But regarding status, I conclude

that the answer to webchick's question about the status values is that projects are either 1 = enabled or 0 = disabled.

todo about instantiating %language and %filename

this is regarding function locale_translation_build_server_pattern($project, $template) in core/modules/locale/locale.compare.inc
looks like locale_translation_build_server_pattern() used to be called from locale_translation_build_projects() (see patch 9) but I dont see it being called anymore.

the call is in patch #36 but not #38

about the instantiating, that code comes from http://api.drupalhelp.net/api/l10n_update/l10n_update.inc/function/l10n_...
So, I dont know.

This patch

Cleans up the comments, takes out the grep for HEAD and updates the info about status values.

Next steps

This is about the limit of what I can wrap my head around. I need someone else to pick this up from here, or I need some hand holding.

#90: convert-new-variables-to-cmi-1627006-90.patch queued for re-testing.

+++ b/core/modules/locale/locale.installundefined
@@ -678,6 +678,9 @@ function locale_update_8010() {
+  // @todo http://drupal.org/node/1627006#comment-6396658
+  // Use a similar method to http://api.drupal.org/api/drupal/modules%21update%21update.install/function/update_update_7001/7
+  // instead of re-defining the schema again.
   // Add a 'locale' cache table.
   db_create_table('cache_locale', array(
     'description' => 'Cache table for the locale module to store various data.',

I think this should either still be fixed in this patch, or the todo should be removed.

Looking at http://api.drupal.org/api/drupal/modules%21system%21system.install/funct..., it states it was introduced because multiple cache tables were needed (vs. just inlining the cache schema in the update function like we did). We can introduce a similar helper function for the D8 upgrade path with this schema and reuse here. The only question is the right name for that update function :)

However, there is no immediate need to reuse this, so we should put it at a visible place (system module again?) so it is visibly reusable.

I can take a quick stab at this.

Retitled issue to be more precise as to what we are doing.

Attached new patch that introduces system_schema_cache_8001() as a utility function to get a cache table in the D8 upgrade. Not sure about the correct naming, should consult @catch about this. Otherwise I think the patch resolves what was requested.

Asked for feedback on the naming from catch, he pointed out we need a name change to match the system update function where we changed the schema:

[12:59pm] GaborHojtsy: catch: introducing an update helper function for cache schema in http://drupal.org/node/1627006#comment-6462244 and wanted to ask your feedback about naming 
[12:59pm] Druplicon: http://drupal.org/node/1627006 => CMI conversion and minor followups to "Collect project information for translation update" => Drupal core, locale.module, normal, needs review, 94 comments, 49 IRC mentions
[1:00pm] catch: GaborHojtsy: I don't think we have a standard for schema helpers, but that looks reasonable to me.
[1:01pm] catch: system_update_8001?
[1:02pm] catch: GaborHojtsy: well not quite, just a sec.
[1:02pm] GaborHojtsy: catch: the D7 example that we follow was for a specific system update function
[1:02pm] GaborHojtsy: catch: this supports a locale function but wanted to put in system
[1:02pm] GaborHojtsy: catch: because this makes it more reusable 
[1:02pm] GaborHojtsy: catch: so cannot really match the number to the update function as in D7
[1:02pm] catch: GaborHojtsy: so this should be based on the update where the cache table schema was introduced, which was system_update_8007()
[1:06pm] GaborHojtsy: catch: ok, so update the schema according to that, but then what should be the right function name?
[1:07pm] catch: GaborHojtsy: no I mean the function name should match that update.
[1:07pm] catch: GaborHojtsy: so just 8007 instead of 8001
[1:07pm] GaborHojtsy: catch: right, it already has tags and checksum columns 
[1:08pm] GaborHojtsy: catch: ok, that is good then, thanks!
[1:08pm] catch: GaborHojtsy: yep 
[1:08pm] catch: GaborHojtsy: potentially we could have added a cache table in entity_update_8002() before the system update, then we'd need two helpers, we don't but that'd be the reason to name after the update
[1:09pm] GaborHojtsy: catch: makes sense, thanks!
[1:09pm] GaborHojtsy: catch: I have not noticed this schema change in system, but should have looked

Given that outstanding concerns were resolved, this should be at RTBC now :)

#95: convert-new-variables-to-cmi-1627006-96.patch queued for re-testing.

It helps if we actually invoke the right function, not just rename it :)

So then should be back on RTBC.

Wow, awesome! It looks like this resolves all of my feedback, and then some! :)

Committed and pushed to 8.x. Thanks!

Thanks all, this has been a long ride! Removing from sprint.

Sorry for pinging everyone. I was looking for this issue to add a follow-up about the conversion of the cache_locale table to the key value store, see #1801962: [meta] Convert custom non-volatile cache-alike things to k/v store and it took me a while to find it because of the title, restoring that one.

updating the summary to point to #83