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

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 previous 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 as 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 ✨ Allow for custom user registration approval email address Fixed

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.

larowlan → credited alexpott → .

larowlan → credited Gábor Hojtsy → .

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

I appreciate all of the time and effort addressing the fact that the description of the field does not currently match the current functionality.

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 (would be something like no-reply@companyname.com), the "reply-to" address (would 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 there 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.