| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | locale.module |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D8MI, feature freeze, language-ui |
Issue Summary
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.
- Checkout the latest Drupal core code.
- Apply the latest patch from this issue.
- 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.
- Install the site.
- Enable the locale and locale_test_translate module (language module is a dependency)
- 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.)
- Add the Dutch language. The translations of the locale_test_translate module will now get imported.
- 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.)
- 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.
- 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
- 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.
- 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:
- A settings page for translation update at: admin/config/regional/translate/settings
- Future translation status page at admin/reports/translations. Here you can click "Check manually" to update the translation status.
- (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_localeImprove 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 serverChange the batch operation by handling one project/language a the time (#14)
Comments
#1
#2
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.#3
+++ 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.
#4
@ 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
#5
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?
#6
Elevating to major.
#7
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.
#8
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).
#9
Included:
locale_translation_batch_fetch_build().locale_translation_batch_status_build()from locale.batch.in to locale.compare.inc.locale_translation_source_build()).To do:
#10
Merging with changes in the 8.x branche introduces some undesired changes. This is the patch as it was intended.
#11
The last submitted patch, locale-download-import-1804688-10.patch, failed testing.
#12
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
#13
+++ 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.
#14
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.
#15
Re. Berdir's comment in #13
It's been a long time since this discussion, but I think we can drop it now in favor of the using state().
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.
I'll take care of that.
Re. Jose Reyero's comments in #14
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.
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 was working on that too as I needed it for development. I'll share my code in #1804702: Display interface translation status too.
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 complexThe 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.
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.
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-6363698The 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.
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.
For previous discussion on Update module dependency see: http://drupal.org/node/1627006#comment-6107816
#16
Add feature freeze tag.
#17
Patch updated with:
#18
Extracted the project, version and langcode parsing from
locale_translate_batch_build()#19
The last submitted patch, locale-download-import-1804688-18.patch, failed testing.
#20
#18: locale-download-import-1804688-18.patch queued for re-testing.
#21
The last submitted patch, locale-download-import-1804688-18.patch, failed testing.
#22
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.
Then I found the Reports page, but it looked a bit empty and had an Error:
then I clicked "update manually" and it updated somehow:
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?
#23
#18: locale-download-import-1804688-18.patch queued for re-testing.
#24
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?
#25
Good advice for testing:
#26
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)
#27
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.
#28
(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-dropand a manual install is working (of course)#29
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]
#30
The last submitted patch, locale-download-import-1804688-29.patch, failed testing.
#31
Locale module has no .admin.inc (yet) only a .pages.inc. Which contains all is current configuration pages too.
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.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.
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."
#32
I've updated the summary to make clear what happens when update module is not available.
#33
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:
<?phpgrep -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.
#34
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
#35
@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
#36
Both variables from #35
<?php$remote_url = 'http://drupal8.rhum/translations_remote/';
$local_url = 'translations/';
?>
are not used. So I don't get this to work :-(
<?phpfunction locale_tamper_locale_translation_projects_alter()
?>
#37
@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.
#38
yes, and now go to "admin/reports/translations" and there you can update the translations and they will be downloaded :)
#39
@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
and
and
Thanks.
#40
This patch:
The remaining thing to do are added to issue summary.
#41
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.
#42
The last submitted patch, locale-download-import-1804688-40.patch, failed testing.
#43
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: Create a small project API to be used by Update and Locale.
#44
Locale module now automatically imports translations when languages are added and modules are enabled!
Included:
#45
The last submitted patch, locale-download-import-1804688-44.patch, failed testing.
#46
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
#47
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.
#48
I also created #1845016: Decide on a standard English style guide so we can quit making one up ad-hoc in individual issues
#49
Wanted to make a screenshot of the settings page in the patch for anybody interested in doing any quick UI reviews:
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.
#50
Changes:
I'm doing the same here as all the other test files do. No change.
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.
(almost) every function needs this. In the name-space issue (...) this will be addressed. Probably by combining all functions in one or two files.
Left over work. Now made into a test function.
No it is not. I've changed the comments and hopefully clarified it with that.
I don't know. But I don't want to mess with other peoples code which I don't understand.
Changed it to type 'text' as the Aggregator module does. 16K should be enough space ;)
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
#51
Now at needs review.
#52
The last submitted patch, locale-download-import-1804688-50.patch, failed testing.
#53
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
#54
Included:
#55
The last submitted patch, locale-download-import-1804688-54.patch, failed testing.
#56
#54: locale-download-import-1804688-54.patch queued for re-testing.
#57
test bot not reporting back, but manual check shows
http://qa.drupal.org/pifr/test/390193
fail on ModulesDisabledUpgradePathTest
something about server_pattern
#58
Trying to run testbot with a few debug calls in the patch. No idea why it fails.
#59
Go for it!
#60
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.
#61
Testbot does not report back (yet), but the patch passed the test!
#62
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.
#63
trying this manually. will post results soon.
#64
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
extend: enabling locale module
add a language (under configuration). added german.
02. look for something to click on to download translation file
03. click the download link and go to localize.drupal.org, click german po file
04. save the po file in my downloads folder
05. back to site, look for what to do next, try "translated" link
06. there is an import tab. try that.
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.
08. get error when import. sigh.
09. remember reading instructions on install how to install in another language to make a translations directory. try that.
10. try browse and import again after mkdir
11. import is doing something. get some hope.
12. import worked!
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
14. reports page
15. admin/reports/translations shows the todo before the patch
16. /check page did something. I thought it would not since the patch says it will add the /check page
Next
next I'll upload steps to test this patch and the results (screenshots)
#65
@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 ;)
#66
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.
#67
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:
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:
So check that screen to see if the help text there says to go to languages first.
s02b:
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:
I add german, record the video and then pause it so I can see what happened.
s04:
and then
s05:
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
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:
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:
s08:
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:
10. Settings tab
configuration ui translation page now has a settings tab.
See #64 step 13 (no patch)
s10:
s11:
11. Check link
s11b:
s12:
#68
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
#69
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 :))
#70
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.
#71
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.
#72
@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?
#73
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.
#74
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.
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.
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.
clicking on the settings tab shows us the settings page this patch adds.
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
#75
Interdiff of #74 looks sane.
#76
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.
#77
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#78
attiks thanks. My mind was drawing a blank on what b should have been! Doing it now.
#79
here is the b -> strong
#80
Bot is still happy, and code looks good, so back to RTBC
#81
Adding API changes for the issue summary.
#82
The same API changes, but now annotated.
#83
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:
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.
#84
@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);
#85
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:
#86
Go botty
#87
+++ 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?
#88
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.
#89
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?
#90
@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.
#91
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 :)
#92
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.
#93
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.
#94
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.
#95
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.
#96
The last submitted patch, locale-download-import-1804688-95.patch, failed testing.
#97
#95: locale-download-import-1804688-95.patch queued for re-testing.
#98
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
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.
#99
Help text changes to grammar, to be general, have improved word choices.
#100
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.
#101
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.
#102
The last submitted patch, locale-download-import-1804688-101.patch, failed testing.
#103
#101: locale-download-import-1804688-101.patch queued for re-testing.
#104
Great changes. The tests failed for unrelated reasons earlier, rerunning them showed they work.
#105
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 299Call 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
:(
#106
The class name is null btw.
#107
@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).
#108
Yes, just noticed this is not Views, but 'ViewSubscriber' :-(
#109
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
fixesmakes sure no download/import or cleanup action is taken when no language is enabled.#110
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...
#111
Oops, I didn't intend to change the status
#112
The last submitted patch, locale-download-import-1804688-106.patch, failed testing.
#113
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!StatementPrefetch.php/function/StatementPrefetch%3A%3AfetchObject/8
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)
#114
I'm using 5.3.6 and I did not get the errors.
#115
#109: locale-download-import-1804688-106.patch queued for re-testing.
#116
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.
#117
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.
#118
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.
#119
(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.#120
@clemens.tolboom Yeah, #101 is related to that. [edit: changed 110 to 101. and fixed link.]
#121
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?
#122
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.
#123
Since #85 the directory is created when Locale module is installed (or at least it should be).
#124
#109: locale-download-import-1804688-106.patch queued for re-testing.
#125
Some fixes:
#126
Changes look good to me, great fixes :)
#127
#125: locale-download-import-1804688-125.patch queued for re-testing.
#128
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.
#129
oops. ;P
#130
@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.
#131
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.
#132
@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.
#133
Nice. :D Well-earned pancakes, for sure. :D
#134
Updated issue summary with help text follow up issues.
#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
#135
Maybe we should invent a "congrats" Button, so cheering for stuff like this does not spam so hard :P
#136
Thanks all, especially Sutharsan! This was a truly great effort :)
#137
Automatically closed -- issue fixed for 2 weeks with no activity.
#138
#125 could be relavent to #1974048: Limited display of languages when going back in installer is confusing