Updated summary: this had an initial commit, and the things left to do are summarized in comment #83.

To be able to update a module's translation, we need to collect the project info. The process is similar to Update module but required additional data for which some changes were made in #1329410: Extend update data collection to support localization update.
This State diagram shows the project info collection process in Localization Update module and how it is related to all other processes.

Files: 
CommentFileSizeAuthor
#99 interdiff.txt590 bytesGábor Hojtsy
#99 convert-new-variables-to-cmi-1627006-99.patch19.94 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 41,255 pass(es). View
#95 convert-new-variables-to-cmi-1627006-96.patch19.94 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 41,072 pass(es), 77 fail(s), and 40 exception(s). View
#95 interdiff.txt688 bytesGábor Hojtsy
#94 interdiff.txt4.89 KBGábor Hojtsy
#94 convert-new-variables-to-cmi-1627006-94.patch19.94 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 41,254 pass(es). View
#90 convert-new-variables-to-cmi-1627006-90.patch16.08 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 41,243 pass(es). View
#90 interdiff-1627006-88-90.txt3.3 KBYesCT
#88 interdiff-1627006-87-88.txt1.94 KBYesCT
#88 convert-new-variables-to-cmi-1627006-88.patch15.37 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 41,228 pass(es). View
#87 convert-new-variables-to-cmi-1627006-87.patch15.29 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 41,188 pass(es). View
#87 1627006-interdiff-82-87.txt1.36 KBSutharsan
#82 convert-new-variables-to-cmi-1627006-82.patch15.3 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 41,101 pass(es). View
#82 interdiff-1627006-81-82.txt3.19 KBYesCT
#81 convert-new-variables-to-cmi-1627006-81.patch15.3 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 41,103 pass(es). View
#81 interdiff-1627006-76-81.txt5.1 KBYesCT
#76 convert-new-variables-to-cmi-1627006-76.patch14.16 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es). View
#76 interdiff-1627006-73-76.txt1.12 KBYesCT
#73 convert-new-variables-to-cmi-1627006-73.patch14.08 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 41,093 pass(es), 1 fail(s), and 1 exception(s). View
#73 interdiff-1627006-63-73.txt13.94 KBYesCT
#69 interdiff-1627006-63-revert.txt2.47 KBYesCT
#67 interdiff-1627006-63-new.txt767 bytesYesCT
#63 convert-new-variables-to-cmi-1627006-2.patch6.08 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View
#61 convert-new-variables-to-cmi-1627006.patch6.01 KBwebflo
FAILED: [[SimpleTest]]: [MySQL] 40,398 pass(es), 5 fail(s), and 6 exception(s). View
#53 locale-project-info-1627006-52.patch27 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 40,646 pass(es). View
#53 locale-project-info-1627006-52-interdiff.txt3.8 KBwebflo
#52 locale-project-info-1627006-51.patch26.86 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 40,652 pass(es). View
#50 locale-project-info-1627006-50.patch26.86 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.install. View
#50 interdiff.txt3.53 KBGábor Hojtsy
#48 locale-project-info-1627006-48.patch25.41 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 40,573 pass(es). View
#48 1627006-intediff-46_48.txt694 bytesandypost
#46 1627006-interdiff-45-46.txt605 bytesSutharsan
#46 locale-project-info-1627006-46.patch25.6 KBSutharsan
FAILED: [[SimpleTest]]: [MySQL] 40,571 pass(es), 1 fail(s), and 0 exception(s). View
#45 locale-project-info-1627006-45.patch25.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 40,582 pass(es), 2 fail(s), and 0 exception(s). View
#45 1627006-interdiff-44_45.txt6.78 KBandypost
#44 1627006-interdiff-42-44.txt3.89 KBMantasK
#44 locale-project-info-1627006-44.patch25.6 KBMantasK
FAILED: [[SimpleTest]]: [MySQL] 40,574 pass(es), 2 fail(s), and 0 exception(s). View
#42 1627006-interdiff-38-42.txt1.13 KBSutharsan
#42 locale-project-info-1627006-42.patch25.69 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 40,587 pass(es). View
#38 1627006-interdiff-36-38.txt24.76 KBSutharsan
#38 locale-project-info-1627006-38.patch25.69 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 40,263 pass(es). View
#36 1627006-interdiff-26-36.txt8.52 KBSutharsan
#36 locale-project-info-1627006-36-do-not-test.patch118.84 KBSutharsan
#28 1627006-interdiff-22-26.txt4.68 KBSutharsan
#27 interdiff-locale-project-information.patch2.64 KBlazysoundsystem
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es). View
#26 locale-project-information-1627006-26.patch31.06 KBlazysoundsystem
PASSED: [[SimpleTest]]: [MySQL] 40,269 pass(es). View
#22 locale-project-information-1627006-22.patch31.12 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View
#21 locale-project-information-1627006-21.patch31.07 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View
#21 locale-project-information-1627006-21-interdiff.txt9.46 KBBerdir
#17 locale-project-info-1627006-17.patch30.99 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 36,903 pass(es). View
#17 interdiff-16-17.txt955 bytesSutharsan
#16 locale-project-info-1627006-16.patch31.05 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 36,917 pass(es). View
#16 interdiff-10-16.txt8.21 KBSutharsan
#14 interdiff-11-12.txt4.97 KBSutharsan
#12 locale-project-info-1627006-11_0.patch64.24 KBjepSter
FAILED: [[SimpleTest]]: [MySQL] 36,920 pass(es), 1 fail(s), and 0 exception(s). View
#11 locale-project-info-1627006-10.patch31.29 KBSutharsan
FAILED: [[SimpleTest]]: [MySQL] 36,905 pass(es), 5 fail(s), and 7 exception(s). View
#11 interdiff-9-10.txt12.66 KBSutharsan
#9 locale-project-info-1627006-9.patch26.29 KBSutharsan
FAILED: [[SimpleTest]]: [MySQL] 36,852 pass(es), 0 fail(s), and 30 exception(s). View
#9 interdiff-7-9.txt3.62 KBSutharsan
#7 locale-project-info-1627006-7.patch21.59 KBSutharsan
FAILED: [[SimpleTest]]: [MySQL] 36,871 pass(es), 0 fail(s), and 30 exception(s). View
#7 interdiff-2-7.txt17.1 KBSutharsan
#2 locale-project-info-1627006-1.patch8.79 KBSutharsan
PASSED: [[SimpleTest]]: [MySQL] 36,828 pass(es). View
l10n_update-state-diagram.gif72.13 KBSutharsan

