Mail Helper Refactor - Call for Feedback

To make this easier to collaborate, I've moved the proposed interface here.

@caseyw

@caseyw, @vitya1, @Braunson, @cbschuld, @paoga87, @Taluu, @mazanax, @ninsuo, @ianh2, @WadeShuler, @jaimehing, @KnightAR

If you are tagged on this message, it means we are particularly interested in your feedback :)

If you don't have the time, no worries and my apologies for the disturbance.

If you do have the time, please take a look at the proposed helper upgrade above and let us know what you think. Any and all feedback is greatly appreciated.

Thanks in advance!

Hi there,

I think you can simplify a bit the API.

First, for simple or multiple recipients, this is just a matter of array. So we may use the same class (like Message) instead of CreateMultipleEmailsToMultipleRecipients.

For example:

/*
 * 1 recipient
 */
 
// (...)
$to = new \SendGrid\Mail\To("ninsuo@gmail.com", "Ninsuo");

$msg = new \SendGrid\Mail\Message($from,
                                  $to,
                                  $subject,
                                  $plainTextContent,
                                  $htmlContent);

/*
 * N recipients
 */
 
// (...)
$tos = [
    new \SendGrid\Mail\To("ninsuo@gmail.com", "Ninsuo"),
    new \SendGrid\Mail\To("bob@example.com", "Bob"),
];

$msg = new \SendGrid\Mail\Message($from,
                                  $tos,
                                  $subject,
                                  $plainTextContent,
                                  $htmlContent);

Playing with indexes doesn't look very friendly:

$msg = new \SendGrid\Mail\CreateMultipleEmailsToMultipleRecipients($from,
                                                                   $tos,
                                                                   $subjects,
                                                                   $plainTextContent,
                                                                   $htmlContent,
                                                                   $substitutions);

Having $tos and $subjects and moreover $substitutions separated like that is not really friendly to manage, you should put the right receiver in front of the right subject playing with indexes, and it adds complexity.

IMO, it will be easier if everything related to one receipient is in the same object. And by the way, if we have substitutions in the recipient object, we don't even need to put several subjects.

For example :

$from = new \SendGrid\Mail\From("bob@example.org", "Bob");

$to = new \SendGrid\Mail\To("ninsuo@gmail.com" /* email */, "Ninsuo" /* name */, [
  '-name-' => 'Alain',
  '-github-' => 'http://github.com/ninsuo'
] /* receipient substitutions */);

$globalSubstitutions = [
  '-time-' => date("Y-m-d H:i:s"),
];

$subject = 'Hi -name-!',
$plainTextContent = "Hello -name-, your github is -github-, email sent at -time-";
$htmlContent = "<strong>"Hello -name-, your github is <a href="-github-">here</a></strong> email sent at -time-";

$msg = new \SendGrid\Mail\Message($from,
                                  $to,
                                  $subject,
                                  $plainTextContent,
                                  $htmlContent,
                                  $globalSubstitutions);

My last remark is about the attachments:

$msg->addAttachment("balance_001.pdf",
                    "base64 encoded content",
                    "application/pdf",
                    "attachment",
                    "Balance Sheet");

Why not giving contents as is, and SDK would take care of the base64 encoding?

Cheers

@ninsuo

Thanks @ninsuo!

This is how the Kitchen Sink version works. Except, I should change SendGridMessage to Message since we are already in the SendGrid namespace.
I like the idea of adding an optional parameter to the email object that accepts substitutions.
I agree, but that is not in the scope if this release.

Thanks again for taking your time to submit some feedback and I welcome further feedback if you have any :)

With Best Regards,

Elmer

I'm also a fan of the array of to objects. Having separate methods seems a bit much for the use case.

I'm not using substitutions in our system in this fashion, however I like the idea of being specific on a per "To" basis.

@caseyw

Thanks @caseyw!

Are you referring to item #2 above?

I should note that those specific helper methods (and we will likely develop more based on feedback) are designed for those who just need that simple use case and don't need to go any deeper. The idea is to them to sending their first email ASAP. While the Kitchen Sink example is meant for the power user.

With Best Regards,

Elmer

@JoeD101

Add @JoeD101 to the list :)

@ninsuo

@ninsuo, please take a moment to fill out this form so that we can send you some swag. Thanks!

@ninsuo

Hello Everyone!

Good news! Development on this has begun. When we have a workable beta, a PR will be published.

