Sending to multiple email addresses with comma-delimited list with spaces gives fatal error: "Invalid address"

The patch worked for me--thanks! For some reason git apply -v 2753115-2-smtp-multiple-to-addresses-error.patch didn't work, so I applied it manually.

This bug also tripped me up. I had comma separated recipients, with spaces between. Only the first recipient was getting the emails. By just removing the spaces between recipients, all recipients were included.

This seems like kind of a major bug for a module used by so many people... I would have thought that the author would push a maintenance update pretty quickly :/

This is a critical issue. Having recipients separated by commas is well with the email spec. This needs to be addressed promptly. I've downgraded to 7.x-1.3 in the meantime.

This issue is still present today in 7.x-1.x-dev so I've updated this issue and created a slightly better patch.

Used this patch and it works . . .thanks Sam.

related: https://www.drupal.org/node/2812651

royerd: Glad to hear it worked! With your approval and the two others from #2812651: Invalid email address: Error due to not checking trimmed email address., I'm going to mark this as RTBC. I'm also going to update the original report.

This does not fix all bugs with multiple addresses.

@hass, it looks like this other issue is for the 7.x-2.x branch. How would you like to handle this? Keep a separate issue for each branch?

I do not know. I guess it also exists in 1.x.

I just moved it here as the case opener has not provided the version he is using.

Howdy,

I recently updated this module to 7.x-1.6, and sending to multiple comma delimited recipients stopped working (only the first in the list would receive the email).

In my case, there were no spaces between the comma delimited emails (just commas).

Anyway, applied #6 (to 7.x-1.6), and now sending to multiple comma delimited recipients is working fine again.

Jorge

Applied this patch to 7.x-1.6 and it worked great. Some Webform 4.x recipient lists, by default, have e-mails delimited with commas and spaces ', ' and thus were not sending right when switching to SMTP. This patch fixes it. Will need to be using this with SMTP 7.x-1.6 until is is committed.

Actually, looking at the code, adding the trim() at the start of the _get_components() function is what fixes this... It might be counterproductive to remove the trim() later on as well, but I suppose once it's been done, it doesn't need to be done again, so maybe removing it does make sense.

OK, never mind. I was just thinking out loud and I guess i found my point is meaningless.

Coming to this thread via https://www.drupal.org/node/2856373, which was closed as a duplicate. The patch here, and the patch in the other issue are nearly identical. We have six RTBCs from the other thread, and a few here as well. Both patches pass automated testing.

What's the hold up with committing one or the other of these? The issues have been open for months with functioning and tested patches and resolve an existing bug in functionality.

The #6 patch solved the bug.

#6 worked for me.
• SMTP 7.x-1.x
• Mass Contact 7.x-1.1 (sending BCC with multiple addresses)
• Drupal core 7.56

What's the hold up with committing one or the other of these? The issues have been open for months with functioning and tested patches and resolve an existing bug in functionality.

I don't know. Maintainer has not identified any blocking issue on either of these threads. Tests pass. RTBC. It seems pretty straightforward, but perhaps there is a reason that is not spelled out here.

The #6 patch resolves the bug; Tested and working. Please commit. Thank you.

apotek,
As hass mentioned in #11, this doesn't solve all errors related with multiple e-mail addresses.

If this patched added more tests to make sure this feature works perfectly I'd feel safer to commit it than how it as is now.

wundo, comment #11 refers to #2847251: Multiple emails not validated properly which was closed as a duplicate of this issue. Furthermore, #2847251: Multiple emails not validated properly doesn't report any additional errors related to multiple email addresses. While this patch doesn't include tests, it has 8 Drupal.org users reporting that it works. What makes this particular issue so frustrating is that it is a critical issue on a supposedly actively maintained module and it's been unresolved for two years. And yet when a simple, two-line patch that has been reviewed by the community is submitted, it is rejected a year later.

You may reviewed, but not tested with multiple email addresses... code wise this cannot work.

@hass yes the patch does not include tests. However, I have this patch on at least a dozen production sites and it solves the issue. I believe that's what the other 7 people on this ticket have done as well to label it as "reviewed". So it has in fact been tested in production.

My point about this module being actively maintained or not still remains. Where is the maintainer? Why has the maintainer not been involved more in this critical issue, perhaps writing the tests required for this patch to be committed?

> Where is the maintainer?

Hi, @pianomansam I'm a co-maintainer, I'm here.

> Why has the maintainer not been involved more in this critical issue, perhaps writing the tests required for this patch to be committed?

Because we maintainers do this on our own time, and we are free to decide what we think is more important or a better use of our time.

It's really great that you use this module that you've got for free in at least a dozen clients.
I feel proud that I and the other maintainers and all the other people who contributed to this module were able to provide you with this great piece of software that solves yours and your clients business need.

If you feel this issue is a high priority (I don't, as it doesn't impact the majority of the users), please contribute in a positive way, no one here is obligated to write a patch or test just because you want it to.

I spent about an hour yesterday trying to figure out why emails from Webform 4.x weren't going out and finally tracked it down to the fact that there were spaces in the comma-delimited list of email addresses. Then this morning I thought, "I wonder if anyone else has run into this issue?", so I searched and found this issue thread at what appears to be an unfortunately tense moment.

@wundo, I want to say a big thanks to you and the other maintainers of this module. You're right that module maintainers provide a huge service to the Drupal community free of charge on your own time, and very rarely if ever get the thanks and recognition you deserve. I sincerely appreciate this module and all the benefits it brings to my sites and clients, and I appreciate your work that makes it possible.

