Problem/Motivation

The aim of this issue is to give e-mails from the site administrator a "from" title set to the website name and not just an e-mail address.

In Drupal 5 this was possible by configuring "Site Webmaster" <webmaster@site.com> in Site information, but since 6.x the additional validation prevents such an e-mail address configuration.

You are now limited to just the e-mail address, without a name. Hence site e-mails appear to be from just webmaster@site.com, which is just not as nice :)

Proposed resolution

Specify in the "From:" header the site name along with the site email address.

Remaining tasks

User interface changes

In your email, you'll see the Site name instead of the email address.

Screen capture attached of my email client:
Screen capture of emails before and after

API changes

None.

Original report by dshaw

In Drupal 5.x, I configure the site e-mail address to be something like "Site Webmaster" <webmaster@site.com> (at the site info page: example.com/admin/settings/site-information). The effect of this is that e-mails from the site have a nicely formatted "From" field. That is, they appear to be from "Site Webmaster", rather than webmaster@site.com. Which is a bit nicer and cleaner.

In Drupal 6.x, the additional validation prevents such an e-mail address configuration. You are now limited to just the e-mail address, without a name. Hence site e-mails appear to be from just webmaster@site.com, which is just not as nice :)

Very minor style issue, I know. But is it possible to retain nice addresses, while still validating the e-mail address?

(The field validation could be altered. Or a site name field could be added to site info. Mail could then be sent from "site name" )

CommentFileSizeAuthor
#146 209672-mail-from-146.patch3.88 KBjcisio
#144 209672-mail-from-144.patch2.85 KBjcisio
#143 site_name_in_emails-209672-143.patch621 bytesAndrew Answer
#127 d7-site-name-in-emails-209672-128-testsonly.patch1.47 KBnaxoc
#127 d7-site-name-in-emails-209672-128-tests+fix.patch2.24 KBnaxoc
#123 209672.png16.33 KBpenyaskito
#122 interdiff.119-122.txt2.69 KBpenyaskito
#122 site-name-in-emails-209672-122-testsonly.patch1.39 KBpenyaskito
#122 site-name-in-emails-209672-122-tests+fix.patch2.58 KBpenyaskito
#120 d7-site-name-in-emails-209672-120-testsonly-do-not-test.patch1.47 KBpenyaskito
#120 d7-site-name-in-emails-209672-120-tests+fix-do-not-test.patch2.24 KBpenyaskito
#119 interdiff.117-119.txt686 bytespenyaskito
#119 site-name-in-emails-209672-119-testsonly.patch1.53 KBpenyaskito
#119 site-name-in-emails-209672-119-tests+fix.patch2.32 KBpenyaskito
#117 site-name-in-emails-209672-117-testsonly.patch1.54 KBpenyaskito
#117 site-name-in-emails-209672-117-tests+fix.patch2.34 KBpenyaskito
#114 site-name-in-emails-209672-114-testsonly.patch1.57 KBnaxoc
#114 site-name-in-emails-209672-114-tests+fix.patch2.37 KBnaxoc
#112 site-name-in-emails-209672-112-testsonly.patch1.99 KBpillarsdotnet
#112 site-name-in-emails-209672-112-tests+fix.patch2.91 KBpillarsdotnet
#107 site-name-in-emails-209672-107-test-only.patch1.5 KBnaxoc
#107 site-name-in-emails-209672-107.patch2.3 KBnaxoc
#102 site-name-in-emails-209672-102.patch1.5 KBpillarsdotnet
#101 drupal_mail-209672-101.patch1.2 KBolamaekle
#86 drupal_mail-209672-86.patch1.64 KBpillarsdotnet
#84 drupal_mail-209672-84.patch1.95 KBpillarsdotnet
#83 drupal_mail-209672-83-d7.patch1.62 KBpillarsdotnet
#83 drupal_mail-209672-83-d6.patch1.69 KBpillarsdotnet
#82 drupal_mail-209672-82.patch1.62 KBpillarsdotnet
#79 drupal_mail-209672-79.patch1.57 KBpillarsdotnet
#78 mail_from_site_name-209672.patch819 bytesgdud
#52 nice_from_header.patch1.04 KBcarlos8f
#34 mail_from_site_name-209672-34.patch819 bytescburschka
#28 system_site_email_mailbox_spec_2.patch2.76 KBRobLoach
#25 system_site_email_mailbox_spec-209672-25.patch2.74 KBcburschka
#23 site_mail_validation.patch1.03 KBtheborg
#17 valid_email_2.patch1.17 KBtheborg
#10 valid_email_1.patch1.17 KBtheborg
#9 valid_email.patch1018 bytestheborg
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Component: admin.module » system.module

D6 uses the new function system_site_information_settings_validate, and specifically user_validate_mail to check the site_mail field, which in turns uses common.inc's valid_email function. So, there are three different ways of solving this problem, without adding a new field (and as such having to change even more code for mailing).

1) Modifiy system_site_information_settings_validate in system.admin.inc to execute a regex and split the string based on finding , and only validate the second portion which is the actual e-mail address.

2) Modify user_validate_mail to do the same stripping as above.

3) Modify the core valid_email function (not recommended, as this could have many ramifications).

Even though this was a "feature" in D5, I don't believe that it's a bug just because it's not in D6.

I'm changing the component to point the appropriate place and will leave it as a bug report until discussion can take place on whether this is a bug or a request.

dshaw’s picture

