Multilanguage support for user emails

Changing status since no patch file is attached. See http://drupal.org/patch/create.

Generally you should let a second person review your code, who might set the status to ready-to-commit.

No, no new settings will be added to Drupal 5.x unless absolutely necessary.

Hi bibo!

We had run to the same situation and I will try to use your approach. But - at a first sight - your patch file for drupal 5.1

---
// Check if an admin setting overrides the default string.
if ($admin_setting = variable_get('user_mail_'. $messageid, FALSE)) {
- return t($admin_setting, $variables);
+ return strtr($admin_setting, $variables);
}
---

looks like you do change [t] to [strtr] instead of opposite, isn't it?

I have a little experience with patching utilities but for my patch 2.5.9 win32 util the minus sign is stand for string to exclude and the plus sign is stand to added string.

Hi bibo

thanks for quick and informative reply!

I think to initialize instead of sending all emails one can just select default (English) locale then go to

http://sitename/admin/user/settings

and save data.

As a result 8 items from variables (use SELECT * FROM {variable} WHERE `name` LIKE 'user_mail%' ) will be copied to localizer table (use SELECT * FROM {localizertranslation} WHERE `object_key` LIKE 'user_mail%' to see )

All above is correct for my 5.1 version and I think you can test it and simplify your patch.

I'll continue applying and checking this to my site and will inform you if something else useful will be discovered. ; )

Regards, Dennis

As far as I know, the policy for Drupal 5.x and earlier is that we do not translate user-supplied text. This will be much improved in Drupal 6.

This is one way to. However, I believe this is a short-hand approach. I did the same by invalidating the IF statement (IF stuff AND FALSE) so that _user_mail_text always returns the translation for the email subject and body strings as hardcoded into it.

I think that it would be more effective to store different admin variables per language: instead of user_mail_SOMETHING, let's say, try and store user_mail_SOMETHING_LOCALE.

Just a quick thought at 5.30 am.

Did the change manually and it does work

I like Pulpzebra's idea also ...

re #13:

Unfortunately it doesn't seem that 6.2 includes this change - the emails are still monolingual. I'll try your change and see what happens...

Hmmm, seems to work - although I have to make the change in the locale translator; changing the actual settings on the page applies the change to all languages... not so elegant.

EDIT: That made no sense. What I meant was: I have to provide the translated version using the locale translator. If I just enter text on the User Settings page, that text is applied to all languages.

A new question:

What would I need to change to force my site always to use the user's preferred email setting? At the moment, if I (as administrator) am using the site in English and I approve a user's account application, the confirmation goes to the user in English, regardless of his/her preferred language setting.

The user does not care what language I speak or what language I am viewing the site in. It must be easy to change this as I assume the code is currently designed deliberately to check the language of the page that triggered the email, which seems initially intuitively correct, but actually makes no sense with admin -> user emails.

The answer, as it has taken me about 24 hours (and much tearing-out of hair) to discover, is that the line we have altered (context as above) should now read:

return t($admin_setting, $variables, $langcode);

If you do not include the $langcode, then emails which have been modified (i.e. the ones we are concerned about!) are sent in the language of the active page which triggered the email, due to the way that t() is coded. $langcode is calculated by the user module to be the preferred language of the recipient of the email - which is what we want.

I am not clever enough to provide a patch, but someone else is welcome so to do. EDIT: See #20.

Further experimentation has enabled me to ensure that:

- new user accounts are initially assigned the language of the page in which they signed up (i.e. if I sign up using the German page, my account is initially set to default German). Therefore the correct version of the pending email (or whatever) is sent to the user.

- notifications are sent to users in their preferred language, regardless of the language of the page that triggers the notification.

- notifications of new users are sent to the administrator (the user with uid==1) in that administrator's preferred language. Previously, this was sent in the site's default language. In my case, the administrator does not speak the default language of the site, and using the admin's preferred language seems to be the more logical way to choose the right language to contact the admin in!

- a more informative/appropriate notification is sent to the administrator, making it clear that action is required (this is essentially a cosmetic change).

As before, I do not know how to get around the fact that the emails to users have to be translated through the localization module (translate) rather than by accessing the admin page in the language concerned.

So with that caveat I attach a working patch - although I am not sure what to do about version numbering etc in the first lines, so I may have got that wrong. Please put me right if so.

