[Proposal] De-couple mail component a bit more

[driesvints]

I wrote this PR because recently I tried integrating the mail component in a legacy Zend 1 app. I noticed some caveats with using it because at some points it was pretty tightly coupled to the Laravel framework. I made some improvements to decouple it a bit more so others may use it more easily in applications different then Laravel. This PR should be without BC breaks so I could also send this to 4.2 if you'd like. Let me know.

Improvements:

• Usage of an interface and trait for the log writer class. This allows us to more easily use our own log classes in Laravel components. I've replaced the hard-coded Writer dependency in the Mailer class with the interface so we can now easily use our own loggers with the mail component.
• I've added Guzzle as a suggest in composer.json. I didn't get the mandrill component working immediately and it turned out that it was because Guzzle didn't came along as a dependency. While I agree that it may be overkill to always include it in the "require" list we should definitely let developers know to install it if they want to use the Mailgun or Mandrill driver.
• Currently, the logic for choosing a transport based on the selected driver is hardcoded into the service provider. This makes it virtually impossible to use it outside of Laravel. I ended up just copy/pasting all the code. I've moved this to an external class so it's easier to re-use.
• Some minor docblock and namespace import fixes.

Hope you like it! I'll write a blog post on how to integrate the mail component into other apps after this gets merged in. Enjoy!

PS.: I'm still wondering about cutting out the config repository as a constructor argument and just pass along an array with all the options. This will make it even more easier to re-use outside Laravel. What do you think?

Update 1

• Replaced the Writer dependency in the Mailer class with the PSR LoggerInterface.
• Removed the Config Repository dependency from the Transport class and to just pass the array with options.

Update 2

• Will send the docblock and namespace fixes as a separate PR so it can be included in the 4.2 branch.
• The mail Transport class now extends the Manager class. One big change is that swift.transport now is an instance of TransportManager rather than Swift_Transport

[KennedyTedesco]

My opinion: I think this is a cool idea, but worry about the use of laravel components "out of the box" is a bit tricky (Maybe is not the Goal of the project).

Anyway, you do a nice job! Congrats.

[taylorotwell]

Initial thought is instead of a WriterInterface just use the PSR Logging interface.

[driesvints]

@KennedyTedesco my opinion is that if you can de-couple more you should. This way we allow developers to easily use each component individually if they want to without having to require to use the entire framework.

@taylorotwell done, I've rebased my commits. It's true, that makes more sense.

[GrahamCampbell]

Files should end in a single new line character.

[driesvints]

Done

[GrahamCampbell]

🍻

[driesvints]

I've removed the config repository from the Transport class as well. This way people don't need to use the Laravel config component to use the Transport class. Just pass in the array of config options. The support package is required anyway so we can use the array_get method and keep BC.

[crynobone]

While in theory this doesn't introduce any breaking BC for out of the box user, will there be issue if for example someone override MailServiceProvider to introduce other 3rd party transport? Amazon SESs for example?

[driesvints]

@crynobone yes of course, they'll have to extend the Transport class and override the registerSwiftTransport method in the service provider to use their own class. But tbh, it's worth it because this improvement really comes in handy for using the component outside Laravel.

Perhaps it's best that this should be kept to the master branch then for the 4.3 release.

[GrahamCampbell]

Nice work. Is there any change you could send the other fixes you made here to 4.2 (like the constructor docblock fixes)?

[KennedyTedesco]

@driesvints Yes, you're right. Looks realy good.

[crynobone]

Also would be nice if we can have a service location for Transport and allow it to utilize MacroableTrait, this way it is easier to include 3rd party transport in the future.

[crynobone]

Or convert Transport to utilize Illuminate\Support\Manager.

[driesvints]

@GrahamCampbell I'll send in the docblock fixes and namespace imports as a separate PR to 4.2.

@crynobone I've included the MacroableTrait in the Transport class. Let me know if I can improve this. It's the first time I've used it. I didn't opt for the Manager class because that would make the Transport class too tightly coupled to Laravel, something I really want to avoid if we want to give people the opportunity to utilise the component outside Laravel. It's still easy to extend it imo.

[crynobone]

Need at least before using switch.

if (static::hasMacro($driver)) {
    return call_user_func(static::$macros[$driver], array_get($this->config, 'mail'));
}

[driesvints]

After consulting with @crynobone I've refactored the Transport class to extend the Manager class. This again tightly couples the mail component to Laravel but perhaps we should look at refactoring the Manager class to a trait like crynobone suggested to me. This will more easily allow us to use classes like the transport class outside of Laravel.

[crynobone]

This again tightly couples the mail component to Laravel but perhaps we should look at refactoring the Manager class to a trait like crynobone suggested to me.

We could also consider providing a capsule manager for this, where the "config" service location will utilize Illuminate\Support\Fluent.

[franzliedke]

I don't think extending the manager is that bad, since we require illuminate/support anyway. Or am I missing something?

[GrahamCampbell]

👍 For using a manager class. I think I requested that a long time ago.

[crynobone]

I don't think extending the manager is that bad, since we require illuminate/support anyway. Or am I missing something?

Technically it should still need Illuminate\Container\Container and Illuminate\Config\Repository to fetch the config, which might be undesirable when using outside of Laravel. Which is why database and queue capsule only utilize Illuminate\Support\Fluent for "config" service location.

[driesvints]

I don't think extending the manager is that bad, since we require illuminate/support anyway. Or am I missing something?

We're contemplating on refactoring the Manager class to make the classes which it extends easier to use outside Laravel.

Edit: Crynobone beat me to it :)

[driesvints]

But anyway, for now this PR is as good as finished so @taylorotwell you can review this if you want.

[GrahamCampbell]

@driesvints Did you ever make that other pull with the non-related fixes? I see you've made the fixes on a branch, but have not actually made a pull.

[driesvints]

@GrahamCampbell not yet, I have some other stuff on my list that I want to fix and send along. Will do that once I find time for it.

[GrahamCampbell]

I've sent quite a few related fixes in pulls recently, I don't know if that takes anything off your list.

[taylorotwell]

This looks pretty good. Will try to merge soon. 😄

[driesvints]

Rebased :)

[driesvints]

Just FYI, I've removed the extract call from a function in the TransportManager class because Scrutinizer was complaining about undefined variables. I haven't found yet how to circumvent this to keep Scrutinizer happy. Better is to avoid the extract function for now.

[crynobone]

I haven't found yet how to circumvent this to keep Scrutinizer happy. Better is to avoid the extract function for now.

I don't think we should totally based on code on Scrutinizer report, as it seem to be Scrutinizer limitation (same when we use compact etc).

[GrahamCampbell]

Recent changes to the master branch mean rebasing is required yet again. :(

[driesvints]

Recent changes to the master branch mean rebasing is required yet again. :(

Done.

Well, it seems the PSR logger interface got implemented in another commit. This PR now mostly contains the TransportManager class.