sendgrid / sendgrid-php

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

Mail Helper Refactor - Call for Feedback #413

Closed thinkingserious closed 6 years ago

thinkingserious commented 7 years ago

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

thinkingserious commented 7 years ago

@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!

ninsuo commented 7 years ago

Hi there,

I think you can simplify a bit the API.

1) 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);

2) 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);

3) 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

thinkingserious commented 7 years ago

Thanks @ninsuo!

  1. This is how the Kitchen Sink version works. Except, I should change SendGridMessage to Message since we are already in the SendGrid namespace.

  2. I like the idea of adding an optional parameter to the email object that accepts substitutions.

  3. 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

caseyw commented 7 years ago

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.

thinkingserious commented 7 years ago

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

thinkingserious commented 7 years ago

Add @JoeD101 to the list :)

thinkingserious commented 7 years ago

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

thinkingserious commented 7 years ago

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

thinkingserious commented 7 years ago

Hi @shonjord,

Any thoughts on the above? Thanks in advance :)

ianh2 commented 7 years ago

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 https://github.com/sendgrid/sendgrid-php/issues/413#issuecomment-327293454 , or mute the thread https://github.com/notifications/unsubscribe-auth/AceR6hwg0KERJU4VkuE5YTN6mQXJRZASks5sfa7VgaJpZM4OVCuV . https://github.com/notifications/beacon/AceR6jbk2XBYnPWv0vECPoKx78X91pHtks5sfa7VgaJpZM4OVCuV.gif

albert-agelviz commented 7 years ago

@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;
    }
}
ianh2 commented 7 years ago

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 https://github.com/sendgrid/sendgrid-php/issues/413#issuecomment-327423568 , or mute the thread https://github.com/notifications/unsubscribe-auth/AceR6qy-wf6BdoK0uJPvSPBa3m0abwL2ks5sfmFGgaJpZM4OVCuV . https://github.com/notifications/beacon/AceR6uUKIRDZq3qb9CXbJhYsOKvmBrCxks5sfmFGgaJpZM4OVCuV.gif

thinkingserious commented 7 years ago

@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

thinkingserious commented 7 years ago

Hello Everyone!

I have updated the proposed API above based on all of your feedback. I should have a working branch for you all to play with shortly.

Please let me know if you have any further feedback to incorporate.

@shonjord,

You are correct, it is the intent of the helper objects to serve as validators as well, but that is out of scope for this release. We will enforce the minimum required parameters to create a message object in this release, however.

With Best Regards,

Elmer

albert-agelviz commented 7 years ago

Hi @thinkingserious

My PR #427 is just a proposal and my idea is to help to make the library better, you can borrow all the ideas from that PR to include into your refactoring (if needed), following your suggestion to review the use cases, here are my thoughts:

I see that we don't use Subject object any more, may I ask why?, I think we should keep it, since we may want to encapsulate more logic in the subject.

Why are we using a Factory to send an e-mail?, that shouldn't be responsibility of this Library, this library should be responsible to construct an e-mail object and send it, let the users of the library make their own implementation of a factory if needed.

I think it makes more sense and looks cleaner if we do it this way:

(new SendGrid('key'))->send(Mail::class);

For collection of e-mails (to, bcc, cc, reply-to):

My PR #427 propose the use of Object to handle collections, since we can encapsulate all the logic we need to manipulate our arrays in one place and also use the beauty of type hinting, e.g:

$tos = new ToCollection([
    new To('example@email.com'),
    new To('other@email.com')
]);

and in our Mail or Message object:

public function addTos(ToCollection $tos);
public function addTo(To $to);

It's more clear to the user, because they know that ToCollection is a collection which holds to objects, simple and straight forward.

I have also implemented some methods map, each, filter which I consider to be very useful, since we have inside the Collection object all php array methods array map, filter, walk, this helps to follow DRY principle.

It caught my attention also that when we want to for example add a bcc we're using the e-mail object, I wouldn't do it that way because we have the freedom to create objects, why don't create one to represent its form?, we can create bcc, cc, reply-to objects and extend it from Email, OOP provides inheritance, why don't use it?

IMHO, makes more sense to:

$mail->addBcc(new Bcc('email@example.com'));
instead of $mail->addBcc(new Email('email@example.com'));

Let's call things by their actual domain, let's see another example to make my statement clear:

$baseBallTeam->add(new Person);

A base ball team is composed of persons but, a person can be a Baseball player, a Football player, it would make more sens if we represent it this way

$baseBallTeam->add(new BaseBallPlayer));

And BaseBallPlayer inherits Person.

I have seen also that in the current library as well as in the proposal of the PRsno one use type hinting, it would be very great to start using it, since it helps a lot to debug!

Please refer to my PR, it's well detailed all my ideas to make this library more robust, flexible, maintainable, extendable, readable and a joy to contribute to !.

Thanks. Albert.

P.S: please also don't forget to add the feature of allowing this library to send multiple reply-to since it's very needed for me and my team!!!.

CyberPunkCodes commented 7 years ago

The biggest issue with the old way, was lack of ability to bulk send. We were left with handling the splitting of batches ourselves.

This issue doesn't solve that, and seems like a waste of time. We are just putting lipstick on a pig here...

Most uses of a service like SendGrid, besides for sending transactional "your password reset link" or "thank you for registering", is to send an email to your entire system. Most systems would easily have over 1,000 people (the limit of the recipients SG handles).

We should be able to queue up 10k people and let SendGrid figure it out behind the scenes, or this library still does nothing for us.

albert-agelviz commented 7 years ago

@WadeShuler As you said, the responsibility of this library is not to send bulk e-mails, that should be handled by SendGrid's Api which is totally out of scope for this library.

CyberPunkCodes commented 7 years ago

@shonjord I never said it wasn't the responsibility of this library. This library's responsibility is to handle the app to the SG server. Since the server handles only 1000, that makes it this library's responsibility to fill in the gap.

Every single website has the need to send emails to their entire system. There must be an easier way to do it. Failing to adhere to this will result in people using other services.

albert-agelviz commented 7 years ago

@WadeShuler I don't agree with you, why would the responsibility of this library to handle n amount of e-mails?, it doesn't make any sense.

Your Application should be able to manage the amount of e-mails it needs to send, not this library, this library should be use to construct an e-mail object and send it, if you need to send bulk then that's your Application's concern my friend, not this library, and to achieve that is very simple.

CyberPunkCodes commented 7 years ago

Because SendGrid is a bulk email service. Because looping and sending 10,000 times over and over again is inefficient. Because the SendGrid API supports bulk/batch sending, hence the "batch id". Because their competition does!

https://sendgrid.com/docs/Glossary/bulk_email_service.html

SendGrid is a bulk email service for both marketing email and transactional email...

It is a bulk email service, not a "less than 1,000 recipient" email service..

https://github.com/mailgun/mailgun-php/blob/master/src/Mailgun/Messages/README.md#usage---batch-message - Mailgun automatically kicks off the email once you build the mailer with over 1,000 recipients.

Not every system is big enough for a full fledged queue with bounce processing, resend attempts, etc. However, 1,000 is too small of a limit. I understand the 1,000 limit on the API. There are physical data limits on POST requests, so the 1k limit on a single API request is spot on. Therefore, passing the blame on the API itself is nothing more than avoiding the issue.

Businesses with a list of say 4,000 recipients are stuck in the middle. A fancy queue system or expensive 3rd party solution, is unnecessary. However, SendGrid falls short. SendGrid, from a business model, is leaving a large gap, especially when their competition does this already.

I am no longer going to argue about this issue. I already have started using other services for new clients. I will come back to SendGrid when it can actually do what it is supposed to do, bulk send.

It is a shame, because from a nerd standpoint, SendGrid's servers are awesome in terms of quickness to send, inbox rate, etc. Unfortunately their API and PHP library fall extremely short.

albert-agelviz commented 7 years ago

I think you totally missed out my point, the library at the moment has an option to add multiple recipients, and its responsibility should be simple, to be able to communicate with SendGrid's API, that's it, you can add as many recipients as you want in the Mailer class, let's say a million, but if SendGrid's API can't handle it, that's a different story, and again, not responsibility of the library.

But I'm not going to argue about this anymore.

Peace.

thinkingserious commented 7 years ago

Thanks for the spirited feedback @WadeShuler and @shonjord, your thoughts will definitely help make this library better!

@WadeShuler,