1. new features for D6 will not be accepted
2. text changes for D6 will not be accepted
3. can you write the list of problems (and only the problems) with the actual solution in D6, because bug fixes will be accepted

Hi Martinquested, hi All,

I have tried to apply the patch at #20 on my development environment, which is D6.3 + i18n-6.x-1.0-beta1.
The new user registration is configured without admin approval.
the first test went ok:
- the first email to the user in the selected page language.
- a second email to the user is in his preferred language as per user profile
Then the second and subsequent attempts didn't work properly, as the default language was used instend for the first and subsequent emails (regardless the user preferred language).

Unfortunately I have no experience in drupal development, so I can report only about testing. But I would appreciate a feedback.

Thanks for your work on this patch, which is really useful.
univac

=================================================
21/10/2008 update

Dear Martin,

thank you for looking at my observation.

Actually, after some days of thrubleshooting I managed to trace back the
problem to an installation issue.
I cannot reproduce the problem systematically, but I noted that in some
cases the installation with multiple languages does not translate the
emails so regardless the language is selected, the emails from the User
Module remain available only in the default language. Then of course
having one language version of the emails, your patch cannot work
properly.

I have now a clean new installation with both D6.4 i18n-6.x-1.0-beta4
and D6.5 i18n-6.x-1.0-beta4, and
your patch works pretty fine i.e. the emails are available from
installation in all different languages and the emails are sent in the
expected language (thanks your modifications ;-).

So at this stage, I can state that your patch is ok. I will see whether
to send an observation to the thread of discussion
http://drupal.org/node/275705
where milti-language installation is discussed.

To conclude, I think your patch should be marked as "tested".

Pasqualle,

I'm not sure whether the problem is a bug or not. Basically the issue is that users receive emails in a language which is not the one they have chosen in their user settings. My user should not receive emails in a language other than the one s/he has selected.

As far as I can tell this is how the module has been designed, so I don't know if I can call it a bug. Personally, I think it makes the module unusable in its current form, but maybe that's a matter of opinion. You may call rectifying this a new feature addition, but I think it is a gaping hole in the existing implementation, and a matter of priority to fix it.

So, in short:

Problem:
Emails sent to users are not translateable, so do not respect user's chosen preferred language. This makes that option on the user configuration page look meaningless.

Solution (proposed by bibo):
Email texts can be translated using the translation module.

However, this gives rise to further problems:

Problem:
Language of emails is determined by the current language of the page on which the email is triggered.

Solution:
In my patch, language of emails is determined by selected default language of the user to whom the language is being sent.

Problem:
System assumes that Admin (user with uid 1) wishes to receive emails in the site's default language. This is not always a valid assumption.

Solution:
In my patch, system emails sent to the Admin are sent in user 1's selected default language.

Problem:
When users sign up, their account's default language is not necessarily set to the language of the page they used to sign up. So they sign up in French, but receive the confirmation email in English.

Solution:
In my patch, user's default language is set to be the language they used to sign up in.

Hope that makes it clearer. Whether or not you commit it to D6, I will be using it on my site, as the deficiency of the official version of the module is unacceptable for my site. And the patch is here for anyone else who is of the same opinion. Either way, perhaps it can be considered for D7.

I did not tested the multilingual email functionality, but as I know the language should be based on user settings. So if there is a problem, then it should be fixed.

Emails are sent in the language of the user targeted

Page 34
http://hojtsy.hu/files/GaborHojtsy-FrOSCon-2007-Slides.pdf

the presentation:
http://ftp.stw-bonn.de/pub/froscon/09_FrOSCon_Drupal_Human_Language_Supp...

OK. A fix is proposed. See my patch at #20.

#22 has been updated. Does one successful test post constitute "reviewed and tested"?

This patch contains various bit which make it unsuitable for Drupal 6.

1. You are using t() on user/admin provided values. t() is to be used on strings stored in source code. This is why the default email text is properly translated to different languages, but user settings are not. The i18n module provides support for user translatable variables. Using t() is a dirty hack, and while you'll get away using it on your own site, we are not going to introduce that in Drupal itself.

2. You modify existing email text and subjects. These improvements will be great for Drupal 7. Otherwise they break strings which are exposed to users, and especially with the user management interface where keeping translation consistency is also important for security.

