Consolidate .po file import to one directory

Disscussed at DrupalCon London with Gábor Hojtsy and luksak.

We should be able to handle the following scenario's:
* Distribution: Has pre-installed po files in sites/all/translations or custom director(y)(ies)
* Installation profile: The profile kicks off Drupal installation. See below.
* Install drupal: Some process loads from l.d.o (configurable) and writes po files in temp or sites/all/translations

Where local po files are available:
* Configurable via admin interface:
* * Temp directory (default)
* * Save in local directory. One directory only, defaults to 'sites/all/translations'.
* Via API: Additionally multiple directories via hook_..._alter()

Yes, I have some code, but not finished yet. It was more than I first estimated and I collapsed after DrupalCon. But I will work on it by the end of the week.

subscribe

Very interesting. When localize.drupal.org was first introduced I strongly disagreed with this whole approach, but I'm interested in what becomes of this. Subscribing for now.

That's absolutely correct, and the idea of a sites/all/translations intrigues me very much.
I haven't (yet) overcome my reservations, though, against lumping translations of all core modules, a lot of which will never be used, into one big .po file. That's not strictly related to this issue, but it is where we are currently headed, if I didn't miss anything. But again, I'm not here to complain, I'm just interested in where this is headed. And I'll voice concrete concerns, if any, when there is code to review.

I put an Arabic translation file into `sites/default/files/translations/install.ar.po`

The installer detected it, but when setting up the database in installer (i.e. installing modules), an error occured:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost/drupal_dev/install.php?profile=standard&locale=ar&id=1&... StatusText: OK ResponseText: Fatal error: Call to undefined function locale_batch_by_component() in /var/www/drupal_dev/modules/locale/locale.module on line 844

The problem is in local.module `locale_system_update` function:

function locale_system_update($components) {
  include_once drupal_get_path('module', 'locale') . '/locale.bulk.inc';
  if ($batch = locale_batch_by_component($components)) {
    batch_set($batch);
  }
}

And the patch deleted this function `locale_batch_by_component` already.

Another error is:

Error message
Notice: Undefined variable: components in locale_translate_batch_build() (line 210 of /var/www/drupal_dev/modules/locale/locale.bulk.inc).

$components var is not defined in that function.

Also, one small note:

+++ b/includes/install.core.inc
@@ -1128,7 +1128,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  $locale_translate_file_directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
+  $locales = file_scan_directory($locale_translate_file_directory, '/\.po$/', array('recurse' => FALSE));

Those lines appear three times, do we need them after installation? if so it's better to put them in function, if no, leave them as is.

