framework icon indicating copy to clipboard operation
framework copied to clipboard

[11.x] Updates mail notifications to use mailables

Open joedixon opened this issue 2 years ago • 6 comments

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.

joedixon avatar Feb 03 '23 14:02 joedixon

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.

driesvints avatar Feb 03 '23 14:02 driesvints

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

taylorotwell avatar Feb 14 '23 21:02 taylorotwell

@joedixon maybe re-open against master?

driesvints avatar Feb 15 '23 09:02 driesvints

Is there any chance this can be merged into the 11 release?

Alex-S-Davies avatar Jan 14 '24 15:01 Alex-S-Davies

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)

oniice avatar Jan 22 '24 20:01 oniice

@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?

Jubeki avatar Feb 21 '24 08:02 Jubeki