We do wan't to make sure that sending a bulk of emails is easy by allowing you to get your data into our system without friction. For this iteration, the goal is making the use cases outlined above super simple to execute. The Send Multiple Emails to Multiple Recipients and Kitchen Sink use case appear to be most relevant to your use case. Is there anything specific regarding those use case that you would like to see different?

@shonjord,

Thanks for mega-PR :) and for clarifying your intent. I will definitely incorporate elements of it into the redesign.

With regards to your specific feedback:

Yes, we will still include a Subject object.

We want to keep the Client object separate as in our experience we have many users who need customization. I do like how you have shorted the call to a one liner.

I like the idea of a collection for the emails.

I also like $mail->addBcc(new Bcc('email@example.com')); vs $mail->addBcc(new Email('email@example.com'));

Unfortunately, multiple reply-to's is not supported by the underlying API. Can you please elaborate on your use case so that I may make a case to our product team?

@all,

I'm going to update the proposal again today to reflect this new feedback. The plan is to have a working beta for you this week.

thinkingserious commented 7 years ago

Hello Everyone!

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

The new prototype is here (to hopefully make it easier to collaborate on).

Thanks again to everyone who has spent time contributing. Please let me know if I did not include you on the swag train :)

With Best Regards,

Elmer

thinkingserious commented 7 years ago

HI @lode,

Just hoping you had some feedback :)

albert-agelviz commented 7 years ago

@thinkingserious

Everything looks better to me 👍

Regarding the multiple reply to feature, should I post it here?, this feature is very important for us.

thinkingserious commented 7 years ago

@shonjord,

Thanks for the quick feedback!

Please create a new issue for the reply_to feature explaining the use case so that I can reference it to our product team.

With Best Regards,

Elmer

thinkingserious commented 7 years ago

Hello Everyone!

Quick update.

I'm looking for at least two other thumbs up before working any further on the code. As always, your feedback is greatly appreciated :)

With Best Regards,

Elmer

thinkingserious commented 7 years ago

@motdotla, @eddiezane,

If you have a moment, I would appreciate a critical eye on this proposed new interface. If not, worries :)

With Best Regards,

Elmer

motdotla commented 6 years ago

@thinkingserious ! looks really nice man :+1: love the work that has gone on for these repos.

thinkingserious commented 6 years ago

Thanks for the kind words @motdotla and for responding so quickly!

albert-agelviz commented 6 years ago

@thinkingserious any news on the beta release?, it might be very helpful to me since I'm working with the current release and it's super hard to for example add a to collection, I need to hack a lot.

thinkingserious commented 6 years ago

@shonjord,

I need one more vote to proceed. If you know a PHP developer who is willing to take a look, please ask if they would mind providing some feedback.

With Best Regards,

Elmer

albert-agelviz commented 6 years ago

@thinkingserious It's ok if I tell to one of my colleagues?

albert-agelviz commented 6 years ago

@robertke can you give your thoughts on this please?.

thinkingserious commented 6 years ago

Implementation will be tracked in this issue.

jopanel commented 6 years ago

This looks fantastic. I think the recipients can be redefined better. Instead of writing New To over and over again recalling the function. I also think that the mail function should be able to be submitted without "plaintextcontent" using html as the default.

Also I don't see anywhere in the All Setting section specifying the custom category name. Category is really nice to get stats on specific campaigns you might run. Although sendgrid.com doesn't offer an export feature per email basis but just basic stats.

thinkingserious commented 6 years ago

Thanks for the feedback @jopanel!

We require HTML and plain text for deliverability reasons. If you send only HTML content, your delivery score reduces significantly. We are looking into creating a function that does the HTML to plain text version for your automatically.

For the rest of your feedback, would you mind making a PR against this file? That would qualify you for our Hacktoberfest festivities :)

With Best Regards,

Elmer

albert-agelviz commented 6 years ago

@jopanel I have to disagree to you on the recipients, I think it's more readable if you find this:

new Email(
    new From($user),
    new ToCollection([
        new To($secondUser),
        new To($thirdUser)
    ]),
    new Subject($subject),
    new HtmlContent($content)
)

Rather than:

new Email($user, [$secondUser, $thirdUser], $subject, $content)