At the same time, I also appreciate @pianomansam for providing a patch that solves an issue that caused me a good bit of frustration yesterday.

I think we all want the same thing - to make this module the best it can be. @wundo said in #20 that he would be more comfortable committing the patch if it had further testing. I will try to write a unit test that tests for sending emails to a comma-delimited list with spaces. But I can confirm that I have tested the patch on my own sites, and it works perfectly. Please let me know if there is anything else I can do to help move the process forward.

@wundo thank you for the recent prompt replies to this conversation. It's a great change from the two years of silence.
I understand the open-source model as I too am a contrib maintainer. I know the reality of balancing work, life, and open-source software contribution on personal time.
Rather than continue to argue over this topic, I've gone ahead and written the tests for it.
For what it's worth, I do consider this a critical issue as it prevents this module from following the RFC 2822 spec.

I'm attaching a patch that adds tests for email address parsing. These tests check the following recipient strings to see if they pass validation.

1. to_test@example.com
2. to_test@example.com,to_test2@example.com
3. to_test@example.com, to_test2@example.com

This patch will result in a failing test as the third string will fail, which is the cause of this bug report to begin with.

Once the tests have completed for this patch, I will submit another patch that will include the changes from #6.

Sigh... looks like there was a PHP class autoloader issue. Here's an update, and I removed the failing mock data, so hopefully this test will pass.

Autoloading continues to be an issue, so let's try to add a public method exposing the protected _get_components() method.

Now that we have a test for parsing addresses, let's add the failing test to confirm this issue. The mock data will be this, which is RFC 2822 compliant:

to_test@example.com, to_test2@example.com

Reapply the patch from #6 and see if it makes the new tests pass.

Since patch #33 passes the tests, I'm updating the summary.

Thanks @pianomansam for writing these tests.

Since the new tests confirm that the issue exists and that the supplied patch resolves the issue, and since a number of people have already reviewed and tested the patch, I'm updating the status to RTBC.

I was getting ready to commit this when I saw that you created a new method only to expose the _get_components so you could test it.

IMHO a better way of handling it was creating an anonymous class that extends the main one and exposes the method you want, something like:

(...)
$mailer = new class() extends SmtpMailSystem {

  /**
   * Exposes the protected _get_components method, returning an array of name and email address from a string.
   *
   * @param $input
   *  A string that contains different possible combinations of names and
   *  email address.
   * @return
   *  An array containing a name and an email address.
   */
  public function getComponents($input) {
    return $this->_get_components($input);
  }
}

(...)

This way you don't have to change the interface of the main class.

You might take a look on this patch (for D8) as well: https://www.drupal.org/project/smtp/issues/2972280

I've ported this patch to D8 and committed it.

I've also written PHPUnit tests for this issue (check the 8.x-1.x branch), if anyone could step in in follow that example and back port the test to D7 I'd really appreciate, otherwise I will tackle that myself the next time I'm available.

@wundo, good call on the anonymous class. That didn't even cross my mind, but it's a much better solution. Here's an updated patch that uses an anonymous class to expose _get_components() instead of adding it to the main SmtpMailSystem class.

@wundo The anonymous class in the test failed because it's using PHP 5.6. It looks like you discovered that as well for the 8.x-1.x version. Unfortunately, I can't use the approach you ended up using for 8.x as autoloading classes is difficult in D7. And actually, if you at the patch I created for #26, your 8.x approach is exactly what I first attempted.

@pianomansam, actually your original tests were missing some scenarios that I've implemented as well (and that was impacting coverage), you can declare more than one class in the same file, like I did for D8: https://cgit.drupalcode.org/smtp/commit/?id=10a6483

@wundo, I may leave this in your court, then, since you have a better handle of what you did in D8.

Thank you for the patch, pianomansam! Works for me! I hope it gets merged into the 7.x branch at some point so it will accept recognized address formats from drupal_mail(), but until then, at least this patch works. :)

Thanks @pianomansam for the patch. Applied #41 to current 7.x-1.7.

Multiple email addresses were being mapped in webform select component values. The first would work but then trigger the invalid email for the rest.

Confirm working for us.

I'm working on improving #41 in order to:

1. Use an intermediary class to expose _get_components() as a public method.
2. Check for some other scenarios, as shown in the D8 commits linked by @wundo

I'm attaching a set of files for testing: tests-only (must fail), full (must pass), interdiff against #41.

It'd be very useful if somebody can check if my approach is sound. I'll now continue working on the 2nd bullet point.

The test bot is not friends with the custom autoloader... trying now without it.

Well, the testbot is failing when doing "php run-tests.sh --list", which is not reproducible in my local.

I'm attaching a set of patches with a new approach: declaring SmtpMailSystemWrapper as part of smtp_tests module, registering it via files[] in smtp_tests.info.

Alright! finally learned how to do class autoloading in D7 :).

Now that the testbot is actually running the tests, let's try with porting the testing logic from the D8 tests. This is now a heavy refactor of #41. Interdiff is vs. #41.

Testbot results look good now, bumping to "Needs review".

Attaching patches with coding standards fixes, only for testAddressParsing(). No functional change vs. #51.

Please note the actual fix is exactly the same as in #6, which has already been tested and reported to work a few times in this thread. So, IMHO this should get in after just getting the tests reviewed and approved.

Looks good to me and testbot thinks so too!

Thank you guys for the wonderful collaboration on this issue!

I really appreciate all your effort, really thank you.