Summary
The user_mail function respects the language value specfied by _user_mail_notify() for the text of user emails, but it ignores it for the tokens in emails, such as password recovery links. Therefore in some cases the text and token languages can be different. For example, you can get email content in french, but the password reset URL comes as "/en/user/reset/..." .
Cause
There are a total of three API setting $langcode along the way to set the language of recovery email.
- submitForm() in UserPasswordFrom.php, taking the current browsing langcode and passed to _user_mail_notify().
- _user_mail_notify in user.module line 1042, it has a condition written to use the $langcode as $account->getPreferredLangcode if not specified. Here is where it sends an email request by invoking MailManagerInterface::mail() service and passing $langcode in its parameter, which goes through user_mail (hook_mail) to modify it's email contents.
- user_mail in user.module line 772, it has set to retrieve it's email content base on getPreferredLangcode of User while token option was set to follow the $message[langcode] variable from MailManagerInterface::mail().
The descriptions above reveals the bug how email's text content doesn't align with content replaced by tokens (URL with langcode prefix in this case) is within user_mail, whereas it should use the langcode that _user_mail_notify() provided as said by @jonathanshaw #23.
To understand the details of discussion, you can start from #21.
Password reset urls and account preferred language
This issue does not fully address password recovery emails not respecting the user's preferred language, because the language is also set in UserPasswordForm::submitForm() and there is an old issue for that: #86287: Password reset process ignores the user's language preference.
Original Problem/Motivation
In the user_mail function, the language manager is set to the preferred langcode of the user.
$language = $language_manager->getLanguage($params['account']->getPreferredLangcode());
$original_language = $language_manager->getConfigOverrideLanguage();
$language_manager->setConfigOverrideLanguage($language);
In some cases if we load config, the mail_config is translated in the language of the user:
$mail_config = \Drupal::config('user.mail');
While loading the token options, an other langcode is set?!
Steps required to reproduce the bug
While sending a "Password Recovery" mail, the mail subject and body is in the language of the user.
The [user:one-time-login-url] token is in another language.
Solution, see patch.
Proposed resolution
New proposal
Quoting #23:
The only thing we need to fix is in user_mail():
$language = $language_manager->getLanguage($params['account']->getPreferredLangcode());
should become
$language = $language_manager->getLanguage($langcode);
user_mail() should use the langcode that _user_mail_notify() has provided.
Original proposal
Set the correct langcode in $token_options array:
$token_options = ['langcode' => $language->getId(), 'callback' => 'user_mail_tokens', 'clear' => TRUE];
See Patch.
Remaining tasks
None.
User interface changes
None
API changes
None
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#61 | 2991677-61.patch | 4.98 KB | paulocs |
#61 | interdiff-57-61.txt | 1.32 KB | paulocs |
#57 | 2991677-57.patch | 4.44 KB | paulocs |
#57 | interdiff-51-57.txt | 1.15 KB | paulocs |
#52 | interdiff-49_51.txt | 290 bytes | g-brodiei |
Comments
Comment #2
tomhollevoet CreditAttribution: tomhollevoet at Calibrate commentedComment #3
tomhollevoet CreditAttribution: tomhollevoet at Calibrate commentedComment #7
Kristen PolPatch applies cleanly to 9.1.x.
Comment #8
Kristen PolThanks for the issue and patch.
1) Changes seem ok.
2) Marking for manual testing.
3) Kicking off tests for 9.1.x.
Comment #9
Kristen PolTests are green. This just needs manual testing now and probably needs automated tests too.
Comment #10
g-brodieiI'll work on this!
Comment #11
g-brodieiManually tested patch, and it works like a charm.
Tested by following steps:
Will continue to work on tests.
Remove "need manual testing.
Comment #12
g-brodieiComment #13
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAdding test coverage for user password reset with preferred language and language prefixes
Comment #14
g-brodieiThanks @raman.b for this thorough testing patch!!
Added failtest patch, altered the link to request reset password in test to assure it respects user's preferred language instead of site use language.
Changed to 'user/password'.
Add interdiff, ready for review once the patch passes.
Comment #16
jonathanshawIt's right that the token language needs to be consistent with the message language.
But I'm not sure about the fix.
We have two sources for what the language should be: the language specified in $message, and the user's preferred language.
If a language is specified in $message, maybe we should be using that? It seems right to defer to the calling code about language, instead of overriding it.
We could though default to the user's preferred language if no language is set in $message (although at the moment the code doesn't even consider that, is $message['langcode'] always set?).
Comment #17
g-brodieiYes, $message['langcode'] is always set in user_mail_tokens() as I see it.
After running through xdebug, $message['langcode'] is base on the language browsed by the user when they apply to reset password, not the preferred language set in user account, thereby the current site language. Without other second language installed, the preferred language is set to "en" by default.
Since all the user related emails are expected to respect the preferred language in account settings, maybe we should not make use of $message['langcode']? I think that's how this fix came out?
If the current language_manager is not override by the preferred language, then body and message language will remain same with the current viewing site prefix language, base on the language detection method, either /en or /zh on my setting.
Comment #18
jonathanshawWhere is message['langcode'] set? Should we not set it there if we are going to ignore it in user_mail()?
Comment #19
g-brodieiOh sorry, my bad.
I meant it was passed into the $user_mail().
I'll search for the code where it is set and passed in then 😂
Comment #20
g-brodieiI found it, and also lost what to do next here.
The langcode was first set once user submitted the UserPasswordForm.php
Then the actual call of mail service and to send mail + set $message['langcode'] in user.module, the call on mail service will then pass the $langcode as $message['langcode'] to user_mail().
Are you suggesting that we should set the $langcode earlier to $preferredLangcode() before it's passed into the mail()?
Thanks for all the help and review!
Comment #21
jonathanshawSo we have a 3 level API: form, user_mail_notify, and user_mail. Currently we're trying to set the language in all 3!
user_mail_notify invokes MailManagerInterface::mail(). That method has langcode as a required argument.
Therefore I propose we make language determination the responsibility of user_mail_notify.
This has 4 implications:
1) user_mail should trust the language param it is supplied with, and we can trust that it will have one because it can only be invoked via MailManagerInterface::mail().
2) Code that calls user_mail_notify may specify the langcode, and it will be used if provided.
3) Core code that calls user_mail_notify should never specify the language, it should leave it to user_mail_notify to determine.
4) We should trigger a deprecation error if the langcode argument is provided to user_mail_notify. Technically it's an internal method and we could just remove the argument, but I suspect its being used often in contrib and custom code.
Comment #22
g-brodieiOkay, I looked into the code within _user_mail_notify, and I think it already has the langcode determination part included within its function already.
function _user_mail_notify($op, AccountInterface $account, $langcode = NULL)
Therefore, I say we change to part at the first part of the api as the fix, the submitForm of UserPasswordForm.php.
Original
to this
Since the account has it's preferred langcode set to it's default language already. I don't think we need to get current langcode from languageManager anymore when the reset form is first submitted.
Comment #23
jonathanshawI found #86287-60: Password reset process ignores the user's language preference which suggests that sometimes there is a purpose to specifying the langcode when calling _user_mail_notify(). I actually suspect that in user_mail_notify we should always be using the current language if there is no account preferred langage, but that's a separate issue.
So I think we should ignore #21.2, #21.3 and #21.4 (sorry for time wasting!)
The only thing we need to fix is in user_mail():
$language = $language_manager->getLanguage($params['account']->getPreferredLangcode());
should become
$language = $language_manager->getLanguage($langcode);
user_mail() should use the langcode that _user_mail_notify() has provided.
Comment #24
g-brodieiI agree with you on the point that user_mail should follow the language code given when it's being called in _user_mail_notify, where the ultimate culprit actually started from submitForm() after reading through #86287-60: Password reset process ignores the user's language preference.
The approach of @wengerk in #60 is what I have in mind after our discussion. Once it is fixed, we can set the fix you mentioned on #23 in place all together. Or we should just fix it once and for all together in that issue?
Updated Issue Summary base on #23.
Setting #86287: Password reset process ignores the user's language preference as related issue, which is the blocker.
Comment #25
jonathanshawI think we can do #23 now, no need to postpone this that I can see.
Comment #26
g-brodieiNeed to add new test for this piece of fix. This is simply fixing the alignment of language sent by recovery email request between content and reset link by allowing user_mail() function to respect the set $langcode in it's variable. It will not fix the issue where user's preferred langcode is ignored.
The test @raman.b wrote #13 should be added to #86287: Password reset process ignores the user's language preference where the user preferred langcode is to be fixed.
Add new patch base on #23.
Have also tested this patch with user preferred langcode as zh-hant.
1. "sending confirmation request on delete" email
2. "sending new account creation" email
It respects the user's language preference as normal.
Comment #27
g-brodieiWas running a kernal test on this earlier LoL, and then I discovered that even though _user_mail_notify was invoked directly in UserMailNotifyTest.php, it doesn't go through user_mail or token replacement at all lmao, nor does it have any config within "user.mail". Gonna make use of functional test instead.
Current idea will be:
1. Enable interface translation (locale), language (language).
2. Add new language.
3. Set language prefix.
4. Set account language preference to new foreign lang.
4. Add language name (or identical strings) to subject and body interface of recovery emails, both in original and translated interface relatively.
5. Retrieve the content and find recovery url, and do an assertion.
Comment #28
jonathanshawMaybe UserMailNotifyTest needs to enable the user module or similar basic stuff?
Comment #29
g-brodieiI figured out how to set the prefix, langcode and loading config from user module and override config translation for password recovery.
But the token link doesn't come along with the langcode prefix which I set..... in the email content where have I done wrong??
Comment #30
jonathanshawI suggest we want to test:
- user preferred language zh-hant, no preferred language specified as a third parameter to user_mail_notify: email should use zh-hant
- user preferred language zh-hant, fr specified as preferred language for user_mail_notify: email should use fr
Can you make a failing test case for that? It would then be easier to diagnose and be sure I understand what you mean. I can't see any obvious bug ...
Comment #31
g-brodieiYeah sure! I'll work on a fail test first.
Comment #33
g-brodieiAdd failtest patch and testing patch.
Both patch will fail on the first assertion when testing the token link with langcode prefix which should exist. This is the part were I'm having trouble at the moment, don't know what will be the reason to allow token link to return with langcode prefix for a kernal test? I had the language url.prefixes set correctly.
If we simply comment all three attempts of language prefix assertion ('zh/user/reset', 'fr/user/reset') and re-run the test with fix, it will confirm that the email's content does respect the $langcode variable given to _user_mail_notify() function when provided.
Comment #35
jonathanshawThe function
user_pass_reset_url()
(which generates the reset url token value) has no test coverage for language. Maybe try testing that directly, it could be buggy.Comment #36
g-brodieiDiscovered that the prefix was added by processOutBound function in file "PathProcesserManager.php" when it runs through the PathProcessorLanguage object. This object has the language prefix upon http request. On the contrary, the kernal testing doesn't go through http request.
I'll try to mimic/add a similar processor object in setup and see how it goes.
Request by website form directly.
Request with phpunit testing tool.
Comment #37
g-brodieiI found out what happened with language prefix not added by the token service, the kernal service wasn't rebuilt LoL.
Added fail patch, a working patch to fix $langcode in user_mail.
Comment #38
g-brodieiRemove tag needs test, set to Needs review.
Comment #39
jonathanshawNice! i think this is ready, just some nits:
All of this setup code is only used in testUserRecovery, which also does its own further setup steps. I'm not sure what should be in setUp() (maybe nothing) and what in testUserRecovery()
This variable is unused. I suggest asserting it.
Should be $french_email
Last, the IS needs updating to state the problem as we now understand it and the solution as we propose it. It could be much simpler. The current IS has too much history and is confusing.
Comment #40
g-brodieiUpdated issue summary.
Remove need issue summary tag.
Comment #41
g-brodieiThanks for spending time review and providing constructive feedbacks @jonathanshaw!!
Let's hope this will be the final patch to fix this bug, hahaha.
Providing patch to fix on points in #39.
1. Changed all code for language setup within setUp() to be within testUserRecoveryMailLanguage(), since only this test needed it.
2. Assert current language and preferred language codes with assertSame().
3. Fixed Typo of "Franch" to "French", silly mistake LoL.
Comment #43
jonathanshawComment #44
alexpottShould #86287: Password reset process ignores the user's language preference land first? It kinda feels that way. If it doesn't that the while the password reset email becomes more consistent it moves further away from the user's preferred langcode.
Comment #45
jonathanshawAgreed
Comment #46
jonathanshawBlocker was committed, thanks to @alexpott.
Comment #47
alexpottYou only need to add the new modules here... so locale and language I think.
Attaching these services to the test class and then rebuilding the container is a way to cause interesting bugs. Let's not do this.
THese are not necessary.
Let's do
$this->container->get('kernel')->rebuildContainer();
- it'd be nice if a container invalidation caused the container to be rebuilt in a kernel test - because the code is assuming that the container will be rebuilt on the next request.Comment #48
g-brodieiComment #49
g-brodiei1. Fixed patch to meet changes from deprecation #3186752: Deprecate langcode argument to _user_mail_notify() for _user_mail_notify.
2. Revised base on #47. Thank you @alexpott, I had no idea how the flow of config process works in the system, was just looking up other test codes as reference to use.
Tested this patch on local, will fail when fix not applied. So only providing fixed patch here. ;)
Comment #50
g-brodieiComment #52
g-brodieiFix empty space. (coding standard issue)
What is the reason causing the test to fail...?
Comment #53
jonathanshawSpecifying the notification language using the $langcode parameter is deprecated in drupal:9.2.0
We are moving so fast we are tripping ourselves up!7
Comment #54
g-brodieiHahaha, so that mean we can't use deprecated code in tests?
Comment #55
jonathanshawCorrect, we can't introduce new uses of deprecated code.
Comment #56
paulocsComment #57
paulocsI was wondering if we have to have the tests for when the langcode is specified if the user has already a preferred language.
I did a patch that removes the deprecated code but not sure if it will cover less cases.
Please review.
Comment #58
alexpottI think that rather than calling _user_mail_notify here you are going to need to call $mail = \Drupal::service('plugin.manager.mail')->mail('user', $op, $account->getEmail(), $langcode, $params, $site_mail);
Comment #59
paulocsThanks, I'll work on it.
Comment #60
g-brodieiThanks @paulocs, you're the man!! Here's the baton~ My timezone is in the moon phase now LoL.
Comment #61
paulocsThanks hahaha, good night!
Comment #62
jonathanshawLooking good now. @alexpott's review points from #47 are addressed and we are no longer using the deprecated $langcode parameter to _user_mail_notify(), but we are still testing things properly.
Comment #63
alexpottCommitted 34f0542 and pushed to 9.2.x. Thanks!
Committed abe9d46 and pushed to 9.1.x. Thanks!
Backported to 9.1.x as this change makes the function behave the way it should and you would expect it to.