sendgrid / sendgrid-php

The Official Twilio SendGrid PHP API Library
https://sendgrid.com
MIT License
1.49k stars 623 forks source link

Documentation: fix "Send Multiple Emails to Multiple Recipients"? #1040

Open quentin-st opened 3 years ago

quentin-st commented 3 years ago

Issue Summary

The "Send Multiple Emails to Multiple Recipients" documentation section basically uses this to demonstrate sending an email to multiple recipients:

new Mail(
    $from,
    [new To("test+test1@example.com"), new To("test+test2@example.com")],
    // ...
);

In our use case (no substitution, same body for everyone, meaning to send a separate email to each recipient individually), this lead to sending a single email with everyone in the "to" recipients list.

The right way to achieve this is to use create N Personalizations:

$email = new Mail(
    new From(self::MAIL_FROM, self::FROM_NAME),
    null,
    new Subject($subject),
    $plainTextContent,
    $htmlContent
);

// Add recipients
foreach ($emails as $emailAddress) {
    // Note: "To" constructor may throw an exception when encountering an invalid email.
    try {
        $personalization = new Personalization();
        $personalization
            ->addTo(new To($emailAddress));

        $email->addPersonalization($personalization);
    } catch (TypeException $ex) {
        continue;
    }
}

Are mails separately sent only if there is at least one substitution? Shouldn't we update the documentation to add a warning if that's the case?

Waiting for feedback before opening a PR to update the documentation!

Steps to Reproduce

  1. Try to send n emails to n recipients
  2. Use the example in the documentation (->addTo(new To($email)))
  3. Notice that a single mail is sent, with n recipients instead

Technical details:

thinkingserious commented 3 years ago

Hello @chteuchteu,

Thank you for taking the time to raise this issue!

I don't think for this use case that we should only create personalizations in the case of substitutions.

Looking at the tests, it seems we did not check for the case when no substitutions are present :(

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog.

With best regards,

Elmer