The language_count
variable tracks how many languages are enabled and is used to tell if a site is multilingual and hence trigger the language negotiation process. It is updated everytime a new language is enabled or disabled, but it is not decremented when removing a language, hence it might hold a wrong value.
Comment | File | Size | Author |
---|---|---|---|
#32 | locale.patch | 1.31 KB | droplet |
#29 | language-1099396-29.D7.patch | 4.27 KB | plach |
#29 | language-1099396-29.patch | 3.81 KB | plach |
#28 | 1099396-language_count-28-d7_only.patch | 3.73 KB | Désiré |
#28 | 1099396-language_count-28-d8_only.patch | 3.26 KB | Désiré |
Comments
Comment #1
droplet CreditAttribution: droplet commentedComment #2
plachThanks for the patch, but please don't change the component: the
language_count
variable is at the very core of the whole language subsystem.However:
Minor:
variable_get()
is missing the default value.We should update the tests to capture this bug: checking that the
language_count
is equal to the number of enabled languages after deleting one should be enough.Powered by Dreditor.
Comment #3
droplet CreditAttribution: droplet commentedwe are called locale_languages_delete_form_submit which in locale module to remove a language. It is happened in locale module ??
If not, where should tests go into? simpltest/tests? system module ? thanks.
(but if you only run locale tests, then it won't catch errors ?)
Comment #4
plachPart of the language system code is in the locale.inc / locale.admin.inc files and all the simpletests are in locale.test, so updating
LocaleConfigurationTest
should be fine.We probably need also an update function ensuring that language_count is equal to the number of enabled languages.
Comment #5
Désiré CreditAttribution: Désiré commentedHere is a patch for test this bug.
Comment #6
Désiré CreditAttribution: Désiré commentedfor the testbot
Comment #7
plach@Désiré:
Thanks for the patch, the test looks good except for a couple of minor things. Anyway we need a unique file shipping the fix and the related test. Could you roll a patch merging #1+#2 and #5+#7?
The comment should wrap at column 80 and should end with a dot. A rephrase is also needed here:
"Make sure the "locale_count" variable is equal to the number of enabled languages."
Normally to get a list of enabled languages we use
language_list('enabled')
. For consistency we might do it also here.Powered by Dreditor.
Comment #8
Désiré CreditAttribution: Désiré commentedHere is a patch for the bug and for the test.
I think both patches are good, but the test fails... So pleas review it.
Comment #10
plachThanks!
Please move this hunk before the hook invocation. However I guess it's safer to simply decrement the
language_count
variable to avoid tricky static cache issues (see below).Comment lines should wrap at column 80 or the nearest lesser value: "enabled" can be moved one line up.
Probably before this line you need to clear the static cache through
drupal_static_reset('language_list')
to make the test pass.Powered by Dreditor.
Comment #11
plachBugs are fixed in the development version first, backported then.
Comment #12
Désiré CreditAttribution: Désiré commentedHere is the good patch.
I tested it on D7 and D8.
@plach thx for your help!
Comment #13
Désiré CreditAttribution: Désiré commentedreupload it, without the D8 extension, to run the testbot
Comment #14
Désiré CreditAttribution: Désiré commentedI found a better fix for the bug.
Here is the new patch (tested on D8 but should work on D7).
Comment #15
plachNice catch! Now we should update the test to check that the
language_count
variable is not altered when deleting a disabled language.Comment #16
Désiré CreditAttribution: Désiré commentedHere is it.
Comment #17
plachErr, sorry but I meant we should test both scenarios:
language_count
has not changedlanguage_count
has been updated correctlyThis way we are sure that both behaviors are correct.
We need a rephrase here: "Disable an enabled language. Disabling a language will update the "locale_count" variable. This must be done before deleting any language to test that the variable is updated correctly."
Powered by Dreditor.
Comment #18
plachOne thing that had been forgot from #4:
This should be as easy as putting in
locale.install
the following code, but only for the D7 patch:Comment #19
Dries CreditAttribution: Dries commentedWould be good to add a code comment describing what the variable is for. Some context would help.
Comment #20
plachHonestly I never quite understood the need for this variable but never worried about it too much. Probably we need Gabor or Jose on this, I ain't sure who introduced it.
In D7 core it is read only while updating it and by
drupal_multilingual()
, where it could be replaced by:Comment #21
plachGot it. It avoids the need of querying the
{languages}
table on monolingual sites:Comment #22
Désiré CreditAttribution: Désiré commentedHere is two patches:
for D8 with the test for all scenarios in #17
D7 only with the update hook
Comment #23
Désiré CreditAttribution: Désiré commentedComment #24
plachPersonally I don't think this is the right place to document the variable. For consistency we would need to add similar/identical comments around all the other code updating it. I guess the right place to document it is
drupal_multilingual()
. Anyway we need a more meaningful explanation, something like "The language_count variable stores the number of enabled languages, to avoid unnecessarily querying the database when building the list of enabled languages on monolingual sites."Sorry, I missed this one previously: the default "language_count" value should be "1" and not "NULL".
As above the default "language_count" value should be "1" and not "NULL".
Powered by Dreditor.
Comment #25
droplet CreditAttribution: droplet commentedthis "language_count" default value should be "2"
we always have 1 language actives.
Comment #26
plachThe default value for a variable should be always the same. The default for
language_count
is 1:Comment #27
Gábor HojtsyYes, the idea is that this would avoid extra queries where not needed, since we already have the default language as a variable for other reasons (eg. to work on a site without locale enabled)
Comment #28
Désiré CreditAttribution: Désiré commentedHere is the fixed patches.
And I found a comment for the variable in locale.inc, I think no other documentation needed:
Comment #29
plachThe patches look good and fix the issue (tested both on D7 + update and on D8).
Since Dries explictly asked for additional documentation I introduced a comment as suggested in #24. Aside from this the patches are the same as those in #28 so setting RTBC.
Comment #30
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks all.
Comment #31
plachThis can be easily backported to D6.
Comment #32
droplet CreditAttribution: droplet commentedit's maybe my first D6 branch patch :)
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedChanging issue status to reflect that it was fixed in 7.x.