search

jdavidbakr / mail-tracker

Package for Laravel to inject tracking code into outgoing emails.
MIT License
567 stars 129 forks source link

Multiple mailer support #185

Closed louis-l closed 2 years ago

louis-l commented 2 years ago

Hi,

Im using this library in an internal dashboard where we send emails using different mail drivers (e.g. SES, SMTP). SES is our default mail config, this line detects whether to call updateSesMessageId() or updateMessageId() https://github.com/jdavidbakr/mail-tracker/blob/6d5abc962142a1f05a011ffae37e5abf7be5035c/src/MailTracker.php#L53-L60

The issue is: when sending email via SMTP on-the-fly by calling Mail::mailer('smtp')->..., the logic above does not honour this and trying to access SES message ID and breaks.

I tried to take a look into MessageSent but could not find anything linked to the mailer that used to send the email.

Any ideas?

Cheers,

louis-l commented 2 years ago

Im thinking of this solution:

Im not sure if there is any better ideas than this?

jdavidbakr commented 2 years ago

I wonder if the we can make the driver an attribute of the MessageSent event if it's available when that event is fired?

louis-l commented 2 years ago

I wonder if the we can make the driver an attribute of the MessageSent event if it's available when that event is fired?

The MessageSent is a Laravel event, did you mean we need a PR to add mail driver as an attribute in that even? Or do you have different idea?

jdavidbakr commented 2 years ago

Oh, you're right, I was thinking that was my event for some reason. hmm.... maybe in updateSesMessageId we have a fallback to call updateMessageId if the X-SES-Message-Id header is empty?

louis-l commented 2 years ago

Oh, you're right, I was thinking that was my event for some reason. hmm.... maybe in updateSesMessageId we have a fallback to call updateMessageId if the X-SES-Message-Id header is empty?

I think we should change this line: https://github.com/jdavidbakr/mail-tracker/blob/6d5abc962142a1f05a011ffae37e5abf7be5035c/src/MailTracker.php#L55

It worked fine until Laravel supports multiple mail drivers. Maybe as you suggested: check the X-SES-Message-Id instead of config.

Do you want a PR for this?

jdavidbakr commented 2 years ago

That sounds good - yes, please if you have time to do a PR that would be great.

louis-l commented 2 years ago

No probs, will do that soon.

louis-l commented 2 years ago

@jdavidbakr

PR submitted https://github.com/jdavidbakr/mail-tracker/pull/186 Can you have a look when you have time?

I did not write any new tests for this as I thought its unnecessary and the new code does not change any current behaviour.