3. Spacing around your admin_preferred_language() function is excessive. Spacing between the comment block and the function makes the comment block disconnected. Multiple lines of starting comment is not valid according to our standards.

4. Anyway, your assumption which lead to the admin_preferred_language() function is not right. The user approval admin email is sent to the site email address, not the user 1's email. So applying user 1's preferred language is not applicable there. Also, there might be multiple admins with multiple user preferences, why pick user 1? Drupal's assumption is that you specify a site email where people reading that are fluent in the site default language.

5. I don't get why you add $language to $merge_data.

Except maybe (5), I don't see anything here which might be possible to change in Drupal 6, so moving to Drupal 7.

Gábor,

thank you for your detailed reply. I appreciate your patience as I am still really quite new to Drupal.

1. Yes, it is a dirty hack. I guess I had convinced myself it would be OK as a stop-gap, but on reflection I very much respect your unwillingness to include hacks in Drupal core even as a stop-gap until a better solution is found.

2. I hadn't thought of that aspect. Fair enough.
3. Guilty as charged.
4. Ah, I had forgotten that the user #1 email and the site email were distinct, and indeed, I made the wrong assumption. Sorry.
Meanwhile Drupal's assumption, as you set it out, makes perfect sense.

5. Here's the important one. At present, I think user accounts are activated with preferences set to the site default language, and users are only able to alter that language by later accessing user/n/edit. This is counter-intuitive when the user has already selected an alternative language while accessing user/register. By adding $language to $merge_data, I ensure that a user's preference is initially set to the language which s/he was using when the account was created. Although this is somewhat less of an improvement until the account confirmation emails can be made to go out in the preferred language, I think this change is still important. There is no reason to assume that the user who has chosen to create an account while viewing the site in German would be able to read (or even change settings) in a site's default language, say, Hungarian. So her/his account should definitely not initially be set to that default language.

So: I will certainly look at a better solution to number 1.

Whether number 5 is worthy of patching into D6 I don't feel able to judge any more. What do you think?

@martinquested: (5) sounds like a good improvement for Drupal 6 and 7, please roll a patch with only that change included, so people can test.

OK, here it is. Not sure if I got the first couple of lines of the patch right - more guesswork than knowing what I'm doing as far as the header bit is concerned.

subscribing

1. Even when using the l18n module this doesn't solve the problem. (At least, as far as I can tell from my experiments). I translated the strings yet they keep showing up in the language of the site and not in the preferred language of the user.

A possible solution would be to overwrite the existing message using hook_mail_alter() if that would allow us to access the variables given to the message.

I suspected as much, but haven't had a chance to look at this yet. hook_mail_alter() sounds like it has potential...

In the meantime, would be good to get this simple patch reviewed and committed...

New things go to Drupal 7 first.

here's a re-roll for HEAD

I think this issue has gone off topic!

The current patch afaik is for sending the admin generated mails in the user language. The original question from post 1 was about translating _user_mail_text

I started discussions about it:

i18n Group english
http://groups.drupal.org/node/18179

Drupalcenter german
http://www.drupalcenter.de/node/15396

The only hack seems to be in #20 and the thread goes offtopic in #28
#20 only relies on t() which is not a solution as the current admin language could be other than English and t() always expect a fixed English string.

For two weeks I am looking into this without a real solution and I am finding more and more modules that share this problem! The issue makes it impossible to run an international site without server hacking of modules and core files!

I will happily solve this issue if I only could think of a possibility to make a workaround to for "variable_get('user_mail_'. $key, FALSE)" The best way would be to use hook_local and tt() imho but this would make the core depend on the i18n module.

Is there any solution to this that could be done with the D6 core alone?

Could at least in D7 the tt() function be applied to the core?

A solution. Tested by me in D6:

You don't need a core hack to achieve that the current site language is used as the users default.

Use a module to set the user to the current language:

function mymodule_user($op, &$edit, &$account, $category = NULL) {
  if ($op == 'insert') {
    global $language;
    user_save($account, array('language' => $language->language));
  }
}

You could update by SQL directly which would be faster than use user_save.

By this a new user will have the current language.

Than you can use

http://drupal.org/project/mail_edit

to translate the emails as user.module will use the current users language but maybe send a message in a different language as set in the system variable. This module uses its own table with a language column and will find the correct message. By some magic it seems to replace all tokens correctly.

The Patch above just hat a typo with "language" and "languages". Its "$language->language."

