// This may not work. The MTA may rewrite the Return-Path.
      if (!isset($headers['Return-Path']) || $headers['Return-Path'] == $default_from) {
        if (preg_match('/[a-z\d\-\.\+_]+@(?:[a-z\d\-]+\.)+[a-z\d]{2,4}/i', $from, $matches)) {
          $headers['Return-Path'] = "<$matches[0]>";
        }
      }

What's this regular expression supposed to do? If it's just to extract the user@domain part of the $from string, then we should use our 'helper' function mimeMailAddress() which already has a full test suite to prove it works, rather than relying on an untested regular expression.

      // This may not work. The MTA may rewrite the Return-Path.
      if (!isset($headers['Return-Path']) || $headers['Return-Path'] == $default_from) {
        $headers['Return-Path'] = '<' . static::mimeMailAddress($from, TRUE) . '>';
      }

Another advantage is that mimeMailAddress() is documented so we know what it's supposed to do, unlike the regular expression.

CommentFileSizeAuthor
#10 3156239-10-replace-regular-expression.patch683 bytestr
#2 3156239-2-replace-regular-expression.patch1.58 KBtr

Comments

TR created an issue. See original summary.

tr’s picture

tr commented
Status: Active » Needs review
Related issues: +#2463583: Sender and Return-Path headers are rewritten when they are equal to $default_from
StatusFileSize
new 3156239-2-replace-regular-expression.patch1.58 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3156239-2-replace-regular-expression.patch, failed testing. View results

tr’s picture

tr commented
Status: Needs work » Needs review
imclean’s picture

imclean commented 
+++ b/src/Utility/MimeMailFormatHelper.php
@@ -730,9 +818,7 @@ class MimeMailFormatHelper {
       // This may not work. The MTA may rewrite the Return-Path.

This is sort of correct. A good MTA will just remove the "Return-Path" header. The final MDA will then add it based on the envelope sender.

Apologies, I won't keep spamming the issue queue after this. As it is an RFC violation, is there any interest in simply removing it altogether? Would it actually break anything (which isn't already broken itself)?

tr’s picture

tr commented

@imclean: Yes, I've read your comments in the other issues and I agree - Return-Path: should probably be removed entirely. That's why I added #3089331: Return-Path is set blank in MimeMailFormatHelper::mimeMailHeaders as a related issue - we can handle the removal in that issue.

I'm also not sure we should be messing with From: and Sender: here - and I'm not sure why we need to deal with From: being an array (other than that's one of the weird structures Mime Mail uses internally - storing email addresses as an associative array with keys 'name' and 'mail'). I don't know why we would be passing that array form around in the From: header, and whether removing that would break things...

The only part I'm sure we need is the loop over all headers and calling Unicode::mimeHeaderEncode() on each. But I don't know why mimeHeaderEncode() breaks lines with \n because that's also against RFC - all headers MUST be terminated with \r\n specification.

There's just so much nasty legacy code involved here that the only way I can deal with it is to break it into small chunks and work on it piece by piece. The legacy code is in Mime Mail, it's in Drupal core (like Unicode::mimeHeaderEncode()) it's in PhpMail, it's in the PHP mail() function, it's platform-dependent because of PHP, it deliberately violates the RFC specifications in so many places. And hacks that were put into Mime Mail and Drupal 15 years ago to get around problems with PHP and MTAs from that time may no longer be needed, but to be sure of that I first need good test coverage.

The big picture of how I'm trying to make this module work properly and robustly is this:

  1. First figure out what all the MimeMailFormatHelper methods are supposed to do. There was no documentation for these (there is some now), and they all call each other several times, so I'm working on one at a time from the simplest to the most complex.
  2. Second, document these methods. The documentation should say what the method is supposed to do and what all the expected inputs and output are. All of these function seem to accept many different things in the same input parameter - one parameter in mimeMailAddress() for example can be a string, or an array, or an array of arrays, or a User object, etc. And the return value of mimeMailFile() can be a string Content-ID, or a URL stored in a string, or an array of metatdata, or NULL, depending on how you call the function. Etc. - Every method seems to try to do three things and there's always one or more of the things that doesn't work completely.
  3. Third, write tests to ensure these methods are working as intended. This means a complete set of example inputs and expected outputs, using all the different parameter and return types. This step often points out serious problems that require me to patch the method then go back to step 1 or 2.

When these steps are complete, then I will feel comfortable making major changes in functionality and implementation because then I will be able to verify that the new code has the same result as the old code. I've got half of the methods in MimeMailFormatHelper finished, and I'm working on the others (like this one),

For this issue, for mimeMailHeaders(), I'm trying to write tests and I saw that regular expression and realized it would be hard to test this and it NEEDS to have tests to demonstrate what it is supposed to do and to make sure it works properly. I also realize we had another function mimeMailAddress() which is supposed to do the same thing as the regular expression and which I've already written tests for. So this issue is to eliminate that regular expression so I can finish off testing mimeMailHeaders(). mimeMailHeaders() is used by the MimeMail @Mail plugin, so to ensure the @Mail plugin works correctly I need to make sure all the methods it uses are working and tested.

A shorter answer is, it's going to be a while until I get around to some of the major changes that MUST be done and WILL be done as soon as I have complete testing for this module and have this module completely working the way it should be.

imclean’s picture

imclean commented

@TR thanks for the comprehensive response. I can see there is a lot of work to be done.

It would be useful at some point to determine what the scope of the module should actually be and compare it to what it's doing now. But as you say, that could be established after you've understood and refactored the current code.

In case it helps, for the From address and envelope sender I've taken an opinionated approach with PHPMailer SMTP.

$message['headers']['From'] - The From header name and address.
$message['from'] - The envelope sender, which can be changed in the UI.

These are often be the same but it is possible to specify different addresses.

imclean’s picture

imclean commented

$message is described in the hook_mail_alter() documentation.

tr’s picture

tr commented

I have been gradually creating that documentation at https://www.drupal.org/docs/contributed-modules/mime-mail and have been using the mimemail_example submodule as a concrete implementation of how to use Mime Mail - I am updating both of these as I get each piece working. I have a more documentation written but not posted yet because I don't want to describe how features should work before they actually do work.

tr’s picture

tr commented
StatusFileSize
new 3156239-10-replace-regular-expression.patch683 bytes

Re-rolled against current HEAD.

  • TR committed b50c64a2 on 8.x-1.x 
    Issue #3156239 by TR: Replace regular expression with mimeMailAddress()
tr’s picture

tr commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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