In the spirit of the issues listed below, where we placed the responsibility of language support on the core modules vs. implementing it for themselves from the outside, this patch moves language assignment to user module. As with the rest of the patches, the actual support for language is already in user module. It maintains two language properties. It stores those values and uses them properly for customizing emails in proper languages for example. So as usual only the assignment of the language was in an outside module which is not good separation of concerns. I kept the functionality 100% (eg. the language selector will not show on the registration form but will set to the current interface language instead - the user can edit once the profile is created). There is no functional change here, and I think that would be good to debate later. With this one and #1468930: Clean up and move the code for the negotiation functionality from locale module to language module, locale module is almost only interface translation, which is one of our goals.

Precursors:
#1236680: Move path language settings from Locale to Path module
#540294: Move node language settings from Locale to Node module
#1415764: Untangle comment module and locale module

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, move-language-to-user-module.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.28 KB

Update the patch to fix the notices.

Gábor Hojtsy’s picture

Fixed all the notices by checking for the presence of the #description of the field.

Status: Needs review » Needs work

The last submitted patch, move-language-to-user-module-2.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Ha, crosspost :) Still need to fix the tests, which I assume would be due to the locale -> language change in the wrapper form element. I did not actually look (yet). Unassigning in case someone wants to look, since I'll not be available for a while.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.03 KB
1.97 KB

Okay fixed all the remaining test problems.

The interdiff shows the problem with existed.

Testbot: Test it!

Gábor Hojtsy’s picture

Looks good overall. One question though: why did you make preferred_langcode empty in the update if language is not enabled?

RobLoach’s picture

Issue tags: +Framework Initiative

Adding Framework Initiative, as thats the tag associated with decoupling Drupal.

dawehner’s picture

Assigned: Unassigned » dawehner
Status: Needs review » Needs work

The tests got changed by the change in http://drupal.org/node/1519942#comment-5841234 but actually the behavior should change.

If you have just a simple plain drupal site the langcode of the user should be the default language of your site.

Gábor Hojtsy’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
1.69 KB
9.33 KB

Well, the OpenID tests are not really great because they all test variants of en, en-GB, etc. So the users will end up being 'en'. Then with our change to make the site default assigned to users right away also means 'en' will be assigned on new installs. So this is not really a differentiator in the tests at this point. I think we should modify some of the rest of the OpenID tests to add a foreign language and test with that instead.

Anyway, here are modifications to likely make OpenID test pass well with users now being saved in the site default language.

Gábor Hojtsy’s picture

(Unassign was accidental, I cant fix it though, as you are not an option for me, sorry).

Status: Needs review » Needs work

The last submitted patch, move-language-to-user-module-10.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
10.51 KB
1.19 KB

User.test had two similar tests which looked for empty initial value. Fixed those too. This should now work. Looking into making the OpenID tests actually more versatile to better test what they do.

RobLoach’s picture

