Bogardo / Mailgun

Mailgun package for Laravel
MIT License
295 stars 115 forks source link

Design Concept #87

Closed dustingraham closed 7 years ago

dustingraham commented 7 years ago

Hi,

I couldn't find any way to contact you directly via your website or github, so I figure this is on-topic enough to post an issue. Feel free to close it, or delete it.

I was wondering about a fundamental design concept of this library. We're currently evaluating whether we want to stay with mailgun or not. And, one thing I realize is that it will be slightly more painful to switch out mailgun than it probably should be.

Meaning, the code uses \Mailgun::send() rather than laravel's \Mail::send(). It seems like mail libraries ought to be designed under the \Mail::send() so that we could swap out implementations and use mailgun or sendgrid or some other option.

Or, if we can't get into the laravel framework effectively enough to put the mailgun implementation below the \Mail::send() then maybe make an abstract implementation above \Mail::send() like \Mailer::send() which \Mailer could use a mailgun implementation or a sendgrid implementation or whatever.

Thoughts? Thanks, -Dustin

Bogardo commented 7 years ago

Hi @dustingraham,

This is a decision (concession?) I made back when I started out with this project. I initially wanted to create a new mail driver to add support for Mailgun (which didn't exist yet, back then) but after a lot of fiddling around in the code without success I figured I would be better of moving it to a separate service avoiding the whole Mail implementation of Laravel, which relies heavily on Swift Mailer, which I do not have a lot of experience in.

In the meantime Taylor added native support for a couple of mail services, including Mailgun which you can use out of the box with the Mail::send() method. Unfortunately this implementation does not support certain features that services like Mailgun offer like Tagging, Click/Open Tracking or Batch sending.

If you do not need any features like the ones mentioned above and just want to send regular transactional emails via the Mailgun API you're probably better off using the integrated driver.

As for your suggestion about an abstract Mailer implementation. My main concern is the issue Taylor probably faced when integrating mailgun and sparkpost support are the different kind of features both services offer. They probably overlap but probably aren't exactly the same (I haven't looked into Sparkpost much). So if your application is using Mailgun and you want to switch to another service but the other service does not support some features of Mailgun that you currently rely on in your app you end up in the same situation. So in this case I feel that separating these services and their features in separate packages is the safest route.

Of course this same exact issue arises if I would convert this project to a laravel mail driver, to be compatible with the other drivers I would have to drop all the features that are specific to Mailgun, which in turn would defeat the whole purpose of this package because such implementation is already integrated in Laravel as mentioned before.

dustingraham commented 7 years ago

Thanks Chris.

I do use click/open tracking as well as batch sending (critically useful feature!) I don't use tagging, but I've wanted to, and may soon. (If I continue with mailgun.)

One consideration might be to allow for some sort of Mail::params(['tag' => 'yellow'])->send(...) where there is a generic params or config call. If using smtp, tag is meaningless so it'll be ignored, but if using a platform where tagging is supported, passing in the params will be used.

I guess another option might be to ask myself, "is this feature a feature I consider fundamental to email as a global principle?" If the answer is yes, then build the support into the native Mail::send() such as open/click tracking I do think that's a globally valuable feature. With swiftmailer, or services that don't offer open/click tracking, setting it would do nothing. But with services that do support it like mailgun, it would be easy to turn on with the native implementation.

But, even as I consider these two options, I think they may not actually be good ideas. I really appreciate your feedback and insight, and it makes a lot more sense to me now.

Bogardo commented 7 years ago

Adding such a feature to a codebase that you yourself are familiar with would probably be fine but the main issue I have with it is when a new developer comes on the team or when a complete project is handed over to another developer. They see something like Mail::params(['tag' => 'yellow''])->send(...) in the codebase and they might assume that it works and keep using it throughout the code. Not knowing that the feature might not be supported by their preferred/selected mail service at all, which might lead to big issues later on.

I really appreciate your feedback and ideas. I even booted up a fresh install of Laravel yesterday to see if the process of integrating a (native) mail driver has changed in the lastest version(s) but unfortunately it hasn't changed a lot. Don't get me wrong, it is doable but I just don't think it's the right thing to do considering the things discussed earlier.

One of the main goals I had when starting this project was to make the interface as similar to the native Mail component as possible to make a transition from Mail to Mailgun (or the other way around) as easy as possible. This goal has faded a little over time with the addition of some features like batch sending. This is also a sign for that keeping a feature as simple as sending mail compatible with all kinds of services and implementations is a very hard thing to do, especially if you want to integrate all the cool features. :)

dustingraham commented 7 years ago

Agreed on all points.

A side point. The batch feature is so valuable. I had a cron script where my coworkers could schedule an email to go out at say 10am. Using individual emails, it would take 2-3 hours to deliver 10k+ emails. Sometimes longer for our bi-annual trade show announcements. Switching to batching reduced that to less than a minute. Crazy valuable feature.

Bogardo commented 7 years ago

I'm glad we can agree. And I'm very glad that you are benefitting from the package. (though the credits go to Mailgun mostly)