Thanks for the follow-up. I'd lean towards option 2 (or maybe 1) as a good way to go.

Your point about whether this a bug is a fair one. My initial reaction to D6 was, "hey, I can't do what I could in D5", hence I chose bug. But you're right it was never an explicit feature, so this could become a feature request. Of course that would mean it wouldn't be fixed until D7, wouldn't it? :(

Pancho’s picture

Title: Site e-mail address: can't give a name anymore » Site e-mail address: can't give a sender name anymore

This is not just a "nice to have" feature but a serious bug, as emails w/o sender name are often penalized by raising the SPAM index value, AFAIK.
To make this implicit D5-feature explicit, I'd prefer adding a separate name field to the form. We'll have to add or change one or two strings anyway. The fact that adding a string is the lesser crime during string freeze, would be an additional argument for a separate field.
Some response would be great before any work is done on this.

ruharen’s picture

IMO the best we can do is to add another field for sender's name. email is not sender, so no reason to have it on the same field.

theborg’s picture

Shold we follow RFC 2822 and match mailbox definition?:
http://www.faqs.org/rfcs/rfc2822.html

Note that a mailbox is composed of mailbox = name-addr / addr-spec

cburschka’s picture

+1 for using mailbox spec (with optional displayname angle-addr) rather than forcing addr-spec. However, such a change would have to be accompanied by a careful check that no other part of core relies on the addr-spec assumption.

If some code took it on itself to construct "Name" <$email> anywhere, allowing $email to already be a mailbox specification would break it.

theborg’s picture

Agree with Arancaytar,
The document defines an address as:

address         =       mailbox / group
mailbox         =       name-addr / addr-spec
name-addr       =       [display-name] angle-addr
angle-addr      =       [CFWS] "<" addr-spec ">" [CFWS] / obs-angle-addr
display-name    =       phrase

So the sender field can be : phrase "<" addr-spec ">".

I vote for modifing the core function.

ScoutBaker’s picture

+1 for following the spec.

theborg’s picture

Status: Active » Needs review
FileSize
1018 bytes

This patch modifies the core function valid_email_address to adapt it to the spec. The optional new format is:
"display name" <user@domain>, the old format, without the first part, is still valid.

Extensive testing is needed.

theborg’s picture

FileSize
1.17 KB

Same as #9 but using & l t ; and & g t ; instead of angle chars.

cburschka’s picture

Why do you need to use entities in this validation - has the input already been through check_plain?

theborg’s picture

@Arancaytar: Entities are not needed, but I think that for #9 to work we need to modify drupal_send_mail function

cburschka’s picture

Oh, so you actually have to *enter* "Name" &lt;mail@mail&gt;. Well... inconvenient, but I suppose that's better than nothing. Perhaps this could be improved in a later patch.

cburschka’s picture

Status: Needs review » Needs work

Angle addresses break badly.

Address entered: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran&gt;

Mail headers:

To: arancaytar.ilyaran@gmail.com
Subject: Replacement login information for Arancaytar at Drupal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes
Content-Transfer-Encoding: 8Bit
X-Mailer: Drupal
Errors-To: "Arancaytar Ilyaran"@millhouse.dreamhost.com,
	&lt@millhouse.dreamhost.com;, arancaytar.ilyaran@gmail.com&gt;
Sender: "Arancaytar Ilyaran"@millhouse.dreamhost.com,
	&lt@millhouse.dreamhost.com;, arancaytar.ilyaran@gmail.com&gt;
Reply-To: "Arancaytar Ilyaran"@millhouse.dreamhost.com,
	&lt@millhouse.dreamhost.com;, arancaytar.ilyaran@gmail.com&gt;
From: "Arancaytar Ilyaran"@millhouse.dreamhost.com,
	&lt@millhouse.dreamhost.com;, arancaytar.ilyaran@gmail.com&gt;
Message-Id: <20080122212432.4950ACAE1@millhouse.dreamhost.com>
Date: Tue, 22 Jan 2008 13:24:32 -0800 (PST)

I'll try to get debug output too, in a bit.

cburschka’s picture

This is the exact content that gets passed to the PHP mail() function as its fourth (mime-headers) parameter:

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes
Content-Transfer-Encoding: 8Bit
X-Mailer: Drupal
Errors-To: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran@gmail.com&gt;
Return-Path: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran@gmail.com&gt;

Sender: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran@gmail.com&gt;
Reply-To: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran@gmail.com&gt;
From: "Arancaytar Ilyaran" &lt;arancaytar.ilyaran@gmail.com&gt;

Somewhere along the line, it ends up as above. My guess is that the entities aren't working out...

cburschka’s picture

Status: Needs work » Needs review

I have applied patch #9 instead of #10. Works.

Site mail entered: "Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com>

Mail headers:

To: arancaytar.ilyaran@gmail.com
Subject: Replacement login information for Arancaytar at Drupal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed; delsp=yes
Content-Transfer-Encoding: 8Bit
X-Mailer: Drupal
Errors-To: "Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com>
Sender: "Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com>
Reply-To: "Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com>
From: "Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com>
Message-Id: <20080122215206.6273ACAE1@millhouse.dreamhost.com>
Date: Tue, 22 Jan 2008 13:52:06 -0800 (PST)

#9 (non-escaped angle characters) works fine for my test, without any other modification. #10 does not.

Unless there is some other issue, I guess #9 is the one we need to use.