Comments

Gábor Hojtsy’s picture

Looks great :)

Sutharsan’s picture

FileSize
8.79 KB
PASSED: [[SimpleTest]]: [MySQL] 36,828 pass(es). View

This patch adds a locale_project_list() function including tests. It collects project data for projects which interface translations may be imported automatically including custom modules which have an 'interface translation project' property in their .info file. Other new info file properties included are:

  • interface translation server
  • interface translation url
  • interface translation pattern
Sutharsan’s picture

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

Status: Needs review » Needs work

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.

Sutharsan’s picture

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.

Update module is required for update_process_info_list(). Without this Locale can not update its project data when new projects are added, module are updated or modules are removed. It can only update translations of existing modules. IMHO this would seriously cripple the functionality.

Gábor Hojtsy’s picture

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.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
17.1 KB
21.59 KB
FAILED: [[SimpleTest]]: [MySQL] 36,871 pass(es), 0 fail(s), and 30 exception(s). View

Agree with the gracefull degradation. But then we should notify the user for example on the translation update page. Since translations of new modules are not imported, which is an unexpected behaviour which can not be predicted.

New patch contains:

  • Storage of project data in new {locale_project} table.
  • locale_get_projects() function to be called by the 'Get current translation status' in the UML above.
  • No upgrade path for database.
  • No gracefull degradation.

