Problem/Motivation

The SymfonyMailer class that was added in #3165762: Add symfony/mailer into core does not properly handle multiple comma-separated email addresses in the $message['to'] variable that is produced by Drupal. It attempts to send the entire message['to'] string into the Symfony mailer to() method, which expects a single address.

This results in an exception like the following:

Symfony\Component\Mime\Exception\RfcComplianceException: Email "test1@example.com, test2.example.com" does not comply with addr-spec of RFC 2822. in Symfony\Component\Mime\Address-\>__construct() (line 54 of /opt/drupal/vendor/symfony/mime/Address.php).

The addTo() method should be used to add additional addresses, or we can use to(...$toAddresses) with an array.

Reference: https://symfony.com/doc/current/mailer.html#email-addresses

Drupal core's MailManagerInterface::mail expects a string $to parameter, so sending to multiple addresses requires formatting them as a comma-separated list (and that is the recommendation).

Reference: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Mail%21Ma...

Steps to reproduce

1. Enable the Symfony mailer per the instructions in this change record: https://www.drupal.org/node/3369935
2. Trigger an email to be sent to multiple comma-separated addresses.

Proposed resolution

The SymfonyMailer class should parse $message['to'] into an array of addresses, and then use to(...$toAddresses) to pass them to Symfony mailer.

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

CommentFileSizeAuthor
#21 3446368-21.patch676 bytesm.stenta
#14 3446368-14.patch705 bytesm.stenta

Issue fork drupal-3446368

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

m.stenta created an issue. See original summary.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Issue summary: View changes
m.stenta’s picture

m.stenta
he/him
Primary language English
commented

parse $message['to'] into an array of addresses

It appears Symfony devs have explored possible solutions for this, although it is unclear if they implemented anything: https://github.com/symfony/symfony/issues/40205

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Parent issue: » #1803948: [META] Adopt the symfony mailer component
Related issues: +#3165762: Add symfony/mailer into core
m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Notably, it appears the same exception was encountered in Drupal core in the past, but in a slightly different context: #3226117: Uncaught RfcComplianceException when email From name contains a comma

The exception comes from Symfony\Component\Mime\Address

Drupal core uses Address to check that the "From" address in MailManager in order to "Make sure the site-name is a RFC-2822 compliant 'display-name'":

https://git.drupalcode.org/project/drupal/-/blob/f8ede99aabdf39e95aeb259...

But it does not check the "To" address(es) with Address at any point. Symfony mailer does, however, which is why this issue starts to happen once you start using Symfony mailer.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Issue summary: View changes

Updated the description to include the full error message.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Related issues: +#3226117: Uncaught RfcComplianceException when email From name contains a comma
cilefen’s picture

cilefen commented

Would it be more elegant or a better API for $message['to'] to contain either a string (one address) or an array (multiple addresses)?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

@cilefen Yes that would be more elegant, but I think it would require much bigger changes to Drupal mail handling outside the scope of this specific SymfonyMailer class. So unfortunately, in order to fix this bug, we need to deal with the fact that $message['to'] is a string, and figure out how to parse it into an array. As it stands right now, SymfonyMailer is broken when sending to multiple email addresses.

cilefen’s picture

cilefen commented

So this is like a two-line fix.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Well... based on the comments in the Symfony issue I linked to above, it may not be as simple as adding an explode(). 😅

@althaus says:

My main concern would be the how as the RFCs (with https://tools.ietf.org/html/rfc5322 being the latest) seems quite nasty. Especially the part with quoting and the comma separation seem to be tricky.

@javiereguiluz says:

We could get inspiration from Go which parses addresses according to RFC 5322 but makes some needed simplifications. See https://github.com/golang/go/blob/master/src/net/mail/message.go

@fabpot suggests he would be open to that approach:

I'm ok with adding such a feature in Symfony Mime and getting inspiration from Go might help.

But as far as I can tell this has not been done yet. So we are left with the problem, because Drupal core is starting with a string of multiple comma-separated addresses.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

We could consider using imap_rfc822_parse_adrlist(), but it sounds like that has some problems, which is what prompted the Symfony issue...

We're currently evaluating to replace the unreliable function imap_rfc822_parse_adrlist (https://www.php.net/manual/function.imap-rfc822-parse-adrlist.php) from our codebase as it has some real flaws on its error handling.

Maybe it's the best way to start, and we could potentially leverage something better provided by Symfony in the future (if they ever implement it)?

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

We could consider using imap_rfc822_parse_adrlist()

Scratch that idea. It requires the imap extension to be installed.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Active » Needs work
Issue tags: +Needs tests
StatusFileSize
new 3446368-14.patch705 bytes

Here is a simple MR+patch that uses str_getcsv() (instead of explode()), because it should ignore commas that are enclosed in double-quotes.

str_getcsv() was also used in #3226117: Uncaught RfcComplianceException when email From name contains a comma...

Instead of using explode() to split From addresses by comma, use str_getcsv() instead. That function supports ignoring comma's encapsulated in double quotes. Besides that, it seems to have the same effect as explode().

https://www.drupal.org/project/drupal/issues/3226117#comment-14192545

This might not work for all cases, but it's a first step to get the ball rolling at least. Needs tests too, of course.

m.stenta opened merge request !8024

znerol’s picture

znerol commented

Thanks, I agree with the approach. Left a small note in the MR.

A simple test case could go into SymfonyMailerTest.php

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented
Status: Needs work » Needs review
Issue tags: -Needs tests

I added a test. To do it I reconfigured SymfonyMailerTest::testMail() so that it uses a provider. So expect a lot of changes related to that. That test was originally testing newline characters in the subject. That too was extracted to one of the test cases.

Also, there's a PHP 8.4 deprecation for str_getcsv(). To fix it we have to use named parameters so that we can specify the escape character. See https://www.php.net/manual/en/function.str-getcsv.php.

znerol’s picture

znerol commented

Thanks for taking this up. Adding a data provider is useful in this case I think. It needs some improvements in order to improve readability I think.

znerol’s picture

znerol commented
Status: Needs review » Needs work
m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new 3446368-21.patch676 bytes

Attached is an updated patch that includes the PHP 8.4 fix described by @dcam in #18 above.

This patch does NOT include the test, and is only meant for applying patches via cweagans/composer-patches while we wait for this to be merged.

dcam changed the visibility of the branch 3446368-symfonymailer-multiple-recipients to hidden.

dcam opened merge request !13781

dcam changed the visibility of the branch 11.x to hidden.

dcam closed merge request !8024

dcam’s picture

dcam commented
Status: Needs work » Needs review

I made the suggested changes, but had to open a new branch and MR because I messed up the rebase.

znerol’s picture

znerol commented
Status: Needs review » Reviewed & tested by the community

Thank you @dcam.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

MR needs rebase.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Rebased. Tests are passing. Re-setting to RTBC.

  • longwave committed cbf36cb7 on 11.x 
    fix: #3446368 SymfonyMailer RfcComplianceException when sending to...

  • longwave committed f1a6a0c7 on 11.3.x 
    fix: #3446368 SymfonyMailer RfcComplianceException when sending to...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed cbf36cb7238 to 11.x. Thanks!

Committed f1a6a0c and pushed to 11.3.x. Thanks!

Initially I was not going to backport this to 11.3.x but then I realised someone might still be using this even though the experimental mailer module is not included in 11.3.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

longwave closed merge request !13781

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Thank you @longwave!!

> someone might still be using this even though the experimental mailer module is not included in 11.3

I am that someone! And this was the last core patch I've been applying! :-D

Status: Fixed » Closed (fixed)

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