I am shying away from RTBCing though, because this has to be tested on another PHP setup, and with other components than the site address. For further testing, these are the references to valid_email_address() and drupal_mail() and drupal_mail_send() in core.

$ grep "valid_email_address" -R *
includes/common.inc:function valid_email_address($mail) {
modules/comment/comment.module:        if (!valid_email_address($edit['mail'])) {
modules/contact/contact.admin.inc:      if (!valid_email_address(trim($recipient))) {
modules/contact/contact.pages.inc:  if (!valid_email_address($form_state['values']['mail'])) {
modules/contact/contact.pages.inc:  if (!valid_email_address($user->mail)) {
modules/system/system.module:  if (!valid_email_address($form_values['recipient']) && $form_values['recipient'] != '%author') {
modules/update/update.settings.inc:        if (valid_email_address($email)) {
modules/user/user.module:  if (!valid_email_address($mail)) {
$ grep "drupal_mail(" -R *
includes/mail.inc: *       // example_mail() will be called based on the first drupal_mail() parameter.
includes/mail.inc: *       drupal_mail('example', 'notify', $account->mail, user_preferred_language($account), $params);
includes/mail.inc:function drupal_mail($module, $key, $to, $language, $params = array(), $from = NULL, $send = TRUE) {
includes/mail.inc: * PHP function reference for mail()</a>. See drupal_mail() for information on
includes/mail.inc: * We deliberately use LF rather than CRLF, see drupal_mail().
includes/mail.inc: * (RFC 3676) and can be passed directly to drupal_mail() for sending.
includes/mail.inc: * We deliberately use LF rather than CRLF, see drupal_mail().
modules/contact/contact.pages.inc:  drupal_mail('contact', 'page_mail', $contact['recipients'], language_default(), $values, $from);
modules/contact/contact.pages.inc:    drupal_mail('contact', 'page_copy', $from, $language, $values, $from);
modules/contact/contact.pages.inc:    drupal_mail('contact', 'page_autoreply', $from, $language, $values, $contact['recipients']);
modules/contact/contact.pages.inc:  drupal_mail('contact', 'user_mail', $to, user_preferred_language($account), $values, $from);
modules/contact/contact.pages.inc:    drupal_mail('contact', 'user_copy', $from, $language, $values, $from);
modules/system/system.module:  if (drupal_mail('system', 'action_send_email', $recipient, $language, $params)) {
modules/update/update.fetch.inc:        drupal_mail('update', 'status_notify', $target, $target_language, $params);
modules/update/update.module: * @see drupal_mail()
modules/user/user.module: * @see drupal_mail()
modules/user/user.module:    $mail = drupal_mail('user', $op, $account->mail, $language, $params);
modules/user/user.module:      drupal_mail('user', 'register_pending_approval_admin', variable_get('site_mail', ini_get('sendmail_from')), language_default(), $params);
$ grep "drupal_mail_send(" -R *
includes/mail.inc: * Finally drupal_mail_send() sends the e-mail, which can be reused
includes/mail.inc: *   Send the message directly, without calling drupal_mail_send() manually.
includes/mail.inc:    $message['result'] = drupal_mail_send($message);
includes/mail.inc:function drupal_mail_send($message) {
modules/user/user.module: *  The return value from drupal_mail_send(), if ends up being called.
theborg’s picture

Assigned: Unassigned » theborg
FileSize
1.17 KB

@Arancaytar: Thanks!

I've tried to break it on all forms that a valid email is involved but I couldn't and the regex added is optional.
The angle address works ok for me, so I'm attaching #9 again with a comment about the two valid formats.

cburschka’s picture

On reading that regex again...

Won't the following two be "valid" email addresses?

"Arancaytar Ilyaran" <arancaytar.ilyaran@gmail.com
arancaytar.ilyaran@gmail.com>

I may have a bit of a gap in my understanding of substring expressions, however. Some of those question marks look odd...

theborg’s picture

No, this addresses are not valid, the trick here is the code:
$angle = '(?(1)>)';
Is a conditional regex that *only* is validated when the (1) regex has been tested and is TRUE.

The (1) regex is the display name part including the white space and the first angle:
$name = '((?:"[^\"\f\n\r\t\v]+" <))?';
Here, we specifically look for quotes surrounding a phrase (one or more words, no control characters), but this is all optional so it can be FALSE, when not valid or nothing. Then $angle only is active whe this is TRUE.

cburschka’s picture

Thanks for teaching me something new about regex! I think I remember dozens of places in my own code where I can clean up a regular expression or cut down from two to a single one using this particular trick.

In any case, I haven't found any errors on applying this patch. Perhaps another reviewer should test it, but as far as I'm concerned, it's ready.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks like this is a feature addition (in email validation) and also an API break since those expecting that their validation is done on an email *address* will get names sneaking in where they don't want it (like on the user account email field).

cburschka’s picture

However, the "feature" it adds was in D5 and removed by regression. As the site email was allowed to contain a name in D5, any expectation of site email being a pure address would have been broken in D5, and will be broken in D6 for upgraded sites that retain their newly invalidated site email field. So at least this field can contain a name without adding any additional API problems. I'll readily admit that the other references to valid_email_address assume absence of names.

Can we at least make the mailbox spec optional and default it to false?

 function valid_email_address($mail, $allow_name = FALSE) {

...

 }

function system_site_information_settings_validate() {

...
  $validate_email = valid_email_address($site_mail, TRUE);

}
theborg’s picture

Status: Needs work » Needs review
FileSize
1.03 KB

Ok, so this should be divided:
a) A feature request to include the validation on the core function: http://drupal.org/node/214114.
b) A patch to modify 'system_site_information_settings' function because this is the part affected by the issue.

This patch looks if there is a display name inside the mail parameter and extracts the address part to pass it to validation.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Hm, $allow_name sounds like reasonable for now.

cburschka’s picture

$allow_name implemented.

As site-info uses user_validate_email for generating the appropriate form errors, user_validate_email takes the $allow_name = FALSE argument and passes it on to valid_email_address.

NOTE - this patch is racing against another one that fixes the "trailing period in hostname" bug. Both patches change the same lines of common.inc, and the losing patch will require a (mostly trivial) conflict-resolving re-roll.

cburschka’s picture

Status: Needs work » Needs review

Status update.

dshaw’s picture

I've tested this patch and it appears to work. I can now enter e-mail addresses with a sender name, a-la Drupal 5.x.
I also tried several almost-correct addresses with sender names and they were all flagged as errors. So I think what's there is good to go.

I think the use of extra variables in the functions and the defaults chosen should prevent any breaks to existing code.

My only concern is whether there are more places where support for a sender name could be enabled. I don't feel I can comment on the entire code base. Perhaps someone else ...

RobLoach’s picture

Title: Site e-mail address: can't give a sender name anymore » Regression: Site e-mail address can't state the sender's name
Status: Needs review » Reviewed & tested by the community
FileSize
2.76 KB

I did a file search for "valid_email_address", and I believe there are a couple places where it is used:

  1. User account email address: acts accordingly, doesn't allow email address name
  2. User comment form: acts accordingly, doesn't allow email address name
  3. Personal contact form: acts accordingly, doesn't allow email address name
  4. Contact form admin recipients: doesn't allow email address name
  5. Site Information form: acts accordingly, allows email address name

I tested it in all these locations and they seem to be working nicely. You could use the sender's name in Drupal 5, so renaming this issue to a regression issue.

This attached patch adds code documentation for the new argument passed to the valid_email_address function.

cburschka’s picture

Possible follow-up issue: The installer "recommends" using the same address for site contact and the root user. Javascript code copies the form value from one email field to the other automatically.

With this patch, the site contact address will allow values that a user address does not. It is usually a reasonable expectation that the suggested defaults actually pass validation.

However, that shouldn't be a show-stopper, because it's pretty clear to the user what is happening, and if they know what they're doing when they add a name to the first field, they should also know that the second field may not have a name.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev

While I understand that this is a regression, I don't think that API change is a good idea now. Please revisit the issue in Drupal 7. For Drupal 6, the form and the mail is equally alterable, so there are multiple places to solve this. This regression appeared at all, because there was no email validation in Drupal 5 there.

cburschka’s picture

Hm. Okay. Of course, if we had known this two weeks ago, we could have spent more time on critical issues instead of implementing the $allow_name argument to get this into 6.x... ;)

RobLoach’s picture

Status: Reviewed & tested by the community » Needs work

So, for DRUPAL-6, we remove the validation on the site information settings page, and for HEAD, we add $allow_name to valid_email_address. Does that sound reasonable?

Gábor Hojtsy’s picture

In D6, I'd rather use the site name as name and site email as email in the outgoing mail, if you insist on having a name by default. Checking the format of an email input field is IMHO an improvement not a bad thing to have.

cburschka’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review
FileSize
819 bytes

Like this?

function drupal_mail($module, $key, $to, $language, $params = array(), $from = NULL, $send = TRUE) {
  $default_from = variable_get('site_mail', ini_get('sendmail_from'));
  // The ini variable may not conform to the addr-spec format site_mail has. A check is needed.
  if (is_valid_email($default_from)) {
    $default_from = '"'. variable_get('site_name', 'Drupal') .'" <'. $default_from .'>';
  }

This patch is for 6.x.

cburschka’s picture

Note: I don't know what characters the site name allows, and if it potentially needs to be check_plained or otherwise filtered before conforming to RFC 2822.

babbage’s picture

Subscribing.

JamieR’s picture

Anyone know what happened here? Thanks!

Anonymous’s picture

#34 patch works great, thanks. This should definitely be included in core IMHO.

mfb’s picture

Subscribing.

GregoryHeller’s picture

So a i reading this right? you need to patch core in order to get the site email address to show up prefaced with an actual name like

"My Site"

cburschka’s picture

Version: 6.x-dev » 7.x-dev

This issue is one major release old and needs to be moved up.

Incidentally, on giving this some more thought I am wondering if it might not make more sense to drop support for this completely. Instead of allowing the email address to contain a specific name, just format outgoing From headers as "$site_name" &lt;$site_mail&gt;

Anonymous’s picture

I disagree it should be dropped. That method might work but it's hardly user friendly!

Dries’s picture

I'm not 100% either.

kprmca’s picture

patch (#9) is working fine... i mean email field is accespting both name & mail address.... but while sending a mail from that mail address its giving problem like this

warning: mail() [function.mail]: SMTP server response: 501 5.5.4 Invalid Address in C:\wamp\www\drupal-6.11\includes\mail.inc on line 193.
Unable to send e-mail. Please contact the site administrator if the problem persists.

kprmca’s picture

patch (#9) is working fine... i mean email field is accespting both name & mail address.... but while sending a mail from that mail address its giving problem like this

warning: mail() [function.mail]: SMTP server response: 501 5.5.4 Invalid Address in C:\wamp\www\drupal-6.11\includes\mail.inc on line 193.
Unable to send e-mail. Please contact the site administrator if the problem persists.

Damien Tournoud’s picture

I agree with Arancaytar on using the site name as the human readable part of the From: header.

jcruz’s picture

@GregoryHeller You could also create a custom module and do a hook_mail_alter

function custom_module_mail_alter(&$message){
  $default_from = variable_get('site_mail', ini_get('sendmail_from'));
  
  if($message['from'] == $default_from){
    $message['from'] = '"'. variable_get('site_name', 'Drupal') .'" <'. $default_from .'>';
    $message['headers']['From'] = $message['headers']['Sender'] = $message['headers']['Return-Path'] = $message['headers']['Errors-To'] = $message['headers']['Reply-To'] = $message['from'];
  }
}
perceptum’s picture

Version: 7.x-dev » 6.12
Status: Needs review » Active

Hi guys

Personally I think this is quite important for professional sites. People just tend to overlook mails that are not formatted nicely.

I tried jcruz's module-hack and it works like the bizz! Would be easy to extend this further into core...

Bryan

VM’s picture

Version: 6.12 » 7.x-dev

This needs to stay at 7.x and back ported from there.

Matt V.’s picture

Another workaround, which I just tested, is to edit the variables table entry directly. I used the "Variable editor" link in the Devel module block, but presumably you could use PHPMyAdmin or another database client tool.

mrfelton’s picture

The custom module in #47 is a good approach and works well for me. I do feel that this feature should be in core though.

carlos8f’s picture

Title: Regression: Site e-mail address can't state the sender's name » Use site name in From: header for system e-mails
Component: system.module » base system
Assigned: theborg » Unassigned
Category: bug » task
Status: Active » Needs review
FileSize
1.04 KB

Mail 'from the system' ($from argument is omitted in drupal_mail) should use "Site name" <site mail> as its From: header. Any disagreements? Please review.

carlos8f’s picture

I meant "Site name" <site mail> in #52 :)

carlos8f’s picture

Issue tags: +Usability

Tagging with usability, since I think seeing the name of the website in your inbox is less confusing than just seeing whatever the site_mail is set to.

Dave Reid’s picture

Component: base system » mail system
Gabriel R.’s picture

Boing.

Re-test of valid_email_1.patch from comment #10 was requested by lot007.

benone’s picture

So how it looks now ? Is there any module for D6 which can give a simple site name to the email sender name ?
I use 6.15 and still have only option for email address. No name.
Subscribing

JaredAM’s picture

+1

coyotemojo’s picture

Hello,

I tried the patches in

Hello,

I tried both patches #34 and #52 but I could not get the site emails to come "from" the site name.

Do I need to do anything else besides applying those patches?

Jason

pazzypunk’s picture

#52: nice_from_header.patch queued for re-testing.

perke’s picture

same as #60, wonder if the method that includes module and not hacking core would work better

c960657’s picture

Status: Needs review » Needs work

If site_name contains 8bit characters, the From header line becomes From: =?UTF-8?B?InTDqXN0IiA8Zm9vQGV4YW1wbGUuY29tPg==?=. My mail server changes this to From: =?UTF-8?B?InTDqXN0IiA8Zm9vQGV4YW1wbGUuY29tPg==?=@mail.example.org, and my mail client displays it as “tést @mail.example.org”.

babbage’s picture

Version: 7.x-dev » 8.x-dev

Too late for this to get into D7 now, moving to D8...

shyam541’s picture

Subcribing

GuyPaddock’s picture

We'd better move it to Drupal 9, so this patch makes it out in time for Duke Nukem Forever.

Come on, guys, this can't be THAT hard to include in core already.

GuyPaddock’s picture

For now, it looks like the Personalized E-mails module will address this for D6.

Daniel Norton’s picture

Not convinced that this can’t easily be addressed in D7 (and D6), I added this issue and a corresponding patch:

  • #834202: Provide for RFC 2822 display-name in email sent from default site email address
  • lukebrooker’s picture

    My solution to this problem is I was already using the Mime Module and by changing the site email in it's settings page to something like "Site Name" it bypasses the normal validation and also updates the email in the site settings page. I don't even have to have "send html emails" turned on in mime mail's settings, as long as the module is installed.

    Cyberwolf’s picture

    Subscribing.

    glitz’s picture

    Mime Module did not work for me. Any other solutions?

    glitz’s picture

    an anyone else confirm that http://drupal.org/project/pmail will take care of this without an issue?

    TheodorosPloumis’s picture

    Subscribe

    glitz’s picture

    Verified PMail will take care of this

    g000fy’s picture

    I just tested PMail. It works. Didn't even have to configure the module!

    frankfarm’s picture

    I confirm that in Drupal 6.20 installing
    http://ftp.drupal.org/files/projects/pmail-6.x-1.x-dev.tar.gz
    results in the site name followed by the email address you specify in
    /admin/settings/site-information
    and the email address appears in angle brackets. I also think a better solution is desired for the future.

    My concern as Pancho mentioned in #3 is that the email address alone appears spammy. Without Pmail, when importing users, our automated email invitations ended up in junk email and spam folders, so this was problematic for us until Pmail resolved it.

    I also agree with carlos8f that this is to some extent a usability issue since a site name is more likely to be recognized than the email address specified in
    /admin/settings/site-information

    The dev release of the Personalized E-mail module seems to solve the problem nicely for D6, but it is not clear to me if this might also work as a workaround for D7.

    Anonymous’s picture

    I'm confused how such a seemingly simple D6 issue can remain open for over 3 years, and in the meantime get reassigned to 2 versions after the original?! Providing the site name in the From header is basic core functionality. It shouldn't require the addition of a contrib module to fix.

    gdud’s picture

    I attached version of patch #34 that works wit non-ASCII characters in site_name. This patch is for Drupal 6.x.

    pillarsdotnet’s picture

    Category: task » feature
    Status: Needs work » Needs review
    Issue tags: +Needs backport to D6, +Regression, +Needs backport to D7
    FileSize
    1.57 KB

    Re-rolled against 8.x using current patch standards.

    pillarsdotnet’s picture

    Category: feature » bug

    Bah. regression should be "bug report" not "feature request".

    Status: Needs review » Needs work

    The last submitted patch, drupal_mail-209672-79.patch, failed testing.

    pillarsdotnet’s picture

    Status: Needs work » Needs review
    FileSize
    1.62 KB

    Oops.

    pillarsdotnet’s picture

    d8 patch still applies.

    Rolled d7 and d6 versions.

    Note that this fixes a regression of functionality available in d5 (!!!), hence the backports.

    pillarsdotnet’s picture

    FileSize
    1.95 KB

    Slight improvement over #82:

    • Check that $default_from is both non-empty and valid, because an empty string is also a valid email address.
    • Avoid setting the From: header twice.

    Status: Needs review » Needs work

    The last submitted patch, drupal_mail-209672-84.patch, failed testing.

    pillarsdotnet’s picture

    FileSize
    1.64 KB

    silly me. Corrected.

    pillarsdotnet’s picture

    Status: Needs work » Needs review
    pillarsdotnet’s picture

    #86: drupal_mail-209672-86.patch queued for re-testing.

    pillarsdotnet’s picture

    Status: Needs review » Reviewed & tested by the community
    pillarsdotnet’s picture

    Status: Reviewed & tested by the community » Needs review
    pillarsdotnet’s picture

    pillarsdotnet’s picture

    drupal_was_my_past’s picture

    #86 still applies cleanly to 8.x. I think it just needs to be reviewed or is there another reason this issue has stagnated?

    pillarsdotnet’s picture

    Yup; it needs review. I'd do it myself, but the person who authored or last modified a patch should not review their own work.

    If you feel up to it, read the (updated) Patch review guidelines and feel free to submit your own review. It's a great learning experience.

    Lars Toomre’s picture

    #86: drupal_mail-209672-86.patch queued for re-testing.

    trzczy0’s picture

    Version: 8.x-dev » 7.8
    Assigned: Unassigned » trzczy0
    Category: bug » task

    This is for 7.8. It works like a charm.

    root@voyage:~# diff -u -p modules/contact/contact.admin.inc{.bak,}
    --- modules/contact/contact.admin.inc.bak    2011-10-14 12:00:25.000000000 +0200
    +++ modules/contact/contact.admin.inc        2011-10-14 12:31:15.000000000 +0200
    @@ -136,7 +136,8 @@ function contact_category_edit_form_vali
     
       foreach ($recipients as &$recipient) {
         $recipient = trim($recipient);
    -    if (!valid_email_address($recipient)) {
    +               preg_match('/^"[^\"\f\n\r\t\v]+" <(.+)>$/', $recipient, $matches);     
    +  if (user_validate_mail($matches ? $matches[1] : $recipient)) {
           form_set_error('recipients', t('%recipient is an invalid e-mail address.', array('%recipient' => $recipient)));
         }
       }
    root@voyage:~# 
    pillarsdotnet’s picture

    Version: 7.8 » 8.x-dev
    Assigned: trzczy0 » Unassigned

    Please read and understand the backport policy before changing the version, or assigning yourself to an issue. Thank you.

    This issue has been stalled for five months while awaiting a review of the patch in #86.

    If nobody is willing to provide such a review (and RTBC), this issue should be closed (won't fix).

    pillarsdotnet’s picture

    Status: Needs review » Closed (won't fix)

    Closing for lack of interest. Please re-open only if you are willing to provide a patch review.

    inkage’s picture

    Just go for mimemail projet http://drupal.org/project/mimemail

    bismigalis’s picture

    #34 is how people expect this should work

    olamaekle’s picture

    Status: Closed (won't fix) » Needs work
    FileSize
    1.2 KB

    Updated #86 against the current 8.x-dev and tested it. It just gives you an empty "From" field in the mail.

    Heres the new patch from #86.

    pillarsdotnet’s picture

    Status: Needs work » Needs review
    FileSize
    1.5 KB

    This won't generate an empty From: header.

    olamaekle’s picture

    I get an error when I try to apply the patch:

    error: while searching for:
        'Content-Transfer-Encoding' => '8Bit',
        'X-Mailer'                  => 'Drupal'
      );
      if ($default_from) {
        // To prevent e-mail from looking like spam, the addresses in the Sender and
        // Return-Path headers should have a domain authorized to use the originating
        // SMTP server.
        $headers['From'] = $headers['Sender'] = $headers['Return-Path'] = $default_from;
      }
      if ($from) {
        $headers['From'] = $from;
    
    error: patch failed: core/includes/mail.inc:141
    error: core/includes/mail.inc: patch does not apply
    

    I tested the code and the site name is showing but not the site email. It did that with #101 too but Gmail does not display it when the email is missing (Mail.app does).

    This is from the email source:
    From: "Drupal" <>

    I don´t know programing that well but I´ll try my best to help with testing.

    pillarsdotnet’s picture

    This is from the email source:

    From: "Drupal" <>
    

    That's a valid email address according to RFC 2822.

    olamaekle’s picture

    Strange that Gmail does not recognize the "From:" field then. But how do I add the email address between the < and > signs?

    So that i get something like this:
    From: "Drupal" <drupal@example.com>

    OK... forget that seems to be working now. Going to reinstall and retest now.

    olamaekle’s picture

    Right after I install drupal 8 with #102 patch I get this in the email:
    From: "Drupal" <>

    When i visit admin/config I get an error:

    One or more problems were detected with your Drupal installation. Check the status report for more information.

    After that it works just fine:
    From: "Drupal" <drupal@example.com>

    Any idea?

    naxoc’s picture

    Here is a test and a re-roll of the patch. mime_header_encode() is called on all header fields in mail() so I removed that function call. Made the patch smaller and simpler.

    ryan.gibson’s picture

    Assigned: Unassigned » ryan.gibson

    ya lo tengo

    ryan.gibson’s picture

    Issue summary: View changes

    Updated issue summary to match template standards.

    ryan.gibson’s picture

    Issue summary: View changes

    Updated issue summary.

    ryan.gibson’s picture

    Status: Needs review » Reviewed & tested by the community

    Alright, I tested the patch from #107; it applied cleanly. Before applying the patch, emails appeared as from the site administrator's email account only. After applying the patch, the e-mail did have a "from" and displayed the site name. If this is what we're wanting then the patch works great.

    ryan.gibson’s picture

    Assigned: ryan.gibson » Unassigned
    xjm’s picture

    Itsy-bitsy correction:

    +++ b/core/modules/system/tests/mail.testundefined
    @@ -66,6 +66,29 @@ class MailTestCase extends DrupalWebTestCase implements MailInterface {
    +   * Tests that the From: header contains the site name if a from address is
    +   * not specified.
    

    This should be one line of 80 characters or fewer. Reference: http://drupal.org/node/1354#functions

    Patch looks great to me other than that, and the test coverage is correctly exposed in #107.

    pillarsdotnet’s picture

    Adjusted according to #111 and re-rolled with "git format-patch". No code changes, hence no change in status.

    --- b/core/modules/system/tests/mail.test
    +++ b/core/modules/system/tests/mail.test
    @@ -67,8 +67,7 @@
       }
     
       /**
    -   * Tests that the From: header contains the site name if a from address is
    -   * not specified.
    +   * Checks for the site name in an auto-generated From: header.
        */
       function testFromHeader() {
         global $language;
    
    sun’s picture

    Status: Reviewed & tested by the community » Needs work
    +++ b/core/modules/system/tests/mail.test
    @@ -67,6 +67,28 @@ class MailTestCase extends WebTestBase implements MailInterface {
    +  }
    +  /**
    

    Missing blank line.

    naxoc’s picture

    Reroll and fixed the missing blank line.

    penyaskito’s picture

    Status: Needs review » Needs work
    Issue tags: +Needs backport to D6, +Usability, +Regression, +Needs backport to D7

    The last submitted patch, site-name-in-emails-209672-114-tests+fix.patch, failed testing.

    penyaskito’s picture

    Category: task » feature
    Status: Needs work » Needs review
    FileSize
    2.34 KB
    1.54 KB

    This should be a feature request, not a task, and it should be committed before the Feature Freeze deadline.

    I tested the patch above and worked fine. Just rerolled it because the test moved from Common to Mail.

    penyaskito’s picture

    Status: Needs review » Needs work

    This failed, but testbot didnt gave feedback.

    penyaskito’s picture

    Site mail variable is already converted to CMI. New try.

    penyaskito’s picture

    penyaskito’s picture

    Status: Needs review » Needs work

    Needs work. The test is not well designed, it should have failed. This needs CMI in site.name too.

    penyaskito’s picture

    New attempt, now with site.name converted to CMI too. Edited the test so it doesn't use variable_get, but asserts for the settings at the WebTestBase class setUp.

    penyaskito’s picture

    Issue summary: View changes

    Updated issue summary.

    penyaskito’s picture

    FileSize
    16.33 KB

    Updated summary.

    penyaskito’s picture

    Issue summary: View changes

    Updated summary

    penyaskito’s picture

    Issue summary: View changes

    img

    e0ipso’s picture

    Status: Needs review » Reviewed & tested by the community

    Works for me.

    Marking as RTBC.

    catch’s picture

    Version: 8.x-dev » 7.x-dev
    Category: feature » task
    Status: Reviewed & tested by the community » Patch (to be ported)

    Committed/pushed to 8.x, thanks! Moving to 7.x for backport. This isn't really a new feature either, although not sure what David will think about backport.

    geerlingguy’s picture

    Latest D7 patches are in #120. (FYI)

    naxoc’s picture

    Status: Patch (to be ported) » Needs review
    FileSize
    2.24 KB
    1.47 KB

    Here are the patch(es) from #120 for the testbot.

    penyaskito’s picture

    Status: Needs review » Reviewed & tested by the community

    Works for me and tests passed. Let's see what David thinks about it.

    David_Rothstein’s picture

    Status: Reviewed & tested by the community » Needs review
    -  if ($from) {
    +  if ($from && $from != $default_from) {
         $headers['From'] = $from;
       }
    

    Well, at a minimum I think the above could use more discussion. It's documented that if you explicitly pass in $from to drupal_mail(), the "From" header will get set to that, and I'm not sure there's any good reason to create exceptions to that.

    As for the overall issue... hm. This change is probably what most sites would want. But we'd basically be changing it for every Drupal 7 site (at least ones that don't specifically override it) in the middle of a stable release, and I think some sites might be quite happy with just the email address being used here. (It might be coming from a specific person, for example, and their personal e-mail address alone could make more sense than having it wrapped in the site name.)

    Given that it should be very very easy to write a contrib module that adds this behavior (there are already code snippets above) wouldn't it be better to just leave it to contrib in Drupal 7?

    Anonymous’s picture

    The last thing that is required is more discussion. In a few days time, this issue will have been open for 5 years! That's ridiculous...

    xjm’s picture

    @martinjbaker, such remarks are not helpful and are discouraging to the contributors who've donated their time to work on this issue.

    We have to be careful what changes we backport to Drupal 7, because it is very important not to unexpectedly alter production sites in a stable release. See what @David_Rothstein said above:

    we'd basically be changing it for every Drupal 7 site (at least ones that don't specifically override it) in the middle of a stable release, and I think some sites might be quite happy with just the email address being used here.

    So, if you want to see this patch get in, update the patch in #127 to remove the change @David_Rothstein pointed out in #129 and leave the other change.

    xjm’s picture

    Issue summary: View changes

    li's

    ogomez78’s picture

    Status: Needs review » Needs work

    The last submitted patch, 127: d7-site-name-in-emails-209672-128-testsonly.patch, failed testing.

    vasi1186’s picture

    Maybe we should enclose the name of the site in double quotes, so we would have

    $headers['From'] = '"' . variable_get('site_name', 'Drupal') . '" <' . $default_from . '>';
    

    instead of

    $headers['From'] = variable_get('site_name', 'Drupal') . ' <' . $default_from . '>';
    

    I had an issue with the From header when the site name contained characters like '>'. For example, like this: >>site_name>>

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 127: d7-site-name-in-emails-209672-128-testsonly.patch, failed testing.

    • catch committed 33e284e on 8.3.x
      Issue #209672 by pillarsdotnet, theborg, penyaskito, cburschka, naxoc,...

    • catch committed 33e284e on 8.3.x
      Issue #209672 by pillarsdotnet, theborg, penyaskito, cburschka, naxoc,...

    • catch committed 33e284e on 8.4.x
      Issue #209672 by pillarsdotnet, theborg, penyaskito, cburschka, naxoc,...

    • catch committed 33e284e on 8.4.x
      Issue #209672 by pillarsdotnet, theborg, penyaskito, cburschka, naxoc,...
    jochenf’s picture

    I'm using Drupal 8.4 which already uses sitename in mail from:
    When sitename contains UTF-8 characters (german umlauts), the whole from header gets encoded (base64?). According to RFC 2047/RFC 822 the mail address must be parsable and is not allowed to be encoded.

    Instead of encoding the whole line like this:

    From: =?UTF-8?B?QmFyZnXDnyBpbSBBbGxnw6R1IDxhZG1pbkBiYXJmdXNzLWFsbGdhZXUuZGU+?=

    It should be
    From: =?UTF-8?B?QmFyZnXDnyBpbSBBbGxnw6R1?=

    Andrew Answer’s picture

    I found this issue because of now (in 2019) I see what in D8.7 no sender name (seriously?) How I can implement this?

    Andrew Answer’s picture

    This patch allow me to use site name as sender in From field.

    jcisio’s picture

    Version: 7.x-dev » 8.8.x-dev
    Status: Needs work » Needs review
    FileSize
    2.85 KB

    It seems that the fix missed one place. I don't know the difference between $message['from'] and $headers['From'], but here is a patch which should fix the test in #143.

    Status: Needs review » Needs work

    The last submitted patch, 144: 209672-mail-from-144.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    jcisio’s picture

    Status: Needs work » Needs review
    FileSize
    3.88 KB

    Ok, now I fixed and tested locally.

    The last submitted patch, 144: 209672-mail-from-144.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Version: 8.8.x-dev » 8.9.x-dev

    Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    sokru’s picture

    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Needs backport to D6

    Manually tested patch from #146, with multilingual site (different site name per language). No problems, so RTBC.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs followup

    This issue was about the backport of the fix to Drupal 7. The omission from the original patch discovered by @Andrew Answer needs a new issue. We shouldn't be piling on to this one years after it has been committed to Drupal 8. Also in the meantime Drupal 7 backport policy has changed to requiring a new issue to opened. So can someone:

    • Open an issue for the backport and post the latest D7 patch there - I think that is #127
    • Open an issue for the bug discovered in #143 - @Andrew Answer thanks for reporting this - it's just we need to report it in the right place.
    alexpott’s picture

    alexpott’s picture

    Also fwiw I think we need to understand more about the problems @Andrew Answer is experiencing - because with the default Drupal 8 core mailer the from address on the email will come from the headers.

    sokru’s picture

    alexpott’s picture

    Status: Needs work » Fixed

    I created the D7 issue - #3098058: [D7] Use site name in From: header for system e-mails - so now we can close this one as fixed.

    alexpott’s picture

    Issue tags: -Needs followup

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.