Problem/Motivation

As part of the l10n_update integration in core the motivation of this issue is to provide automatic update of interface translations when new translations are available.
The interface to set the update frequency is already in core, but is not functional.

Proposed resolution

Use cron to check and update available translations. Queue tasks are used for this cron updates. Queue uses the functions (operations) also used for batch update of translations. Drupal's cron processing (utilising hook_queue_info) is used tho perform the queue tasks.

Batch operations need to be modified to be used for queue execution. Batch operations are executed consecutively, but queue operations may be divided over different cron jobs.
For use in queue, the batch operations need to be modified to run independently from eachother. With this modification will also fix #1884996: Progress of Import translations automatically during installation goes to warp speed at the end after 60%.

Remaining tasks

None

Manual testing

Automated tests are available, but if you like to see the patch at work, feel free.
TEST 1
Prepare test:
- Make sure you have enabled Update module and (or course) the Interface translation module, and you have at least one language enabled.
- Install and enable module A (Visitors *)
- Check translation status (admin/reports/translations): no translation for module A available
- Change translation file timestamp: $ touch sites/default/files/translations/...module A translation...
- Check translation status (admin/reports/translations): translation for module A available
- Set the batch frequency to 'Weekly' (admin/config/regional/translate/settings).
- Either wait a week or set the last checked value of module A to zero (UPDATE locale_file SET last_checked = 0 WHERE project = 'visitors') .
Start test:
- execute cron manually (admin/reports/status)

Check:
- table {locale_file}.last_checked of module A is modified. Remember the value

TEST 2
- Install and enable module B (Image Desaturate Formatter **)
- Check translation status (admin/reports/translations): no translation for module B available
- Change translation file B timestamp: $ touch sites/default/files/translations/...
- Check translation status (admin/reports/translations): translation for module B available
- Wait another week or set the last checked value of module B to zero (UPDATE locale_file SET last_checked = 0 WHERE project = 'image_desaturate_formatter') .

Start test:
- execute cron manually (on: admin/reports/status)

Check:
- table {locale_file}.last_checked of module B is modified
- table {locale_file}.last_checked of module A is not changed

*) Note on Vistors
Create a visitors.info.yml file with the following content

name: Visitors
type: module
description: 'This module used for displaying a visitors info.'
package: Visitors
core: 8.x
configure: admin/config/system/visitors
version: 8.x-1.0
project: visitors

**) Note on Image Desaturate Formatter
Make sure the image_desaturate_formatter.info.yml file of the module includes the following lines:

project: image_desaturate_formatter
version: 8.x-1.0-alpha1

User interface changes

Changed cron update frequency from daily/weekly to weekly/monthly. Defaults to: None (Manually).

API changes

None

#1191488: META: Integrate l10n_update functionality in core

CommentFileSizeAuthor
#41 locale-cron-translation-update-1998056-41.patch98.8 KBSutharsan
#41 interdiff-1998056-38-41.txt1001 bytesSutharsan
#38 locale-cron-translation-update-1998056-38.patch98.6 KBSutharsan
#38 interdiff-1998056-35-38.txt2.26 KBSutharsan
#35 interdiff-1998056-33-35.txt3.48 KBpenyaskito
#35 locale-cron-translation-update-1998056-35.patch98.97 KBpenyaskito
#33 locale-cron-translation-update-1998056-33.patch98.89 KBSutharsan
#33 interdiff-1998056-32-33.txt1.71 KBSutharsan
#32 locale-cron-translation-update-1998056-32.patch99.23 KBSutharsan
#27 locale-cron-translation-update-1998056-27.patch99.05 KBSutharsan
#27 interdiff-1998056-25-27.txt2.19 KBSutharsan
#27 locale-no-core-update--do-not-test.patch1.76 KBSutharsan
#23 locale-cron-translation-update-1998056-23.patch98.76 KBSutharsan
#23 interdiff-1998056-22-23.txt37.09 KBSutharsan
#22 locale-cron-translation-update-1998056-22.patch72.3 KBSutharsan
#22 interdiff-1998056-17-22.txt36.29 KBSutharsan
#17 locale-cron-translation-update-1998056-17.patch71.78 KBSutharsan
#17 interdiff-1998056-16-17.txt6.48 KBSutharsan
#16 locale-cron-translation-update-1998056-16.patch74.57 KBSutharsan
#16 interdiff-1998056-12-16.txt17.28 KBSutharsan
#12 locale-cron-translation-update-1998056-12.patch82.44 KBSutharsan
#12 interdiff-1998056-11-12.txt14.63 KBSutharsan
#11 locale-cron-translation-update-1998056-11.patch72.82 KBSutharsan
#11 interdiff-1998056-8-11.txt8.95 KBSutharsan
#9 interdiff-1998056-2-8.txt3.34 KBclemens.tolboom
#8 locale-cron-translation-update-1998056-8.patch64.02 KBclemens.tolboom
#7 locale-cron-translation-update-1998056-5.patch62.99 KBclemens.tolboom
#5 locale-cron-translation-update-1998056-5.patch62.99 KBclemens.tolboom
#5 interdiff-1998056-2-5.txt1.78 KBclemens.tolboom
#2 locale-cron-translation-update-1998056-2.patch62.06 KBSutharsan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan’s picture

Issue tags: +D8MI, +language-ui

Tagging

Sutharsan’s picture

Status: Active » Needs review
Issue tags: +sprint
FileSize
62.06 KB

A work in progress. The patch does the following:

  • Simplify the batch functions:
    - Call each batch operation with only simple lists of project names and language codes. Use of $sources as parameter has been eliminated. Batch functions are now called the same way.
    - Removes the 'compare' and 'save' batch functions. This makes the batch functions more loosely coupled. And this will fix #1884996: Progress of Import translations automatically during installation goes to warp speed at the end after 60% too.
  • Make the translation directory required. I discussed this previously with Gabor in IRC. It makes the download/imprt code much simpler and does not harm the user experience. Remote translations files are always stored (call it cached) locally. Removing the translation files afterwards will not harm.
  • First code for cron update added, but not yet functional. To be used for cron, the batch functions must become available as queue functions. The above changes to make the batch functions self encapsulated were needed to prepare the operations for queue.
  • Some related changes to test scripts.

Still to do:

  • Create queue function that share the code of the current batch function for check, download and import.
  • Make cron trigger the queue to check and update interface translations.
  • Test scripts for the above (if possible to test queue)
Gábor Hojtsy’s picture

Woot, thanks for working on this! :) Do you think there are good manual steps we can apply to test this?

clemens.tolboom’s picture