The only downside of this patch I see right now is that the admin has no gui to decide what he is doing. By this it will be fixed to sign up in current site langue. This ofcourse should be the thing everybody wants.

But the admin user creation form in the backend should get a radio list.

Resubmitting patches setting issue to "code needs review"

Resubmitting D7 this time with correct path set.

the problem is a test in testUserRegistration which checks for an empty language

I see you patch the test to resolve the testing failure.

// Set global language.
+    $lang = $this->randomName(2);
+    global $language;
+    $language = $lang;

You copy a two char random string into $lang.
But why are you making it global? global $language should be an object and $language->language the current language string.

$this->assertEqual($user->language, $lang, t('Correct language field.'));

And if you check for an random string shouldn't the test nearly always fail because the user object has a string mathing the current site language like "de" or "en" and the random is well random and wouldn't basically never match?
Shouldn't the test be a regexpression like "\[a-z]{2}\"

ah i thought that global language is a string.

do you want to rewrite the patch?

why i used randomName?:
to take care that the value is handled corectly and its not only random that it returns en for example

Dear Kars-T,

your patch at #40 user.module-D6.patch will fail when an update to the User Settings under admin/user/settings is done.
In fact the translations into all other languages are lost and only the default language is kept (regardless the email content is not changed at all !!!).
Eventually this is another "feature" from Drupal, which gives as side effect a single email language...
Some of us - funs of this thread of discussion - already pointed out that this is due to the way the translation mechanism is designed.
To me, looks like we should simply check the translation dynamically at every User Setting update.
Unfortunately I am still a dummy user in Drupal, so I cannot implement this fix.
Is anybody in a position to investigate further into this?

bye

I have opened a separate bug report for the email translation cancellation under #369305: User Setting update deletes email translations.

Hi univac,

the patch in #40 is only to set the user language to the current site language. In #38 I said this thread is basically offtopic and in #39 provided the possibility to use modules to solve the original issue of this thread. Very confusing isn't it ;)

Best regards,
Kars-T

Changed the user.test to use a regular expression that checks for 2 lowercase chars. This makes the test run without failure.