+++ b/core/modules/user/user.moduleundefined
@@ -920,7 +920,7 @@ function user_user_view($account) {
@@ -1088,6 +1088,70 @@ function user_account_form(&$form, &$form_state) {

@@ -1088,6 +1088,70 @@ function user_account_form(&$form, &$form_state) {
     '#description' => t('Your virtual face or picture. Pictures larger than @dimensions pixels will be scaled down.', array('@dimensions' => variable_get('user_picture_dimensions', '85x85'))) . ' ' . filter_xss_admin(variable_get('user_picture_guidelines', '')),
   );
   $form['#validate'][] = 'user_validate_picture';
+
+  if (module_exists('language')) {
+    // Get list of enabled languages only.
+    $languages = language_list(TRUE);
+
+    // If the user is being created, we set the user language to the page language.
+    $user_preferred_language = $register ? $language_interface : user_preferred_language($account);
+
+    $names = array();
+    foreach ($languages as $langcode => $item) {
+      $names[$langcode] = $item->name;

Could this be in a language_form_user_account_form_alter() function?

-8 days to next Drupal core point release.

Gábor Hojtsy’s picture

@Rob Loach: no, the whole point of the series of issues linked above is that we move all language handling to the respective modules. Most of the language handling was already with the modules (such as storing this information, or in case of users computing preferred language info for others or in case of nodes, listing nodes filtered to language, etc). A small amount of code (for language assignment) used to be with locale module, and we are moving that code in with the rest that was already in the modules in D7 and before. Same already happened for all other modules, see "Precursors" listed above.

Gábor Hojtsy’s picture

Updated the OpenID tests to test for real languages instead of just en-GB all the time. This uncovered two bugs, one introduced with the patch above, one existing in openid module :)

1. The bug in the patch was that it used a language_negotiation_*() function that is still in locale module (but is being moved to language module in the RTBC patch at #1468930: Clean up and move the code for the negotiation functionality from locale module to language module. Once that lands, we can fix this to check for language module. Added a @todo, and I don't think this should land before that one.

2. The other bug was in openid language matching. It did check from the most specific out to the least specific, but then it overwrote the most specific with the least specific, because it did not stop after finding the most specific. This is pre-existing bug, but since I modified one of the language detection tests to have pt-PT and have both pt and pt-pt on the system, it did pick 'pt' erroneously instead of 'pt-pt' due to this bug. This fix should be backportable to D7 and will be once this is in D8 :)

That is all. I think this should now be complete except the @todo mentioned in (1) above.

Gábor Hojtsy’s picture

Status: Needs review » Postponed

Since this one is blocked on #1468930: Clean up and move the code for the negotiation functionality from locale module to language module, and managing the backportable part as its own patch is simpler, I moved the OpenID test improvements and bugfix to #1527720: OpenID language matching finds least specific instead of most specific match.

Marking this postponed then on those two.

Gábor Hojtsy’s picture

#1468930: Clean up and move the code for the negotiation functionality from locale module to language module landed, so with the changes in #16 now broken out to its own patch at #1527720: OpenID language matching finds least specific instead of most specific match, we should be back at the patch from #13. Reuploading with no changes (just renamed).

Gábor Hojtsy’s picture

Status: Postponed » Needs review
Gábor Hojtsy’s picture

Reuploading because testbot seems to be stuck with the previous one. It is marked postponed but I don't see it in the queue.

balintk’s picture

When I tried to manually test the patch, got an error after enabling Language module, and reloaded the user edit page. The language_negotiation_method_get_first() function was undefined, because the language.inc file is not included.
So I believe, we need the following line of code inside the user_account_form method, within the if (module_exists('language')) statement:

include_once DRUPAL_ROOT . '/core/includes/language.inc';

I have noticed, that there is a wrapper function for including this file, but that method also includes another file, which we don't need. I don't know, what's the convention in this case; to stick to the readable, more standard code, or to use the aforementioned line. Anyway, the alternative would be:

language_negotiation_include();

Also, I have a suggestion. The form item appears between the role settings and picture uploading items. I would show it either above or under the locale settings, because they're logically related. What do you think?

Gábor Hojtsy’s picture

@balintk: Thanks for the review. Hm, why is this not caught by the tests. Need to look into why is there no test coverage for this. I think using language_negotiation_include() would be fine.

On the placement in the form, is this different before/after the patch? I'd like to keep its current placement if it makes sense :)

balintk’s picture

I have found out, that the problem occurs only when there is only one language enabled. Gábor pointed out, that this is natural, since there is no need for language negotiation (that's why that code is not included), moreover, the Language settings form item can be omitted in this case. So I modified the patch a little bit; the form item won't be shown, and the language_negotiation_method_get_first method won't cause problem, because it's not going to be used.

@Gábor Hojtsy: The placement of the form item is the same as before, so the patch doesn't change it. But as we're working with this, I thought maybe we could reconsider this position.

Gábor Hojtsy’s picture

I think that change looks good. I cannot actually mark this RTBC, and of course it would be best to get #1527720: OpenID language matching finds least specific instead of most specific match land first, since that exposed the test failures for this patch at the first place. I'll try and get webchick look at that one sooner :) We'll see :) Reviews still more than welcome in the meantime.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +D8MI, +sprint, +language-base

The last submitted patch, move-language-to-user-module-23.patch, failed testing.

kalman.hosszu’s picture

Status: Needs work » Needs review
FileSize
10.63 KB

The new patch is attached.

One modification in

-          $form['locale']['preferred_langcode']['#type'] = 'hidden';
-          $form['locale']['preferred_langcode']['#value'] = $candidate_language;
+          $form['language']['preferred_langcode']['#type'] = 'hidden';
+          $form['language']['preferred_langcode']['#value'] = $candidate_language;

$candidate_language variable is renamed to $candidate_langcode.

And one question for:

+    // Get language negotiation settings.
+    $mode = language_negotiation_method_get_first(LANGUAGE_TYPE_INTERFACE) != LANGUAGE_NEGOTIATION_DEFAULT;

+      '#description' => $mode ? t("This account's preferred language for e-mails and site presentation.") : t("This account's preferred language for e-mails."),

$mode is the best name for this variabel? This is not a setting variable but a boolean, so I think we should change the variable's comment and name. What's your opinion?

Best

Gábor Hojtsy’s picture

I agree with the rename suggestion. Maybe make $interface_language_is_default and swap its meaning, so we don't need to name it $interface_language_is_not_default? :)

kalman.hosszu’s picture

Ok Gábor, if the test runs successfull I'll rename the variabel to $interface_language_is_default.

kalman.hosszu’s picture

FileSize
10.68 KB

The test run sucessfull so I renamed the $mode variable.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, let's get this in.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK, replacing the hook_form_alter() with module_exists() still makes me uncomfortable, but that's mostly dislike of module_exists(). C I wondered a bit when reading through this if the session-based language negotiation should move to user module too, but that's only really coupled to user module because of $GLOBALS['user']->uid - which we should really try to move that check into the actual session instead, in which case the user module reference would go anyway.

Committed/pushed this one to 8.x.

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks.

Gábor Hojtsy’s picture

Status: Fixed » Reviewed & tested by the community

You have not actually pushed this(?). Hat tip to @kalmanhosszu for noticing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Sorry, yes I'd committed but not pushed :( Should be OK now.

Automatically closed -- issue fixed for 2 weeks with no activity.