| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | locale.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D8MI, language-base, language-ui |
Issue Summary
Currently, the languages table stores values on behalf of multiple functionalities. It stores language data, then it stores language negotiation values (to be decoupled in #1301460: Decouple domain/path language negotiation storage from language config storage), and it stores data on behalf of interface translation (plural numbers and keys, the javascript translation filename, etc). Because we are separating the interface translation module from the language module with the intention to make this pluggable in true ways and because languages can exist without interface translation, we should decouple the storage for them as well.
Here are the languages table columns and where do they belong:
| Column name | Role |
|---|---|
| language | Language list |
| name | Language list |
| direction | Language list |
| enabled | Universal (for now), see #1314250: Allow filtering/configuration of which languages apply to what (UI, nodes, files, etc) |
| plurals | UI translation, should refactor here |
| formula | UI translation, should refactor here |
| domain | Language negotiation, refactoring going on in #1301460: Decouple domain/path language negotiation storage from language config storage |
| prefix | Language negotiation, refactoring going on in #1301460: Decouple domain/path language negotiation storage from language config storage |
| enabled | Language list |
| javascript | UI translation, should refactor here |
Advantages of decoupling are numerous. Language save operations and hooks should not be invoked (the language did not change) if we regenerated the javascript file for the language. Only UI translation code should be concerned if there were any changes in the plurals, etc. The schema is going to be more lightweight if there is no UI translation.
Need to figure out how to store these additional values though and update queries for these values in code.
Comments
#1
Discussed this with @catch in IRC. We agreed using simple variables should suffice for now. The locale JS system also uses the 'javascript_parsed' (to be renamed later) variable to hold the list of JS files parsed. Also, we expect CMI to come around and give us better ways to store this data.
If you look at this patch, it should comfort you a great deal that it removes direct queries to the language tables that were not using the API *and* it even removes a brute-force change of the language_default variable too. Hah!
Reviews please and let's see what the testbot thinks! (Edit: no update function yet, TBD).
#2
The last submitted patch, decouple-locale-from-language-schema.patch, failed testing.
#3
Looked at those test failures. A copy paste error and three missed queries. Let's see if this covers it all :)
#4
#5
The idea, and quick overview of the code looks good to me.
#6
The patch looks good and the issue's goal seems reasonable to me too. Should we consider dropping the languages table altogether?
#7
@plach: well, I think the language list in the database works good for now in case anyone needs to join on that for other values. Practically people don't do that, so we'll probably migrate that to CMI too later. However, we don't yet have CMI, so I'm inclined to just keep it as-is and return to that when we have the CMI tools.
@plach, @Jose: so what do you think, should this patch go in? :)
#8
Well, you still owe us the update function :)
#9
LOL, right. Well, doing that then :)
#10
There you go, now complete with an update function :)
#11
Overall looks good, very minor nitpicks. I'll test everything tonight.
+++ b/modules/locale/locale.install@@ -250,6 +232,29 @@ function locale_update_8000() {
+ $languages = db_query('SELECT * from {languages}')->execute();
Why not DBTNG? Can we have "FROM" (uppercase) at least?
+++ b/modules/locale/locale.install@@ -250,6 +232,29 @@ function locale_update_8000() {
+ $plurals = $javascript = array();
I've been whipped by @DamZ for using chained assignments, it seems it's an unrecommended practice :)
-26 days to next Drupal core point release.
#12
FROM I agree with, the chained assignments, I don't care much :) Updated patch with both fixed.
#13
+++ b/modules/locale/locale.install@@ -250,6 +232,30 @@ function locale_update_8000() {
+ $languages = db_query('SELECT * FROM {languages}')->execute();
Tested the upgrade path, the line above is wrong: the
execute()invocation should be removed, asdb_query()already returns an iterable result. Any reason not to convert this to DTBNG?<?php$languages = db_select('languages', 'l')
->fields('l')
->execute();
?>
-27 days to next Drupal core point release.
#14
Rerolled for new core dir structure and the DBTNG structure as suggested. Any other isses?
#15
The last submitted patch, decouple-locale-from-language-schema-14.patch, failed testing.
#16
I ain't sure what the current politics are: should we provide test coverage for the update function?
#17
@plach: other issues in the initiative worked with updates and did not introduce tests for them. See #1301148: Stop pretending we have configuration translation for languages. I don't think we need one here. Also, once the CMI backend hits, we'll need to either do yet another update function or rewrite this one altogether to use the CMI backend, so this is supposed to be an interim solution for now to let us move on with separating the language module.
#18
Uhm, the patch did apply for me with minor offsets. Rerolling to apply 100%.
GaborH:d8 gabor$ git pullAlready up-to-date.
GaborH:d8 gabor$ patch -p1 < decouple-locale-from-language-schema-14.patch
patching file core/includes/gettext.inc
patching file core/includes/locale.inc
patching file core/modules/locale/locale.install
patching file core/modules/locale/locale.module
Hunk #1 succeeded at 725 (offset 3 lines).
Hunk #2 succeeded at 924 (offset 3 lines).
patching file core/modules/locale/locale.test
GaborH:d8 gabor$ git diff > decouple-locale-from-language-schema-18.patch
#19
@plach: any other concerns?
#20
I'm going to test the upgrade path again tonight to be sure everything is ok :)
#21
Now everything seems ok. I had a hard time testing the upgrade path due to:
#1331350: [Upgrade path broken] Entity module not enabled during D8 upgrade
#1331370: [Upgrade path broken] Stored include paths need to be updated to /core before running the upgrade path
#22
Since you mentioned problems testing the upgrade path due to other issues, I checked upgrade-filled and noticed it does not have locale module enabled at all. This upgrade is straightforward and has had manual testing, so I've committed and pushed this to 8.x.
However I've opened #1336170: Add locale module to upgrade tests as a critical task to add some locale data and/or locale-specific upgrade path testing since I don't want to add any more untested updates to 8.x.
#23
EDIT: I think variable change should be in lock_*() to prevent possible data-loss in race condition, same could be doem for JS
This change require change notification because table field are removed and new variable introduced
+++ b/core/includes/gettext.incundefined@@ -401,24 +401,17 @@ function _locale_import_one_string($op, $value = NULL, $mode = NULL, $lang = NUL
+ $locale_plurals[$lang] = array(
+ 'plurals' => $nplurals,
+ 'formula' => $formula,
+ );
+ variable_set('locale_translation_plurals', $locale_plurals);
variable introduced
+++ b/core/modules/locale/locale.installundefined
@@ -97,19 +99,6 @@ function locale_schema() {
- 'plurals' => array(
- 'type' => 'int',
- 'not null' => TRUE,
- 'default' => 0,
- 'description' => 'Number of plural indexes in this language.',
- ),
- 'formula' => array(
- 'type' => 'varchar',
- 'length' => 128,
- 'not null' => TRUE,
- 'default' => '',
- 'description' => 'Plural formula in PHP code to evaluate to get plural indexes.',
- ),
'domain' => array(
'type' => 'varchar',
'length' => 128,
@@ -130,13 +119,6 @@ function locale_schema() {
@@ -130,13 +119,6 @@ function locale_schema() {
'default' => 0,
'description' => 'Weight, used in lists of languages.',
),
- 'javascript' => array(
- 'type' => 'varchar',
- 'length' => 64,
- 'not null' => TRUE,
- 'default' => '',
- 'description' => 'Location of JavaScript translation file.',
this fields been removed
#24
Added http://drupal.org/node/1337464.
#25
@Gabor:
What about these lines from #23?
#26
@plach: First off, I've read the comment in email and consider later time edits not good for discussion (easy to miss like I did). Second, in talking to @heyrocker, @sun and @chx they reassured me that the CMI storage is not far off. It is supposed to land this year or at least before Denver. Then they said converting existing config storage like this one to CMI is a massively parallelized task. Now, since all this would use the CMI storage layer (and that needs to handle conflict/locking resolution in itself), I don't hink we should focus on building a temporary workaround here and instead work on stuff that matters.
#27
Yes, I supposed so. I have been lucky to read the comment when already edited.
Ok, thanks for the update :)
#28
Automatically closed -- issue fixed for 2 weeks with no activity.
#29
Tagging for base language system.
#30
Adding UI language translation tag.