Skip to content
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.4] Dispatch Illuminate\Mail\Events\MessageSent event #18744

Merged
merged 3 commits into from
Apr 10, 2017

Conversation

CyrilMazur
Copy link
Contributor

Related to proposal laravel/ideas#505. This PR dispatches a new Illuminate\Mail\Events\MessageSent event AFTER the message is actually sent (to the contrary of MessageSending which is dispatched BEFORE the message is sent).

Pros:

  • MessageSent is dispatched only if the message is successfully sent. It is not dispatched if the message isn't successfully sent (ie the Mailer throws an exception).
  • The $message variable contains the message id (SES) / transmission id (SparkPost) that's set into the swift message's headers after the message is sent.

I think the use case in the example from the official doc is a bit clumsy (https://laravel.com/docs/5.4/mail#events), because the listener for logging the message is executed whether the email is actually sent or not. There could be an error while actually sending the email (ex: network issue, temporary API error from Mandrill / SparkPost etc...) and MessageSending would still be triggered. MessageSent would be a better fit for this use case, because when it's triggered it's guaranteed the message was actually sent.

@tillkruss
Copy link
Contributor

tillkruss commented Apr 9, 2017

Would you mind adding a test?

@tillkruss tillkruss changed the title Dispatch Illuminate\Mail\Events\MessageSent event [5.4] Dispatch Illuminate\Mail\Events\MessageSent event Apr 9, 2017
@CyrilMazur
Copy link
Contributor Author

Done. The test checks that both MessageSending and MessageSent events are fired. I also handled the case when $this->events is null.

@taylorotwell taylorotwell merged commit 7cf5d6d into laravel:5.4 Apr 10, 2017
@CyrilMazur
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants