As part of the l10n_update integration in core this issue adds the function to Download and import interface translations. See the UML in #1191488: META: Integrate l10n_update functionality in core for the big picture where this fits in.

Problem/Motivation

Creating the translations directory, downloading .po files, and importing them is a pain. Getting updated files meant going to redownload newer .po files.

Proposed resolution

Import .po files from d.o automatically when languages are added or modules or themes are enabled. Slick.
Add settings to control also getting automatic updates, and a way to manual check for updates.

This issue includes

  • Remote po translation files are downloaded and stored. Depending on the configuration the po files are stored temporary or permanent.
  • Translations files are parsed and imported into the database. The status of the translation history is updated.
  • The download and import process is executed as a batch process.
  • The two batch processes of "check status of available translations" and "Download and import" can be executed consecutively.
  • A configuration page for translation update settings ('admin/config/regional/translate/settings')
  • A URL to manually check the translation status ('admin/reports/translations/check')
  • A page with a button to update the translations ('admin/reports/translations'). Displaying the current interface translation status is handled in #1804702: Display interface translation status.
  • Tests for the above functions

To test this patch

This test uses a build-in test module to provide translations for two languages. This patch does not include a translation status page.

  1. Checkout the latest Drupal core code.
  2. Apply the latest patch from this issue.
  3. Locate the locale_test_translate module in the code (core/modules/locale/tests/modules/locale_test_translate/locale_test_translate.info) and remove the line "hidden = TRUE". This module brings its own translation. Unhiding it lets you enable it in the ui.
  4. Install the site.
  5. Enable the locale and locale_test_translate module (language module is a dependency)
  6. Test automatic import by: Add the Dutch language. The translations of the locale_test_translate module will now get imported. (Observe it works. Translation automatically imported message shows.)
  7. Add the Dutch language. The translations of the locale_test_translate module will now get imported.
  8. Observe it worked. The translations at admin/config/regional/translate show strings in Dutch (see it easy by changing "search in" to "only translated strings". Monday is an example string that is now translated.)
  9. Test the updating when a new translation .po file is available by adding or modifing strings in the Dutch or German language files at core/modules/locale/tests/test.*.po.
  10. Refresh the translation status by:
    • clicking on the settings tab, and then the check updates now link, or
    • clicking the "Check manually" link at admin/reports/translations
  11. Test updating by clicking "update" on the check page (from the previous step) Observe it works by seeing the message that X were added, Y were updated, Z were deleted.
  12. Observe it worked. The strings were added/updated/deleted from the translations by going to admin/config/regional/translate and looking for the modified or added strings.

Currently only few Drupal 8 translations are yet available, which makes this patch difficult to test in the wild. To test this patch in the wild you can:

  • Install Locale Tamper module
  • Install Drupal 8 modules for which translations are available at localize.drupal.org (e.g. Echo, Aloha Editor, Design Test, XHTML, HTML Mail). To avoid running into bugs in D8 modules, it is advised to strip the modules by removing all files except the .info and .module file and remove all code from the .module file. This leaves only the .info file and an empty .module file, which is all this patch needs to work with.

See the pages this patch has added:

  1. A settings page for translation update at: admin/config/regional/translate/settings
  2. Future translation status page at admin/reports/translations. Here you can click "Check manually" to update the translation status.
  3. (optional) Apply the patch in #1804702: Display interface translation status for translation status information at: admin/reports/translations.

Things to do

Now

