New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.7] Multiple mail connections #23183

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
@Propaganistas
Contributor

Propaganistas commented Feb 15, 2018

I'm currently working on a project where multiple mails should be sent in a single request, each requiring their own SMTP settings and there's even one that could be sent using another driver. At the moment Laravel constructs the Mailer during first initialisation and then doesn't allow switching driver or even configuration like SMTP settings that easily.

I think the Mail component could benefit greatly from adopting a similar setup as the Database component: 1) multiple connections can be defined, which accommodates the need for multiple configurations using the same driver but with different settings; and 2) the "connection" can be chosen on runtime without any hassle.

For example, this could be the config/mail.php file, and then mails could be sent as follows:

Mail::send(...); // Uses the default connection.
Mail::connection('other_smtp')->send(...); // Uses the "other_smtp" connection
Mail::connection('sparkpost')->send(...); // Uses the "sparkpost" connection

I'm eager to learn your insights.

@GrahamCampbell GrahamCampbell changed the title from Multiple mail connections to [5.7] Multiple mail connections Feb 15, 2018

/**
* Get a mailer connection instance.
*
* @param string $name

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Feb 15, 2018

Member

string|null

@GrahamCampbell

This comment has been minimized.

Member

GrahamCampbell commented Feb 15, 2018

The manager is missing the ability to extend with more drivers. Might be cool to go as far as having a custom driver like the LogManager does.

@Propaganistas

This comment has been minimized.

Contributor

Propaganistas commented Feb 16, 2018

Yeah as I mentioned in the disclaimer it's far from perfect (and perhaps isn't even working in its current state). Just wanted to reach out first to avoid spending too much time on a concept that doesn't fit in the framework.

@Propaganistas

This comment has been minimized.

Contributor

Propaganistas commented Feb 16, 2018

As an additional note: the DatabaseManager deviates from Illuminate\Support\Manager in the sense that a driver is defined on connection level and a driver can be implemented by multiple connections, while the Manager directly asks for and returns various drivers.

If the overall concept is ok, perhaps this is a good moment to introduce a second Manager-like Support class (e.g. ConnectionManager) and let DatabaseManager and MailManager extend from it?

@decadence

This comment has been minimized.

Contributor

decadence commented Feb 16, 2018

It's certainly the thing Laravel is missing.
When I need this I use this workaround

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Feb 16, 2018

I'm generally fine with the idea behind it if you want to polish it up and test it.

@regiszanandrea

This comment has been minimized.

regiszanandrea commented Feb 16, 2018

@decadence Me too, I had to override the method sendof Mailable Class. This is a thing that is missing in Laravel

@Propaganistas

This comment has been minimized.

Contributor

Propaganistas commented Feb 18, 2018

Awaiting feedback on the referenced PR that adds the mentioned ConnectionManager support class.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Feb 19, 2018

I don't want to get bogged down with new abstractions and generalizations yet. Let's just focus on the mail aspect.

Propaganistas added some commits Feb 21, 2018

Propaganistas added some commits Feb 22, 2018

@Propaganistas

This comment has been minimized.

Contributor

Propaganistas commented Feb 22, 2018

Here's a first take on this. I'm unsure if the tests are fine and could use some advice on this.
This is the PR for changes in the default config file.

Looking forward to feedback.

}
/**
* Make the database connection instance.

This comment has been minimized.

@victorlap

victorlap Feb 26, 2018

Should be mailer connection

* @param string|null $name
* @return \Illuminate\Mail\Mailer
*/
public function connection($name = null)

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member
public function connection(?string $name = null)

This comment has been minimized.

@deleugpn

deleugpn Apr 6, 2018

Contributor

The framework code doesn't make use of primitive type hints.

*
* @throws \InvalidArgumentException
*/
protected function makeConnection($name)

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member
protected function makeConnection(string $name)

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Apr 6, 2018

Member

No, Laravel does not use scalar type hints. @TBlindaruk Please stop suggesting changes that violate Laravel's code style. If you'd like Laravel to change this, please raise an RFC on internals.

This comment has been minimized.

@goodevilgenius

goodevilgenius Jun 1, 2018

I've never been able to find Laravel's specific code style guidelines. Could you provide a link to where it prohibits scalar type hints?

This comment has been minimized.

@devcircus

devcircus Jun 1, 2018

Contributor

There's nothing official that explicitly prohibits them, they're just not used elsewhere in the framework (except in the case of extending a third party dependency that uses them). Generally, Laravel follows psr2 with a few exceptions, like spaces in docblocks, as referenced here.

This comment has been minimized.

@goodevilgenius

goodevilgenius Jun 1, 2018

But PSR-2 doesn't prohibit type-hinting primitives, so if the guideline is PSR-2, there's no specific reason to disallow it. If it is specifically disallowed, it should be mentioned in the link you just posted. If it's not specifically disallowed, there's no reason not to include them (type safety, in general, is a good thing).

*
* @throws \InvalidArgumentException
*/
protected function configuration($name)

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member
 protected function configuration(string $name)

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Apr 6, 2018

Member

Please don't add the string type hint.

* @param array $config
* @return \Swift_Transport
*/
protected function makeTransport($driver, $config)

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member
protected function makeTransport(string $driver, array $config)

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Apr 6, 2018

Member

Please don't add the string type hint.

* @param string $name
* @return void
*/
public function setDefaultConnection($name)

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member
public function setDefaultConnection(string $name)

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Apr 6, 2018

Member

No.

$this->registerIlluminateMailer();
$this->registerMarkdownRenderer();
}
/**
* Register the Illuminate mailer connection manager.
*

This comment has been minimized.

@TBlindaruk

TBlindaruk Apr 1, 2018

Member

Please add phpdoc

@see MailManager

This comment has been minimized.

@Propaganistas

Propaganistas Apr 7, 2018

Contributor

The framework uses @see only at class level.

@brunogaspar

This comment has been minimized.

Contributor

brunogaspar commented May 4, 2018

Hey @Propaganistas

This looks good overall, not sure if you plan on working on this a bit further so we can get this merged soon?

I've a question though, would this allow each connection to have it's send from.name and from.address instead of using the global one?
The idea would be to still have the global one though, but been able to override it per connection i guess.

@Propaganistas

This comment has been minimized.

Contributor

Propaganistas commented May 5, 2018

Hi @brunogaspar, I'm still waiting for some feedback as well. I think the core team wants to finish up some other stuff before merging this one.

Currently allowing each connection to specify its own from address isn't implemented but I really like the idea. Gonna take a shot at it. Thanks!

@brunogaspar

This comment has been minimized.

Contributor

brunogaspar commented May 10, 2018

@Propaganistas

Currently allowing each connection to specify its own from address isn't implemented but I really like the idea. Gonna take a shot at it. Thanks!

That's cool :)

I'm still waiting for some feedback as well.

I'm going to give this PR a try really soon and will leave some feedback after.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Jun 12, 2018

I think I'm going to hold off on this for now. I will try my own take at it sometime.

@jozeflambrecht

This comment has been minimized.

jozeflambrecht commented Nov 1, 2018

Is this still on the list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment