The current version does not correctly split up "from" headers that already contain an e-mail address, e.g. some name <address@example.com>
The following header will be generated: "some name <address@example.com>" <address@example.com>, which is ugly, confusing and results in a higher likelihood of spam classification.

The attached patch correctly splits up such from headers and also validates the address, which is not done in the current version. It does this through a new method. It also cleans up a lot of the code that was previously hacky:

// e.g.
$reply = preg_replace('/>.*/', '', preg_replace('/.*</', '', $from));

// or
$toparts = explode(' <', $torecipient);
$toname = $toparts[0];
$toaddr = rtrim($toparts[1], '>');

// or 
$replyToParts = explode('<', $value);
$replyToName = trim($replyToParts[0]);
$replyToName = trim($replyToName, '"');
$replyToAddr = rtrim($replyToParts[1], '>');
CommentFileSizeAuthor
#12 Screen shot 2016-01-30 at 10.36.04 PM.png35.97 KBAnonymous (not verified)
#10 smtp-n2596413-10.patch6.07 KBDamienMcKenna
#9 smtp-from_header_is_not-2596413-9.patch6.06 KBivanstegic
#7 smtp-from_header_is_not-2596413-7.patch6.06 KBivanstegic
#3 smtp-from_header_is_not-2596413-3.patch5.54 KBivanstegic
#2 smtp-2596413-2-fix_from_header_and_standardize_name_addr_componentization.patch5.99 KBivanstegic
smtp_fix_from_header.patch1.3 KBsammuell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sammuell created an issue. See original summary.

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Related issues: +#1815720: Notice: Undefined offset: 1 in SmtpMailSystem->mail() (line 125 of ..\sites\all\modules\smtp\smtp.mail.inc).
FileSize
smtp-2596413-2-fix_from_header_and_standardize_name_addr_componentization.patch5.99 KB

I discovered this issue when a client's site stopped sending out emails because of a badly formed from string: there was no space between the name and the email address. This meant that the way angle brackets are handled in this module *requiring* a space broke things. The preg_match here is a better way of doing things, and I went ahead and fixed the side note you mentioned @sammuell at the bottom of #1. Here's a patch that cleans the code up, fixes my issue and probably related issues.

I added a new protected function to the class and perhaps this makes it more readable and consistent:

    // Check if the provided $name_and_address already has the one of the following formats and split them up.
    // - some name <address@example.com>
    // - "another name" <address@example.com>
    // - <address@example.com>
    $comp = array();
    if (preg_match('/^"?([^"\t\n]*)"?\s*<([^>\t\n]*)>$/', $name_and_address, $matches)) {
      $comp['name'] = trim($matches[1]);
      $comp['addr'] = trim($matches[2]);
    }
    return $comp;
  }  //  End of _smtp_get_components().
ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Issue summary: View changes
FileSize
smtp-from_header_is_not-2596413-3.patch5.54 KB

Re-rolled the last patch with some improvements, most notably such that the method returns immediately if the supplied string is already a valid email address. Reduced number of variables being used too.

Status: Needs review » Needs work

The last submitted patch, 3: smtp-from_header_is_not-2596413-3.patch, failed testing.

Status: Needs work » Needs review

sammuell queued 3: smtp-from_header_is_not-2596413-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: smtp-from_header_is_not-2596413-3.patch, failed testing.

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
FileSize
smtp-from_header_is_not-2596413-7.patch6.06 KB

OK, one more time!

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Status: Needs work » Needs review
ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
FileSize
smtp-from_header_is_not-2596413-9.patch6.06 KB

I think this needs to be submitted again to be retested?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
FileSize
smtp-n2596413-10.patch6.07 KB

Rerolled.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Once there are some more tests committed to the module it'll be easier to test to make sure this works as intended.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous as a volunteer and commented
FileSize
Screen shot 2016-01-30 at 10.36.04 PM.png35.97 KB

Worked great for me on this one. Terminal screenshot included.

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Status: Needs review » Reviewed & tested by the community

The code changes here are almost identical to the issue in Drupal 8 which has already been committed #2627432: Clean up name and email address handling

wundo’s picture

wundo CreditAttribution: wundo at Chuva Inc. commented
wundo’s picture

wundo CreditAttribution: wundo at Chuva Inc. commented
wundo’s picture

wundo CreditAttribution: wundo at Chuva Inc. commented

  • wundo committed 7550c73 on 7.x-1.x authored by ivanstegic 
    Issue #2596413 by ivanstegic, sammuell, DamienMcKenna, alex_drupal_dev,...
wundo’s picture

wundo CreditAttribution: wundo at Chuva Inc. commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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