If you come back to read your code (after a while), you inmediatly know who is from, to, etc.

And if you find yourself having to instantiate way too many times the email class then the problem is in your code, you're repeating yourself and the code needs to be refactored 😃

jopanel commented 6 years ago

Okay,

Well here there are three examples in which case this is used:

Multi-Person $tos = new ToCollection([ new To("test1@example.com", "Example User1"), new To("test2@example.com", "Example User2"), new To("test3@example.com", "Example User3") ]);

Multi-Email/Person $tos = new ToCollection([ new To("test1@example.com", "Example User1", [ '-name-' => 'Alain', '-github-' => 'http://github.com/ninsuo' ]),

Then there is single email:

$to = new To("test@example.com", "Example User");

More than likely if somebody needs to send one email like a reactivation email, register email, shipping email, notification etc. The single email is perfectly acceptable. However in cases of multi-emails more than likely a user is going to make a call to the database and return an object or an array. In which case they will use that to create their multi-person API request.

A programmer would have to run a foreach($array as $value) loop through all instances adhering towards the "new ToCollection" function array. I think this could lead to trouble programming. As well its overly complicated. If you have the array key 0, 1 as [email, name], then just ran a simple call like $tos = new ToCollection([$emails]); I don't see why this would be a bad way to go. I don't see the point in overly complicating sending an email. Nobody hard codes emails into code.

ianh2 commented 6 years ago

I agree with Joseph

I keep saying this but I don't understand why you don't look at the V2 interface which handles one or many e-mail recipients in an entirely efficient way

/First set up the standard bits for all recipients /

$email

  ->setFrom('somebody@domainclub.co.uk')

  ->setFromName($sendername)

  ->setSubject($subject)

  ->setHtml($message);

/ Now we need to add the list of addressees. We do this by reading all of the addressees from the SQL query /

while ($ReadQuery2->fetch()) { 

  $email

    ->AddTo($useremail); //loop round adding all e-mail recipients

}  //end while ... while we have records in query

All handled by one bit of SendGrid code. No need for different interfaces

This is why I shall stick with V2

Ian

From: Joseph F. Opanel IV [mailto:notifications@github.com] Sent: 04 October 2017 17:40 To: sendgrid/sendgrid-php Cc: ianh2; Mention Subject: Re: [sendgrid/sendgrid-php] Mail Helper Refactor - Call for Feedback (#413)

Okay,

Well here there are three examples in which case this is used:

Multi-Person $tos = new ToCollection([ new To("test1@example.com", "Example User1"), new To("test2@example.com", "Example User2"), new To("test3@example.com", "Example User3") ]);

Multi-Email/Person $tos = new ToCollection([ new To("test1@example.com", "Example User1", [ '-name-' => 'Alain', '-github-' => 'http://github.com/ninsuo' ]),

Then there is single email:

$to = new To("test@example.com", "Example User");

More than likely if somebody needs to send one email like a reactivation email, register email, shipping email, notification etc. The single email is perfectly acceptable. However in cases of multi-emails more than likely a user is going to make a call to the database and return an object or an array. In which case they will use that to create their multi-person API request.

A programmer would have to run a foreach($array as $value) loop through all instances adhering towards the "new ToCollection" function array. I think this could lead to trouble programming. As well its overly complicated. If you have the array key 0, 1 as [email, name], then just ran a simple call like $tos = new ToCollection([$emails]); I don't see why this would be a bad way to go. I don't see the point in overly complicating sending an email. Nobody hard codes emails into code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sendgrid/sendgrid-php/issues/413#issuecomment-334216047 , or mute the thread https://github.com/notifications/unsubscribe-auth/AceR6k1OYiLUQtzFVWxna9BWgz9bRDsbks5so7TbgaJpZM4OVCuV . https://github.com/notifications/beacon/AceR6t48WvzjSPqeK3J7zA8rp9CRIqAkks5so7TbgaJpZM4OVCuV.gif

jopanel commented 6 years ago

I use V2 as well. It is just much much easier to pass data. I made a pull request here with my changes. I think this removes a lot of redundant code making it faster/easier to develop. Also makes it more friendly for the developer in building/creating the emails needed to send in mass or single.

albert-agelviz commented 6 years ago

@ianh2 same can be done with the proposal, but much better for you and the library:

instead of:

 while ($ReadQuery2->fetch()) {
    $email->AddTo($useremail); //loop round adding all e-mail recipients
} //end while ... while we have records in query

You cand do:


$email = new Email($from, new ToCollection, $subject, $content);

 while ($ReadQuery2->fetch()) {
    $email->AddTo(new To($useremail)); //loop round adding all e-mail recipients
} //end while ... while we have records in query

Or even better:

$tos = new ToCollection($this->findUserEmails());
new Email($from, $tos, $subject, $content)
jopanel commented 6 years ago

Why not let the developer decide if they want their items apart of a collection or not? Seems highly overkill and impractical for anybody not using laravel.

Here is the Collections class from laravel. 99.99% of the time you're not gonna be using collection functions when sending mail. You're going to be using MySQL fetch array/object and return to get your data, build your request, and mail it out. Not create 10 collections along the way, use collection classes in laravel, and send mail. It is overkill even for laravel fanboys.

albert-agelviz commented 6 years ago

@jopanel Collections have nothing to do with Laravel or any other framework, Java and C# have these natively among other languages.

The idea behind of this is simple, type hinting and to have objects that represent what they are.

If you don't want to instantiate objects, then don't write OOP software, simple, you have functional programming for that.

jopanel commented 6 years ago

I keep bringing up Laravel cause its a framework I know of that utilizes a "collection class" (also Symfony does too). This is PHP not C# or Java. Ive worked with many companies who utilize OOP frameworks that don't have a collection class out the box (including my current).

albert-agelviz commented 6 years ago

You'r argument is invalid @jopanel , what's the problem of bringing good practices over to PHP?, if the developers of PHP thought the same way as you, PHP wouldn't have type hinting, return type hint, scalar type hinting, strict types, interfaces, etc, because, if other languages have it, why do we need them in PHP, it's PHP, it's not X language.

jopanel commented 6 years ago

Your comments sound very insulting. Including your first where you ask a question then answer it multiple times. I appreciate your responses. However if I could clone myself and have enough lives to where I can contribute towards PHP as a whole including frameworks that don't utilize things like collections I would develop them. I am not saying they should never be used or that they are bad. I am simply stating that it is overkill, not needed, and unncessary to be included in the original documentation.

Its like we bought a car (this php api), it runs, it drives. However we are fighting over the cupholder (the collections class). I think the cupholder should be a slide out or a pop out if you need it. You think the cupholder should constantly be there from the beginning. I have a few cars, BMW, Volvos. They all have the cupholder available if you need. You drive chevy where you want cupholders visible everywhere. We get it. You like cupholders. They are needed if you need them. You can use it if you need them. However this is a racecar and we probably won't need a cupholder much.

albert-agelviz commented 6 years ago

First of all, I'm not fighting, just giving my thoughts.

Second If you feel offended by my comments, I apologise, although, I don't feel that I have insulted you at all.

Third, ok, you don't like collections, you hate them, that's fine, but, why do you need to remove every object from the proposal?, it's the same as if you were to json_encode your data and send it, imho, you don't need the mailer class, you would be just fine with strings, which is what you want.

jopanel commented 6 years ago

You're right about one thing. It is the same as if you were to json_encode. I made a bad assumption that there would be no validation for the mailer class. I created a new pull request removing only the useless code. Which is collections. I don't hate them.

Why would you need: header collection, substitution collection, custom arg collection, content collection, attachment collection, global header collection, section collection?

Let's say you're using laravel or symfony or some other highly supported framework that contains a collections class.

What functions normal setting would this attribute to that not including a collection class wouldn't?

Am I wrong to say that in almost every case possible somebody will NOT need a collection class for them?

If you noticed I removed bcc, cc, and to collection classes from this list because its the only ones I can think of an impractical reason to use.

albert-agelviz commented 6 years ago

@jopanel These objects are part of the library, you don't need to use them in your Application at all (only to construct the mail object), in fact, that's a bad practice to couple your software with any library in particular, the idea behind these objects (all, not only collections) is to allow the library to be more robust, follow better OOP principles and practices, easy to maintain, extend and read.

As I said before, you don't need for example To object in your application other than using it only to construct the E-mail object, so in the end you know that you can rely on it because it will always be in a valid state.