Solve todo's in the code or create follow-up issues.
Modify state() variables to have namespace prefix (#13)
Drop the cache_locale table (#12)#1826294: Remove cache_locale
Improve and extend fault handling for download import process. (#14)
Modify the import process to only save the file status if the filename matches the l.d.o file name pattern. (#14)
Modify the import process to use the batch operation when an language is enabled. (#14)
Review the bulk.inc code for other batch download/import integration issues. e.g. when a module is enabled. see locale_system_update() (#14)
On the settings page: Add a manual update link to the Reports page. (#22)
On the settings page: Use different wording for 'remote' or explain. (#22)
Comments by attiks #41

User interface changes

See recent screenshots in #74 and #76

API changes

This summary of api changes is a work in progress. More detail will be added when making the change notice.
Detailed list of API changes.

Followup issues or architectural changes

#1842362: Replace locale_project table and improve caching Change table {locale_project} to state() variable. (#14)
#1842380: Convert $source object to a TranslatableProject class Convert $source-file to a typed class. (#13)
#1842380: Convert $source object to a TranslatableProject class Convert $source to a TranslatableProject class (#13)
#1842380: Convert $source object to a TranslatableProject class Simplify the way the $source data is handled within the batch operations (#14)
#1777106: Make check for out of date project update information more robust for sites that are not running cron regularly (follow-up)
#1834298: Unify name space in Locale module
#1787520: Translations not imported on installation
#1861360: Refactor localization update test so people can just enable the test module to test
#1861934: Rework help text for UI translations and importing po files
#1861930: Use "Drupal translations website" instead of Drupal translation server or Community translation server
Change the batch operation by handling one project/language a the time (#14)

CommentFileSizeAuthor
#125 locale-download-import-1804688-125.patch198.61 KBSutharsan
#125 interdiff-1804688-106-125.txt4.99 KBSutharsan
#116 debugpatch.txt1.11 KBGábor Hojtsy
#109 locale-download-import-1804688-106.patch196.81 KBSutharsan
#109 interdiff-1804688-101-106.txt2.89 KBSutharsan
#101 locale-download-import-1804688-101.patch196.64 KBSutharsan
#101 interdiff-1804688-100-101.txt819 bytesSutharsan
#100 locale-download-import-1804688-100.patch196.91 KBSutharsan
#100 interdiff-1804688-99-100.txt507 bytesSutharsan
#99 locale-download-import-1804688-99.patch196.77 KBYesCT
#99 interdiff-95-99.txt3.09 KBYesCT
#95 locale-download-import-1804688-92b-do-not-test.patch196.64 KBYesCT
#95 locale-download-import-1804688-95.patch196.63 KBYesCT
#92 locale-download-import-1804688-92.patch196.64 KBJose Reyero
#92 locale-download-import-1804688-92-diff.txt2.65 KBJose Reyero
#88 locale-download-import-1804688-88.patch197.96 KBGábor Hojtsy
#85 locale-download-import-1804688-85.patch198.39 KBSutharsan
#85 interdiff-1804688-79-85.txt9.84 KBSutharsan
#83 blank-page.png22.89 KBwebchick
#82 locale-api-changes-1804688-annotated.txt2.88 KBSutharsan
#81 locale-api-changes-1804688.txt3.06 KBSutharsan
#79 locale-download-import-1804688-79.patch193.91 KBYesCT
#79 interdiff-76-79.txt2.18 KBYesCT
#76 help-s01-2012-11-26_0505.png138.61 KBYesCT
#76 help-s02-2012-11-26_0507.png96.76 KBYesCT
#76 locale-download-import-1804688-76.patch193.9 KBYesCT
#76 interdiff-73-76.txt3.91 KBYesCT
#74 di-wui-w01-Languages | localhost 2012-11-26 03-28-02.png176.91 KBYesCT
#74 di-wui-w02-german-2012-11-26_0330.png73.84 KBYesCT
#74 di-wui-w03-settings-2012-11-26_0331.png98.8 KBYesCT
#74 di-wui-settings-page-2012-11-26_0333.png122.59 KBYesCT
#74 di-wui-w04-Locale Tamper translation updates | localhost 2012-11-26 03-38-17.png112.2 KBYesCT
#73 locale-download-import-1804688-73.patch190.17 KBSutharsan
#73 interdiff-1804688-60-73.txt7.68 KBSutharsan
#67 ld-s01-enable-locale-2012-11-25_0238.png63.03 KBYesCT
#67 di-s02-config-languages-2012-11-25_0242.png99.08 KBYesCT
#67 di-s02b-import-help-text-2012-11-25_0305.png126.13 KBYesCT
#67 di-s03-add-spanish-2012-11-25_0245.png112.33 KBYesCT
#67 di-s04-add-german-2012-11-25_0249.png128.31 KBYesCT
#67 di-s05-8of8-2012-11-25_0249.png91.86 KBYesCT
#67 di-s06a-2012-11-25_0324.png86.45 KBYesCT
#67 di-s06b-it_worked_and_error_Languages | locale-download-import-1804688-60 2012-11-25 03-29-47.png181.67 KBYesCT
#67 di-s07-2012-11-25_0337.png90.52 KBYesCT
#67 di-s08-2012-11-25_0337.png40.91 KBYesCT
#67 di-s09-tamper-report-2012-11-25_0338.png91.16 KBYesCT
#67 di-s10-config-ui-trans-had-settings-tab-2012-11-25_0346.png103.2 KBYesCT
#67 di-s11-settings-tab-User interface translation | locale-download-import-1804688-60 2012-11-25 03-49-43.png74.07 KBYesCT
#67 locale-download-s11-link_to-admin_reports_translations_check-2012-11-25_0037.png122.93 KBYesCT
#67 locale-download-s12-manual-check-2012-11-25_0039.png47.48 KBYesCT
#64 d8-s01-plan-2012-11-25_0050.png93.35 KBYesCT
#64 d8-s02-languages-2012-11-25_0054.png123.53 KBYesCT
#64 d8-s03-translation-list-2012-11-25_0056.png79.52 KBYesCT
#64 d8-s04-download-2012-11-25_0057.png100.44 KBYesCT
#64 d8-s05-try-translated-link-2012-11-25_0057.png89.91 KBYesCT
#64 d8-s06-translated-try-import-tab-2012-11-25_0059.png114.17 KBYesCT
#64 d8-s07-import-tab-2012-11-25_0100.png150.97 KBYesCT
#64 d8-s08-error-2012-11-25_0102.png182.09 KBYesCT
#64 d8-s09-mkdir-translations-2012-11-25_0104.png53.5 KBYesCT
#64 d8-s10-import-after-mkdir-2012-11-25_0105.png114.79 KBYesCT
#64 d8-s11-importing-2012-11-25_0105.png54.22 KBYesCT
#64 d8-s12-import-worked-2012-11-25_0107.png113.61 KBYesCT
#64 d8-s13-no_translate_settings-2012-11-25_0109.png110.04 KBYesCT
#64 d8-s14-admin_report-2012-11-25_0111.png88.09 KBYesCT
#64 d8-s15-admin_reports_translations-2012-11-25_0112.png47.07 KBYesCT
#64 d8-s16-check-page-2012-11-25_0114.png67.71 KBYesCT
#60 locale-download-import-1804688-60.patch189.76 KBSutharsan
#60 interdiff-1804688-54-60.txt900 bytesSutharsan
#58 locale-download-import-1804688-57.patch189.55 KBSutharsan
#54 locale-download-import-1804688-54.patch189.33 KBSutharsan
#54 interdiff-1804688-50-54.txt5.63 KBSutharsan
#50 locale-download-import-1804688-50.patch188.56 KBSutharsan
#50 interdiff-1804688-44-50.txt65.74 KBSutharsan
#49 UITranslation.jpg69.89 KBGábor Hojtsy
#44 locale-download-import-1804688-44.patch184.39 KBSutharsan
#44 interdiff-1804688-40-44.txt109.38 KBSutharsan
#40 locale-download-import-1804688-40.patch136.73 KBSutharsan
#40 interdiff-1804688-29-40.txt81.22 KBSutharsan
#35 locale_tamper.info159 bytesclemens.tolboom
#35 locale_tamper.module.txt8.64 KBclemens.tolboom
#29 Available translation updates | drupal.d8.png34.16 KBclemens.tolboom
#29 locale-download-import-1804688-18-29-interdiff.txt946 bytesclemens.tolboom
#29 locale-download-import-1804688-29.patch112.83 KBclemens.tolboom
#22 User interface translation | local.drupal 02.11.12 14:41-6.jpeg1.34 MBSchnitzel
#22 Error | local.drupal 02.11.12 13:29-4.jpeg952.88 KBSchnitzel
#22 Available translation updates | local.drupal 02.11.12 14:45.jpeg1.1 MBSchnitzel
#18 locale-download-import-1804688-18.patch112.62 KBwebflo
#18 locale-download-import-1804688-18.interdiff.txt4.12 KBwebflo
#17 locale-download-import-1804688-17.patch111.82 KBSutharsan
#17 interdiff-1804688-10-17.txt57.99 KBSutharsan
#10 interdiff-1804688-2-10.txt64.32 KBSutharsan
#10 locale-download-import-1804688-10.patch64.86 KBSutharsan
#9 locale-download-import-1804688-9.patch58.33 KBSutharsan
#9 interdiff-1804688-2-9.txt56.42 KBSutharsan
#2 locale-download-import-1804688-2.patch40.56 KBSutharsan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +sprint
Sutharsan’s picture

Status: Active » Needs work
FileSize
40.56 KB

This patch is for evaluation and discussion. Still plenty to do ;)

Some questions/remarks:
* I needed t0o make changes to the gettext import batch operation locale_translate_batch_import() in locale.bulk.inc to make it possible to re-use it for the download-import (fetch) batch. Data processing and data storing were moved out of the function.
* locale_translate_batch_import_save() now saves the file status upon manual import, but do we really need to? As far as I remember no translation status was stored upon manual import in D7 (this was introduced by l10n_update) and it possibly collides with the automatic import. I propose to only save the file status (and update the translations status with it) if the filename matches the l.d.o filename pattern. In that case the file was probably manually downloaded from l.d.o and manually uploaded. OR we don't make assumptions and don't update any translation status upon manual import. Just because we can't control.

Berdir’s picture

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseReader.phpundefined
@@ -12,6 +12,7 @@
 use Drupal\locale\TranslationString;
 use PDO;
+use Exception;
 
 /**
  * Gettext PO reader working with the locale module database.
diff --git a/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.php b/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.php

@@ -13,6 +13,7 @@
 use Drupal\Component\Gettext\PoWriterInterface;
 use Drupal\locale\SourceString;
 use Drupal\locale\TranslationString;
+use Exception;

According to the new coding standard, global classes should be used by using \ClassName, instead of a use statement.

Haven't really looked beyond that yet, lots of @todo's :) Just wanted to mention this since you might not be aware of it.

Sutharsan’s picture

@ berdir, I was not aware of it. Only copied what was already there. Document of this new standard is found at http://drupal.org/node/1353118

Gábor Hojtsy’s picture

Reviewed the code, and I see you moved the preg matching into the file processing batch setup and the status update to the general import. This indeed introduces both functionality for even manual file imports. I'm not sure that is necessarily a wrong thing. If I manually download a file and import it through the UI, it would be nice if it participates in the file tracking, so it is not (unnecessarily downloaded and imported again). What kind of drawbacks are you seeing there?

Gábor Hojtsy’s picture

Priority: Normal » Major

Elevating to major.

Sutharsan’s picture

Moving the preg_match() upstream is not strictly required. It is more a thing of separating functionality and which data to feed into the batch import operation. For the download (and import) batch I rely more and more on having all file data in one object. And since the different batch functions will be integrated, for cleanness of code, I want the batch functions to use the same input and output data structure as much a possible. For this reason I want the file's langcode, and secundary it's project name and version to be in the same file object. These are all properties of the same file.
As a todo states, the preg_match() could even be moved further upstream to the import form submit function.

I'm not sure if we need to continue file tracking for all of the manual imported po files. It is actually not 'file tracking' it is tracking the status of imported strings. It stores when and from which source is a set of translations (i.e. a po file) was imported. That is what is stored, but why do we store this data? For automatically imported files, and manually imported l.d.o files we need to know the status of the imported translations. To descide whether or not we should import a new po file. For manual imports whe might present the user with a history of imported files. If we take manual import seriously we need a description per file too. But once the automated import is in place I see only few usecases left for manual import: 1. When using an external translation tool. 2. For sites that use translation staging or deployment but can not write a module with one hook_locale_translation_projects_alter() 3. For cases where the custom po files can not be stored in the file system. But the real problem comes from combining manual and automatic import. Automatic import requires a unique project-langcode pair. The current code already uses on the file name for the language code. But we now also need to extract a project name. We could 1. add a project name input field to the form, 2. fall-back to use the full filename as project name or 3. don't store the history of files if they don't match the {project name}-{version}.{langcode}.po pattern. I'd opt for the last.

As a side note, I created an issue to support deployment of customized translations: #1820542: Make customized translations deployable.

Gábor Hojtsy’s picture

Yeah, I agree we should not keep history if the file name is not in the recognised pattern, that would just litter our DB with improper data. Let's just store history for files with the recognised file name format! I responded on the custom translation issue (the title shocked me).

Sutharsan’s picture

Included:

  • #3 Coding standard for global classes. But only in the files I worked with, did not go after the remainder of locale module.
  • Working batch for download and import. See locale_translation_batch_fetch_build().
  • Temporary translation status display at admin/reports/translations.
  • Moved the batch builder function locale_translation_batch_status_build() from locale.batch.in to locale.compare.inc.
  • Extended the data structure of the $source object (see locale_translation_source_build()).
  • Removed l10n_* function which are not yet migrated.

To do:

  • Test scripts
  • Store translation state only if the file matches the l.d.o pattern. #7, #8
  • Documentation
  • Remaining todos in the code.
Sutharsan’s picture

Status: Needs work » Needs review
FileSize
64.86 KB
64.32 KB

Merging with changes in the 8.x branche introduces some undesired changes. This is the patch as it was intended.

Status: Needs review » Needs work

The last submitted patch, locale-download-import-1804688-10.patch, failed testing.

Berdir’s picture

Was the cache_locale table added for this issue? If so, then this can already move to state()/keyvaluestore, right and we can remove it again?

#1826294: Remove cache_locale

Berdir’s picture

+++ b/core/modules/locale/locale.fetch.inc
@@ -0,0 +1,96 @@
+  if ($uri = system_retrieve_file($source_file->uri, 'temporary://')) {
+    $file = new stdClass();
+    $file->project = $source_file->project;
+    $file->langcode = $source_file->langcode;
+    $file->version = $source_file->version;
+    $file->type = 'download';
+    $file->uri = $uri;
+    $file->filename = $source_file->filename;
+    return $file;
+  }

Can we use a typed class for this? There also seem to be some other properties used through the code and having a class would allow us to document it at a single place.

+++ b/core/modules/locale/locale.module
@@ -745,6 +751,83 @@ function locale_preprocess_node(&$variables) {
+
+  state()->set('locale_translation_status', $status);
+  state()->set('locale_translation_status_last_update', REQUEST_TIME);
+}

You're just moving it I think, but state() variables should be namespaces by the module name, so just use locale.translation_status and so on.

Jose Reyero’s picture

I've been giving a try to this patch and it seems to work, I think it looks pretty well overall. Some notes:

- Besides the state() namespace issues mentioned by @Berdir #13, the whole thing should have some more consistent naming, for which I suggest locale_update_* and maybe moving all the libraries to a single file (there are too many includes everywhere). This includes variables too that could be locale.update.* Constant names are really long too, like LOCALE_TRANSLATION_USE_SOURCE_REMOTE_AND_LOCAL, can we find better names?

- For easier development and testing, I think a minimal UI should be integrated in the patch. I've posted some code here, that will add some UI (for update pages and settings), we could merge it (?) see #1804702: Display interface translation status

- For the project list, I don't think we need a table, and we may be better off using a cached array, like update module does. Also with human readable project names. About the integration with update we should clean up this part, maybe moving the project list to system module (?) so it can be shared by both without further dependencies (I am still not convinced that this should require update module to work).

- The 'locale_translation_status' array is really too complex and very difficult to handle, we should try to simplify this. Maybe as @Berdir suggests adding a typed class but also we don't need to stick all the file data in the status array and we can grab it from the db just when we need it for each project.

- The integration with locale install (import files) and upload/import is not very clean to me. I am seeing an entry in the locale_file table with langcode 'es_', file translations://drupal-8.0.es_.po (which is a file I've uploaded with es.po name). Can we do something about this renaming or maybe not merging uploaded files here? The problem is these may have a name non consistent with project-version.langcode.po

- I see no support for per-project different loalization servers which is important to support distributions in the future. Also the l10n_server data could be simplified as all we need is this URL: http://ftp.drupal.org/files/translations/l10n_server.xml And from it we can fetch file name patterns, etc..

@Gábor Hojtsy, can we add the drupal 8.0 package to the LDO server? I've had to install some 8.x modules to see some available updates. Here's a list of the available ones for testing, http://ftp.drupal.org/files/translations/8.x/

- The 'remote_and_local' option really makes things too complex, I think we could drop it in favor of only two simpler ones: 'remote' and 'local' (And maybe we don't even need these ones as the file name pattern could already handle these cases). This is also consistent with 'update' module which doesn't provide that many options.
Note: the whole idea of this in l10n_server was being able to download only once and reuse it for multisite installs but maybe we don't need to get into these complexities here.

- Can we simplify batch parameters?, we don't need to use $source always, it may be enough with (project, langcode) or we may create a batch to simply import last downloaded file. This will make batches more flexible and allow us to chain refresh + download + import batches, see @todo in locale_translation_status_form_submit() in the patch.
Also we should mark on the file table whether the file has been imported or not.

Other minor issues:

- Function name?
- locale_translate_update_file_history($file);
+ locale_translate_updation_file_history($file);

- Move into the configuration system?
+ if ($import_type == 'download' && variable_get('locale_translate_file_directory', conf_path() . '/file
+ // Move the temporary file to the local translations directory.

- More 'watchdog' for download / import errors.

- The 'locale_translate_*' namespace is bit too crowded and makes functions spread around and difficult to find, move to locale_update_* or similar.

- Finally if we need to make this dependent on update module, we may consider adding one more module (locale_update). The other option may be to make it independent of that module, which I think is better.

Sutharsan’s picture

Re. Berdir's comment in #13

Was the cache_locale table added for this issue?

It's been a long time since this discussion, but I think we can drop it now in favor of the using state().

Can we use a typed class for this? (source file objects)

Yes, we can. I've been thinking about this. But postponed refactoring the code untill I've completed the test scripts. Which will be next.

variables should be namespaces by the module name

I'll take care of that.

Re. Jose Reyero's comments in #14

more consistent naming ... locale_update_*

At the beginning of the hole l10n_update migration we've chosen to use 'locale_translation' as namespace. At that time there were also plans to create a complete new module out of this, we that has been postponed (no issue was created). I don't think it this is a good moment to change the naming. I also saw that the batch import code used 'locale_translate'. This adds to the confusion we should fix some time later.

moving all the libraries to a single file

Agree. This is work in progress. I guess this is my way of working, to get woking code first and decide about where to place the functions later. But there is certainly room for improvement.

I think a minimal UI should be integrated in the patch.

I was working on that too as I needed it for development. I'll share my code in #1804702: Display interface translation status too.

For the project list, I don't think we need a table

I don't understand why? And yes, we need the human readable project name. There is no API for that and it a hassle to load it when we display the project status and it is easy to fetch it when loading the project data initially.

The 'locale_translation_status' array is really too complex
The main purpose of the array is to store the translation status of remote and local translation sources after the batch operation. It also contains data from the locale_project table as the initial data in the batch operation. The current translation status is stored there for convenience, but it duplicates the data in the locale_files table.

The integration with locale install (import files) and upload/import is not very clean to me.

I've descrive my thoughts on the integration of manual and automatic file import in #7 I propose to not keep track of manually imported files unless they match the l.d.o. file name pattern.

I see no support for per-project different loalization servers

Custom modules can provide a 'interface translation project' and 'interface translation server pattern' in their .info file and further the hook_locale_translation_projects_alter() provides this option. Collecting the server configuration was dropped in #1627006: Collect project information for translation update. You can find discussion starting at: http://drupal.org/node/1627006#comment-6363698

The 'remote_and_local' option really makes things too complex

The choice for the current 'remote and local' and 'local' is described here: http://drupal.org/node/1742894#comment-6566580 We do need the possibility to load local po files of custom modules even when remote is the main source. If we drop the ability to locally save the remote translations the code would be simplified and no option should be presented to the user. It should be investigated if or how a contrib module could provide this option. I suggest we make a follow up issue for this.

Can we simplify batch parameters? ... it may be enough with (project, langcode)

Yes, that's exactly my plan. The two separate batches 'check update status' and 'import and download' need to be combined in one. That requires to revamp the batch build and the batch operation function parameters. The best option is to use an arrays of projects and langcodes.

Finally if we need to make this dependent on update module

For previous discussion on Update module dependency see: http://drupal.org/node/1627006#comment-6107816

Gábor Hojtsy’s picture

Issue tags: +feature freeze

Add feature freeze tag.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
57.99 KB
111.82 KB

Patch updated with:

  • Working test scripts for download and import.
  • Various small bug that where caught by the test scripts.
  • Add a configuration form at 'admin/config/regional/translate/settings'
webflo’s picture

Extracted the project, version and langcode parsing from locale_translate_batch_build()

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

The last submitted patch, locale-download-import-1804688-18.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

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

The last submitted patch, locale-download-import-1804688-18.patch, failed testing.

Schnitzel’s picture

Installed a fresh Drupal with the patch and added some languages. I

I found the Translation Update Settings, but had some issues there to then actually found a way where I can manually download the translations. And also it is not fully clear what "remote" means.
41-6.jpeg

Then I found the Reports page, but it looked a bit empty and had an Error:
29-4.jpeg

then I clicked "update manually" and it updated somehow:
45.jpeg

Probably the reason why no translation updates are show is because we don't have yet translations on the l.d.o servers? Is there any whay how to test this?

webflo’s picture

Status: Needs work » Needs review
Schnitzel’s picture

Status: Needs review » Needs work

mhh just found out, that there is a separate Issue for the UI and also a separate patch here: #1804702: Display interface translation status, not sure if the UI Change should be even in the patch in this issue?

Gábor Hojtsy’s picture

Good advice for testing:

Sutharsan: balintk: if you paste this code http://pastebin.com/ReeF4GpN in a mode (test) the site you will be able to work with real D7 translations being downloaded from l.d.o

Jose Reyero’s picture

I see very good notes on #22 about why this current UI is not usable at all.

I think we need a really simple one (but working 100%) to be able to move on with this patch. I'd suggest: single big 'update' button, (or select by language at most)

Sutharsan’s picture

The Update Status interface (#22 2nd and 3rd image) in this patch can be ignored completely. It was only added for development purpose. Let's continue the UI discussion in #1804702: Display interface translation status.

Sutharsan’s picture

Issue summary: View changes

Link to #1191488 restored

clemens.tolboom’s picture

(tiny observation)
Weird the UI is split from this as core behavior on page http://drupal.d8/admin/reports/translations is now empty.

Available translation updates
  Add or remove shortcut

  TODO: Show the translation status here

switching to the patch (branch) I can run update.php then it fails miserably.

Additional uncaught exception thrown while handling exception.
Original

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Users/clemens/Sites/drupal/d8/www/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Additional

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Users/clemens/Sites/drupal/d8/www/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

(this is probably a core problem)

[edit]
Doing a drush --yes @drupal.d8 sql-drop and a manual install is working (of course)

clemens.tolboom’s picture

Hmmm ... I don't get it. My notes so far

Shouldn't the admin pages reside in locale.admin.inc?

Why are local files not done by batch? Suppose we have 120 modules installed. What happens then?

Notice: Undefined index: sources in locale_translation_batch_status_finished() (line 151 of core/modules/locale/locale.batch.inc).

No projects found: I have core + devel:
- $projects = locale_translation_get_projects();

Projects are zapped then checked?!?
- locale_translation_flush_projects();
- $projects = locale_translation_get_projects();
- locale_translation_check_projects($projects);

Clicking on 'Check manually' admin/reports/translations/check on admin/reports/translations does not give me translated Drupal (dutch)
- This is bad thought as it checks ? the current installed translations

Clicking update in admin/reports/translations I get Projects field required.
- wtf?

Attached a small patch

[edit]added indentation '-'[/edit]

Status: Needs review » Needs work

The last submitted patch, locale-download-import-1804688-29.patch, failed testing.

Sutharsan’s picture

Shouldn't the admin pages reside in locale.admin.inc?

Locale module has no .admin.inc (yet) only a .pages.inc. Which contains all is current configuration pages too.

Why are local files not done by batch?

I guess you refer to locale_translation_check_projects_local(). This is the function that checks the status of local files. It only requires to access the file system, I expect it to scale beyond 120 modules. In _locale_translation_fetch_operations() you can see the batch function being called for importing local files.

No projects found: I have core + devel:

I can't explain why you have no projects. It should at least detect existence of the projects. But note that the admin-interface that is (still) in this patch is not hardened for zero projects, zero languages, and other exceptional behaviours.

Projects are zapped then checked??

It is clearing the project cache. This is probably not needed for the actual code, but very handy for development. Adding a 'todo'.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -148,13 +148,16 @@ function locale_translation_batch_status_compare(&$context) {
+    else {
+      drupal_set_message(t('Nothing to check.'));
+    }

[edit]As Jose already noted: "- More 'watchdog' for download / import errors."

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

I've updated the summary to make clear what happens when update module is not available.

clemens.tolboom’s picture

Sorry guys but the on boarding for this issue is way to hard for me. I spend another 2 hours to find out my user errors : enable update module and trying to fetch 8.x translations. (debugging takes some time :-/)

What I don't understand is why we do not have a core 8.x on l.d.o (that would help a lot). It took me a while to understand the pastebin stuff :-/

Regarding update.module:

grep -r "update" * | grep module_exists
locale.compare.inc:    if ($result->rowCount() == 0 && module_exists('update')) {
locale.compare.inc:  if (!module_exists('update')) {
locale.compare.inc:  if (!module_exists('update')) {

These checks for update.module should have never made it into code. That is the menu items should not come into existence unless update.module is enabled.

I'll try to update the issues summary to reflect these remarks.

clemens.tolboom’s picture

Issue summary: View changes

Ordered the steps and added a few

clemens.tolboom’s picture

Darn. I still don't get the pastebin.

I tried to tamper with files/config_../active/locale.settings.yml to let it select 7.x-1.x versions which let it test for devel-7.x-1.x but that never made it to the overview page.

Anyway: tiny update of the summary

clemens.tolboom’s picture

FileSize
8.64 KB
159 bytes

@penyaskito @berdir : tnx for the quick feedback on IRC :-)

I've added the tamper code in here so we don't loose the pastebin and have a true module.

I'll update the summary to refer to both pastebin and #35

clemens.tolboom’s picture

Issue summary: View changes

Made required strong :-/

clemens.tolboom’s picture

Both variables from #35

  $remote_url = 'http://drupal8.rhum/translations_remote/';
  $local_url = 'translations/';

are not used. So I don't get this to work :-(

function locale_tamper_locale_translation_projects_alter() is filling admin/reports/translations so that hurdle is taken :-)

Gábor Hojtsy’s picture

@clemens.tolboom: I did not personally try yet, but observed Schnitzel trying it out. As long as the download source is set to not just local stuff, it worked flawlessly with the pastebin. He suspected for some reason the local file setup was default, which did make it not work out of the box. He thought he did not set that but it was pre-existing somehow.

Schnitzel’s picture

is filling admin/reports/translations so that hurdle is taken :-)

yes, and now go to "admin/reports/translations" and there you can update the translations and they will be downloaded :)

clemens.tolboom’s picture

@Schnitzel: unfortunately I did not get any downloads :-(

Can you please update the issue summary adding all steps needed to make this work? Esp. the remarks from @Gábor Hojtsy in #37

As long as the download source is set to not just local stuff, it worked flawlessly with the pastebin.

and

He suspected for some reason the local file setup was default, which did make it not work out of the box.

and

He thought he did not set that but it was pre-existing somehow.

Thanks.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
81.22 KB
136.73 KB

This patch:

  • Removes the development UI in favour of #1804702: Display interface translation status. The page callbacks are still in this patch as they are required for testing.
  • Supports per project and per language import. Relevant functions can be called with use $projects and $languages arrays. This also changes the batch parameters.
  • Adds more documentation.
  • Moves functions used in both compare.inc, fetch.inc and batch.inc a common API file: locale.translation.inc. This name change later.

The remaining thing to do are added to issue summary.

Sutharsan’s picture

Issue summary: View changes

Added link to #35

attiks’s picture

I did a dreditor review only, the code looks good, but there are a a lot of @todo's in there, can you create new issues if they will be handled in follow ups?

There are also some minor comments issues, but we can fix those in a follow-up.

+++ b/core/modules/locale/lib/Drupal/locale/Gettext.phpundefined
@@ -53,10 +53,11 @@ static function filesToArray($langcode, array $files) {
+   *   @todo: Describe Required parts of the file object.
+   *   - 'langcode': The language code.

is the todo still needed? If so can you create a follow-up issue and link to it.

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.phpundefined
@@ -160,14 +160,14 @@ function setHeader(PoHeader $header) {
+      throw new \Exception("Options should be set before assigning a PoHeader.");
...
+      throw new \Exception("Langcode should be set before assigning a PoHeader.");

use ' instead of "

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -85,11 +85,11 @@ function mockImportedPoFile($langcode, $timestamp_difference = 0) {
+      locale_translation_update_file_history($file);

locale_translate_update_file_history? not locale_translation.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+   * Timestamp for a translation file or existing translations.
...
+   * Timestamp for a translation file or existing translations.
...
+   * Timestamp for a translation file or existing translations.
...
+   * Timestamp for a translation file or existing translations.

comments are the same for all 4 variables

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+    // Convert array of translations to Gettext source an translation strings.

an --> and

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+   * Setup existing translations in the database and setup the status of existing translations.

too long

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+    // Add non customized translations to the database.
+    $customized_translations = array(

comments says non customized, code says customized.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+    // Get status of translation sources at both local and remote the locations.

... remote locations.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,638 @@
+    // The static cache needs to be flushed firt to get the most recent data

firt --> first

+++ b/core/modules/locale/locale.batch.incundefined
@@ -6,40 +6,9 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

can we move this into the functions that need this functionality.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -62,16 +31,16 @@ function locale_translation_batch_status_fetch_remote($source, &$context) {
+    // store the resulting uri.

store --> Store

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +169,270 @@ function locale_translation_batch_status_finished($success, $results) {
+        // @todo Error handling when download fails.

is this for a follow-up?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +169,270 @@ function locale_translation_batch_status_finished($success, $results) {
+ * Batch process: Import translation file..

.. --> .

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +169,270 @@ function locale_translation_batch_status_finished($success, $results) {
+ *   Array of import options. See locale_translate_batch_import_files().

change the See ... to a real @see

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +169,270 @@ function locale_translation_batch_status_finished($success, $results) {
+      // @todo Fault handling. Using try/catch? For other batch functions too?

follow-up issue

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +169,270 @@ function locale_translation_batch_status_finished($success, $results) {
+          // @todo Leave unset() disabled for debugging.
+          //unset($source->files['import']);

can you remove the comment so the test are using the non-debug code.

+++ b/core/modules/locale/locale.compare.incundefined
@@ -5,81 +5,12 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

can we move this into the functions.

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,111 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

dito

+++ b/core/modules/locale/locale.pages.incundefined
@@ -467,19 +467,183 @@ function locale_translation_manual_status() {
+/**
+ * @todo
+ */

function description.

Status: Needs review » Needs work

The last submitted patch, locale-download-import-1804688-40.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Added 'things to do'

Sutharsan’s picture

Issue summary: View changes

Added todo details

Sutharsan’s picture

Issue summary: View changes

Updated the how to test secgtion.

Sutharsan’s picture

In IRC I discussed with Gabor some of the outstanding comments above.

Consider changing namespace from locale_translation and locale_translate to locale_update. Consider creating a new module. (#14)
* A follow-up issue has been created to unify the name space. Creating a separate module module is not supported by Gabor as a separate module may be overlooked which harms the experience when enabling new modules and languages. #1834298: Unify name space in Locale module

Consider removing the 'local'/'remote and local' choice from the interface and use 'remote and local' only. Consider removing it from the code too. (#14)
* The 'local' option is required for multi sites and is important for deployment of translations in a staging / production environment. Removing it is not supported by Gabor.

Consider making the translate process independent of update module or create a locale_update module. (#14)
* Jose Reyero already started working on this at #1832946: Runtime translation download fallback works different from installer translation download fallback.

Sutharsan’s picture

Issue summary: View changes

Update 'how to test' section.

Sutharsan’s picture

Issue summary: View changes

'Needs discussion' items removed

Sutharsan’s picture

Issue summary: View changes

Progress marked in todo list

Sutharsan’s picture

Issue summary: View changes

Progress marked in todo

Sutharsan’s picture

Issue summary: View changes

Updated 'Things to do'.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
109.38 KB
184.39 KB

Locale module now automatically imports translations when languages are added and modules are enabled!
Included:

  • Solve todo's in the code or create follow-up issues.
  • Modify state() variables to have namespace prefix (#13)
  • Drop the cache_locale table (#12)#1826294: Remove cache_locale
  • Improve and extend fault handling for download import process. (#14)
  • Modify the import process to only save the file status if the filename matches the l.d.o file name pattern. (#14)
  • Modify the import process to use the batch operation when an language is enabled. (#14)
  • Review the bulk.inc code for other batch download/import integration issues. e.g. when a module is enabled. see locale_system_update() (#14)
  • On the settings page: Add a manual update link to the Reports page. (#22)
  • On the settings page: Use different wording for 'remote' or explain. (#22)
  • Comments by attiks (#41)
  • Test file LocaleFileImportStatus.php removed after changing the behaviour when enabling an language. Remaining test integrated in LocaleUpdateTest.php
  • Made the translation:// streamwrapper hidden

Status: Needs review » Needs work

The last submitted patch, locale-download-import-1804688-44.patch, failed testing.

attiks’s picture

Most of this is nitpicking, so it is a bit long ;-)

+++ b/core/modules/locale/lib/Drupal/locale/Gettext.phpundefined
@@ -53,10 +53,10 @@ static function filesToArray($langcode, array $files) {
+   *   - "langcode": The language the language the strings will be added to.

The language the language
double?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+ * Definition of Drupal\locale\Tests\LocaleCompareTest.
...
+class LocaleUpdateTest extends WebTestBase {

comment != class name

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Set the value of the default translations directory.

Set --> Sets

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Add a language.

Add -> Adds

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * @param $langcode

string missing

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+        $text .= 'msgid "'. $source . '"' . "\n";
+        $text .= 'msgstr "'. $target . '"' . "\n";

What if either $source or $target contains a " (double quote)

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Setup the environment containting local and remote translation files.

Sets up

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Setup existing translations in the database and setup the status of

Sets up ... sets up ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    $customized = 0;
...
+        'customized' => $customized,

This is only used once, and should probably be FALSE.
Better to put it inline.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    $customized = 1;
...
+        'customized' => $customized,

This is only used once, and should probably be TRUE. Better to put it inline.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Check the translation of an array of strings.
+   *
+   * @param string $source
+   *   Translation source string
+   * @param string $translation
+   *   Translation to check. Use empty string to check for a not existing
...
+    $this->assertEqual((string) $translation, (string) $db_translation, $message ? $message : format_string('Correct translation of %source (%language)', array('%source' => $source, '%language' => $langcode)));

Checks

Comments talks about an array, but @param is a string. Assert casts is to string.

If this is needed - am I missing something - this should probably need a comment to clarify why.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Check if a list of translatable projects gets build.

Checks

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Check if a list of translatable projects can include hidden projects.

Checks

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * fixed file names and release versions is used. Using a
+   * hook_locale_translation_projects_alter implementation in the locale_test
+   * module this custom project definition is applied.

This custom project definition is applied using a hook_locale_translation_projects_alter implementation in the locale_test module.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * for local files only, check for remote files only, check for both local and
+   * remote files.

'remote only' isn't tested by this function

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    $edit = array(
+      'use_source' => LOCALE_TRANSLATION_USE_SOURCE_LOCAL,
+    );
+    $this->drupalPost('admin/config/regional/translate/settings', $edit, t('Save configuration'));
+
+    // Get status of translation sources at local file system.
+    $config->set('translation.use_source', LOCALE_TRANSLATION_USE_SOURCE_LOCAL)->save();

Isn't the config set twice?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test translation import from remote sources.

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test translation import from local sources.

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // The static cache needs to be flushed firt to get the most recent data

firt --> first

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test translation import without a translations directory.

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // The static cache needs to be flushed firt to get the most recent data

firt --> first

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test translation import with a translations directory and only overwrite

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test translation import with a translations directory and don't overwrite

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test automatic translation import when a module is enabled.

Tests

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Check is there is no translation yet.

Check if there is ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * Test automatic translation import when a langauge is enabled.

Tests ... language ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+   * will remove all translations of that langague from the database.

langague --> language

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Check is there is no Dutch translation yet.

Check if ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Add a language

missing .

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Remove a language

missing .

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Check is there is no more Duth translation.

Check if ...

Reworded sentence: "Check that the Dutch translation is gone."

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+/*
+   function testAutomaticModuleTranslationImportLanguageEnable() {
+    // Enable a module.
+    $edit = array(

This function is commented out, why?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // The English name for the language.

for --> of

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Ensure the translation file was automatically imported when language was
+    // added.

... when the language ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Ensure strings were successfully imported.
+    $search = array(

Ensure the ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+    // Ensure multiline string was imported.

Ensure the ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
\ No newline at end of file

Missing new line

+++ b/core/modules/locale/locale.batch.incundefined
@@ -6,40 +6,9 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -127,27 +107,51 @@ function locale_translation_batch_status_fetch_local($sources, &$context) {
+        // The available translation files are compare and data of the most recent

... compared ...

comment is > 80c

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * Load translation source data for the projects to be updated.

Loads

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * @see locale_translation_batch_fetch_sources().
+ * @see locale_translation_batch_fetch_import().
+ * @see locale_translation_batch_fetch_update_status().
+ * @see locale_translation_batch_status_compare().

no period as the end

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * This batch operations imports either a local gettext file or a downloaded

This batch operation ...

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * @see locale_translate_batch_import_files().
+ * @see locale_translation_batch_fetch_sources().
+ * @see locale_translation_batch_fetch_download().
+ * @see locale_translation_batch_fetch_update_status().
+ * @see locale_translation_batch_status_compare().

no period at the end

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+      if ($source->type == 'remote' || $source->type == 'local') {

Don't we have a constant for this?

Can type be something else?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+        // If we are working on a remote file we will import the downloaded file and
+        // use the result data from the

Comment too long and missing a part

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+            // Keep the data of imported source. In following batch operations

Keep the data of imported source. In following batch operation

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+            // succesfull import, the files are moved to the local destination

destination --> translations

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+            if ($import_type == 'download') {
+              // The temporary file is now imported and will not be used further.
+              // Store the timestamp and delete the file.
+              $timestamp = filemtime($source_result->files[$import_type]->uri);

So the file is downloaded to /tmp, moved to translations and then deleted? If so, why the need to move it?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+              // Import type is 'local'.

This comment is a bit redundant, the code is clear.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * @see locale_translation_batch_fetch_sources().
+ * @see locale_translation_batch_fetch_download().
+ * @see locale_translation_batch_fetch_import().
+ * @see locale_translation_batch_status_compare().

No periods

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+        // During the batch execution data of imported files is temporary stored

... the data of the imported ...

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+        // in $context['results']['sources']. Now is will be stored in the
+        // database. Afterwards the temporary import and download data can be

Now it will ...

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+        // operation this can be use to update the translation status.

use --> used

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * @param $success

boolean

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * @param $results

array

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+ * Download source file from a remote server.

Downloads

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -265,112 +266,42 @@ function locale_translate_export_form_submit($form, &$form_state) {
+ *   Defaults to all languagues

missing .

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+      // Each import itteration reports statistics in an array. The results of

itteration --> iteration

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+      // each itteration are added and merged here and stored per file.

itteration --> iteration

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+        if (is_numeric($report[$key])) {

Why do we check for is_numeric?

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+      // Update the file history if both project and version are known. This table

Comment too long

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+      // than installed projects would stay in the table for ever never.

for ever never? ;-)

+++ b/core/modules/locale/locale.compare.incundefined
@@ -5,86 +5,19 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can this be moved inside the functions needing it?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -325,27 +259,79 @@ function locale_translation_check_projects($projects, $langcodes = NULL) {
+ * Build a batch to get the status of remote and local translation files.

Builds

+++ b/core/modules/locale/locale.compare.incundefined
@@ -325,27 +259,79 @@ function locale_translation_check_projects($projects, $langcodes = NULL) {
+ * Helper function to construct batch operations checking remote translation status.

Comment is too long.

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,113 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can this one be moved? If not can we add a comment?

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,113 @@
+ * Build a batch to check, download and import project translations.

Builds

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,113 @@
+  // @todo Merge compare.inc and fetch.inc? Merge with batch.inc?

@todo still needed?

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,113 @@
+ * Build a batch to download and import project translations.

Builds

+++ b/core/modules/locale/locale.installundefined
@@ -171,34 +171,55 @@ function locale_schema() {
       'uri' => array(
-        'description' => 'File system path for importing the file.',
         'type' => 'varchar',
         'length' => 255,
         'not null' => TRUE,
         'default' => '',
+        'description' => 'URI of the remote file, the resulting local file or the locally imported file.',

Is 255 long enough to handle all remote files?

+++ b/core/modules/locale/locale.installundefined
@@ -788,6 +809,72 @@ function locale_update_8014() {
+      'uri' => array(
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+        'default' => '',
+        'description' => 'URI of the remote file, the resulting local file or the locally imported file.',
+      ),

See previous comment

+++ b/core/modules/locale/locale.moduleundefined
@@ -745,11 +824,97 @@ function locale_preprocess_node(&$variables) {
+ * Get current translation status from the {locale_file} table.

Gets

+++ b/core/modules/locale/locale.moduleundefined
@@ -745,11 +824,97 @@ function locale_preprocess_node(&$variables) {
+ * Update the {locale_file} table.

Updates

+++ b/core/modules/locale/locale.moduleundefined
@@ -745,11 +824,97 @@ function locale_preprocess_node(&$variables) {
+ * @param $file

object missing

+++ b/core/modules/locale/locale.moduleundefined
@@ -745,11 +824,97 @@ function locale_preprocess_node(&$variables) {
+ * Delete the history of downloaded translations.

Deletes

+++ b/core/modules/locale/locale.moduleundefined
@@ -745,11 +824,97 @@ function locale_preprocess_node(&$variables) {
+ * Save the status of translation sources in static cache.

Saves

+++ b/core/modules/locale/locale.pages.incundefined
@@ -467,19 +470,179 @@ function locale_translation_manual_status() {
+      $config->set('translation.overwrite_customized', TRUE)->save();
+      $config->set('translation.overwrite_not_customized', TRUE)->save();
...
+      $config->set('translation.overwrite_customized', FALSE)->save();
+      $config->set('translation.overwrite_not_customized', TRUE)->save();
...
+      $config->set('translation.overwrite_customized', FALSE)->save();
+      $config->set('translation.overwrite_not_customized', FALSE)->save();

You can get rid of 1 call to save using something like:
$config
->set('translation.overwrite_customized', TRUE)
->set('translation.overwrite_not_customized', TRUE)
->save();

Or skip the save all together because it's called after the switch.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -467,19 +470,179 @@ function locale_translation_manual_status() {
+  //locale_translation_flush_projects();

can this be removed?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -467,19 +470,179 @@ function locale_translation_manual_status() {
+  // and Translations update feature user experience http://drupal.org/node/1029554

user experience -> UX so the comment no longer exceeds 80 c

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Get array of projects which are available for interface translation.

Gets

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @see locale_translation_build_projects().

no period and (i think) @see should be at the end of the doc block

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ *   Array of project data for translation update. See
+ *   locale_translation_build_projects() for details.

The inline "See ... " can be removed

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Clear the projects cache.

Clears

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Load cached translation sources containing current translation status.
+ *

Loads

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @see locale_translation_source_build().

no period

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Build translation sources.

Builds

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @see locale_translation_source_build().

No period

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Check whether a po file exists in the local filesystem.

Checks

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @param stdClass $source
...
+ * @return stdClass

We use object in doc blocks.

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Build abstract translation source.

Builds

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @param stdClass $project

object

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ *   File name of translation file. May contains placeholders.

contains --> contain

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+  // remote file. The local version of this file will will only be checked is a

will will --> will
is --> if

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * @param stdClass $source1
...
+ * @param stdClass $source2

object

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
+ * Return default import options for translation update.

Returns

+++ b/core/modules/locale/locale.translation.incundefined
@@ -0,0 +1,354 @@
\ No newline at end of file

Missing new line

+++ b/core/modules/locale/tests/modules/locale_test_translate/locale_test_translate.moduleundefined
@@ -0,0 +1,6 @@
+ * Simulates a custom module with a local po files.

files --> file

attiks’s picture

Short summary of the non-comment ones:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,790 @@
+/*
+   function testAutomaticModuleTranslationImportLanguageEnable() {
+    // Enable a module.
+    $edit = array(

This function is commented out, why?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -6,40 +6,9 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this? If not this deserves a comment to explain why.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+      if ($source->type == 'remote' || $source->type == 'local') {

Don't we have a constant for this?

Can type be something else?

+++ b/core/modules/locale/locale.batch.incundefined
@@ -175,20 +191,300 @@ function locale_translation_batch_status_finished($success, $results) {
+            if ($import_type == 'download') {
+              // The temporary file is now imported and will not be used further.
+              // Store the timestamp and delete the file.
+              $timestamp = filemtime($source_result->files[$import_type]->uri);

So the file is downloaded to /tmp, moved to translations and then deleted? If so, why the need to move it? A comment might explain it.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -477,41 +403,74 @@ function locale_translate_batch_import($filepath, $options, &$context) {
+        if (is_numeric($report[$key])) {

Why do we check for is_numeric? Can you add a comment?

+++ b/core/modules/locale/locale.compare.incundefined
@@ -5,86 +5,19 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this? If not this deserves a comment to explain why.

+++ b/core/modules/locale/locale.fetch.incundefined
@@ -0,0 +1,113 @@
+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this? If not this deserves a comment to explain why.

+++ b/core/modules/locale/locale.installundefined
@@ -171,34 +171,55 @@ function locale_schema() {
       'uri' => array(
-        'description' => 'File system path for importing the file.',
         'type' => 'varchar',
         'length' => 255,
         'not null' => TRUE,
         'default' => '',
+        'description' => 'URI of the remote file, the resulting local file or the locally imported file.',

Is 255 long enough to handle all remote files? There's no real limit on URI, browsers/servers use 2000 as a safe upper limit.

+++ b/core/modules/locale/locale.installundefined
@@ -788,6 +809,72 @@ function locale_update_8014() {
+      'uri' => array(
+        'type' => 'varchar',
+        'length' => 255,
+        'not null' => TRUE,
+        'default' => '',
+        'description' => 'URI of the remote file, the resulting local file or the locally imported file.',
+      ),

See previous comment

+++ b/core/modules/locale/locale.pages.incundefined
@@ -467,19 +470,179 @@ function locale_translation_manual_status() {
+      $config->set('translation.overwrite_customized', TRUE)->save();
+      $config->set('translation.overwrite_not_customized', TRUE)->save();
...
+      $config->set('translation.overwrite_customized', FALSE)->save();
+      $config->set('translation.overwrite_not_customized', TRUE)->save();
...
+      $config->set('translation.overwrite_customized', FALSE)->save();
+      $config->set('translation.overwrite_not_customized', FALSE)->save();

You can get rid of 1 call to save using something like:
$config
->set('translation.overwrite_customized', TRUE)
->set('translation.overwrite_not_customized', TRUE)
->save();

Or skip the save all together because it's called after the switch.

attiks’s picture

Gábor Hojtsy’s picture

FileSize
69.89 KB

Wanted to make a screenshot of the settings page in the patch for anybody interested in doing any quick UI reviews:

UITranslation.jpg

Also noted in the meantime that the newly added Locale test translate module should be hidden = TRUE in its .info file. It shows up in the module list.

Sutharsan’s picture

Changes:

  • Comments by @attiks processed. My response below. (#47)
  • locale_test_translate module made hidden (#49)
  • Test module 'locale_test_disabled' replaced by 'locale_test_translate' as both are now hidden.
+class LocaleUpdateTest extends WebTestBase {

comment != class name

I'm doing the same here as all the other test files do. No change.

+        $text .= 'msgid "'. $source . '"' . "\n";
+        $text .= 'msgstr "'. $target . '"' . "\n";

What if either $source or $target contains a " (double quote)

Then it breaks. I've investigated to make a po with API functions, but found no easy way to do it. Added a restriction to the comments.

+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this?

(almost) every function needs this. In the name-space issue (...) this will be addressed. Probably by combining all functions in one or two files.

+   function testAutomaticModuleTranslationImportLanguageEnable() {
+    // Enable a module.
+    $edit = array(

This function is commented out, why?

Left over work. Now made into a test function.

So the file is downloaded to /tmp, moved to translations and then deleted? If so, why the need to move it?

No it is not. I've changed the comments and hopefully clarified it with that.

+        if (is_numeric($report[$key])) {

Why do we check for is_numeric?

I don't know. But I don't want to mess with other peoples code which I don't understand.

+        'length' => 255,

Is 255 long enough to handle all remote files?

Changed it to type 'text' as the Aggregator module does. 16K should be enough space ;)

+require_once DRUPAL_ROOT . '/core/modules/locale/locale.translation.inc';

Can we move this inside the functions that need this? If not this deserves a comment to explain why.

I like to clean this up buy combining functions differently in .inc files. Perhaps even all in one. This patch is big as it is, and moving even more functions around would not make it easier to review. I will add this to an existing follow-up issue: #1834298: Unify name space in Locale module

Sutharsan’s picture

Status: Needs work » Needs review

Now at needs review.

Status: Needs review » Needs work

The last submitted patch, locale-download-import-1804688-50.patch, failed testing.

attiks’s picture

I had a look at the interdiff and looks good, nice work. If the bot is happy, this should be as good as RTBC.

If you have to reroll, some minor comment issues, otherwise it's for a follow up.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -58,7 +58,7 @@ public static function getInfo() {
+   * Set up the test environment.

@@ -71,7 +71,7 @@ function setUp() {
+    // Set up timestamps to identify old and new translation sources.

@@ -155,7 +156,7 @@ private function makePoFile($path, $filename, $timestamp = NULL, $translations =
+   * Set up the environment containting local and remote translation files.

"* Set up " -> "* Sets up "

+++ b/core/modules/locale/locale.batch.incundefined
@@ -314,43 +316,43 @@ function locale_translation_batch_fetch_import($project, $langcode, $options, &$
+            // translations after successfull import. Otherwise the temporary
+            // file is are deleted after being imported.

is are --> is

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
5.63 KB
189.33 KB

Included:

  • Fixed the test in LocaleUpdateTest
  • Can't reproduce the exception in ModulesDisabledUpgradePathTest. But lets see if this patch does it better.
  • Included the comment improvements suggested by attiks.

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

The last submitted patch, locale-download-import-1804688-54.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-ui, +feature freeze
YesCT’s picture

Status: Needs review » Needs work

test bot not reporting back, but manual check shows
http://qa.drupal.org/pifr/test/390193
fail on ModulesDisabledUpgradePathTest

something about server_pattern

Sutharsan’s picture

Trying to run testbot with a few debug calls in the patch. No idea why it fails.

Sutharsan’s picture

Status: Needs work » Needs review

Go for it!

Sutharsan’s picture

Apparently config('locale.settings')->get('translation.default_server_pattern') returns NULL during upgrade test (but not in my sandbox). Added an additional check and fallback value for the server_pattern value.

Sutharsan’s picture

Testbot does not report back (yet), but the patch passed the test!

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Only thing is see is that you changed "Set up" to "Setup", but it should be "Sets up", but this can be done in a follow up. So nice work and RTBC.

YesCT’s picture

trying this manually. will post results soon.

YesCT’s picture

used the steps from the issue summary as a basis to test, but did a little different:

checkout the latest Drupal core code
Apply the latest patch from this issue
used ui to install
Enable Locale module
[this was already enabled] Enable Update module (this is required to check for updates)
Add 1 or more languages: admin/config/regional/language
[skipped] Install and enable the Locale Temper module
clicked link to download translation
downloaded german, saved to downloads folder
... got error.

Maybe better follow the directions and get the tamper module.
still get error.

The tamper testing module is confusing me... and even though I read the issue summary, I'm not sure what this patch is supposed to do in terms of improvement to the workflow of downloading translations and importing them.

I'm going to try testing again.

do it (download a translation and import it) first with no patch.

01. plan: from scratch download and import

installed d8 with no patch
d8-s01-plan-2012-11-25_0050.png

extend: enabling locale module
add a language (under configuration). added german.

02. look for something to click on to download translation file

d8-s02-languages-2012-11-25_0054.png

03. click the download link and go to localize.drupal.org, click german po file

d8-s03-translation-list-2012-11-25_0056.png

04. save the po file in my downloads folder

d8-s04-download-2012-11-25_0057.png

05. back to site, look for what to do next, try "translated" link

d8-s05-try-translated-link-2012-11-25_0057.png

06. there is an import tab. try that.

d8-s06-translated-try-import-tab-2012-11-25_0059.png

07. get frustrated because the text tells me I'm doing it the hard way. get frustrated this is taking a long time. browse anyway.

d8-s07-import-tab-2012-11-25_0100.png

08. get error when import. sigh.

d8-s08-error-2012-11-25_0102.png

09. remember reading instructions on install how to install in another language to make a translations directory. try that.

d8-s09-mkdir-translations-2012-11-25_0104.png

10. try browse and import again after mkdir

d8-s10-import-after-mkdir-2012-11-25_0105.png

11. import is doing something. get some hope.

d8-s11-importing-2012-11-25_0105.png

12. import worked!

d8-s12-import-worked-2012-11-25_0107.png

break for discussion

this seems like overkill to do these screenshots, but since this patch is ui changes,
core gates asks for before and after screenshots. so these can serve as the before ones for how downloading and importing is done before the patch.
the patch also adds some other screens (or changes them) so next are the rest of the before screenshots.
hopefully this will also help clarify what this patch does.

13. admin/config/regional/translate/settings

d8-s13-no_translate_settings-2012-11-25_0109.png

14. reports page

d8-s14-admin_report-2012-11-25_0111.png

15. admin/reports/translations shows the todo before the patch

d8-s15-admin_reports_translations-2012-11-25_0112.png

16. /check page did something. I thought it would not since the patch says it will add the /check page

d8-s16-check-page-2012-11-25_0114.png

Next

next I'll upload steps to test this patch and the results (screenshots)

Sutharsan’s picture

@YesCT, the steps you followed are the D7 route. All you need to do is install drupal (English) and add a language. The instruction are confusing because they have not been changed. mkdir translation directory is not required. if you do, the downloaded files will get saved locally for re-use later or by multisite sibblings. Hope this helps.
UI is included in #1804702: Display interface translation status, installation in a non-Englisch language is in #1848490: Import translations automatically during installation. But you are not testing those ;)

Sutharsan’s picture

I forgot to mention that it will not download/import translation for dev modules. Bad first experience, but no translations exist at l.d.o for dev modules. Therefore it will not download the core translation if you enable a language. This is where Locale Temper comes it, it simulates D7 core and some D7 modules to be installed. Alternatively install a few D8 modules which already have stable releases. But be careful because many of them are not stable. I am using: Echo, xhtml, Mail System, HTML Mail, Search Autocomplete, Aloha Editor, Visitor. But I stripped all of them from their code and only thing the .info file and an empty .module file.

YesCT’s picture

trying steps to test patch

1. checkout the latest Drupal core code
2. Apply the latest patch from this issue
3. used ui to install
4. Enable Locale module
s01:
ld-s01-enable-locale-2012-11-25_0238.png
5. [skipped because this was already enabled] Enable Update module (this is required to check for updates)
6. Add 1 or more languages: admin/config/regional/language
This is the config page.
It's tempting to go to the UI trans first, since that is what I want to do instead of languages.
s02:
di-s02-config-languages-2012-11-25_0242.png
So check that screen to see if the help text there says to go to languages first.
s02b:
di-s02b-import-help-text-2012-11-25_0305.png
it has help text:

This page imports the translated strings contained in an individual Gettext Portable Object (.po) file. Normally distributed as part of a translation package (each translation package may contain several .po files), a .po file may need to be imported after offline editing in a Gettext translation editor. Importing an individual .po file may be a lengthy process.

Note that the .po files within a translation package are imported automatically (if available) when new modules or themes are enabled, or as new languages are added. Since this page only allows the import of one .po file at a time, it may be simpler to download and extract a translation package into your Drupal installation directory and add the language (which automatically imports all .po files within the package). Translation packages are available for download on the Drupal translation page.

change that to something.. that tells me that I can just go add a language and I dont ... even need to worry about what is a .po, downloading it or where it comes from. (maybe)

Back to adding a language.
I add spanish. But a message zips by too fast. And it doesn't look like any ui was imported.
s03:
di-s03-add-spanish-2012-11-25_0245.png

I add german, record the video and then pause it so I can see what happened.

s04:
di-s04-add-german-2012-11-25_0249.png
and then
s05:
di-s05-8of8-2012-11-25_0249.png

It does not look like it did the import...

Ah, in irc @Sutharsan points out that since this is 8.x, and 8.x translations are not on localize yet, ... and:
This is where Locale Temper comes it, it simulates D7 core and some D7 modules to be installed. Alternatively install a few D8 modules which already have stable releases. But becarefull because many of them are not stable. Echo, xhtml, Mail System, HTML Mail, Search Autocomplete, Alaoha Editor, Visitor. Stripped all of them from their code. The only thing left is the .info file and an empty .module file. these modules all have D8 releases.

So this could be working and the files it imported were very small and few.

7. Install and enable locale tamper (this makes more sense now, but maybe should be done before adding the language)
That had a briefly displayed message that said installing 14 of 14.

8. Adding a language
Better this time. progress flashed by (more slowly since there were more/bigger po files)
s06a
di-s06a-2012-11-25_0324.png
Consider some better text while that is happening like saying that the po files are being downloaded and imported (both is happening, right?)

and the message remained: 6 translation files imported. 116 translations were added, 34 translations were updated and 0 translations were removed.
But there were error (I think that having the sites/default/files/translations directory would make those errors go away)

s06b:
di-s06b-it_worked_and_error_Languages | locale-download-import-1804688-60 2012-11-25 03-29-47.png

Here is the text of the error:

The specified file temporary://better_formats-7.x-1.0-beta1.sq.po could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

I'll make the dir, and add another language to verify.

Nope. Maybe a temp dir needs to be made.
Also, perhaps the po files are removed after they are imported? So we dont need to tell people they are "downloaded"?

Continuing testing the other parts of this patch.

9. Reports admin page
s07:
di-s07-2012-11-25_0337.png

s08:
di-s08-2012-11-25_0337.png
clicking update, does something, a progress message I think, and then shows this page again with no message
I think #1804702: Display interface translation status fixes this.

Tamper report page to get an idea without that patch. admin/reports/locale_tamper
s09:
di-s09-tamper-report-2012-11-25_0338.png

10. Settings tab
configuration ui translation page now has a settings tab.
See #64 step 13 (no patch)
s10:
di-s10-config-ui-trans-had-settings-tab-2012-11-25_0346.png

s11:
di-s11-settings-tab-User interface translation | locale-download-import-1804688-60 2012-11-25 03-49-43.png

11. Check link
s11b:
locale-download-s11-link_to-admin_reports_translations_check-2012-11-25_0037.png

s12:
locale-download-s12-manual-check-2012-11-25_0039.png

YesCT’s picture

Status: Reviewed & tested by the community » Needs work

needs work:

to check on the error message about the temp files in #67 s06b.

to fix some of the help text

to update the steps to test

and to update the issue summary in general

tatarbj’s picture

I have downloaded the patch and tested this and i haven't found any problem with that.
I'm going to do the review this code of patch.
(Sorry for my bad English :))

czigor’s picture

I went through the points in 'To test this patch' in the issue description. It imported some translations.

On the admin/reports/locale_tamper page it says:
drupal 8.x-dev Hungarian Unknown

On the admin/config/regional/language the Hungarian line says "1416/1836 (77.12%)". Since hardly anything has got translated I find this number hardly plausible unless this number applies only to the modules listed as 'Up to date' on the admin/reports/locale_tamper page.

Sutharsan’s picture

The fault message is because a translation directory has been defined by default, but has not been created. The directory is used to save downloaded translation files. However the download/import can work without. Saving the translations files is beneficial when the site is part of a multi site setup. I think for the 80% usage this directory is not required. We can change default by not defining a directory which will solve the default without compromising the majority of the use cases.

I will work on better help and interface texts tomorrow.

Gábor Hojtsy’s picture

@czigor: depending on how many modules were available for the update and how big those are, ~1500 strings sound like pretty normal. Core is ~4500-ish in Drupal 7 at least. Assuming that core was not included with your update. Did you see that being available and to be downloaded/imported?

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
7.68 KB
190.17 KB

User feedback updated and completed.
The error message is already reported in #1787520: Translations not imported on installation here in relation to the existing file import during installation. I suggest to use this issue as a follow up. Also because it affects both existing and new code.

YesCT’s picture

testing steps:

git checkout 8.x
drush -y sql-drop --db-url="mysql://root:root@localhost/d8-git"
sudo rm -r sites*
git checkout sites
git pull --rebase
git reset --hard
curl -O http://drupal.org/files/display-interface-translation-status-1804702-37-...
curl -O http://drupal.org/files/locale-download-import-1804688-73.patch
git checkout -b locale-joint
(apply this one, 4688, first)
git apply --index locale-download-import-1804688-73.patch
git commit -m "1804688-73"
git apply --index display-interface-translation-status-1804702-37-do-not-test.patch
git commit -m "1804702-37"
cd core/modules
git clone http://git.drupal.org/sandbox/sutharsan/1833370.git locale_tamper

then install the site
enable locale and tamper, under Extend

skip confusing help text stuff for now. assume people know to go add a language to get the ui translation po files. use the tamper and just see if it works.

di-wui-w01-Languages | localhost 2012-11-26 03-28-02.png

See, it worked. I got german words without manually going to d.o and downloading translation po files, and without manual making the translations directory.

di-wui-w02-german-2012-11-26_0330.png

clicking on the percentage translated, or going to the ui translation configuration page, goes here, and we can see the new settings tab this patch adds.

di-wui-w03-settings-2012-11-26_0331.png

clicking on the settings tab shows us the settings page this patch adds.

di-wui-settings-page-2012-11-26_0333.png

from that settings page, use the check manually now link, or go in the admin menu under reports (use the tamper link to see it working with po files it can find, use the other normal report link to see what happens when no translation files are found: we dont have 8.x specific files right now)

the /check url this patch adds works, it does check for updates

di-wui-w04-Locale Tamper translation updates | localhost 2012-11-26 03-38-17.png

attiks’s picture

Interdiff of #74 looks sane.

YesCT’s picture

I did a change to the help text on the interface translation import tab and also on the languages configuration page to reflect that translations are imported automatically (with this patch). Perfecting the help text and updating it in other places, like the actual help page, should be a follow up.
help-s01-2012-11-26_0505.png

help-s02-2012-11-26_0507.png

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary with links to screenshots, use issue summary template, get ready for committer to review by making it really obvious the pain point and the slick solution.

attiks’s picture

Looks good, but ...

+++ b/core/modules/locale/locale.moduleundefined
@@ -170,7 +170,10 @@ function locale_help($path, $arg) {
+      $output .= '<p>' . '<b>' . t('Automatic imports of translations, if available, are done as new <a href="@language">languages</a> are added, or when new modules or themes are enabled.', array('@language' => url('admin/config/regional/language'))) . '</b>' . '</p>';

can you use a <strong>tag

YesCT’s picture

Status: Needs review » Needs work

attiks thanks. My mind was drawing a blank on what b should have been! Doing it now.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
193.91 KB

here is the b -> strong

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Bot is still happy, and code looks good, so back to RTBC

Sutharsan’s picture

Adding API changes for the issue summary.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary filled in something for api changes while sutharsan writes something

Sutharsan’s picture

The same API changes, but now annotated.

Sutharsan’s picture

Issue summary: View changes

Link to file with API changes

Sutharsan’s picture

Issue summary: View changes

Update API changes.
Update follow up issues.

Sutharsan’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
22.89 KB

Ok, spent most of my afternoon reviewing this patch. EXCITING functionality! :D

I got blocked at various places trying to follow the testing instructions in the issue summary, which I found very overwhelming because of all the external dependencies. I was pretty adamant about being able to test this the whole way through without external dependencies such as Locale Tamper, because there ought to be enough test coverage here for that to be possible. Huge thanks to YesCT, Sutharsan, and Berdir for their help.

Here are the steps that worked for me:

1) Apply the patch.
2) Install Drupal
3) Enable Locale module (and Language module as a side-effect) — NOTE for whatever reason I didn't have the problem in the issue summary w/ the translation directory not being auto-created.
4) Go to the Languages configuration page and add Dutch (needs to be Dutch/German for the following steps to work).
5) Edit core/modules/locale/tests/modules/locale_test_translate/locale_test_translate.info and remove the "hidden = TRUE" line.
6) Enable it at admin/modules. See a batch screen as well as a message that says it imported 8 strings. Yay!
7) Go to the locale overview screen and see 8/40 strings translated. Yay!
8) Manually edit core/modules/locale/tests/test.nl.po and change/add some values. (Didn't test deleting)
9) Go to admin/reports/translations and click "Update." See those values made it in.

That all works, which is as-intended. However, I'd like to see a few things fixed before commit:

1) Locale should take care of the translation directory, without needing to do any manual fiddling. Sutharsan pointed out that there's a separate issue dealing with install-time tasks, but this is not, IMO, an install-time task. This should happen even for sites that were originally installed in English (or upgraded from D7) and want to enable multilingual functionality.

2) There's no link from the Locale screen to the reports section for updating the translations and there should be one, because this is not really discoverable without it.

3) While I understand it's not possible to test this "for reals" until D8 gets at least an alpha release, we should nevertheless have test coverage that simulates remote file downloads like Update module does.

4) The page with the update button is lacking any kind of text on it whatsoever to explain what's happening. While I understand that larger UI issues are being worked out at #1804702: Display interface translation status, this looks buggy:

Update.. what exactly? Can't tell, because there's no text.

Let's add a simple "Last checked X minutes ago" type text for now, with wholesale beautifying to happen in that other issue.

I think that was everything. Hopefully we can bang these things out in a day or so; if not, let me know and we'll try and talk about alternative next steps.

Gábor Hojtsy’s picture

@webchick: great review, thanks. I think most things you are asking for are small adjustments. The tests request I don't fully understand. If you look at the patch, it has sample .po files both as local and remote translations prepared for and used in the test:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.phpundefined
@@ -0,0 +1,797 @@
+    // Add a number of files to the local file system to serve as remote
+    // translation server and match the project definitions set in
+    // locale_test_locale_translation_projects_alter().
+    $this->makePoFile('remote/8.x/contrib_module_one', 'contrib_module_one-8.x-1.1.de._po', $this->timestamp_new, $translations_one);
+    $this->makePoFile('remote/8.x/contrib_module_two', 'contrib_module_two-8.x-2.0-beta4.de._po', $this->timestamp_old, $translations_two);
+    $this->makePoFile('remote/8.x/contrib_module_three', 'contrib_module_three-8.x-1.0.de._po', $this->timestamp_old, $translations_three);
+
+    // Add a number of files to the local file system to serve as local
+    // translation files and match the project definitions set in
+    // locale_test_locale_translation_projects_alter().
+    $this->makePoFile('local', 'contrib_module_one-8.x-1.1.de._po', $this->timestamp_old, $translations_one);
+    $this->makePoFile('local', 'contrib_module_two-8.x-2.0-beta4.de._po', $this->timestamp_new, $translations_two);
+    $this->makePoFile('local', 'contrib_module_three-8.x-1.0.de._po', $this->timestamp_old, $translations_three);
+    $this->makePoFile('local', 'custom_module_one.de.po', $this->timestamp_new);
Sutharsan’s picture

There is sufficient test coverage, but it is all done programatically. The only exception is the locale_test_translate which can be used to test the import of local files. But no modules can be extracted in the same way to test import of remote files.

The attached patch covers webchick's points 1, 2 and 4:

  • 1: locale_install() creates the translations directory.
  • 2: Various links added to revised help texts in locale_help().
  • 4: "Last checked" text and missing UI notice added on the Translations status page.
Sutharsan’s picture

Status: Needs work » Needs review

Go botty

Berdir’s picture

+++ b/core/modules/locale/locale.installundefined
@@ -6,6 +6,20 @@
+  // @todo Automatic translations download/import does not need the translations
+  // directory to exist. Only multi site installations may benefit from having
+  // this directory. A follow up issue will change this default:
+  // http://drupal.org/node/1851442

Nitpick mode: comments that belong to a @todo: should be indented by two spaces. And I think it needs a . after the link?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -605,9 +605,22 @@ function locale_translation_status_form($form, &$form_state) {
   // @todo Add user interface update for translation update.
   // Followup issues: Display translation status http://drupal.org/node/1804702
   // and Translations update feature UX http://drupal.org/node/1029554
+  $form['temporary_note'] = array(
+    '#markup' => '<p>' . 'The translation status will be included here. See <a href="http://drupal.org/node/1804702">http://drupal.org/node/1804702</a> for more information.' . '</p>',

The simple last checked UI makes sense, but I'm not sure if we really need this?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
197.96 KB

Fixed the two notes mentioned by @Berdir (fix indent for @todo, remove the temporary_note, since there is a "last checked" line printed above).

I believe this fixes all concerns raised. As shown above, there is extensive test coverage in the module for remote files, and the functionality is dependent on timestamps of files, so we cannot just include those "remote files" as-is in the test module like update module does. We have no control of timestamps there.

Berdir’s picture

That's fine with me, in regards to the tests. Can we still open a (non-blocking) follow-up to explore ways to add remote tests?

Instead of adding files, I think it would be possible to point it to a page callback that prints a file, that would also give us control over the last-modified header. Another option is Guzzle, which will be committed soon I think. I'm quite sure that guzzle provides mocking/testing capabilities that would allow to add test coverage for locale_translation_http_check() and related functions, even for the real d.o URL's, so that we could test those.

While looking to those possibilities quickly yesterday, I noticed that the tests actually do seem to attempt remote fetchs for core. Somehow the URL override does not affect it, maybe because the module starts to do things as soon as it's enabled? Maybe something that needs to be looked into, as it could eventually cause unexpected behavior once 8.x will have a release?

Gábor Hojtsy’s picture

@Berdir: there is extensive existing code in core to test the remote files even just to test the up-to-date status of the local translations (which was a previous patch). This patch moves around some of those tests and expands on actually downloading/importing those files. I think the tests are in line with usual test setups where the test module just has the Drupal related pieces and test data is created in the test. I recognise that is not how update module tests are written, but we can refactor that in a followup if needed.

Berdir’s picture

I recognise that is not how update module tests are written, but we can refactor that in a followup if needed.

That's exactly what I said: " Can we still open a (non-blocking) follow-up to explore ways to add remote tests?". The other paragraph was just writing down my thoughts about that :)

Berdir’s picture

Issue summary: View changes

Updated inssue summary

Jose Reyero’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.65 KB
196.64 KB

This patch looks good to me. But I've been testing the installation in a non-english language (Spanish) and it is broken.

Fatal error: Call to undefined function locale_translate_batch_import_files() in /var/workspace/drupal8/core/includes/install.core.inc on line 1603

This new patch should fix this. It just adds back that function, updated for the other api changes.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Good find! Looks like that function was removed in a haste. The installer code is further refactored and mapped to this system in #1848490: Import translations automatically during installation later.

YesCT’s picture

I'm trying the instructions in the issue summary to test this patch to verify they work.

the patch from 92 is not applying. I'll look into it and post back.

YesCT’s picture

cache('page')->flush();
changed to
cache('page')->deleteAll();

I admit, I dont know how to handle that with git... and then provide a interdiff. So I made a copy of the patch from 92, called it 92b. Edited 92b to say deleteAll().

$ diff locale-download-import-1804688-92.patch locale-download-import-1804688-92b-do-not-test.patch
3452c3452
< cache('page')->flush();
---
> cache('page')->deleteAll();

I applied 92b, and then did a git diff to 8.x to make patch 95. so 95 has all/only the same changes as 92 did.

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

The last submitted patch, locale-download-import-1804688-95.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-ui, +feature freeze
YesCT’s picture

Status: Needs review » Needs work

The help texts need work.

I won't be able to get to that for a few hours if someone wants to do it first.

Here is some I cut and pasted while testing just to make a note of it.

languages config page

Interface translations are downloaded and imported automatically, check the status of Available translation updates. You can edit the interface translations at the Translate page.

user interface translation page

This page imports the translated strings contained in a Gettext Portable Object (.po) file. Normally downloaded from the Drupal translation server. Also a .po file may need to be imported after offline editing in a Gettext translation editor. Importing an individual .po file may be a lengthy process.

Note that the .po files are automatically downloaded and imported when new modules or themes are enabled. This is strongly encouraged for interface translation.

Also, the

  • Refresh the translation status by
    • clicking on the settings tab, and then the check updates now link, or
    • clicking the "Check manually" link at admin/reports/translations

    Page is replaced by the other issue. but still, I think the minimum help/status text that was recently added there needs a bit of work.

    I can finish a more detailed review later. Have to go for now.

  • YesCT’s picture

    Status: Needs work » Needs review
    FileSize
    3.09 KB
    196.77 KB

    Help text changes to grammar, to be general, have improved word choices.

    Sutharsan’s picture

    When the full import process is called after the status cache has expired, locale_translation_get_file_history() is called twice in one page call, first before the import and secondly after the import. Due to its cache it returns old data at the second call. This patch fixes it by adding a drupal_static_reset() after new data is stored in the database.

    Sutharsan’s picture

    Removing @todo after discussion in #1851442: Create an interface translations directory by default. We concluded to create and use a translations directory by default. And thus don't want to change the code in this patch.

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

    The last submitted patch, locale-download-import-1804688-101.patch, failed testing.

    Gábor Hojtsy’s picture

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

    Status: Needs review » Reviewed & tested by the community

    Great changes. The tests failed for unrelated reasons earlier, rerunning them showed they work.

    webchick’s picture

    I want so very much to be able to commit this patch, but enabling Locale module per the testing instructions is giving me:

    Fatal error: Class name must be a valid object or a string in /Users/abyron/Sites/8.x/core/lib/Drupal/Core/Database/StatementPrefetch.php on line 299
    Call Stack
    #	Time	Memory	Function	Location
    1	0.0009	646632	{main}( )	../index.php:0
    2	0.0175	2924528	Symfony\Component\HttpKernel\Kernel->handle( )	../index.php:35
    3	0.1520	19683408	Drupal\Core\HttpKernel->handle( )	../Kernel.php:193
    4	0.1520	19684584	Symfony\Component\HttpKernel\HttpKernel->handle( )	../HttpKernel.php:51
    5	0.1521	19684584	Symfony\Component\HttpKernel\HttpKernel->handleRaw( )	../HttpKernel.php:73
    6	0.4279	24303832	Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch( )	../HttpKernel.php:134
    7	0.4280	24307928	Symfony\Component\EventDispatcher\EventDispatcher->dispatch( )	../ContainerAwareEventDispatcher.php:165
    8	0.4280	24308336	Symfony\Component\EventDispatcher\EventDispatcher->doDispatch( )	../EventDispatcher.php:53
    9	0.4280	24308688	call_user_func ( )	../EventDispatcher.php:164
    10	0.4280	24308720	Drupal\Core\EventSubscriber\ViewSubscriber->onView( )	../EventDispatcher.php:0
    11	0.4282	24315464	Drupal\Core\EventSubscriber\ViewSubscriber->onHtml( )	../ViewSubscriber.php:59
    12	0.4284	24366728	drupal_render_page( )	../ViewSubscriber.php:145
    13	0.4364	24442520	drupal_render( )	../common.inc:5310
    14	0.5143	25172360	theme( )	../common.inc:5470
    15	0.5156	25203240	template_process_html( )	../theme.inc:1036
    16	0.5357	26138648	drupal_get_js( )	../theme.inc:2763
    17	0.5357	26141208	drupal_alter( )	../common.inc:3800
    18	0.5358	26143088	locale_js_alter( )	../module.inc:1200
    19	0.5558	26143576	_locale_parse_js_file( )	../locale.module:586
    20	0.5575	26251008	Drupal\locale\StringDatabaseStorage->findString( )	../locale.module:1078
    21	0.5581	26255432	Drupal\Core\Database\StatementPrefetch->fetchObject( )	../StringDatabaseStorage.php:68
    22	0.5581	26256016	Drupal\Core\Database\StatementPrefetch->current( )	../StatementPrefetch.php:399
    

    :(

    webchick’s picture

    The class name is null btw.

    Jose Reyero’s picture

    @webchick,
    Funny this looks pretty similar to the install errors I'm getting (though they look my own local issue). For me there's always the ViewSubscriber in the middle (though locale not yet enabled).

    I would bet a beer all these go away by disabling Views before enabling locale :-)
    (Doing some research on it so I may get the beer myself).

    Jose Reyero’s picture

    Yes, just noticed this is not Views, but 'ViewSubscriber' :-(

    Sutharsan’s picture

    Status: Reviewed & tested by the community » Needs review
    FileSize
    2.89 KB
    196.81 KB

    I can't reproduce the above error. Have installed Drupal with drush si, interactive and non-interactive. Enable Locale module and Locale test translate at the same time, Locale first and then Locale test translate. Both via the interface (not drush).
    I have found an other error however, an Ajax HTTP error when a batch was fired after installing Locale + Locale test translate, but before a language is enabled. This patch fixes makes sure no download/import or cleanup action is taken when no language is enabled.

    Jose Reyero’s picture

    More background on this one:
    This looks like another case of a Locale module hook being invoked before the module class namespace has been registered. So what we've got here is a missing class.
    (Which is new to me because of that ViewSubscriber)
    But we have plenty of similar ones documented here, #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc...

    Jose Reyero’s picture

    Oops, I didn't intend to change the status

    Status: Needs review » Needs work

    The last submitted patch, locale-download-import-1804688-106.patch, failed testing.

    Jose Reyero’s picture

    Some findings: (This may be related to php versions / locale_storage() / db connections)

    In #105, the class webchick is is getting from execute() whithin findString( ) is Drupal\locale\StringDatabaseStorage
    for which the fetchObject() method doesn't support a class name

    This (current head) library StatementPrefetch::fetchObject(), is seriously broken but also I don't know whether it should take any parameters at all (it's not even in the interface)
    http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Statement...

    Anyway it doesn't work with a class name which is what LocaleStringStorage::findString() tries.

    The funny thing is that function is working because (we don't know why yet) the object we are getting is not
    Drupal\Core\Database\StatementPrefetch (like webchick)
    but
    Drupal\Core\Database\Statement (which extends PDOStatement) and thus supports fetchObject($class_name) properly.

    Anyway, my preliminary conclusions:
    - There's a bug to fix in LocaleStringStorage because it is using the fetchObject($class_name) method that is not in our StatementInterface (thought it is in the object returned usually, not on the one @webchick is getting though)
    - The StatementPrefetch::fetchObject() method and the rest of the class and the interface need some serious cleanup.

    Then there's the really weird fact that some people, possibly depending on php version, are getting different returned classes under the same circunstances, that won't help debugging at all.

    Notes about php versions, we did some quick survey:
    @Sutharsan: 5.13.15 (NOT getting this error)
    @webchick: 5.3.6 (getting the erros)
    Mine is 5.3.10 (NOT getting this error)

    YesCT’s picture

    I'm using 5.3.6 and I did not get the errors.

    Gábor Hojtsy’s picture

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

    FileSize
    1.11 KB

    I'm running Acquia Dev Desktop with PHP 5.3.15 (same PHP version as Sutharsan). Pulled the latest D8 and applied the latest patch from here. Then got this patch (attached) from @reyero to see what kind of statements am I getting. I'm also getting Drupal\Core\Database\Statement (unlike @webchick). So cannot reproduce the bug on this end either.

    Gábor Hojtsy’s picture

    Status: Needs review » Reviewed & tested by the community

    All right, PHP versions did not matter, the sqlite DBTNG driver seems to be the cause of the problem AND it is reproducible with or without this patch. (I did both). I think it means this issue is still RTBC (no bug identified in this patch), but if we want to make sure locale can be enabled on sqlite so @webchick can verify on SQLite, then we need to fix the other bug first than come back.

    Opened #1854752: Re-Add PDO::FETCH_PROPS_LATE to PDO::FETCH_CLASS for that.

    Gábor Hojtsy’s picture

    Damien Tournoud argues that issue should not be resolved by fixing the driver per say. So opened #1854838: Locale should not use fetchObject with a classname, WSOD on sqlite to have a solution for locale based on suggestions from Damien.

    clemens.tolboom’s picture

    (just for the record)

    I did not follow the instructions. I just

    - enabled locale temper module

    - added Dutch and set it as the only language (deleting English)

    - next visited http://drupal.d8/admin/reports/translations

    - clicked (Check manually) ... it start checking

    - next started the update

    - the batch started nicely but:

    -- reported an error for each locale temper file

    The specified file temporary://better_formats-7.x-1.0-beta1.nl.po could not be copied because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

    -- next reported successful import

    7 translation files imported. 452 translations were added, 56 translations were updated and 0 translations were removed.

    The error log contains messages like

    File temporary://devel-7.x-1.3.nl.po could not be copied because the destination directory translations:// is not configured correctly.

    There are two issues here:

    1. Why is the StreamWrapper translate:// not properly configured?
    2. Why are the translation updates reported as successful?

    The directory ls sites/default/files/translations/ is empty.

    YesCT’s picture

    @clemens.tolboom Yeah, #101 is related to that. [edit: changed 110 to 101. and fixed link.]

    Sutharsan’s picture

    The Directory should be configured automatically when you enable Locale module. did you or was it enabled previously (older patch)? the import does not depend on the directory. download to temp, import, then move to directory.
    edit: did you use a fresh install with the new patch (triggering hook_install), what are the permissions of the translation directory?

    YesCT’s picture

    If I remember correctly, I get those red errors in #74 because the directory does not exist. That is with a totally clean 8.x and new install (sudo rm -r sites; git checkout sites;) I think the problem is that the translations directory is configured when locale is enable, but it not created.

    It does work without the directory because I believe the po files are downloaded to a temp location, imported and then removed.

    Sutharsan’s picture

    Since #85 the directory is created when Locale module is installed (or at least it should be).

    Gábor Hojtsy’s picture

    Sutharsan’s picture

    Status: Reviewed & tested by the community » Needs review
    FileSize
    4.99 KB
    198.61 KB

    Some fixes:

    • When only local translation files are selected as translation source, remote files are still checked. This introduces a new function which returns whether remote sources should be incorporated.
    • When a language is deleted, the translation status of the language is not. This introduces two new functions to clear the status cache per language and per project.
    Gábor Hojtsy’s picture

    Status: Needs review » Reviewed & tested by the community

    Changes look good to me, great fixes :)

    Gábor Hojtsy’s picture

    webchick’s picture

    Ok, tried the instructions once again from the top and this time got a lot further, but still couldn't see the NL translation updating when I changed its .po file on the file system. :\ Which is weird, because I definitely had it working 3 test runs ago (or whatever it was). It could be PEBCAK, I guess, though... Suthasan wasn't around to troubleshoot.

    Given that this and the metadata patch conflict with each other, and given that Dries is hoping to give that one some more look over the weekend for possible commit, I think we might be better served getting this in. It does make me really nervous to commit something without seeing it working with mine own eyes, but the test coverage here is pretty decent, and once the UI issue gets in, testing this will presumably be a lot easier.

    So... committed and pushed to 8.x. YAY!

    Let's also get a follow-up for Berdir's point about turning .po files into menu callbacks so we can manipulate them for testing purposes. I noticed my watchdog had things in it like File not found: http://ftp.drupal.org/files/translations/8.x/drupal/drupal-8.x-dev.nl.po.

    webchick’s picture

    Status: Reviewed & tested by the community » Fixed

    oops. ;P

    Gábor Hojtsy’s picture

    @webchick: Thanks! The file not found is normal operation, it looks at the remote translation source as per the default config first, and obviously ftp.drupal.org does not yet have a translation file to download for 8.x core.

    Gábor Hojtsy’s picture

    Created #1861360: Refactor localization update test so people can just enable the test module to test for the test refactoring. Put it on the sprint so we can take care of it sooner than later.

    Sutharsan’s picture

    @webchick, sorry, I was not around to assist. I was too bussy backing pancakes with bacon, apple, etc. Yummy ;) But thanks for committing is anyway.

    webchick’s picture

    Nice. :D Well-earned pancakes, for sure. :D

    webchick’s picture

    Issue summary: View changes

    updating testing the patch instructions. they would use more work to make it clear which steps are having the patch do some new functionality, and which are just letting the tester see that it worked.

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

    YesCT’s picture

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary added help text follow up issues.

    eigentor’s picture

    Maybe we should invent a "congrats" Button, so cheering for stuff like this does not spam so hard :P

    Gábor Hojtsy’s picture

    Issue tags: -sprint

    Thanks all, especially Sutharsan! This was a truly great effort :)

    Status: Fixed » Closed (fixed)

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

    YesCT’s picture

    YesCT’s picture

    I think since we have a tar ball and alpha https://drupal.org/node/3060/release that when adding a language, the po should be automatically downloaded and imported.
    But it's not. #2030537: Translations not downloaded when adding a new language

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary moved the follow-ups that were created from the followups to be made to just the list of follow ups ... because they were made!