Clarify via the field description that the notification email address is used for the reply-to address, not the from address

Updated issue title. The system email address is NOT the user-1 email address (although, they could be set to the same value/email).

I confirm that when receiving password reset emails and new user notification emails, the From email address does not use the notification email address if set /admin/config/people/accounts and continues to fall back to the system email address set at /admin/config/system/site-information.

Hi All, the issue is present in Drupal 10 Dev version as well, so updating the Drupal version for this issue to Drupal 10 Dev. I have investigated the issue and found that the problem is with the logic written inside doMail() function [File path - web\core\lib\Drupal\Core\Mail\MailManager.php]. Here we are not fetching the data from the "Notification email address" field. As per the existing logic, we are just setting the default site mail address as "From: " address while sending mails and if this value is not there then we are checking in settings file.

So I have corrected the logic in order to fix this issue. As per my analysis, correct logic should be -

1. First we should check if any from email address has been set in the "Notification email address" settings.
2. If not, then only we should set the default site mail address as the "From:" address.
3. At last, if there is no from address set in any of those above mentioned places, then we should check if any value has been set in settings file.

Attaching the patch for this issue. I have tested it from my end and it's working as expected. Request the community to validate my patch and let me know if there is any observation.

Hi All,

I had to modify the following test - "core\modules\user\tests\src\Functional\UserAdminTest.php", Method - "testNotificationEmailAddress" as in this functional test also we were matching the email from address with default system mail even after setting the "Notification email address" value. I have modified the test as per our new logic, so that after setting the notification mail address it will now match the from email address with the value set as "Notification email address" instead of default system mail. Request the community to validate the patch and let me know if there is any observation.

Thanks & Regards,
Anup

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Can confirm the issue still exists on 10.1.x and that patch #10 addresses the issue.

Moving back to NW for additional test coverage.

Will need a scenario that shows what is sent when Notification email address is and isn't set. What email is received.

Thanks!

Hi @smustgrave,

Many thanks for moving it to the review queue. Now to answer your query, the exact same email will be triggered in both the cases. It's just that the from address will be different and this is already adjusted in UserAdminTest.php.

a) When notification email address is not set -> From address will be the default site email address set during Drupal installation (System->Basic Site settings).
b) When notification email address is set -> From address will be the "Notification Email address" set in Account settings.

Could you please let me know if any other scenarios need to be covered for this change?

Thanks & Regards,
Anup

Think those are the two scenarios. Also for tests if you can upload a tests-only patch and the full one together in that order. So we can see the red/green

Hi @smustgrave,

I have added a new test to cover the scenario when notification email address is not set. So now below are the two scenarios and their corresponding tests -

Test file - core\modules\user\tests\src\Functional\UserAdminTest.php
1. When notification email address is set -> Adjusted Test - testNotificationEmailAddress()
1. When notification email address is not set -> newly created Test - testNotificationEmailAddressNotSet()

Also attached test only patch (it will fail as expected as the fix is not included in the patch) and the full patch as requested. Please review the patch and let me know if there is any observation.

Thanks & Regards,
Anup

Thanks! Removing the needs tests tag.

Manually tested the patch following the steps in the IS and confirmed the issue.
With patch confirmed the correct email is being used.

Thanks for working on this bug! There are just a few minor code style and comment issues to fix:

1. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -226,7 +226,13 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
   +    // Get the custom site notification email to use as the from email address.
   +    // if it has been set.
   

   This has some grammatical oddities. I'd suggest:

   If a custom notification email address has been configured, use that address.

2. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -226,7 +226,13 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
   +    $site_mail = \Drupal::config('system.site')->get('mail_notification');
   ...
   +      $site_mail = \Drupal::config('system.site')->get('mail');
   

   Shouldn't we inject the config factory rather than using \Drupal::config()?

3. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -226,7 +226,13 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
   +    // If the custom site notification email has not been set, we use the site default mail.
   

   This comment needs to wrap at 80 characters. I'd also suggest it be changed to:
   Otherwise, use the default site email address.

4. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +   * Tests the email address for mails when notification email address not set.
   

   This also is a bit grammatically weird. I'd suggest:

   Tests email notifications with no custom notification address configured.

5. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +  public function testNotificationEmailAddressNotSet() {
   

   This should have a void return typehint.

6. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +    // Test custom user registration approval email address(es).
   

   Why is the "es" in parentheses? Either we're testing one, or we're testing more than one. I also actually think this comment could be removed, since it sort of duplicates the docblock.

7. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +    // Set the site and notification email addresses.
   ...
   +    // Register a new user account.
   

   I'd suggest adding newlines above both these comments for readability.

8. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +    $system = $this->config('system.site');
   

   $system is a bit of a confusing variable name. I would change this to $site_configuration.

9. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
   +    $server_address = $this->randomMachineName() . '@example.com';
   ...
   +    $edit['name'] = $this->randomMachineName();
   +    $edit['mail'] = $edit['name'] . '@example.com';
   

   In general, it's better to specify a specific, meaningful test value than to use randomMachineName(). (Our best practice or this has changed over the years.

10. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
    +    // When user notification mail is not set
    +    // Email from address will be the default site mail.
    

    This comment is wrapping too early. It should use as close to 80 chars as possible on the first line. I'd suggest:

    When no custom notification email address is set, the email will be sent from the site default email address.

    Also, a newline above here would be good.

11. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
    @@ -222,11 +222,47 @@ public function testNotificationEmailAddress() {
    +    $this->assertCount(1, $user_mail, 'New user mail to user is sent from configured Notification Email address');
    

    We can remove the third parameter from this assertion; we have moved away from using custom assertion messages in core as a best practice.

Hi @Jess,

Thanks for reviewing the changes and providing valuable feedbacks. I have created a new patch by addressing all the code review comments for both my new test function as well as existing "testNotificationEmailAddress" function.

Only the comment #5 not addressed as PHP Code sniffer throwing the below error if I am adding void return-type.
"If there is no return value for a function, there must not be a @return tag". Please check the rest of the changes in my new patch and let me know if there is any observation.

Thanks & Regards,
Anup

@anup.sinha thank you for all the work on the ticket.

When making changes please include an interdiff between two patches so we can easier see the changes.

Also for the CI failure you can test locally with

./core/scripts/dev/commit-code-check.sh

Tried to fix CCF for #19.

Hi @Utsav,
Thanks for fixing the "drupaluser" word. The build for #19 failed as this word is not there in dictionary.txt.

@smustgrave, PFA the interdiff between patch #14 and #19. I have just fixed all the code review comments provided by Jess in patch #19.

Thanks & Regards,
Anup

Thanks @anup.sinha great work!

Thanks everyone!

+++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
@@ -188,45 +188,88 @@ public function testNotificationEmailAddress() {
-    $server_address = $this->randomMachineName() . '@example.com';
-    $notify_address = $this->randomMachineName() . '@example.com';
+    $server_address = 'site.admin@example.com';
+    $notify_address = 'site.notify@example.com';

It's slight scope creep to fix the existing method in the same way as the new method, but since it makes the test much more clear and consistent, we'll go ahead and commit it as-is.

Adding credit for @smustgrave for helpful and thorough reviews, @Ralf Eisler for reporting the issue, and @Christian DeLoach for manual testing and clarifying the exact bug.

I made some small comment fixes on commit:

diff --git a/core/lib/Drupal/Core/Mail/MailManager.php b/core/lib/Drupal/Core/Mail/MailManager.php
index 0bbce7dff7..39ae0ebf1d 100644
--- a/core/lib/Drupal/Core/Mail/MailManager.php
+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -226,13 +226,15 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
    */
   public function doMail($module, $key, $to, $langcode, $params = [], $reply = NULL, $send = TRUE) {
     $site_config = $this->configFactory->get('system.site');
-    // If a custom notification email address has been configured,
-    // use that address.
+    // If a custom notification email address has been configured, use that
+    // address.
     $site_mail = $site_config->get('mail_notification');
     // Otherwise, use the default site email address.
     if (empty($site_mail)) {
       $site_mail = $site_config->get('mail');
     }
+    // Finally, default to the server email address if no site email has been
+    // configured.
     if (empty($site_mail)) {
       $site_mail = ini_get('sendmail_from');
     }

Committed to 10.1.x. I considered backporting this, but it could be slightly disruptive, because the sending email address for many thousands of site account notification emails will change across sites that have this configured. So, I've kept it to 10.1.x only, and added a change record and a release note. I also added it to the release notes draft for 10.1 as per our new policy.

Sorry, in the course of reviewing my own release note and the CR, I realized a fundamental problem: The fix here is changing the sending address for all site emails when the setting is configured (including "site update available" notifications and the like), not just mails sent by the user account.

So, I think we need more structural fix to allow the mail manager to have the sender email set in a separate method, and allow the value to be set/injected by the caller.

Sorry! Uploading a patch from my commit that has the comment fix, but actually we need further work here. =/

Hi Jess,

Many thanks for pointing out the missed scenario. I have implemented a patch to restrict our new condition only for user operations without creating a new method.

> When user module is calling the mail() function from _user_mail_notify(), it's sending the module name (1st parameter) as "user".
> So I have added a check that our new condition will get executed only when the module name is "user", not for other scenarios

Could you please check this solution and let me know if it looks good to you.

Thanks & Regards,
Anup

Thanks @anup.sinha! That's a good start, and will at least avoid the regression that we almost caused. 😉

However, it has the disadvantage of putting logic related to the user module in a low-level core component. That's why I suggested making the sending mail address injectable into MailManager.

Tagging for framework manager feedback on the best architecture to use here.

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -226,7 +226,23 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
+    if ($module == 'user') {
+      // If a custom notification email address has been configured, use that
+      // address.
+      $site_mail = $site_config->get('mail_notification');
+      // Otherwise, use the default site email address.
+      if (empty($site_mail)) {
+        $site_mail = $site_config->get('mail');
+      }
+    }
+    else {
+      $site_mail = $site_config->get('mail');
+    }
+
+    // Finally, default to the server email address if no site email has been
+    // configured.

We shouldn't be special casing the user module here.

doMail fires both hook_mail and hook_mail_alter after this hunk, so this special-case logic should belong in user.module in it's hook_mail implementation

Hi Jess,

Here is my updated patch where the "From" email address has been passed from the user module - _user_mail_notify() function. The changes are as below, also attached the interdiff.

1. Added a new parameter "$from" for mail() function. I thought if we have every parameters available in the function even the "reply-to" then why not $from param.
2. Also initialized the param as NULL and added it as the last param so that it will not break any of our existing code.
3. Now in the doMail() function, I have added the check if the $from param is available then use that as from address, otherwise our default site address.

Right now we don't have any option to set the from address while using Drupal mail() function, so this will allow us to use our custom from address as well. Let me know your thoughts please on this patch. Otherwise, I can just alter the $message['headers']['FROM'] value in user_mail() function as suggested by Lee and can create a new patch.

Thanks & Regards,
Anup

Here is an updated patch by fixing the error from previously uploaded patch.

Thanks,
Anup

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -170,14 +170,14 @@ public function getInstance(array $options) {
+  public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL, $send = TRUE, $from = NULL) {
...
+    return $this->renderer->executeInRenderContext(new RenderContext(), function () use ($module, $key, $to, $langcode, $params, $reply, $send, $from) {
+      return $this->doMail($module, $key, $to, $langcode, $params, $reply, $send, $from);

@@ -214,6 +214,8 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
+   * @param string|null $from
+   *   Optional from email address.

@@ -224,13 +226,19 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
-  public function doMail($module, $key, $to, $langcode, $params = [], $reply = NULL, $send = TRUE) {
+  public function doMail($module, $key, $to, $langcode, $params = [], $reply = NULL, $send = TRUE, $from = NULL) {
...
+    // If from address is set, use that
+    // as reply to and from email.
+    if (!empty($from)) {
+      $site_mail = $from;
+    }

+++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
@@ -113,6 +113,8 @@ interface MailManagerInterface extends PluginManagerInterface {
+   * @param string|null $from
+   *   Optional from email address.

@@ -121,6 +123,6 @@ interface MailManagerInterface extends PluginManagerInterface {
+  public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL, $send = TRUE, $from = NULL);

We don't need any of these changes, $params['from'] can be set

Hi Lee,

PFA the latest patch by altering the from address from hook_mail() in user.module. Also $params['from'] was not setting the from address, then by checking the doMail() function I found out that it's actually $message['from'] variable through which we can alter the "FROM:" address.

Request you to review this patch and let me know please if it looks good to you.

Thanks & Regards,
Anup

Hi Lee and Jess,

Can you please now commit this patch into Drupal 10 branch if everything looks good to you.

Thanks & Regards,
Anup

Retested #38 and I'm not longer receiving emails from my account email just from site email.

Thanks, the approach is looking much cleaner, no API changes, no leaking of user.module into core services.

1. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -188,45 +188,88 @@ public function testNotificationEmailAddress() {
   -    // Test custom user registration approval email address(es).
   ...
   +
   ...
   -    $server_address = $this->randomMachineName() . '@example.com';
   -    $notify_address = $this->randomMachineName() . '@example.com';
   +    $server_address = 'site.admin@example.com';
   +    $notify_address = 'site.notify@example.com';
   ...
   -    $edit['name'] = $this->randomMachineName();
   +    $edit['name'] = 'drupalUser';
   ...
   +
   ...
   -    $this->assertCount(1, $admin_mail, 'New user mail to admin is sent to configured Notification Email address');
   +    $this->assertCount(1, $admin_mail);
   ...
   -    $this->assertCount(1, $user_mail, 'New user mail to user is sent from configured Notification Email address');
   +    $this->assertCount(1, $user_mail);
   

   These changes are out of scope

2. +++ b/core/modules/user/tests/src/Functional/UserAdminTest.php
   @@ -188,45 +188,88 @@ public function testNotificationEmailAddress() {
   +  public function testNotificationEmailAddressNotSet() {
   

   It feels odd that we're adding a new test case for existing functionality, instead of retaining the existing test and adding a new test for the new functionality

3. +++ b/core/modules/user/user.module
   @@ -767,9 +767,22 @@ function user_mail($key, &$message, $params) {
   +  if (empty($notify_mail)) {
   +    $notify_mail = \Drupal::config('system.site')->get('mail');
   +  }
   +  if (empty($notify_mail)) {
   +    $notify_mail = ini_get('sendmail_from');
   +  }
   

   Doesn't the default mail handler do this if we don't pass $message['from']

   i.e. should we conditionally set from, only when mail_notification is set?

We're on the home straight now, thanks!

Hi Lee,

Thanks for reviewing the code. I agree with you that we should alter the value only when notify email is set. Also merged the new test into existing one. Here is my updated patch.

Also the changes which are out of scope are actually code review comments given by Jess in comment #18 for my new test. Like we should not use random function to generate username, in assertCount third message parameter is no longer used etc. So at that time I thought if we are fixing these issues in new test then fix those problems in existing one as well. Currently both the tests are merged, so I think these are in-scope now.

@smustgrave, could you please retest? Both the scenarios are covered in our unit test - 1. when notify mail is set, from address should be notify mail 2. when notify email is not set, from address should be site email.

Thanks & Regards,
Anup

Retested #42 and things are working as expected.

Changes from #41 also appear to be addressed

Doesn't look like #41 items 1 and 2 have been addressed

Hi @Lee,

Please find below my response -

Comment #41 - 2. As you can see in my new patch #42, the new test function testNotificationEmailAddressNotSet has been deleted and the new testcases have been included in the existing testNotificationEmailAddress test itself.

Comment #41 -1. As I mentioned in my comment #42 - The changes which are out of scope are actually code review comments given by Jess in comment #18 for my new test. Like we should not use random function to generate username, in assertCount third message parameter is no longer used etc. As now both the tests are merged, so I believe those code review comments given by Jess are in-scope now.

Please let me know if I should revert these changes, then I will create a new patch.

Thanks & Regards,
Anup

Hi @anup.sinha I'm pretty sure @xjm made those comments in relation to new tests but would agree with me that changing existing tests is out of scope here. But I'll ask @xjm to confirm that.

FWIW I don't agree with using discrete values, as they can lead to collisions, I would always prefer using random values.

Hi @Lee,

Thanks for the update. I have reverted all the unwanted changes and created a new patch which contains changes required for this fix only. PFA the updated patch.

Thanks & Regards,
Anup

Retested #48 and the functionality is still there.

Appears the points by @larowlan in #47 have been addressed.

Thanks, this looks 10x simpler/cleaner now.

I've queued a 9.5 test run, although I suspect that will fail because 9.5.x HEAD is failing due to ::upgradePhpUnit taking us all the way to PHPUnit 9.6 which has new deprecations.

Retesting 9.5.x now that HEAD is fixed

Looking at the current code-base there's a few things that stand out.

1) We don't have a default value for mail_notification in system.site, this feels like it breaks our standard contract for config objects, it should at least be null I think. I want to ask @alexpott about this. It will likely mean a follow-up issue.

2) There is an existing test for this functionality, as follows:

// Ensure that user notification mail is sent from the configured
    // Notification Email address.
    $user_mail = $this->drupalGetMails([
      'to' => $edit['mail'],
      'from' => $server_address,
      'reply-to' => $notify_address,
      'subject' => $subject,
    ]);
    $this->assertCount(1, $user_mail, 'New user mail to user is sent from configured Notification Email address');

As you can see, it is testing that the emails have the notification email as the reply-to.

So perhaps the issue here is that the description is wrong, and it should say 'the reply-to' address.

This feature was added in #494518: Allow for custom user registration approval email address

The issue with having a different from address is the site might need to go through a verification process for that email address to be able to send email on behalf of that address, whilst the reply-to does not have that issue.

I will ask the User module maintainer for their thoughts.

I was summoned here as a User system maintainer.

1. Yeah, I would think null value would improve discovery for those who browse yml files.
2. "So perhaps the issue here is that the description is wrong, and it should say 'the reply-to' address." I agree with this. A contrib module can make further mail alterations for sites that want that. Having said that, I'm not going to block the patch. If the maintainers think its an improvement, feel free to merge.

Seems like we are a go. Opened a follow up for point 1.

Discussed this with other committers and they agreed that the behaviour change here could have unwanted side-effects for sites.

e.g. in many hosting platforms, there is one validated from address for the whole site that has a raft of measures wound up in it to ensure that there is no issues with spam.

With this change, suddenly a second from address would require spam validation.

The consensus was to update the field description in said, so that its clear the notification email is used for the reply-to header, not the from.

So repurposing this issue to achieve that. It means we can back out all the changes and start afresh with just the #description change.

Thank folks for keeping this one moving.

Crediting @alexpott and @Gábor Hojtsy who I discussed this with

I appreciate all of the time and effort addressing this issue.

I just started a new project that required visitors to get admin approval when creating an account and came across a potential side effect that may cause problems with changing how this email address is used. As the description states, this field serves two purposes. Not only is it used as the "from" address of the account-related emails, which we now know is actually behaving as the "reply-to" address, but it's also the "to" address of the emails that get sent for new user account approval notification emails. Ideally, there would be three separate addresses: the "from" address (may be something like no-reply@servername.com), the "reply-to" address (may be something like info@companyname.com), and the "to" address for new user account approval notification emails (would be name-of-admin@companyname.com).

I agree with @larowlan's sentiment, changing this email address to be the "from" address may create unexpected problems for some, potentially reducing the delivery of account-related emails as they may be flagged as spam. Our "from" addresses are typically no-reply@servername.com to help improve mail delivery and avoid having to go through a verification process for a client's email address (e.g. info@companyname.com). Having (keeping) the field as the "reply-to" address allows us to send emails from a no-reply@servername.com email address, but if a recipient were to reply, their email may go to a valid client email address. More importantly, it allows us to use a valid client email address as the "to" address of the new user account approval notification emails.

This was reverted but the CR remained published. I set the CR to draft.

Filed #3579025: Add an admin email field and deprecate user notification email and update email config.