Based on the feedback above, $msg = new \SendGrid\Mail\Message(); will be able to handle all the use cases above by simply changing the values of the passed objects (e.g. passing in an array of to objects), and we'll still include the convenience methods. We will likely implement substitutions in the manner that @ninsuo suggested.

With Best Regards,

Elmer

Hi @shonjord,

Any thoughts on the above? Thanks in advance :)

Elmer I got so dispirited with the first version(s?) of your V3 interface that I gave up and continued my HTML5/PHP 7 /mysqli upgrade on the V2 interface. It seemed to me that I didn't have a stable V3 interface to work with and that the V3 interface was considerably more complex than the V2 interface which does the job Where is the latest version? Ian From: Elmer Thomas [mailto:notifications@github.com] Sent: 05 September 2017 21:28 To: sendgrid/sendgrid-php Cc: ianh2; Mention Subject: Re: [sendgrid/sendgrid-php] Mail Helper Refactor - Call for Feedback (#413) Hi @shonjord <https://github.com/shonjord> , Any thoughts on the above? Thanks in advance :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#413 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AceR6hwg0KERJU4VkuE5YTN6mQXJRZASks5sfa7VgaJpZM4OVCuV> . <https://github.com/notifications/beacon/AceR6jbk2XBYnPWv0vECPoKx78X91pHtks5sfa7VgaJpZM4OVCuV.gif>

@thinkingserious

@thinkingserious I saw @ninsuo's comment and couldn't agree more with him, also would like to leave my thoughts on the initial proposal.

Breaking mail scenarios into multiple classes break Open/closed principle why?, let's say I first start with CreateSingleEmail because I want to send a single e-mail with one recipient, then, down the road I need to send to more recipients, I'm forced then to refactor my code and start using CreateSingleEmailToMultipleRecipients.

Like ninsuo's said, Mail should be responsible for this, we should inject array or collections and then this class should be responsible to handle the one or many entities, e.g:

public function __construct(EmailAddress $from, array $to, Subject $subject, Content $content)

Also, I've seen that we have in our hands some Value Objects like Email or Subject, but, I haven't seen any validation within their constructors, why?.

When ever we use Mail object it should be always in a valid state, I think our value objects should be smart and strict enough to allow our Mail object to be always in a valid state e.g:

class EmailAddress
{
    private $address;
    private $person;

    public function __construct(string $address, string $person)
    {
        if (!filter_var($address, FILTER_VALIDATE_EMAIL)) {
            throw new InvalidEmailAddressException;
        }
        $this->address = $address;
        $this->person = $person;
    }
}

In the V2 interface this was so simple $email ->setFrom($senderemail) ->setFromName($sendername) ->setSubject($subject) ->setHtml($message); /* Now we need to add the list of addressees. SendGrid will separate them out into separate e-mails so we don't need to worry about Bcc. */ Either we do this for a single recipient $email ->AddTo($useremail); //loop round adding all e-mail recipients Or we do this - the fetch can be a single recipient while ($ReadQuery2->fetch()) { $email ->AddTo($useremail); //loop round adding all e-mail recipients } //end while ... while we have records in query Ian From: Albert Agelviz [mailto:notifications@github.com] Sent: 06 September 2017 10:09 To: sendgrid/sendgrid-php Cc: ianh2; Mention Subject: Re: [sendgrid/sendgrid-php] Mail Helper Refactor - Call for Feedback (#413) @thinkingserious <https://github.com/thinkingserious> I saw @ninsuo <https://github.com/ninsuo> 's comment and couldn't agree more with him, also would like to leave my thoughts on the initial proposal. Breaking mail scenarios in multiple classes break Open/closed principle why?, let's say I first start with CreateSingleEmail because I want to send a single e-mail with one recipient, then, down the road I need to send to more recipients, I'm forced then to refactor my code and start using CreateSingleEmailToMultipleRecipients. Like ninsuo's said, Mail should be responsible for this, we should inject array or collections and then this class should be responsible to handle the one or many entities, e.g: public function __construct(Email $from, array $to, Subject $subject, Content $content) Also, I've seen that we have in our hands some Value Objects like Email or Subject, but, I haven't seen any validation within their constructors, why?. When ever we use Mail object it should be always in a valid state, I think our value objects should be smart and strict enough to allow our Mail object to be always in a valid state e.g: public function __construct(string $address, string $person) { if (!filter_var($address, FILTER_VALIDATE_EMAIL)) { throw new InvalidEmailAddressException; } $this->address = $address; $this->person = $person; } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#413 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AceR6qy-wf6BdoK0uJPvSPBa3m0abwL2ks5sfmFGgaJpZM4OVCuV> . <https://github.com/notifications/beacon/AceR6uUKIRDZq3qb9CXbJhYsOKvmBrCxks5sfmFGgaJpZM4OVCuV.gif>