@Gábor Hojtsy manual testing will look like

  1. run cron.
  • Placing items in the queue.
  • The queue table item count increases.
  • in this same cron run some items are processed thus reducing the queue item count
  • 2 run cron again :p
  • I currently run into trouble deciding what to place in the queue to prevent duplicate items.

    clemens.tolboom’s picture

    Added some scaffolding for hook_queue_info()

    I did something similar in graphapi_class which seems to work nicely.

    Sutharsan’s picture

    @Gábor, nothing to test yet. Batch is not yet functional. I have continued working and are making progress. But nothing to test before the end of the weekend. #2 is merely groundwork to make it possible to use queue to execute the update tasks.

    @Clemens, Aggregator module uses a flag 'queued' in the database. From reading the queue documentation, I think we should use a 'reliable' queue in combination with a 'queued' timestamp in the database.

    There are two kinds of queue backends available: reliable, which preserves the order of messages and guarantees that every item will be executed at least once. The non-reliable kind only does a best effort to preserve order in messages and to execute them at least once but there is a small chance that some items get lost.

    clemens.tolboom’s picture

    Attached is a working queue implementation. I'm not sure whether cron is called by the testbot let alone the queues are emptied.

    The download tasks fails obviously with message like "Translation file not found: http://ftp.drupal.org/files/translations/8.x/drupal/drupal-8.x-dev.af.po."

    We need that PO download tool to fake d7 po files.

    @Sutharsan thinks loading one .po can take too much time on a slow environment.

    Patch (netbeans) adds some whitespace problems and fixed others :/

    clemens.tolboom’s picture

    Removed whitespace introduced ... I was too busy ;/ (sorry testbot)

    clemens.tolboom’s picture

    FileSize
    3.34 KB

    And the interdiff between #2 and #8

    Sutharsan’s picture

    Assigned: Unassigned » Sutharsan
    Status: Needs review » Needs work

    @Clemens, Interesting idea to use batch function as-is. I was thinking to split each function up. A core function (checks remote, checks local, downloads, imports) and a batch wrapper which does the interface for batch (reporting results via $context). But it seems a bit hackish to me to re-use the batch function here.

    +++ b/core/modules/locale/locale.moduleundefined
    @@ -502,15 +502,77 @@ function locale_themes_disabled($themes) {
    + * Implements hook_queue_info().
    ...
    +function locale_queue_info() {
    +  $queues['locale_translation_update'] = array(
    +    'title' => t('Updates the translations'),
    +    'worker callback' => 'locale_translation_update_worker',
    +    'cron' => array(
    +      'time' => 60,
    +    ),
    +  );
    +  return $queues;
    +}
    

    This way of calling drupal_cron_run, which uses this queue_info data above, only calls each callback once. This will not work for the import job. It may need to be called multiple times and therefore requires a different caller. So we can not use local_queue_info.

    I am not sure whether we should make four queue tasks (check remote, check local, download, import) or combine them in one. The 'reliable' queue will ensure the order. But it will not prevent that a new task is be called before the first one is completed. If cron is triggered before the last task is completed it will grab the next task from the queue. Or is this prevented by cron itself?

    thinks loading one .po can take too much time on a slow environment.

    Importing large files takes too long for a single queue operation. But we can take care of this. Download large files, may take too long, but I don't see an alternative. At best we could detect and report it.

    Sutharsan’s picture

    Status: Needs work » Needs review
    FileSize
    8.95 KB
    72.82 KB

    More groundwork: Splitting locale_translate_batch_import() into a standalone iterative importer locale_translate_file_import() and a batch operation wrapper locale_translate_batch_import().

    Sutharsan’s picture

    This is a rough patch for evaluation. A working solution the cron can be found in locale.queue.inc. This is not the final location, but just the place I stored my woking code. The starting poing is _locale_cron() which will become locale_cron(). The final code will be a mix between Clemens' solution in #8 and this one.

    Additional changes:

    • User interface for update frequency, changed from "Never (manual), Daily, Weekly" to "Never (manual), Weekly, Monthly". I think daily translation updates are not realistic.
    • locale_translation_batch_status_fetch_local() is now called with one project/language at a time.
    • The change in locale_file table, can be ignored. It will be removed in the next patch.

    The code can be tested manually by calling

    module_load_include('queue.inc', 'locale');
    _locale_cron();
    

    To watch the code in progress:

    • Change the cron execution time limit from 15 sec to 2 sec (in locale.queue.inc): $end = time() + 15; into $end = time() + 2;
    • Change the batch import size from 200 to 20 or so (in locale.batch.inc): 'items' => 200, into 'items' => 20,
    • Change the timestamp of downloaded files in the translations directory. Then run _locale_cron(): $ touch sites/default/files/translations/example.nl.po
    • Watch the content of the {queue} table being reduced with consecutive 'cron' calls.

    Status: Needs review » Needs work

    The last submitted patch, locale-cron-translation-update-1998056-12.patch, failed testing.

    clemens.tolboom’s picture

    @Sutharsan

    So we can not use local_queue_info.

    Huh? A queue is 'just a list of tasks'. So when a tasks 'grows over our head' we stop continuing with the tasks at hand and schedules it's sequel.

    As I understand we may have some bigger tasks. And we can decide it's to big?

    #pseudo code
    function locale_translation_update_worker($item) {
      $result = inspect_item($item);
      if (is_big($result)) {
        process_item($item, 200);
        schedule_new_task($item, 200);
      }
      else {
        process_item($item);
      }
    }
    
    Sutharsan’s picture

    Oops, I thought I read locale_cron_queue_info which is processed by drupal_cron_run() and that cannot be used. But of course we can build our own queue info list.

    Big tags should be build in such a way that they can be called multiple times. The import taks can be broken down. I'm not sure if download can be broken down. The best we can do there is to increase the lease and increase php execution time.

    +++ b/core/modules/locale/locale.queue.incundefined
    @@ -0,0 +1,178 @@
    +function locale_cron_process_queue() {
    +  $end = time() + 15;
    +  $queue = Drupal::queue('locale_translation', TRUE);
    +  while (time() < $end && ($item = $queue->claimItem(300))) {
    +    if (locale_translation_update_queue_worker($item->data)) {
    +      $queue->deleteItem($item);
    +
    +     // Unset queued flag.
    +    ...
    +    }
    +    else {
    +      $queue->releaseItem($item);
    +    }
    +  }
    +}
    

    This is how we can call queue functions multiple times. When the queue worker returns TRUE the queue task is completed and it can be removed from the queue. When the worker returns FALSE, the queue task is released and thus not deleted from the queue. in the next loop (or the next cron run) will be claimed and executed. This repeats until the worker returns TRUE.

    Sutharsan’s picture

    Status: Needs work » Needs review
    FileSize
    17.28 KB
    74.57 KB

    @Clemens, I used your suggestion to completely re-use the batch operation functions and to create a new queue task when a batch operation is not finished.
    It still needs some more work to get the locale_translation_update_worker() working. It must correctly handle the $context for the batch operations.

    I'm not yet satisfied how to prevent tasks to be added to the queue on every cron run. Currently using state()->set('locale_translation_in_queue', $in_queue);. The problem lies in the projects for which no translation was found (e.g. of dev releases). For these there is no record in {locale_file} to keep a last_checked timestamp.

    The patch is still work in progress, but suitable for evaluation.

    Sutharsan’s picture

    Cron is working. The rough phase is now completed :) It's review time!

    Todo:

    • Update the issue summary
    • Describe manual test procedure
    • Write unit tests
    • Improve update status tracking for projects without translation (e.g. dev release)
    • Reduce memory usage of large import process (1M for D7 core import)
    • Resolve various inline todo's
    Schnitzel’s picture

    I took a 15min look at the code and tried to understand it, not easy but possible, definitely needs more review, here two things which came up:

    +++ b/core/modules/locale/locale.batch.incundefined
    @@ -56,7 +59,6 @@ function locale_translation_batch_status_fetch_remote($source, &$context) {
         $context['message'] = $t('Checked translation for %project.', array('%project' => $source->project));
    
    @@ -67,101 +69,31 @@ function locale_translation_batch_status_fetch_remote($source, &$context) {
    +  $context['message'] = $t('Checked translation for %project.', array('%project' => $source->project));
    
    +++ b/core/modules/locale/locale.queue.incundefined
    @@ -0,0 +1,178 @@
    +        'callback' => 'locale_translation_batch_status_fetch_remote',
    

    maybe write here from where it was checked? So Local or Remote

    locale.queue.inc
    

    I'm confused, for what is this file?

    Schnitzel’s picture

    I took a 15min look at the code and tried to understand it, not easy but possible, definitely needs more review, here two things which came up:

    +++ b/core/modules/locale/locale.batch.incundefined
    @@ -56,7 +59,6 @@ function locale_translation_batch_status_fetch_remote($source, &$context) {
         $context['message'] = $t('Checked translation for %project.', array('%project' => $source->project));
    
    @@ -67,101 +69,31 @@ function locale_translation_batch_status_fetch_remote($source, &$context) {
    +  $context['message'] = $t('Checked translation for %project.', array('%project' => $source->project));
    
    +++ b/core/modules/locale/locale.queue.incundefined
    @@ -0,0 +1,178 @@
    +        'callback' => 'locale_translation_batch_status_fetch_remote',
    

    maybe write here from where it was checked? So Local or Remote

    locale.queue.inc
    

    I'm confused, for what is this file?

    Sutharsan’s picture

    maybe write here from where it was checked? So Local or Remote

    There is very little added value to make two different texts. The chance that the message for checking local translation files is displayed is minimal because it takes so very little time to check the timestamp of a locally stored file.

    I'm confused, for what is this file?

    Hm, apparently I forgot to remove it from the patch. It was where I started working on the queue.

    Sutharsan’s picture

    @Schnitzel How can I make the code more easy to understand? What made it hard to understand? Documentation, function names, variable names, functions in different files? etc.

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

    Sutharsan’s picture

    The last big changes.

    • The final cleanup of the batch operations: locale_translation_batch_status_fetch_remote() and locale_translation_batch_status_fetch_local() combined into locale_translation_batch_status_check(). One batch operation to check the status of remote and local translation files. Operation locale_translation_batch_fetch_sources() is removed.
    • The translation status of all project+language combinations are now tracked. The timestamp of each combination is not stored in the locale_files table. Checking for files to be updated during cron in locale_cron_fill_queue() is much simplified.
    • Comments have been reviewed
    • The locale.queue.inc work file is removed from the patch.

    All together this patch reduces the number of batch operations to three: Check status, Download file, Import file. Each batch operation now takes care of storing its resulting translation status. Storage and retrieval of translation status is more concentrated. I'm planning to change this into a class in #1842380: Convert $source object to a TranslatableProject class to make it easier to handle and less complex. And finally, where this patch was about, to enable updating translations using cron. A queue implementation which uses the existing batch functions.

    My final task is to add test coverage for the added cron functions.

    Side note: the patch removes approx. 100 lines of code but does more ;)

    Sutharsan’s picture

    Tests for cron update are added and placed in the LocaleUpdateCronTest class.
    The test helper functions, which are now used in three test classes, are moved to a new LocaleUpdateBase class.
    Note that the interdiff is a bit poluted by a merge fault (-<<<<<<< HEAD and further) and will not apply.

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

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

    The last submitted patch, locale-cron-translation-update-1998056-23.patch, failed testing.

    Sutharsan’s picture

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

    Code review really nice, only minor nitpicks that are non-blockers.

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

    Typo: s/containting/containing/g

    +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateCronTest.phpundefined
    @@ -0,0 +1,126 @@
    +    // @todo Make follow-up issue: Mask the 'drupal' project so it will not show up in this test. When fixed, we expect only 3 tasks in the queue.
    +    //$this->assertEqual($queue->numberOfItems(), 3, 'Queue holds tasks for one project.');
    ...
    +    // @todo Make follow-up issue: Mask the 'drupal' project so it will not show up in this test. When fixed, we expect only 3 tasks in the queue.
    +    //$this->assertEqual($queue->numberOfItems(), 3, 'Queue holds tasks for one project.');
    +    $this->assertEqual($queue->numberOfItems(), 6, 'Queue holds tasks for two projects.');
    

    Needs a follow-up?

    I will manually test it now.

    Sutharsan’s picture

    Patch rerolled to include changes introduced by #1813762: Introduce unified interfaces, use dependency injection for interface translation. Typo corrected.
    I have a patch to "mask core" based on #1883154: Check version before update but I have to rethink its impact before I include it. Attached here for archiving purpose.

    penyaskito’s picture

    I tried to test this one yesterday, but didn't worked for me or I did something wrong. Steps above from top of my head:

    • I applied both patches from #27.
    • I installed D8 from scratch in English.
    • I enabled all multilingual modules.
    • I added Spanish to my site and added the language switcher block.
    • Downloaded image_desaturate_formatter module (visitor module doesnt work anymore for the purpose, it has no yaml info file).
    • Edited image_desaturate_formatter.info.yaml for checking that it had the right version.
    • Enabled image_desaturate_formatter.
    • Downloaded image_desaturate_formatter translation from l.d.o to sites/default/files/translations.
    • Check the translation status, it said that there were translations available, I imported them.
    • Configured cron to 1 hour in cron settings.
    • Run cron. Nothing happened.
    • "Touched" image_desaturate_formatter po file.
    • Run cron. Nothing happened (no timestamp updated in locale_file image_desaturate_formatter row).
    • Updated timestamp to 0 in mysql.
    • Run cron. Nothing happened (no timestamp updated in locale_file image_desaturate_formatter row).
    • ... Some hours later ...
    • Run cron. Nothing happened (no timestamp updated in locale_file image_desaturate_formatter row).
    • Check the translation status (timestamp updated in locale_file image_desaturate_formatter row), it said that there were translations available, I imported them.
    Sutharsan’s picture

    Make sure that in translation settings you set update interval, weekly and monthly both will do. Setting it to manual will disable cron update.

    penyaskito’s picture

    That was the missing piece, thanks Sutharsan!

    Sutharsan’s picture

    I've created a follow-up issue with patch for #27 No. 2: #2021749: Core should not force latest version number translations on sites

    Sutharsan’s picture

    Patch re-rolled.

    Sutharsan’s picture

    comments in #26 processed in new patch.

    Status: Needs review » Needs work

    The last submitted patch, locale-cron-translation-update-1998056-33.patch, failed testing.

    penyaskito’s picture

    Status: Needs work » Needs review
    FileSize
    98.97 KB
    3.48 KB

    Updated for HEAD. get_t has been removed and File entity properties access methods have been added.
    I'm having issues locally running tests, let's see what the bot thinks.

    Status: Needs review » Needs work

    The last submitted patch, locale-cron-translation-update-1998056-35.patch, failed testing.

    YesCT’s picture

    YesCT’s picture

    Issue summary: View changes

    Updated issue summary: Manual test, User interface changes

    Sutharsan’s picture

    Issue summary: View changes

    Updated issue summary.

    Sutharsan’s picture

    Status: Needs work » Needs review
    FileSize
    2.26 KB
    98.6 KB

    Modified the cron test after #2021749: Core should not force latest version number translations on sites landed.
    Reverted the use of getChangedTime() , added in #35. This caused the local update interface test to fail.

    valdo’s picture

    Status: Needs review » Reviewed & tested by the community

    I have followed the instructions in issue description and tested manually, patch worked as desired.

    Not mentioned was that the Update Manager module needs to be enabled to be able to follow the test instructions. There is an info text about this case missing in the "Available translation updates" page (admin/reports/translations) - I have raised an issue how to improve the user experience there : #2026871: Available translation updates page doesn't warn user if Update Manager module is disabled

    Also the Interface Translation and Language modules need to be enabled, and a language has to be added at admin/config/regional/language.

    Also it would be cool if the function locale_cron_fill_queue() included a comment about when/how the created queue will be processed (that hook_queue_info() is used for processing). A note about this could possibly go also to locale_cron() where locale_cron_fill_queue() is called.

    YesCT’s picture

    Status: Reviewed & tested by the community » Needs work

    Needs work for the inline documentation needed in locale_cron_fill_queue().

    Sutharsan’s picture

    Status: Needs work » Needs review
    FileSize
    1001 bytes
    98.8 KB

    Comment added as per remark 3 in #40.

    Gábor Hojtsy’s picture

    Status: Needs review » Reviewed & tested by the community

    Looks like a good straightforward fix.

    Gábor Hojtsy’s picture

    Issue summary: View changes

    Summary update

    Gábor Hojtsy’s picture

    Gábor Hojtsy’s picture

    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed c1f2181 and pushed to 8.x. Thanks!

    Status: Fixed » Closed (fixed)

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

    Gábor Hojtsy’s picture

    Issue tags: -sprint

    Yay!

    andypost’s picture

    andypost’s picture

    Issue summary: View changes

    Updated issue summary.