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
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#41 | locale-cron-translation-update-1998056-41.patch | 98.8 KB | Sutharsan |
#41 | interdiff-1998056-38-41.txt | 1001 bytes | Sutharsan |
#38 | locale-cron-translation-update-1998056-38.patch | 98.6 KB | Sutharsan |
#38 | interdiff-1998056-35-38.txt | 2.26 KB | Sutharsan |
#35 | interdiff-1998056-33-35.txt | 3.48 KB | penyaskito |
Comments
Comment #1
Sutharsan CreditAttribution: Sutharsan commentedTagging
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedA work in progress. The patch does the following:
- 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.
Still to do:
Comment #3
Gábor HojtsyWoot, thanks for working on this! :) Do you think there are good manual steps we can apply to test this?
Comment #4
clemens.tolboom@Gábor Hojtsy manual testing will look like
I currently run into trouble deciding what to place in the queue to prevent duplicate items.
Comment #5
clemens.tolboomAdded some scaffolding for hook_queue_info()
I did something similar in graphapi_class which seems to work nicely.
Comment #6
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #7
clemens.tolboomAttached 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 :/
Comment #8
clemens.tolboomRemoved whitespace introduced ... I was too busy ;/ (sorry testbot)
Comment #9
clemens.tolboomAnd the interdiff between #2 and #8
Comment #10
Sutharsan CreditAttribution: Sutharsan commented@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.
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?
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.
Comment #11
Sutharsan CreditAttribution: Sutharsan commentedMore groundwork: Splitting
locale_translate_batch_import()
into a standalone iterative importerlocale_translate_file_import()
and a batch operation wrapperlocale_translate_batch_import()
.Comment #12
Sutharsan CreditAttribution: Sutharsan commentedThis 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 becomelocale_cron()
. The final code will be a mix between Clemens' solution in #8 and this one.Additional changes:
The code can be tested manually by calling
To watch the code in progress:
$end = time() + 15;
into$end = time() + 2;
'items' => 200,
into'items' => 20,
_locale_cron()
:$ touch sites/default/files/translations/example.nl.po
Comment #14
clemens.tolboom@Sutharsan
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?
Comment #15
Sutharsan CreditAttribution: Sutharsan commentedOops, 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.
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.
Comment #16
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #17
Sutharsan CreditAttribution: Sutharsan commentedCron is working. The rough phase is now completed :) It's review time!
Todo:
Comment #18
Schnitzel CreditAttribution: Schnitzel commentedI 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:
maybe write here from where it was checked? So Local or Remote
I'm confused, for what is this file?
Comment #19
Schnitzel CreditAttribution: Schnitzel commentedI 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:
maybe write here from where it was checked? So Local or Remote
I'm confused, for what is this file?
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedThere 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.
Hm, apparently I forgot to remove it from the patch. It was where I started working on the queue.
Comment #21
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #21.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #21.1
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #21.2
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #22
Sutharsan CreditAttribution: Sutharsan commentedThe last big changes.
locale_translation_batch_status_fetch_remote()
andlocale_translation_batch_status_fetch_local()
combined intolocale_translation_batch_status_check()
. One batch operation to check the status of remote and local translation files. Operationlocale_translation_batch_fetch_sources()
is removed.locale_cron_fill_queue()
is much simplified.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 ;)
Comment #23
Sutharsan CreditAttribution: Sutharsan commentedTests 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.Comment #23.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #25
Sutharsan CreditAttribution: Sutharsan commented#23: locale-cron-translation-update-1998056-23.patch queued for re-testing.
Comment #26
penyaskitoCode review really nice, only minor nitpicks that are non-blockers.
Typo: s/containting/containing/g
Needs a follow-up?
I will manually test it now.
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedPatch 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.
Comment #28
penyaskitoI tried to test this one yesterday, but didn't worked for me or I did something wrong. Steps above from top of my head:
Comment #29
Sutharsan CreditAttribution: Sutharsan commentedMake sure that in translation settings you set update interval, weekly and monthly both will do. Setting it to manual will disable cron update.
Comment #30
penyaskitoThat was the missing piece, thanks Sutharsan!
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedI've created a follow-up issue with patch for #27 No. 2: #2021749: Core should not force latest version number translations on sites
Comment #32
Sutharsan CreditAttribution: Sutharsan commentedPatch re-rolled.
Comment #33
Sutharsan CreditAttribution: Sutharsan commentedcomments in #26 processed in new patch.
Comment #35
penyaskitoUpdated 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.
Comment #37
YesCT CreditAttribution: YesCT commentedwow. #2021749-5: Core should not force latest version number translations on sites is fixed.
Comment #37.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary: Manual test, User interface changes
Comment #37.1
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #38
Sutharsan CreditAttribution: Sutharsan commentedModified 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.
Comment #39
valdo CreditAttribution: valdo commentedI 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 disabledAlso 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 (thathook_queue_info()
is used for processing). A note about this could possibly go also tolocale_cron()
wherelocale_cron_fill_queue()
is called.Comment #40
YesCT CreditAttribution: YesCT commentedNeeds work for the inline documentation needed in locale_cron_fill_queue().
Comment #41
Sutharsan CreditAttribution: Sutharsan commentedComment added as per remark 3 in #40.
Comment #42
Gábor HojtsyLooks like a good straightforward fix.
Comment #42.0
Gábor HojtsySummary update
Comment #43
Gábor Hojtsy#41: locale-cron-translation-update-1998056-41.patch queued for re-testing.
Comment #44
Gábor Hojtsy#41: locale-cron-translation-update-1998056-41.patch queued for re-testing.
Comment #45
alexpottCommitted c1f2181 and pushed to 8.x. Thanks!
Comment #47
Gábor HojtsyYay!
Comment #48
andypostFollow-up #2048379: multilingual first time admin/config exception Field 'uri' doesn't have a default value into {locale_file}
Comment #48.0
andypostUpdated issue summary.