:( Runs perfectly with my D7 Head. I mean I don't touch any comment functions or tests.

New version against current head. If this doesn't comply I give it up...

Suggested modification to the solution proposed in #39:

function mymodule_user($op, &$edit, &$account, $category = NULL) {
  if ($op == 'insert') {
    global $language;
    if(!$account->language) {
      user_save($account, array('language' => $language->language));
    }
  }
}

When admins are creating user accounts, this prevents the current page language (of the admin) overriding the language s/he selects for the user.

I created a module for D6 to get rid of the language at registration issue. For D7 I still hope the few lines of patch will get into the core.
My thanks to martinquested!

http://drupal.org/project/reglang

I still agree that merging the language the page is being displayed into the user being created is a good thing and should even be committed to Drupal 6. Users should be created with the language of the page they registered with.

I don't see however how the test actually tests the language being used, it just matches for whatever random two char sequence there is: assertPattern('~[a-z]{2}~', '', t('Correct language field.')

I don't see however how the test actually tests the language being used, it just matches for whatever random two char sequence there is: assertPattern('~[a-z]{2}~', '', t('Correct language field.')

I did this because in my Drupal "de" was set as default language but Drupal always used en for the tests. The new user did get the default language but the check faild because it did check for en. So I thought that maybe cheking for a valid 2 lowercase chars string should be fair enough as this should pose no thread to the system. I am running some more tests with simpletest to achieve this but maybe someone knows:

I don't know if I can just set global $language for the test enviroment as dereine tried to do?
How can I force by using simpletest the system language better said the language of the current user?

There's some language testing in the path test, you should be able to use something along those lines.

Looks like this needs work.

I finally found some time to look into simpletest.
The test suit sets "en" als global language. So now I simply grab the global language and test if the user is set to "en". By this we can make sure that any language that is currently global should be set for a user and the patch is working.

After rereading the thread I see that martinquested in #55 correct and I changed the patch.

The global language is only used if not given from the form.

Next try...

And next one this time with paths...

This looks fine except for

+    //get global language and test that the user has it set. drupal testcase alsways uses "en"
+    global $language;

Inline comments should be formatted with a capital letter and full stop, should be drupalWebTestCase and a typo on always.

Okay I did read the standards again and fixed the comment.

Resubmitting patch against HEAD

critical, IMO

Hi - very thankful that you are working on this! But... some of us tried to review and test this patch at the la codesprint and we got a little stuck because we didn't feel that we had enough instruction as to how to properly test this patch to see if it is working as expected.

Are modules other than core required to properly review this?
Are there specific steps you'd like us to take in reviewing it to make sure things work as they should?

Thank you

Hi thanks for your work!

How to test this?

Drupal has always a language set which is loaded in the bootstrap process. So this test doesn't require any additional modules.

The test environment uses always English as you can see in drupal_web_test_case.php function setup() in line 1066. The current language is preserved as many other variables during install of the new Drupal which uses "en" and reset the language in teardown() line 1205.

As Drupal always has a language set the purpose of this patch is to always set a language for all users as well. If no language is set at registration this patch makes sure the currently active language will be used and set as the users language.

I mean the PHP reflects what the patch is doing. Use the global language and merge it into the users array if no language is set. And the test checks if the currently global language which should be "en" is found in the users array.

All problems with an admin removing or changing the active languages apply to any user that has a language set even without this patch.

I just think it would be a great move for drupal if there are no users in the system with language NULL so we can make sure the environment and emails are properly translated.

Could you please be more precise in what is bugging you / are your thoughts about the patch?
I try to catch you in IRC

double post...

Kars-T - Thank you for taking the time to explain in more detail! I was totally looking in the wrong area during my manual testing, and you corrected me.

Both the simpletest and the manual test works. My users now show en as a language when no language is selected, BOTH when additional languages are enabled and when no languages are enabled except the default.

I didn't see any problems crop up anywhere else.

Why are those patches retested if they are marked as "Tested and Reviewed"? And this patch has nothing todo with "activate/deactivate modules" :(

I have re-tested this, and it still passes all of my manual testing. Is there a way to resubmit the exiting patch for testing again to see if it still fails?

Posting patch against latest CVS.

This has passed all of my manual testing again, and looks good to me!

@Kars-T about D6 _user_mail_text

As you pointed out in #38 there are 2 problems treated in this issue: registration language and translating _user_mail_text.

The former you fixed with your module http://drupal.org/project/reglang. Thanks!

The latter is addressed by your module calling http://drupal.org/project/mail_edit. I propose an alternative approach using i18n see #563048: Multilanguage support for user emails using i18n.

@amalaer
Hello and thanks for your comment!

The Problem with this thread is that it is about some different issues. Right now we try to get the patch from #78 into core of D7 before codefreeze.

As far as I see the code you added is for the reglang module and D6?
Please open an issue or I will do this later in the issue queue of the reglang module. The dependency on the i18n module should be fine. But maybe the use of tt() would be better in this case? Lets dicuss this completely later.

@Kars-T: Yes, for D6.

Please open an issue or I will do this later in the issue queue of the reglang module.

Done so in #563048: Multilanguage support for user emails using i18n. Sorry to interrupt here.

New version

Seems user.module did change. Would be nice if the test bot would be more precise with its messages.

Big thanks to davereid!

Rerolling patch.

The newest patch, from comment #88, passed my manual testing as well as the simpletests run on my install from head.

Once the patch is applied, newly created users automatically receive an entry for language in the user table.

I hope it doesn't fail system testing again! It seems odd to me that it passes manual testing so nicely and then fails system testing. I know you are working hard on this, and it will be a great detail to add into the project in general, so I hope it is able to be committed!

Does #554088: Move user_mail_tokens() and emails to token system have some influence of this patch?

.

@ddave #91
I can't test this but indirectly it should as now there is a language set and the tokens can be translated. I read through the issue but can't figure what exaclty you mean. Maybe you mean more or less this issue? http://drupal.org/node/590014 This was patched but should never happen again as now a language is always set.

I just committed #554088: Move user_mail_tokens() and emails to token system so let's make sure to re-test this patch (just in case).

+++ modules/user/user.module	10 Sep 2009 13:46:14 -0000
@@ -2960,6 +2960,7 @@
   global $base_url;
+  global $language;

Please declare multiple global variables separated by comma.

+++ modules/user/user.test	10 Sep 2009 13:44:20 -0000
@@ -123,7 +123,9 @@
+    //Get the global language and test if the new user has it set. DrupalTestCase sets up a new Drupal with "en" set as default language.
+    global $language;
+    $this->assertEqual($new_user->language, $language->language, t('Correct language field.'));

This comment exceeds 80 chars. But then again, after reading it, I don't think I've learned anything from it, so we can just as well remove it.

However, instead of declaring global $language in the middle of a function, please use $GLOBALS['language'] here instead.

I'm on crack. Are you, too?

Oh mighty testbot accept my new offering.

I hope we can finally a version of this that makes everybody happy. I mean I did use global $language; in the .test file because I though you people did not like the super globals array I would normally use everywhere. :)

And the comment was to point out that maybe someone could expect that testing uses the default language which it doesn't but yes thats not a real important information. I still have to get used to that you want to commit these patches 100% like I supply them and not an altered version.

I did test this on a blank D7 and it passes all user tests.

Looks good. I verified that Locale module only adds the language selector to the user register form when the current user has the "administer users" permission.

+++ modules/user/user.module	3 Oct 2009 06:32:21 -0000
@@ -3069,6 +3069,11 @@
+  if (empty($form_state['values']['language'])) {
+    $merge_data['language'] = $language->language;
+  }

This could use some code comments, then it is RTBC.

I did decide to make two lines of comment to show what we do and why. Hope it fits.

This should go in right after #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update

+

Rerolled the patch against latest HEAD. I now use $GLOBALS as it seems its done commonly in the user.module now and fixed a typo in my comment. I tested this with latest HEAD by hand and simpletest.

Thanks! There's a tiny bit more to it: If we have more than one enabled language, then the user registration form should contain the language selector from Locale module in an invisible state, which already performs appropriate language negotiation to figure out the user's preferred language.

btw, this is how most of Drupal core's hook_form_alter() operations should work: Instead of only including a form section or element by a certain condition, we always want to include the form elements, but set their #access to FALSE when Drupal core assumes they are not needed. In this case, again, you'd have to go through heaps to get that language selector onto the user registration form if you wanted to allow your users to select their preferred language during registration - and we also do not store an auto-determined default value, which this entire issue clearly shows.

Lessons learned: Always #access, not if (...) {. (when speaking about core forms)

FYI: In #118345: Revamp hook_user_form/_register/_validate/_submit/_insert/_update, I already converted many other forms to follow this, which is a monster YAY! for contributed modules and site builders.

Summary:

1) Without this patch, new registered users do not get a language assigned during registration, so neither registration e-mails nor any other, future e-mails are in the user's language (until the user configured a preferred language in his account).

