Wrong language in token_options in user_mail function

Patch applies cleanly to 9.1.x.

Thanks for the issue and patch.

1) Changes seem ok.

2) Marking for manual testing.

3) Kicking off tests for 9.1.x.

Tests are green. This just needs manual testing now and probably needs automated tests too.

I'll work on this!

Manually tested patch, and it works like a charm.

Tested by following steps:

1. Fresh install Drupal 9.1.x.
2. Enable language module.
3. Install any second language: "Chinese, traditional".
4. Set language detection with path prefix, english => en, Chinese, Traditional => zh .
5. Set language preference as Chinese in admin user account settings.
6. Use incognito mode to request for recovery email.
7. Receives mail with "en" recovery link.
   
8. Apply patch, and redo recovery email step.
9. Receives email with "zh" recovery link.
   

Will continue to work on tests.
Remove "need manual testing.

Adding test coverage for user password reset with preferred language and language prefixes

Thanks @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.

+++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
@@ -205,6 +209,72 @@ public function testUserPasswordReset() {
+    // Test password reset when language prefixes are set.
+    $this->drupalGet('fr/user/password');
+    $edit = ['name' => $this->account->getAccountName()];
+    $this->drupalPostForm(NULL, $edit, t('Submit'));

Changed to 'user/password'.

Add interdiff, ready for review once the patch passes.

It'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?).

Yes, $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.

// browse from /zh/user/password
$message['langcode'] = "zh-hant";

// browse from /en/user/password
$message['langcode'] = "en";

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.

Where is message['langcode'] set? Should we not set it there if we are going to ignore it in user_mail()?

Oh 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 😂

I found it, and also lost what to do next here.

The langcode was first set once user submitted the UserPasswordForm.php

// line 170-175
  public function submitForm(array &$form, FormStateInterface $form_state) {
    $langcode = $this->languageManager->getCurrentLanguage()->getId();

    $account = $form_state->getValue('account');
    // Mail one time login URL and instructions using current language.
    $mail = _user_mail_notify('password_reset', $account, $langcode);

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().

// line 1042
function _user_mail_notify($op, AccountInterface $account, $langcode = NULL) {
  if (\Drupal::config('user.settings')->get('notify.' . $op)) {
    $params['account'] = $account;
    $langcode = $langcode ? $langcode : $account->getPreferredLangcode();

// line 1057
$mail = \Drupal::service('plugin.manager.mail')->mail('user', $op, $account->getEmail(), $langcode, $params, $site_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!

So 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.

Okay, 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)

// 1045
    $langcode = $langcode ? $langcode : $account->getPreferredLangcode();

Therefore, I say we change to part at the first part of the api as the fix, the submitForm of UserPasswordForm.php.

Original

/**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
    $langcode = $this->languageManager->getCurrentLanguage()->getId();

    $account = $form_state->getValue('account');
    // Mail one time login URL and instructions using current language.
    $mail = _user_mail_notify('password_reset', $account, $langcode);

to this

/**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state) {
    $account = $form_state->getValue('account');
    // Mail one time login URL and instructions using current language.
    $mail = _user_mail_notify('password_reset', $account, $account->getPreferredLangcode());

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.

I 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.

I 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.

I think we can do #23 now, no need to postpone this that I can see.

Need 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.

Was 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.

Maybe UserMailNotifyTest needs to enable the user module or similar basic stuff?

I 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??

+++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
@@ -94,4 +158,36 @@ public function testUserMailsNotSent($op) {
+    $configLanguageOverride = $this->languageManager->getLanguageConfigOverride('zh-hant', 'user.mail');
+    $configLanguageOverride->set('password_reset.subject', 'zh-hant [user:display-name]')->save();
...
+    $user->set('preferred_langcode', 'zh-hant')->save();
...
+    $return = _user_mail_notify('password_reset', $this->createUser(), $preferredLangcode);

I 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

But the token link doesn't come along with the langcode prefix which I set..... in the email content where have I done wrong??

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 ...

Yeah sure! I'll work on a fail test first.

Add 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.

    // Set language prefix.
    $config = $this->config('language.negotiation');
    $config->set('url.prefixes', ['en' => 'en', 'zh-hant' => 'zh', 'fr' => 'fr'])->save();
    $this->configFactory->reset('language.negotiation');
    ConfigurableLanguageManager::rebuildServices();

// These will fail no matter what langcode given to _user_mail_notify().
$this->assertMailString('body', 'zh/user/reset', 1);

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.

The 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.

Discovered 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.

I 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.

Remove tag needs test, set to Needs review.

Nice! i think this is ready, just some nits:

1. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -16,6 +18,67 @@ class UserMailNotifyTest extends EntityKernelTestBase {
   +  protected function setUp():void {
   

   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()

2. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -94,4 +157,68 @@ public function testUserMailsNotSent($op) {
   +    // Current language is 'en'.
   +    $currentLanguage = $this->languageManager->getCurrentLanguage()->getId();
   

   This variable is unused. I suggest asserting it.

3. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -94,4 +157,68 @@ public function testUserMailsNotSent($op) {
   +    $franch_email = _user_mail_notify('password_reset', $user, 'fr');
   

   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.

Updated issue summary.
Remove need issue summary tag.

Thanks 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.

Should #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.

Agreed

Blocker was committed, thanks to @alexpott.

1. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -16,6 +18,36 @@ class UserMailNotifyTest extends EntityKernelTestBase {
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  protected static $modules = [
   +    'user',
   +    'system',
   +    'field',
   +    'text',
   +    'filter',
   +    'entity_test',
   +    'locale',
   +    'language',
   +  ];
   

   You only need to add the new modules here... so locale and language I think.

2. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -16,6 +18,36 @@ class UserMailNotifyTest extends EntityKernelTestBase {
   +  /**
   +   * The configurable language manager used in this test.
   +   *
   +   * @var \Drupal\language\ConfigurableLanguageManagerInterface
   +   */
   +  protected $languageManager;
   +
   +  /**
   +   * The configuration factory used in this test.
   +   *
   +   * @var \Drupal\Core\Config\ConfigFactoryInterface
   +   */
   +  protected $configFactory;
   
   @@ -94,4 +126,93 @@ public function testUserMailsNotSent($op) {
   +    $this->configFactory = $this->container->get('config.factory');
   +    $this->languageManager = $this->container->get('language_manager');
   

   Attaching these services to the test class and then rebuilding the container is a way to cause interesting bugs. Let's not do this.

3. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -94,4 +126,93 @@ public function testUserMailsNotSent($op) {
   +    $this->configFactory->reset('language.negotiation');
   ...
   +    $this->configFactory->reset('user.mail');
   ...
   +    $this->configFactory->reset('user.mail');
   

   THese are not necessary.

4. +++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
   @@ -94,4 +126,93 @@ public function testUserMailsNotSent($op) {
   +    // Reset services to apply change.
   +    \Drupal::service('kernel')->rebuildContainer();
   

   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.

1. 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. ;)

Fix empty space. (coding standard issue)

What is the reason causing the test to fail...?

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1129.xml:
PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

Specifying 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

Hahaha, so that mean we can't use deprecated code in tests?

Correct, we can't introduce new uses of deprecated code.

I 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.

+++ b/core/modules/user/tests/src/Kernel/UserMailNotifyTest.php
@@ -105,4 +117,69 @@ public function testUserMailNotifyLangcodeDeprecation() {
+    // Recovery email should respect user preferred langcode by default if
+    // langcode not set.
+    $default_email = _user_mail_notify('password_reset', $user);

+++ b/core/modules/user/user.module
@@ -775,7 +775,7 @@ function user_mail($key, &$message, $params) {
-  $language = $language_manager->getLanguage($params['account']->getPreferredLangcode());

I 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);

Thanks, I'll work on it.

Thanks @paulocs, you're the man!! Here's the baton~ My timezone is in the moon phase now LoL.

Thanks hahaha, good night!

Looking 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.

Committed 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.