Status: Needs review » Needs work

The last submitted patch, locale-project-info-1627006-7.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
3.62 KB
26.29 KB
FAILED: [[SimpleTest]]: [MySQL] 36,852 pass(es), 0 fail(s), and 30 exception(s). View

Changes:

  • Namespace for functions, constants and variables names changed to locale_translation_
  • Documentation todo's resolved
  • Removed the Update module dependency

The interdiff is not complete, but this is the best I can do for now.

Status: Needs review » Needs work

The last submitted patch, locale-project-info-1627006-9.patch, failed testing.

Sutharsan’s picture

FileSize
12.66 KB
31.29 KB
FAILED: [[SimpleTest]]: [MySQL] 36,905 pass(es), 5 fail(s), and 7 exception(s). View

Changes in this patch:

  • Translation server data now added via an info hook instead of settings in the .info file. The .info file method was expected not to be acceptable for commit to core, and is therefore replaced.
  • Added hook_locale_translation_additional_project_info() Allows modules and themes to define or alter project definitions for interface translation.
  • Added upgrade path for {locale_project} table.
jepSter’s picture

FileSize
64.24 KB
FAILED: [[SimpleTest]]: [MySQL] 36,920 pass(es), 1 fail(s), and 0 exception(s). View

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.

Schnitzel’s picture

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’s picture

FileSize
4.97 KB

@jepSter Thanks for stepping up. You have added a patch file to your patch. Please be more carefull next time. Further we use interdiff files to highlight the differences between large patches, that would have helpt greatly to check your work.

Sutharsan’s picture

@jepSter, other comments:
Don't comment lines out, just remove any unused code.
Missing documentation for new locale_test_locale_translation_additional_project_info() function.

Sutharsan’s picture

FileSize
8.21 KB
31.05 KB
PASSED: [[SimpleTest]]: [MySQL] 36,917 pass(es). View

Changes:

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
955 bytes
30.99 KB
PASSED: [[SimpleTest]]: [MySQL] 36,903 pass(es). View

Changes:

  • module_invoke_all() used as @Schnitzel suggested in #13.
jepSter’s picture

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

Sutharsan’s picture

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

Staring: +++ b/core/modules/locale/locale-project-info-1627006-10_0.patch

Which kind of documentation you imageine?

Doxygen comments like

/**
 * Something goes here
 *
 * @param $foo
 *    About foo ...
 */
This patch isn't working for me.

Use a clean 8.x checkout, not a branch, or apply interdiffs.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.46 KB
31.07 KB
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View

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

Berdir’s picture

FileSize
31.12 KB
PASSED: [[SimpleTest]]: [MySQL] 40,010 pass(es). View

Fixed the $modules definition.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

lazysoundsystem’s picture

FileSize
31.06 KB
PASSED: [[SimpleTest]]: [MySQL] 40,269 pass(es). View

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.

lazysoundsystem’s picture

FileSize
2.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es). View

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

Sutharsan’s picture

FileSize
4.68 KB

@lazysoundsystem, thanks for the work. Changes look good. An interdiff file is a text file in patch format that shows the changes between subsequent versions of a patch. (see http://drupal.org/node/1488712) not a diff of patches.
New interdiff attached.

lazysoundsystem’s picture

@Sutharsan - thanks for the clarification. got it.

Gábor Hojtsy’s picture

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?

Sutharsan’s picture

I found two places where the server name is used. In the translation update status list and in a message when the l10n_client saves a string to the remote server. The update status page will change and I don't expect that a remote server name is needed. The l10n_client can probably also do without a server name ('You could share your work with !l10n_server if you set your API key at !user_link.').

If we drop the server URL the pattern must be hard coded in the module and can't be set by the server at runtime. I don't expect this to be a problem but it will be a simplification of the code. If a need to alter the patter should arrise, it can stil be implemented using an alter hook.

So, I agree that the minimum requirement is a project name and a po file pattern.

Gábor Hojtsy’s picture

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

Schnitzel’s picture

Assigned: Sutharsan » Schnitzel

working on this

Schnitzel’s picture

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

Schnitzel’s picture

Assigned: Schnitzel » Unassigned

unsassigning form me, because sutharsan is working on it.

Sutharsan’s picture

@Schnitzel, yes I am ;) but thanks for looking into the degradation issue.

