ActiveCampaign / laravel-postmark-provider

A Postmark adapter for Laravel 5+
30 stars 9 forks source link

Simplify service provider #5

Closed jenssegers closed 6 years ago

jenssegers commented 8 years ago

Depending on what the minimum supported Laravel version is, this change should simplify Postmark integration with Laravel >= 5.0.0. Everything lower will break though.

atheken commented 8 years ago

Hi @jenssegers. Thanks for the PR. I saw you've just started using Postmark, so welcome!

I've actually been looking at deprecating this package. I appreciate your PR, and would like to get your feedback on this direction.

As you saw, we have a Swiftmailer implementation, and if you review issue #4, I found that Laravel can be configured using its IoC Container to use our Swiftmailer implementation, without the need for this intermediate package.

The configuration is pretty easy to document in a README, but will require a little bit more code to be written by consumers. (Basically, registering a function with the IoC Container that returns a Swiftmailer instance).

We wouldn't remove this package, but we would point people over to our Swiftmailer package with instructions for configuring it with Laraval.

What do you think? Should we reduce the "moving parts," or do you think the Laravel package provides enough value on its own to justify maintaining it?

jenssegers commented 8 years ago

@atheken I was looking for a similar solution. But the annoying thing is that as soon as you build the mailer, the ->driver('postmark') is called which will throw something like Unkown driver "postmark".

And the transport manager instance is only registered when the mailer is being built. This is the only solution I could come up with without changing too much of the original Laravel code.

For this to become possible, Laravel needs to modify how their mail dependencies are being registered in the IOC. Ideally the TransportManager should be available in the IOC without having to build the mailer instance first. Then you could just run this code in your own application service provider:

$this->app['swift.transport']->extend('postmark', function () {
    $token = $this->app['config']->get('services.postmark');
    return new Transport($token);
});
atheken commented 8 years ago

Thanks @jenssegers - Let me dig around a bit more. If I can't get the IoC approach to work, I'll merge in this PR. I will probably not have a chance to do so for a couple of days, but I won't forget about this.

jenssegers commented 8 years ago

I might bring this up in a Laravel issue this weekend if I don't forget. But I'm not sure if Taylor will do anything about it.

jenssegers commented 8 years ago

I opened a pull request for the Laravel framework: https://github.com/laravel/framework/pull/12509

This should allow people to extend the transport manager with a postmark driver.

jenssegers commented 8 years ago

Great news, the PR got accepted. Currently it is only on the 5.2 branch, hopefully they cherry pick it for earlier versions as well, if not I will make a PR.

atheken commented 8 years ago

That's awesome, Jans!

I think that's the better route.

Thanks for taking the initiative and contributing, it really helps!

On Feb 26, 2016, at 6:18 PM, Jens Segers notifications@github.com wrote:

Great news, the PR got accepted. Currently it is only on the 5.2 branch, hopefully they cherry pick it for earlier versions as well, if not I will make a PR.

— Reply to this email directly or view it on GitHub.

SanderSander commented 8 years ago

+1 for this :) Currently it's even impossible to use other drivers like log this will fix it.