2) When we have a monolingual site without Local module being enabled, then we always store the current language as default value.

3) When we have a multilingual site with Local module being enabled, then we invisibly render Locale's language selection widget on the user registration form, which will lead to a submitted 'language' value when the form is submitted. Contributed modules or site builders are also able to change the #access for this element to allow users choose the language during registration.

Looks RTBC to me if bot passes.

Thanks sun :)

About 3)
I did think about this but decided not to add this. Why would a user signup in chinese but choose english as language? Less to think about for the user at signup.
contain the language selector from Locale module in an invisible state
Have to read more carefully...

Applied the patch and did test it. Works perfect.

#104 describes some very tweaky edge cases that we need to get encoded into SimpleTests.

What needs to be tested:

- Proper display of language selector.
- Proper storage of user language.
- Both with and without Locale module, and depending on the page being displayed. There are three pages: user/register, user/%/edit, and admin/people/create.

What does not need to be tested:

- Sending an e-mail to a user in the user's language. That should already be covered elsewhere, and even if not, has nothing to do with the functionality, which this patch is touching.

Here is the suns patch from #104 extended by a test.

I came to the opinion that it is unnecessary to patch the user module. Without the locale module there never can be a second language and language negotiation. Without both drupal will not set a different global language. So let the user module be as it is. Any user that signs up after the activation of the local module will have the global language set. The users without an empty language will use the default language which the correct behaviour for them.

Through suns Patch the language selector will always be active. On user registration its loaded in the background. On user edit it is shown. And administrative users can always use it.
I believe my patch does reflect this behaviour and tests it on all URLs requested.

I am still very unsure if my comments and the naming of the class and functions are best use as I don't have a lot experience what you people like and what not.

In line 1298 I reset the users password as I don't know a better way to do this because double opt in is the default registration option. We could change login behaviour to avoid this but I think it is better to test as much default settings as possible.