Attached patch contains a first answer to Gabor's comments (#25) on the documentation in locale.api.php. Due to the change (#30) in using the .info file definition (again) the API will change and the documentation with it. So more documentation changes to locale.api.php to follow. Next I will work on bringing back the .info properties and include Schnitzel's degration suggestions.

Sutharsan’s picture

Assigned: Unassigned » Sutharsan
Sutharsan’s picture

FileSize
25.69 KB
PASSED: [[SimpleTest]]: [MySQL] 40,263 pass(es). View
24.76 KB

Modifications:

  • @file level documentation added to locale.api.php. Including "addtogroup hooks".
  • Documentation in locale.api.php moved to phpdoc comments.
  • Limited the .info file properties to "interface translation project" and "interface translation server pattern".
  • Removed the disabled code for installing 'cache_locale_update'.
  • Documented the fallback behaviour in locale_translation_build_projects() and locale_translation_project_list().
  • Added Schnitzel's degradation code suggestion.

Still needs to do some work on the unifications of key names. I like to get it right, but need some help on it. For example "server_pattern" can not be changed into "server pattern" because it is used as database table name.

Sutharsan’s picture

Status: Needs work » Needs review

Keep for getting this ;)

Sutharsan’s picture

Summary of name unification:

.info properties:
- interface translation project
- interface translation server pattern

Corresponding variables:
- project (No subject to change. Also corresponding to project .info file property)
- server_pattern

Correponsing field in {locale_project}:
- n/a
- server_pattern

Gábor Hojtsy’s picture

I think this sounds like a good plan!

Sutharsan’s picture

FileSize
25.69 KB
PASSED: [[SimpleTest]]: [MySQL] 40,587 pass(es). View
1.13 KB

This is already the current situation. So plan executed ;)
Small documentation change in attached patch.

webflo’s picture

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

MantasK’s picture

FileSize
25.6 KB
FAILED: [[SimpleTest]]: [MySQL] 40,574 pass(es), 2 fail(s), and 0 exception(s). View
3.89 KB

applied changes from #43

andypost’s picture

FileSize
6.78 KB
25.41 KB
FAILED: [[SimpleTest]]: [MySQL] 40,582 pass(es), 2 fail(s), and 0 exception(s). View

#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

Sutharsan’s picture

FileSize
25.6 KB
FAILED: [[SimpleTest]]: [MySQL] 40,571 pass(es), 1 fail(s), and 0 exception(s). View
605 bytes

Typo fixed.

Status: Needs review » Needs work

The last submitted patch, locale-project-info-1627006-46.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
694 bytes
25.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,573 pass(es). View

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 )

webflo’s picture

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.

Gábor Hojtsy’s picture

FileSize
3.53 KB
26.86 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/locale.install. View

You are right on all your points. Updated.

Status: Needs review » Needs work

The last submitted patch, locale-project-info-1627006-50.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
26.86 KB
PASSED: [[SimpleTest]]: [MySQL] 40,652 pass(es). View

Fixed the syntax error.

webflo’s picture

FileSize
3.8 KB
27 KB
PASSED: [[SimpleTest]]: [MySQL] 40,646 pass(es). View

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now!

Gábor Hojtsy’s picture

Priority: Normal » Critical

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

webchick’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

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"

Berdir’s picture

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

rvilar’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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.

webflo’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,398 pass(es), 5 fail(s), and 6 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, convert-new-variables-to-cmi-1627006.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View

I forgot the $config->save().

Kristen Pol’s picture

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

aspilicious’s picture

No it doesn't :)

aspilicious’s picture

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

YesCT’s picture

Status: Needs review » Needs work
FileSize
767 bytes

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

