The Wayback Machine - https://web.archive.org/web/20231128091739/https://github.com/laravel/framework/pull/45943
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

[11.x] Updates mail notifications to use mailables #45943

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joedixon
Copy link
Contributor

@joedixon joedixon commented Feb 3, 2023

For a while now, it has been a small pain point that it's not possible to construct my mail notifications using mailables as I would do for any other outbound email from my applications.

I think the reason for this is that these are supposed to be simple transacational type emails, which makes sense. However, I do think there is some benefit in using mailables under the hood.

It actually is possible to use mailables as the MailChannel does this check:

if ($message instanceof Mailable) {
    return $message->send($this->mailer);
}

$this->mailer->mailer($message->mailer ?? null)->send(
    $this->buildView($message),
    array_merge($message->data(), $this->additionalMessageData($notification)),
    $this->messageBuilder($notifiable, $notification, $message)
);

However, the return type of the toMail method suggests an instance of MailMessage should be returned. This will have an even bigger impact when this is a typed definition in Laravel 10 and I don't think it will be obvious to users that they can in fact return a Mailable object.


So what is this PR doing?

This PR makes the MailMessage class extend from the Mailable class - in doing so, removing a lot of code duplication between the two. Moving forward, mail notifications will inherit all of the new goodies we add to mailables moving forward.

It also means the toMail method can have a return type of Mailable and folks can return whichever email they like if there needs are different from the default.


Breaking changes

As far as I can tell, this causes a breaking change when using the to, cc, bcc and replyTo methods as the MailMessage and Mailable classes handled these slightly differently. You can see this in the tests I have updated in order to suit the new format.

If you think this PR is a good idea, I could probably update the MailMessage class to preserve backward compatibility, but my feeling was it's another place to maintain this functionality.

I also had to rename the public with method of the SimpleMessage class to withLine as it conflicts with the mailable and has a completely different use.

@driesvints
Copy link
Member

This all looks good to me. I feel like we should maybe consolidate how the to, cc, bcc and replyTo methods work but we could also just preserve BC.

@joedixon joedixon marked this pull request as ready for review February 3, 2023 15:03
@taylorotwell
Copy link
Member

Sorry - didn't have time to fully review and consider this before release.

@driesvints
Copy link
Member

@joedixon maybe re-open against master?

@joedixon joedixon reopened this Feb 15, 2023
@joedixon joedixon changed the base branch from 10.x to master February 15, 2023 14:22
@joedixon joedixon changed the title [10.x] Updates mail notifications to use mailables Updates mail notifications to use mailables Feb 15, 2023
@driesvints driesvints changed the title Updates mail notifications to use mailables [11.x] Updates mail notifications to use mailables Feb 16, 2023
@taylorotwell taylorotwell marked this pull request as draft February 16, 2023 21:38
@laravel laravel deleted a comment from Mastacow Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants