[11.x] Updates mail notifications to use mailables
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.
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.
Sorry - didn't have time to fully review and consider this before release.
@joedixon maybe re-open against master?
Is there any chance this can be merged into the 11 release?
This PR looks like it may solve an issue I've been trying to solve...looking forward to seeing it complete!
I have have been looking at trying to test mails that have been sent via a notification.
Currently, with the Notification Fake, it traps mails being sent - but you will not get coverage on mail generation (e.g. something like ResetPassword::createUrlUsing).
Having just the Mail fake will trap mails send by a Notification, but mails are not testable (e.g. assertSent is always 0 due to them not being Mailable and current Fake exiting as a result)
@driesvints and @taylorotwell will this be considered for Laravel 11? (Needs rebasing and fixing conflicts, not sure if much was added to both classes during Laravel 10)
Maybe as a more general question: When will be the feature / BC freeze?