aspilicious’s picture

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.

YesCT’s picture

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.

aspilicious’s picture

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

YesCT’s picture

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.

Gábor Hojtsy’s picture

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.

YesCT’s picture

FileSize
13.94 KB
14.08 KB
FAILED: [[SimpleTest]]: [MySQL] 41,093 pass(es), 1 fail(s), and 1 exception(s). View

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, convert-new-variables-to-cmi-1627006-73.patch, failed testing.

YesCT’s picture

FileSize
1.12 KB
14.16 KB
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es). View

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.

YesCT’s picture

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

Status: Needs review » Needs work

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

YesCT’s picture

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.

heyrocker’s picture

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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
5.1 KB
15.3 KB
PASSED: [[SimpleTest]]: [MySQL] 41,103 pass(es). View

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.

YesCT’s picture

FileSize
3.19 KB
15.3 KB
PASSED: [[SimpleTest]]: [MySQL] 41,101 pass(es). View

cleaned up white space.

YesCT’s picture

Issue summary: View changes

Image added

YesCT’s picture

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]

xjm’s picture

Status: Needs review » Needs work
+++ 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.

xjm’s picture

Status: Needs work » Needs review

Sorry, dreditor.

Sutharsan’s picture

@YesCT

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

For a start, sites that do not run cron for longer time will fail. Core build in cron trigger fires daily. If the locale_project table is empty Locale module is probably freshly enabled but while Update module is disabled. It would be a bad first experience indeed. We could fall back to the system table which holds most of the data we need, except for the project name. It is not an easy task to extract the project name from the file and module name and the results may not be reliable. If we think this should be solved, make it a separate issue.

+ 'description' => 'Core compatibility string for this project, for example: 7.x.',
Is this the right type of compatibility string? Where can I find out?

See http://drupal.org/node/542202#core

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

See http://drupal.org/node/467026

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

Probably a CVS legacy, need to check. This code was originally written for 6.x.

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

You are right, I did some reverse engineering on this to find these states. Did not search for the source (I think). I'm not sure which module(s) at d.o provides the XML data for update module.

@xjm
This code tries to do a version fallback, a thing that Update module does not provide. But possible reuse of code should be investigated.

Sutharsan’s picture

FileSize
1.36 KB
15.29 KB
PASSED: [[SimpleTest]]: [MySQL] 41,188 pass(es). View
+    // @todo: http://drupal.org/node/1627006#comment-6396658
+    // Specify the columns because SELECT * has bad performance on InnoDB.

2 x, done.

YesCT’s picture

FileSize
15.37 KB
PASSED: [[SimpleTest]]: [MySQL] 41,228 pass(es). View
1.94 KB

#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',

chx’s picture

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?

YesCT’s picture

FileSize
3.3 KB
16.08 KB
PASSED: [[SimpleTest]]: [MySQL] 41,243 pass(es). View

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.

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

The last submitted patch, convert-new-variables-to-cmi-1627006-90.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-ui
Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

Gábor Hojtsy’s picture

Title: Collect project information for translation update » CMI conversion and minor followups to "Collect project information for translation update"
Status: Needs work » Needs review
FileSize
19.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,254 pass(es). View
4.89 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
688 bytes
19.94 KB
FAILED: [[SimpleTest]]: [MySQL] 41,072 pass(es), 77 fail(s), and 40 exception(s). View

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

Status: Reviewed & tested by the community » Needs work
Issue tags: -D8MI, -sprint, -language-ui

The last submitted patch, convert-new-variables-to-cmi-1627006-96.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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

The last submitted patch, convert-new-variables-to-cmi-1627006-96.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
19.94 KB
PASSED: [[SimpleTest]]: [MySQL] 41,255 pass(es). View
590 bytes

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

So then should be back on RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

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

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Title: CMI conversion and minor followups to "Collect project information for translation update" » Collect project information for translation update

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.

Berdir’s picture

Issue summary: View changes

updating the summary to point to #83