Thanks!

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+  public static function getInfo() {
+    return array(
+    'name' => 'User creation',
+    'description' => 'Test if the current active language is set for a user on creation and if the language selection is shown.',
+    'group' => 'Locale',
+    );

Wrong indentation here.

Description can be shortened to:

"Tests whether proper language is stored for new users and access to language selector."

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $edit = array(
+        'langcode' => 'fr',
+    );
...
+    $edit = array(
+        'language[enabled][locale-url]' => TRUE,
+    );
...
+    $edit = array (
+        'name' => $username,
+        'mail' => $this->randomName(4) . '@example.com',
+        'pass[pass1]' => $username,
+        'pass[pass2]' => $username,
+    );
...
+    $edit = array (
+        'name' => $username,
+        'mail' => $this->randomName(4) . '@example.com',
+    );

Wrong indentation.

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $this->assertText($langcode, t('Language added successfully.'));

This does not really assert that the language was actually added. The text 'fr' can still appear in the form in case of a validation error, so the assertion will pass. Is there no status message displayed after successful form submission?

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    // Check if the language selector is available in the backend and

"in the backend" ? Drupal has no backend.

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $elements = $this->xpath('//input[@id="edit-language-' . $langcode . '"]');
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), t('Global language set in the language selector.'));

Please simply use

$this->assertFieldChecked("edit-language-$langcode");

here, optionally using a custom message as second param like you did.

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+
+

Duplicate newline.

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $elements = $this->xpath('//input[@id="edit-language-' . $langcode . '"]');
+    $this->assertFalse(isset($elements[0]), t('Language selector is not accessible.'));

Please use

$this->assertNoFieldByName('language[fr]');

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $elements = $this->xpath('//input[@id="edit-language-' . $langcode . '"]');
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), t('Language selector is accessable and correct language is selected.'));
...
+    $elements = $this->xpath('//input[@id="edit-language-' . $langcode . '"]');
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), t('Language selector is accessable and correct language is selected.'));

Ditto (assertFieldChecked).

+++ modules/locale/locale.test Locally Modified (Based On 1.56)
@@ -1207,6 +1207,111 @@
+    $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), t('Language selector is accessable and correct language is selected.'));

s/accessable/accessible/

This review is powered by Dreditor.

Thanks for the review sun. I will never ever trust netbeans autoindent again.

This does not really assert that the language was actually added. The text 'fr' can still appear in the form in case of a validation error, so the assertion will pass. Is there no status message displayed after successful form submission?

I did copy this code from testLanguageConfiguration() line 53. Now I added

$this->assertEqual($this->getUrl(), url('admin/config/regional/language', array('absolute' => TRUE)), t('Correct page redirection.'));

as well so its clear that we are not on the language selection page any more and that the language string will only appear on this page if the language was really added.

Please simply use ... here, optionally using a custom message as second param like you did.

I did copy this from testUserLanguageConfiguration() 1202 and thought it was best use. Great thing simpletest can do this now!

New patch with all the changes attached.

Hello, I was wondering if this will this be backported to Drupal 6? It would be very helpful!

I made a mini module that fixed the problem for D6 http://drupal.org/project/reglang

Thanks Kars-T, that module works great with d6.
I'm looking forward to D7!

Code style issues look to be fixed, this was RTBC before that. Also this is no longer critical, mainly cleanup and tests.

Thanks for solving this issue Kars-T! Hope to see this in D7 :)

It was very annoying to forget the language our customers were making the purchase with - we kept on sending invoices in English to people who don't speak it... now solved, very nice.

#111: d7.163165.user-register-language-v03.patch queued for re-testing.

Excellent. Thanks for the vastly expanded test coverage!

Committed to HEAD.

Awesome!

Is there anyway this can be backported to Drupal 6? Or is it too difficult?
I see there is the contrib module but it would be nice if crucial features such as this can be made available in D6 as well.

Kindest regards

@robby.smith: you must be new to Drupal :). only security and bug fixes go into D6 - no new features, that's what D7 is for. (and what's wrong about the contrib. module? it works fine)

ah i see =) thanks for the kind explanation
in that case - i'm really really looking forward to D7!

so, this is a bug not a feature, therefore it can be backported to D6, and even Gábor agrees that it should be in D6.