+++ b/includes/install.core.inc
@@ -1128,7 +1128,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
 function install_find_locales($profilename) {

We should get rid of the $profilename parameter, if we are already doing this. That will allow to put language selection first in the installer later on.

+++ b/includes/install.core.inc
@@ -1128,7 +1128,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  $locale_translate_file_directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');

And below: Bikeshedding, I know, but since the whole idea of splitting this into its own line is making this more readable, I think $file_directory should be fine.

+++ b/includes/install.core.inc
@@ -1128,7 +1128,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  $locales = file_scan_directory($locale_translate_file_directory, '/\.po$/', array('recurse' => FALSE));

+++ b/includes/install.inc
@@ -1103,10 +1103,11 @@ function st($string, array $args = array(), array $options = array()) {
+      $po_files = file_scan_directory($locale_translate_file_directory, '!install\.'. preg_quote($install_state['parameters']['locale'], '!') .'\.po$!', array('recurse' => FALSE));

Shouldn't these be identical, i.e. shouldn't we be looking for "install.[foo].po" files in install_find_locales too?

+++ b/modules/locale/locale.module
@@ -1105,3 +1105,23 @@ function locale_form_locale_language_overview_form_alter(&$form, &$form_state) {
+    '#description' => t('A local file system path where interface translation files will be stored and looked for. This directory must exist and may be writable by Drupal.'),

This sounds like the writability is optional (which it is (for now)). I think we should either 1. leave that out entirely 2. say it *must* be writable (which we will probably end up doing anyway) or 3. Explain why you would want to make it writable.

Overall this patch looks great. One thing that I thought about, was the fact that we use a single translations directory whereas we used to have multiple ones. A couple scenarios come to mind for which this would not be ideal:
- Custom install profiles, which are language-specific (e.g. site-specific install profiles) and to do not want to use l10n_server, but rather ship with their translations in profiles/$foo/translations (which they could before).
- Multisite installations, which have a module in sites/all must duplicate all of their translations for each site. That's not the end of the world, but I wonder if there is a way we could cater for this situation.
Of course we cannot make e.g. the profiles folder or whatever writable, but in theory we could use "sites/default/translations" for the imported ones and also import from "profiles/$profile/translations". And we could also support "sites/all/translations" in addition to "sites/default/translations". That would be of course a lot of added complexity. For now I think the proposed approach is definitely worth pursuing, and adding complexity at this stage would be wrong, but I just wanted to put that out there nonetheless.

I've talked to Gabor about this patch, he'll review this one shortly.

Changes:
- Put a @todo on `function locale_system_update($components)` to discuss how can we get translations when new module\theme installed. We don't want to repeat the installer task of getting every $langcode.po file and import it, this will hurt performance. I think we need to rewrite `locale_batch_by_component` ?
- Removed the '#components' in batch array.
- Removed $profilename from `install_find_locales` per @tstoeckler review, as it's not needed.

+++ b/includes/install.core.inc
@@ -1128,7 +1128,8 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  $locale_translate_file_directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');

And below: Bikeshedding, I know, but since the whole idea of splitting this into its own line is making this more readable, I think $file_directory should be fine.

Long name, IMO, $translation_directory or $translation_dir is enough.

Shouldn't these be identical, i.e. shouldn't we be looking for "install.[foo].po" files in install_find_locales too?

Didn't quite understood what you want to change here?

re good_man:
Compare the call to file_scan_directory() in install_find_locales() and locale_translate_batch_import_files().
In install_find_locales() we look for files of the form "en.po" and in locale_translate_batch_import_files() we look for files of the form "install.en.po". I don't think that difference is intentional, but if it is, I would like to understand it. :)

Sorry, still needs review probably.

+++ b/includes/install.core.inc
@@ -1125,21 +1125,30 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  foreach ($file as $key => $file) {

That won't work :)

+  array_unshift($files, (object) array('name' => 'en'));

Am I right, that this is temporary until #1266318: Make English a first class language lands?

+++ b/modules/locale/locale.module
@@ -838,10 +838,15 @@ function locale_themes_enabled($themes) {
-  include_once drupal_get_path('module', 'locale') . '/locale.bulk.inc';

That shouldn't be removed, the function has been renamed but it is still in locale.bulk.inc

Also, there is something wrong with the creation of the batches. I don't get any notices/errors with the fixes above, and I can install everything fine, but once it starts importing the translations, there's no progress. Can't debug right now, hopefully will look at it later tonight.

-18 days to next Drupal core point release.

Wow, posted too early, sorry. (also sorry for the noise in general)
After about 5min, the batch started (whyever), and the install worked fine. All the strings where correctly imported!!!
Awesome stuff!

Still needs work for the above. If I get to it later, I'll reroll.

Ahh, that totally makes sense about always adding English. D'uh!
I added a little comment about that.

Here's a reroll, which let me install successfully on my system. Please try this out and see if for you the installation also takes forever (as in ~5min vs. 10sec before), especially the "Importing translations" part. I'm pretty sure that had something to do with my setup, but it would be good to make sure.

This patch seems to destroy the testbot every time.

Does it use more than its share of database space? Is it doing something unusual?

It would help me if this gets set to needs review only when letting me or jthorson know. I've been having to clean up all the testbots every time after this one goes.

@rfay: Oh, I hadn't realized that sorry.
The patch changes a bunch of stuff in install.core.inc, but the installer itself (i.e. the HTML on install.php) is not changed. Maybe in fact the long delay I was seeing, is something inherent with the patch and is causing the testbots to timeout?

What's happening is that this patch (and previous ones) use up the limited RAM-based database space worse than any other patch ever has. And it brings down mysqld, taking the testbot offline (or resulting in the bogus "0 tests passed" pass)

Well, I'm glad. But I had to fix many broken testbots (like 5 yesterday) that all tested this issue as the last thing they did in life :-)

Few things are more motivating than a personal request by Drupal hero. :)

Here's an updated patch. For some reason, the tests never complete on my system. That's why I'm not setting this to "needs review", in order to save @rfay some hair-pulling. It would be awesome if someone could try this out locally.

While I was waiting for the tests to complete (in vain...), I actually tried this manually. I downloaded admin_menu (which has a 8.x release), and manually downloaded translations to /files/translations from localize.drupal.org. And.... *drumroll* ...it works! Enabled the module, and the file was imported. Pretty cool stuff. :)

Well let's try it anyway. It didn't fail yesterday. I'll be out this evening, but will try to look at it when I get back and see if we have just lots of broken testbots :-)

And the testbot did fine.

At least we've learned from this what some of those "0 tests passed" things mean. It seems to mean that mysqld croaked.

Re minor: Sure will do that.

Re major: The patch actually moves the .po file from the translations directory into the module directory of the test module (which is identical to the tests directory of locale.module), which is what the variable is set to (thanks to Git renames are not as visible as they used to be... :)). So that should be fine, actually, I think.

I simply edited the patch file, let's see if I get away with it... :)

One question, that came up when rolling this, but I forgot to post last time:
Since we set a variable in the test, should we unset/delete that variable in a tearDown() for encapsulation's sake? Wasn't really sure about that.

@brain #fail...

Let's try this one.

+++ b/includes/install.core.incundefined
@@ -1125,21 +1125,31 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+  // English does not need a translation file.
+  array_unshift($files, (object) array('name' => 'en'));

Is this still valid when English becomes a first class language? If not is that going to be fixed in the other patch once this is in?

+++ b/includes/install.core.incundefined
@@ -1125,21 +1125,31 @@ function install_select_profile_form($form, &$form_state, $profile_files) {
+ * Find installer translation files either for a specific langcode or generally.

s/generally/all available languages ?

+++ b/includes/install.core.incundefined
@@ -1480,23 +1488,21 @@ function install_configure_form($form, &$form_state, &$install_state) {
+ * @todo
+ *   This currently does the same as the first import step. Need to revisit

Any chance of an issue number here?

+++ b/includes/install.incundefined
@@ -1103,14 +1103,14 @@ function st($string, array $args = array(), array $options = array()) {
+      if (count($locale_files)) {

No need to use count() if we only need to check !empty().

+++ b/modules/locale/locale.bulk.incundefined
@@ -163,97 +163,41 @@ function locale_translate_export_po_form_submit($form, &$form_state) {
+ * Prepare a batch to run to import all translations.

This doesn't read right to me "a batch to run to import" - can we simplify?

-24 days to next Drupal core point release.

Will do. Assigning to me for now, so I (hopefully) don't forget.

Patch looks good to me now.

Committed/pushed to 8.x, thanks!

My toes are quite alright, thanks for finishing this up. :)
Sorry, I didn't come around to getting this done myself. Great to see it in, though. I hope I'll be able to redeem myself in one of the follow-ups. :)

2. The installer now looks for *install.$langcode.po files there to offer languages for selection. We don't have a standard for file naming of these and I'm fairly sure that install.$langcode.po is probably not going to be final, but we needed something interim.

Is there a followup issue for this somewhere?

I tried to install Drupal 8 using a non-English language today, and it didn't work at all. The on-screen instructions say nothing about files needing to follow that install.$langcode.po pattern; they just direct me to localize.drupal.org, where the files I can download don't match that pattern and therefore aren't recognized by the installer.

It seems like we need a critical bug tracking this issue to make sure it gets resolved one way or another?

Created #1392208: Impossible to install Drupal in a non-English language when following the provided instructions as the followup. Someone can close it if it turns out to be a duplicate.