@ianh2

@ianh2, @shonjord,

Thank you for the valuable feedback! I'll be refactoring the design shortly and I'll ping you for further review.

For now, I'd like to express our appreciation for your feedback with some swag, please take a moment to fill out this form.

With Best Regards,

Elmer

As a compromise, why not use iterable type?

Mail {
public function __construct(Email $from, string $subject, iterable $to, Content $content)

This way, both arrays and whatever inherits Traversable will work. If SendGrid chooses not to implement collections, this will not prevent others from quickly making their own.

Since this would be a new library version, supporting PHP 7 is fine. In mean time, compromise for 5.6 would be to accept ArrayObject and if primitive array is passed, convert it to object:

Mail {
/**
* @param Email $from
* @param string $subject
* @param ArrayObject|array $to
* @param Content $content
*/
public function __construct(Email $from, string $subject, $to, Content $content) {
  if(is_array($to)) {
    $to = new \ArrayObject($to);
  }
}

or inversely convert objects to array

if($to instanceof \Iterable) {
   $to = iterator_to_array($to);
}

or

if($to instanceof \ArrayObject) {
   $to = $to->toArray();
}

Speaking of types, I hope PR which includes types is accepted. Currently, this library is a painful nightmare to work with (and looks unprofessional). I never know what goes into any function until API returns errors. The whole point of library is to eliminate uncertainty of how to make a payload. The idea that $mail = new SendGrid\Mail($from, 'Subject', $email, 'content'); works without any errors and I dont know that last parameter should be instance of Content until response sends back errors, is unacceptable. Then, there is

$settings = new SendGrid\MailSettings();
$settings->setSandboxMode(true);

which, again, works in PHP but sendgrid is upset until I guessed to change it to

$sandbox = new \stdClass();
$sandbox->enable = true;
$this->settings->setSandboxMode($sandbox);

When I press control+space, it should list precisely the object types needed.

Didn't check this issue since a long time.
You did a very good job during that time guyz, bravo.
Cheers

@alextech

@alextech excellent, didn't think of this before, I hope sendgrid would implement the way you suggest, it's brilliant.

@alextech

Hi @alextech,

Thanks for the great feedback!

Would you mind making a PR here that incorporates your suggestions?

With Best Regards,

Elmer

Hey everyone! I'm curious about your thoughts on the effects of #450 on this work?

The iterable type sounds good. I could add it to #450

@thinkingserious

@thinkingserious that PR is public interface signature discussion, while my suggestion is about internal implementation of those signatures. So nothing for me to add there.

As for #450 , multiple classes per file is a big no no, so IMO that PR must go through, maybe take the opportunity to add parameter types while at it. It does not even have to be full PHP 7 enforced types, just SOME typing function (type $var) will make a world that much of a better place.

@alextech

Thanks for responding os quickly @alextech!

Navigation Menu

Repository navigation

Mail Helper Refactor - Call for Feedback #413

37 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Mail Helper Refactor - Call for Feedback #413

Description

Activity

thinkingserious commented on Jul 14, 2017

ninsuo commented on Jul 14, 2017

thinkingserious commented on Jul 14, 2017

caseyw commented on Jul 14, 2017

thinkingserious commented on Jul 14, 2017

thinkingserious commented on Jul 19, 2017

thinkingserious commented on Jul 20, 2017

thinkingserious commented on Aug 31, 2017

thinkingserious commented on Sep 5, 2017

ianh2 commented on Sep 6, 2017

albert-agelviz commented on Sep 6, 2017

ianh2 commented on Sep 6, 2017

thinkingserious commented on Sep 6, 2017

37 remaining items

alextech commented on Oct 7, 2017

ninsuo commented on Oct 7, 2017

albert-agelviz commented on Oct 9, 2017

thinkingserious commented on Oct 10, 2017

mbernier commented on Oct 11, 2017

geryjuhasz commented on Oct 11, 2017

alextech commented on Oct 11, 2017

thinkingserious commented on Oct 11, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions