Display names containing a comma cause error

Yes you are right. It is a known and documented restriction on MailerHelperInterface::parseAddress(), so I would prefer to call it a feature request😃.

Since #3254085: Sending mails to multiple email addresses does not work via BC → , nearly a year ago, I've changed my views - back-compatibility mode is likely to stay around for many years, even "forever". So yes I am happy to fix this. We should use library code rather than re-inventing. I wonder about Mail_RFC822 in package pear/mail, see docs.

Thanks very much for the test. Setting to "Needs Review" so we can see that it fails😃.

In the meantime this patch solves the issue.

Also see swiftmailer_parse_mailboxes(), which is even more complex and yet still probably not fully correct or complete with the evolving standards.

Thanks the patch is useful for anyone who wants a solution in the meantime. I don't feel it's right to commit it, as it seems better to use library code.

See also core fix in 🐛 Uncaught RfcComplianceException when email From name contains a comma Fixed .

In my testing of splitting addresses, str_getcsv() is an improvement on explode() however it's not 100%:
- it works if the display name contains a comma
- if fails if the display name contains a less-than (because it removes the quotes within each address)

We had the same issue and the patch fixed the problem. As noted in #5, the str_getcsv() has an issue with "," in an address.

Let's just take the Core fix

        // Split values by comma, but ignore commas encapsulated in double
        // quotes.
        $value = str_getcsv($value, ',');

It goes in ImportHelper::parseAddress() in place of explode(',', $encoded).

Please Review changes in new MR.

Looks Good, hence moving into RTBC

Thanks. The code in Drupal Core has recently changed to support PHP 8.4. Please can we do the same here?

$value = str_getcsv($value, escape: '\\');

Hello @adamps

I have implemented the suggestion provided for extending support to PHP 8.4.

Please review and suggest any other changes is required

Moving the issue to Needs Review

Tests are failing: https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/140/p...

Also the MR should probably include the new unit test from the other branch: https://git.drupalcode.org/issue/symfony_mailer-3350992/-/compare/2.x......

koustav_mondal → changed the visibility of the branch 3350992-display-names-containing to hidden.

Hello @adwivedi008 there was some pieline issue in your code, I fixed it and also the issue was assigned to me already.

Hello @adamps, I've made the changes according to #14 . Please have a look and tell me if any other changes are required.

Thanks the code looks good. I'm confused about the test - it looks like there was a commit that added tests then they were later removed. Please can you check?

MailerHelper is now called ImportHelper in 2.x. The constructor needs only one argument.

Looking into it.

@adamps, resolved the merge error and added the unit test after fixing the issue. Please review MR 140.

Just fixed a resulting issue locally in a EmailAdjuster plugin.. the scenario is that Webform uses tokens in its addresses and sometimes there can be a comma in entity label / field. An email is inited with Drupal\symfony_mailer\Plugin\EmailBuilder\LegacyEmailBuilder setting from_name with a comma and that gets mapped to "from" address in webform_mail. Not even 100% sure where this should be solved but probably here or in a follow-up as those commas will definitely not going to be ever escaped anywhere.
I think comma-separated strings should never be treated as multiple addresses and an array should always be used for strict behavior.

Thanks. Unfortunately when I try to merge I got an error.

Rebase failed: Rebase locally, resolve all conflicts, then push the branch.
Merging failed.

The conflict is with ✨ Trim trailing comma "to" string when it's a comma-separated string Active which changed the same function. We should make sure that we don't lose the fix from there. Probably we can just copy in the 3 lines starting from:
if (empty(trim($part))) {

I think comma-separated strings should never be treated as multiple addresses and an array should always be used for strict behavior.

I entirely agree - and if you use the new API EmailInterface the it works like that. Unfortunately with LegacyEmailBuilder we are stuck with BC - otherwise old code doesn't work which defeats the whole point.

Resolved the conflict.

Thanks. I'm sorry to bother you again but there is now another conflict.

Resolved merge conflict and fixed pipeline.

Thanks you accidentally removed a space - I suggested a change in the MR to put it back

NW over a space? 😂
Can't believe..
Just merge the suggestion and continue reviewing!

Just commit the suggestion and continue reviewing!

D'oh hadn't occurred to me that I could commit my own suggestion😃

Thanks everyone

I see this was marked as fixed.
We were using the patch from #3 against the 1.5.x branch, which worked for us.
Since 1.6 was released and, as far as I can tell, does not contain a fix for this, and the patch from #3 does not apply anymore, attached is an updated patch against 1.6.

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

Too late for this issue, but this looks like it could be a useful library for parsing a string of email addresses: https://packagist.org/packages/mmucklo/email-parse (github repository).