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_locale
Improve and extend fault handling for download import process. (#14)
Modify the import process to only save the file status if the filename matches the l.d.o file name pattern. (#14)
Modify the import process to use the batch operation when an language is enabled. (#14)
Review the bulk.inc code for other batch download/import integration issues. e.g. when a module is enabled. see locale_system_update() (#14)
On the settings page: Add a manual update link to the Reports page. (#22)
On the settings page: Use different wording for 'remote' or explain. (#22)
Comments by attiks #41
User interface changes
See recent screenshots in #74 and #76
API changes
This summary of api changes is a work in progress. More detail will be added when making the change notice.
Detailed list of API changes.
Followup issues or architectural changes
#1842362: Replace locale_project table and improve caching Change table {locale_project} to state() variable. (#14)
#1842380: Convert $source object to a TranslatableProject class Convert $source-file to a typed class. (#13)
#1842380: Convert $source object to a TranslatableProject class Convert $source to a TranslatableProject class (#13)
#1842380: Convert $source object to a TranslatableProject class Simplify the way the $source data is handled within the batch operations (#14)
#1777106: Make check for out of date project update information more robust for sites that are not running cron regularly (follow-up)
#1834298: Unify name space in Locale module
#1787520: Translations not imported on installation
#1861360: Refactor localization update test so people can just enable the test module to test
#1861934: Rework help text for UI translations and importing po files
#1861930: Use "Drupal translations website" instead of Drupal translation server or Community translation server
Change the batch operation by handling one project/language a the time (#14)
Comment | File | Size | Author |
---|---|---|---|
#125 | locale-download-import-1804688-125.patch | 198.61 KB | Sutharsan |
#125 | interdiff-1804688-106-125.txt | 4.99 KB | Sutharsan |
#116 | debugpatch.txt | 1.11 KB | Gábor Hojtsy |
#109 | locale-download-import-1804688-106.patch | 196.81 KB | Sutharsan |
#109 | interdiff-1804688-101-106.txt | 2.89 KB | Sutharsan |
Comments
Comment #1
Gábor HojtsyComment #2
Sutharsan CreditAttribution: Sutharsan commentedThis 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.Comment #3
BerdirAccording 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.
Comment #4
Sutharsan CreditAttribution: Sutharsan commented@ 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
Comment #5
Gábor HojtsyReviewed 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?
Comment #6
Gábor HojtsyElevating to major.
Comment #7
Sutharsan CreditAttribution: Sutharsan commentedMoving 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.
Comment #8
Gábor HojtsyYeah, 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).
Comment #9
Sutharsan CreditAttribution: Sutharsan commentedIncluded:
locale_translation_batch_fetch_build()
.locale_translation_batch_status_build()
from locale.batch.in to locale.compare.inc.locale_translation_source_build()
).To do:
Comment #10
Sutharsan CreditAttribution: Sutharsan commentedMerging with changes in the 8.x branche introduces some undesired changes. This is the patch as it was intended.
Comment #12
BerdirWas 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
Comment #13
BerdirCan 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.
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.
Comment #14
Jose Reyero CreditAttribution: Jose Reyero commentedI'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.
Comment #15
Sutharsan CreditAttribution: Sutharsan commentedRe. 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 complex
The main purpose of the array is to store the translation status of remote and local translation sources after the batch operation. It also contains data from the locale_project table as the initial data in the batch operation. The current translation status is stored there for convenience, but it duplicates the data in the locale_files table.
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
Comment #16
Gábor HojtsyAdd feature freeze tag.
Comment #17
Sutharsan CreditAttribution: Sutharsan commentedPatch updated with:
Comment #18
webflo CreditAttribution: webflo commentedExtracted the project, version and langcode parsing from
locale_translate_batch_build()
Comment #20
webflo CreditAttribution: webflo commented#18: locale-download-import-1804688-18.patch queued for re-testing.
Comment #22
Schnitzel CreditAttribution: Schnitzel commentedInstalled 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?
Comment #23
webflo CreditAttribution: webflo commented#18: locale-download-import-1804688-18.patch queued for re-testing.
Comment #24
Schnitzel CreditAttribution: Schnitzel commentedmhh 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?
Comment #25
Gábor HojtsyGood advice for testing:
Comment #26
Jose Reyero CreditAttribution: Jose Reyero commentedI 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)
Comment #27
Sutharsan CreditAttribution: Sutharsan commentedThe 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.
Comment #27.0
Sutharsan CreditAttribution: Sutharsan commentedLink to #1191488 restored
Comment #28
clemens.tolboom(tiny observation)
Weird the UI is split from this as core behavior on page http://drupal.d8/admin/reports/translations is now empty.
switching to the patch (branch) I can run update.php then it fails miserably.
(this is probably a core problem)
[edit]
Doing a
drush --yes @drupal.d8 sql-drop
and a manual install is working (of course)Comment #29
clemens.tolboomHmmm ... 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]
Comment #31
Sutharsan CreditAttribution: Sutharsan commentedLocale 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'.
[edit]As Jose already noted: "- More 'watchdog' for download / import errors."
Comment #31.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #32
clemens.tolboomI've updated the summary to make clear what happens when update module is not available.
Comment #33
clemens.tolboomSorry 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:
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.
Comment #33.0
clemens.tolboomOrdered the steps and added a few
Comment #34
clemens.tolboomDarn. 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
Comment #35
clemens.tolboom@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
Comment #35.0
clemens.tolboomMade required strong :-/
Comment #36
clemens.tolboomBoth variables from #35
are not used. So I don't get this to work :-(
function locale_tamper_locale_translation_projects_alter()
is filling admin/reports/translations so that hurdle is taken :-)Comment #37
Gábor Hojtsy@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.
Comment #38
Schnitzel CreditAttribution: Schnitzel commentedyes, and now go to "admin/reports/translations" and there you can update the translations and they will be downloaded :)
Comment #39
clemens.tolboom@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.
Comment #40
Sutharsan CreditAttribution: Sutharsan commentedThis patch:
The remaining thing to do are added to issue summary.
Comment #40.0
Sutharsan CreditAttribution: Sutharsan commentedAdded link to #35
Comment #41
attiks CreditAttribution: attiks commentedI 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.
is the todo still needed? If so can you create a follow-up issue and link to it.
use ' instead of "
locale_translate_update_file_history? not locale_translation.
comments are the same for all 4 variables
an --> and
too long
comments says non customized, code says customized.
... remote locations.
firt --> first
can we move this into the functions that need this functionality.
store --> Store
is this for a follow-up?
.. --> .
change the See ... to a real @see
follow-up issue
can you remove the comment so the test are using the non-debug code.
can we move this into the functions.
dito
function description.
Comment #42.0
(not verified) CreditAttribution: commentedAdded 'things to do'
Comment #42.1
Sutharsan CreditAttribution: Sutharsan commentedAdded todo details
Comment #42.2
Sutharsan CreditAttribution: Sutharsan commentedUpdated the how to test secgtion.
Comment #43
Sutharsan CreditAttribution: Sutharsan commentedIn IRC I discussed with Gabor some of the outstanding comments above.
Consider changing namespace from locale_translation and locale_translate to locale_update. Consider creating a new module. (#14)
* A follow-up issue has been created to unify the name space. Creating a separate module module is not supported by Gabor as a separate module may be overlooked which harms the experience when enabling new modules and languages. #1834298: Unify name space in Locale module
Consider removing the 'local'/'remote and local' choice from the interface and use 'remote and local' only. Consider removing it from the code too. (#14)
* The 'local' option is required for multi sites and is important for deployment of translations in a staging / production environment. Removing it is not supported by Gabor.
Consider making the translate process independent of update module or create a locale_update module. (#14)
* Jose Reyero already started working on this at #1832946: Runtime translation download fallback works different from installer translation download fallback.
Comment #43.0
Sutharsan CreditAttribution: Sutharsan commentedUpdate 'how to test' section.
Comment #43.1
Sutharsan CreditAttribution: Sutharsan commented'Needs discussion' items removed
Comment #43.2
Sutharsan CreditAttribution: Sutharsan commentedProgress marked in todo list
Comment #43.3
Sutharsan CreditAttribution: Sutharsan commentedProgress marked in todo
Comment #43.4
Sutharsan CreditAttribution: Sutharsan commentedUpdated 'Things to do'.
Comment #43.5
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #43.6
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #44
Sutharsan CreditAttribution: Sutharsan commentedLocale module now automatically imports translations when languages are added and modules are enabled!
Included:
Comment #46
attiks CreditAttribution: attiks commentedMost of this is nitpicking, so it is a bit long ;-)
The language the language
double?
comment != class name
Set --> Sets
Add -> Adds
string missing
What if either $source or $target contains a " (double quote)
Sets up
Sets up ... sets up ...
This is only used once, and should probably be FALSE.
Better to put it inline.
This is only used once, and should probably be TRUE. Better to put it inline.
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.
Checks
Checks
This custom project definition is applied using a hook_locale_translation_projects_alter implementation in the locale_test module.
'remote only' isn't tested by this function
Isn't the config set twice?
Tests
Tests
firt --> first
Tests
firt --> first
Tests
Tests
Tests
Check if there is ...
Tests ... language ...
langague --> language
Check if ...
missing .
missing .
Check if ...
Reworded sentence: "Check that the Dutch translation is gone."
This function is commented out, why?
for --> of
... when the language ...
Ensure the ...
Ensure the ...
Missing new line
Can we move this inside the functions that need this?
... compared ...
comment is > 80c
Loads
no period as the end
This batch operation ...
no period at the end
Don't we have a constant for this?
Can type be something else?
Comment too long and missing a part
Keep the data of imported source. In following batch operation
destination --> translations
So the file is downloaded to /tmp, moved to translations and then deleted? If so, why the need to move it?
This comment is a bit redundant, the code is clear.
No periods
... the data of the imported ...
Now it will ...
use --> used
boolean
array
Downloads
missing .
itteration --> iteration
itteration --> iteration
Why do we check for is_numeric?
Comment too long
for ever never? ;-)
Can this be moved inside the functions needing it?
Builds
Comment is too long.
Can this one be moved? If not can we add a comment?
Builds
@todo still needed?
Builds
Is 255 long enough to handle all remote files?
See previous comment
Gets
Updates
object missing
Deletes
Saves
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.
can this be removed?
user experience -> UX so the comment no longer exceeds 80 c
Gets
no period and (i think) @see should be at the end of the doc block
The inline "See ... " can be removed
Clears
Loads
no period
Builds
No period
Checks
We use object in doc blocks.
Builds
object
contains --> contain
will will --> will
is --> if
object
Returns
Missing new line
files --> file
Comment #47
attiks CreditAttribution: attiks commentedShort summary of the non-comment ones:
This function is commented out, why?
Can we move this inside the functions that need this? If not this deserves a comment to explain why.
Don't we have a constant for this?
Can type be something else?
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.
Why do we check for is_numeric? Can you add a comment?
Can we move this inside the functions that need this? If not this deserves a comment to explain why.
Can we move this inside the functions that need this? If not this deserves a comment to explain why.
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.
See previous comment
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.
Comment #48
attiks CreditAttribution: attiks commentedI also created #1845016: Decide on a standard English style guide so we can quit making one up ad-hoc in individual issues
Comment #49
Gábor HojtsyWanted 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.
Comment #50
Sutharsan CreditAttribution: Sutharsan commentedChanges:
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
Comment #51
Sutharsan CreditAttribution: Sutharsan commentedNow at needs review.
Comment #53
attiks CreditAttribution: attiks commentedI 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.
"* Set up " -> "* Sets up "
is are --> is
Comment #54
Sutharsan CreditAttribution: Sutharsan commentedIncluded:
Comment #56
Sutharsan CreditAttribution: Sutharsan commented#54: locale-download-import-1804688-54.patch queued for re-testing.
Comment #57
YesCT CreditAttribution: YesCT commentedtest bot not reporting back, but manual check shows
http://qa.drupal.org/pifr/test/390193
fail on ModulesDisabledUpgradePathTest
something about server_pattern
Comment #58
Sutharsan CreditAttribution: Sutharsan commentedTrying to run testbot with a few debug calls in the patch. No idea why it fails.
Comment #59
Sutharsan CreditAttribution: Sutharsan commentedGo for it!
Comment #60
Sutharsan CreditAttribution: Sutharsan commentedApparently 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.
Comment #61
Sutharsan CreditAttribution: Sutharsan commentedTestbot does not report back (yet), but the patch passed the test!
Comment #62
attiks CreditAttribution: attiks commentedOnly 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.
Comment #63
YesCT CreditAttribution: YesCT commentedtrying this manually. will post results soon.
Comment #64
YesCT CreditAttribution: YesCT commentedused 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)
Comment #65
Sutharsan CreditAttribution: Sutharsan commented@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 ;)
Comment #66
Sutharsan CreditAttribution: Sutharsan commentedI 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.
Comment #67
YesCT CreditAttribution: YesCT commentedtrying 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:
Comment #68
YesCT CreditAttribution: YesCT commentedneeds 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
Comment #69
tatarbjI 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 :))
Comment #70
czigor CreditAttribution: czigor commentedI 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.
Comment #71
Sutharsan CreditAttribution: Sutharsan commentedThe 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.
Comment #72
Gábor Hojtsy@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?
Comment #73
Sutharsan CreditAttribution: Sutharsan commentedUser 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.
Comment #74
YesCT CreditAttribution: YesCT commentedtesting 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
Comment #75
attiks CreditAttribution: attiks commentedInterdiff of #74 looks sane.
Comment #76
YesCT CreditAttribution: YesCT commentedI 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.
Comment #76.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #76.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary with links to screenshots, use issue summary template, get ready for committer to review by making it really obvious the pain point and the slick solution.
Comment #77
attiks CreditAttribution: attiks commentedLooks good, but ...
can you use a
<strong>
tagComment #78
YesCT CreditAttribution: YesCT commentedattiks thanks. My mind was drawing a blank on what b should have been! Doing it now.
Comment #79
YesCT CreditAttribution: YesCT commentedhere is the b -> strong
Comment #80
attiks CreditAttribution: attiks commentedBot is still happy, and code looks good, so back to RTBC
Comment #81
Sutharsan CreditAttribution: Sutharsan commentedAdding API changes for the issue summary.
Comment #81.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary filled in something for api changes while sutharsan writes something
Comment #82
Sutharsan CreditAttribution: Sutharsan commentedThe same API changes, but now annotated.
Comment #82.0
Sutharsan CreditAttribution: Sutharsan commentedLink to file with API changes
Comment #82.1
Sutharsan CreditAttribution: Sutharsan commentedUpdate API changes.
Update follow up issues.
Comment #82.2
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #83
webchickOk, 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.
Comment #84
Gábor Hojtsy@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:
Comment #85
Sutharsan CreditAttribution: Sutharsan commentedThere 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:
Comment #86
Sutharsan CreditAttribution: Sutharsan commentedGo botty
Comment #87
BerdirNitpick mode: comments that belong to a @todo: should be indented by two spaces. And I think it needs a . after the link?
The simple last checked UI makes sense, but I'm not sure if we really need this?
Comment #88
Gábor HojtsyFixed 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.
Comment #89
BerdirThat'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?
Comment #90
Gábor Hojtsy@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.
Comment #91
BerdirThat'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 :)
Comment #91.0
BerdirUpdated inssue summary
Comment #92
Jose Reyero CreditAttribution: Jose Reyero commentedThis 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.
Comment #93
Gábor HojtsyGood 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.
Comment #94
YesCT CreditAttribution: YesCT commentedI'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.
Comment #95
YesCT CreditAttribution: YesCT commentedcache('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.
Comment #96.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #97
YesCT CreditAttribution: YesCT commented#95: locale-download-import-1804688-95.patch queued for re-testing.
Comment #98
YesCT CreditAttribution: YesCT commentedThe 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
user interface translation page
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.
Comment #99
YesCT CreditAttribution: YesCT commentedHelp text changes to grammar, to be general, have improved word choices.
Comment #100
Sutharsan CreditAttribution: Sutharsan commentedWhen 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.
Comment #101
Sutharsan CreditAttribution: Sutharsan commentedRemoving @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.
Comment #103
Gábor Hojtsy#101: locale-download-import-1804688-101.patch queued for re-testing.
Comment #104
Gábor HojtsyGreat changes. The tests failed for unrelated reasons earlier, rerunning them showed they work.
Comment #105
webchickI want so very much to be able to commit this patch, but enabling Locale module per the testing instructions is giving me:
:(
Comment #106
webchickThe class name is null btw.
Comment #107
Jose Reyero CreditAttribution: Jose Reyero commented@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).
Comment #108
Jose Reyero CreditAttribution: Jose Reyero commentedYes, just noticed this is not Views, but 'ViewSubscriber' :-(
Comment #109
Sutharsan CreditAttribution: Sutharsan commentedI 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.Comment #110
Jose Reyero CreditAttribution: Jose Reyero commentedMore 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...
Comment #111
Jose Reyero CreditAttribution: Jose Reyero commentedOops, I didn't intend to change the status
Comment #113
Jose Reyero CreditAttribution: Jose Reyero commentedSome findings: (This may be related to php versions / locale_storage() / db connections)
In #105, the class webchick is is getting from execute() whithin findString( ) is Drupal\locale\StringDatabaseStorage
for which the fetchObject() method doesn't support a class name
This (current head) library StatementPrefetch::fetchObject(), is seriously broken but also I don't know whether it should take any parameters at all (it's not even in the interface)
http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Database!Statement...
Anyway it doesn't work with a class name which is what LocaleStringStorage::findString() tries.
The funny thing is that function is working because (we don't know why yet) the object we are getting is not
Drupal\Core\Database\StatementPrefetch (like webchick)
but
Drupal\Core\Database\Statement (which extends PDOStatement) and thus supports fetchObject($class_name) properly.
Anyway, my preliminary conclusions:
- There's a bug to fix in LocaleStringStorage because it is using the fetchObject($class_name) method that is not in our StatementInterface (thought it is in the object returned usually, not on the one @webchick is getting though)
- The StatementPrefetch::fetchObject() method and the rest of the class and the interface need some serious cleanup.
Then there's the really weird fact that some people, possibly depending on php version, are getting different returned classes under the same circunstances, that won't help debugging at all.
Notes about php versions, we did some quick survey:
@Sutharsan: 5.13.15 (NOT getting this error)
@webchick: 5.3.6 (getting the erros)
Mine is 5.3.10 (NOT getting this error)
Comment #114
YesCT CreditAttribution: YesCT commentedI'm using 5.3.6 and I did not get the errors.
Comment #115
Gábor Hojtsy#109: locale-download-import-1804688-106.patch queued for re-testing.
Comment #116
Gábor HojtsyI'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.
Comment #117
Gábor HojtsyAll 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.
Comment #118
Gábor HojtsyDamien 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.
Comment #119
clemens.tolboom(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.Comment #120
YesCT CreditAttribution: YesCT commented@clemens.tolboom Yeah, #101 is related to that. [edit: changed 110 to 101. and fixed link.]
Comment #121
Sutharsan CreditAttribution: Sutharsan commentedThe 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?
Comment #122
YesCT CreditAttribution: YesCT commentedIf 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.
Comment #123
Sutharsan CreditAttribution: Sutharsan commentedSince #85 the directory is created when Locale module is installed (or at least it should be).
Comment #124
Gábor Hojtsy#109: locale-download-import-1804688-106.patch queued for re-testing.
Comment #125
Sutharsan CreditAttribution: Sutharsan commentedSome fixes:
Comment #126
Gábor HojtsyChanges look good to me, great fixes :)
Comment #127
Gábor Hojtsy#125: locale-download-import-1804688-125.patch queued for re-testing.
Comment #128
webchickOk, 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.
Comment #129
webchickoops. ;P
Comment #130
Gábor Hojtsy@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.
Comment #131
Gábor HojtsyCreated #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.
Comment #132
Sutharsan CreditAttribution: Sutharsan commented@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.
Comment #133
webchickNice. :D Well-earned pancakes, for sure. :D
Comment #133.0
webchickupdating testing the patch instructions. they would use more work to make it clear which steps are having the patch do some new functionality, and which are just letting the tester see that it worked.
Comment #133.1
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #134
YesCT CreditAttribution: YesCT commentedUpdated 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
Comment #134.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary added help text follow up issues.
Comment #135
eigentor CreditAttribution: eigentor commentedMaybe we should invent a "congrats" Button, so cheering for stuff like this does not spam so hard :P
Comment #136
Gábor HojtsyThanks all, especially Sutharsan! This was a truly great effort :)
Comment #138
YesCT CreditAttribution: YesCT commented#125 could be relavent to #1974048: Limited display of languages when going back in installer is confusing
Comment #139
YesCT CreditAttribution: YesCT commentedI think since we have a tar ball and alpha https://drupal.org/node/3060/release that when adding a language, the po should be automatically downloaded and imported.
But it's not. #2030537: Translations not downloaded when adding a new language
Comment #139.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary moved the follow-ups that were created from the followups to be made to just the list of follow ups ... because they were made!