[7.x] Add plain mail to notifications

[Jubeki]

This pull request introduces the possibility to add a plain version to an html mail which are send via notifications.

Tools like SpamAssassin give a lower score if no plain version is attached to an email. So it would be better to make it possible to attach a plain version to notifications.

It is also still possible to send pure html emails without a plain version.

#33699 as issue

[driesvints]

Thanks for the PR @Jubeki!

[pawelmysior]

Hey @Jubeki nice work on the PR! However, it is now impossible to send plain text only notifications. And this was possible before by doing this:

public function toMail($notifiable)
{
    return (new MailMessage)
        ->view([
            'html' => null,
            'text' => 'mail.test.plain',
        ]);
}

@driesvints to me, this is unfortunately a breaking change. I've left some comments on the commit as to how it could be fixed. I'll try to write tests for this to make sure that my suggestions work and I'll submit a PR to fix this.

[driesvints]

Link to comments from @pawelmysior: f28bde3

@pawelmysior I don't think this is a breaking change since we didn't support text views officially before. The example you showed would definitely have been a workaround for it but nothing we intended to work like that. I do agree with you that it might be prudent to implement a way to only send text notifications as well.

For not it might be best that you fix the framework at the version before this change and indeed feel free to send in a PR 👍

[pawelmysior]

@pawelmysior I don't think this is a breaking change since we didn't support text views officially before. The example you showed would definitely have been a workaround for it but nothing we intended to work like that. I do agree with you that it might be prudent to implement a way to only send text notifications as well.

You're right, the example I show is definitely a bit of a hack :) calling it a breaking change was maybe a bit rush from me.

For not it might be best that you fix the framework at the version before this change and indeed feel free to send in a PR 👍

I'm not sure I understand correctly what you mean. What do you mean that I should fix the framework at the version before this change? Please elaborate. Thanks!

[driesvints]

Ah sorry, I explained badly: if you rely on the previous way of working you should set your framework version in your composer.json at v7.22.4 for now.

[pawelmysior]

Ah, yeah! I was thinking of the other meaning of "fix" :) I already did that. Thanks ;)

[stayallive]

I'm wondering where I got the idea of doing:

// Notice the array in the `view` method is a number indexed array of view names
return (new MailMessage)
    ->view(['emails.html', 'emails.plain'], $viewData);

This used to combine the HTML view and plain view in a single email.

The MailMessage still defined the $view as an string|array so I suppose this used to be possible and intended (I'm here because my apps broke).

framework/src/Illuminate/Notifications/Messages/MailMessage.php

Lines 13 to 18 in 0c93bf3

	/**
	* The view to be rendered.
	*
	* @var array|string
	*/
	public $view;

When I now pass an array it will fail ofcourse because the view name is supposed to be a string:

ErrorException: strpos() expects parameter 1 to be string, array given in /home/forge/app/vendor/laravel/framework/src/Illuminate/View/ViewName.php:17

Is this an unintended BC or was this never supposed to work?

[driesvints]

@stayallive I'm guessing #33803 by @pawelmysior fixes that?

[stayallive]

@driesvints no. Since the array of views is inside $this->views and that PR just adds keys to the array but still doesn't handle an array in $this->views and just assigns it to the html key which changes nothing with the problem I described.

[driesvints]

Ping @Jubeki @pawelmysior do any of you two know what we're missing here?

[robclancy]

Well this broke our stuff (already seems to be fixed in #33803, wish I saw that before I spent hours digging trying to find the issue). Semantic versioning but not really still, I see.

I don't even understand this PR. I have been sending both HTML and text emails for notifications since day 1. Reverted to 7.22.4 and checked, both emails formats work fine. This PR is just adding some "sugar" to do plain text and broke anyone using plain text already.

EDIT: actually the other PR doesn't fix anything either.

[robclancy]

@stayallive no your code was fine. That's how it has always worked for emails. This PR has broken that because before it just returned the array you defined which got set to $view.
This PR changed it so it either sent [$view] and the "fix" just makes it so it will be ['html' => $view]. Where $view in your case is an array.
So with this PR it's sending

[['emails.html', 'emails.plain'], null]

and the "fix" it's sending

['html' => ['emails.html', 'emails.plain'], 'text' => null]`.

This needs to be reverted. And the proper way to do it would be to not touch buildView because that breaks backwards compatibility and instead modify $message->view.
Or just not do this PR at all since all of this was already possible.

EDIT: Or do a check to see if $message->view is an array and just return that before doing the new stuff. It's also clear that this (laravel, not this PR) was lacking tests in general because this buildView change should have made existing tests fail.

[driesvints]

@robclancy you're correct. This was indeed untested and undocumented but it seems it worked this way. I'm sending in a PR to revert all of this but keep the tests and refactor them to the previous way that things worked.

Semantic versioning but not really still, I see.

Like you said this was untested and undocumented which is why this slipped through. These things happen unfortunately from time to time. Sorry this broke something for you and @stayallive. We're trying to do our best to keep these things from happening.

[driesvints]

Here's the PR: #33816

[pawelmysior]

Thanks @driesvints for reverting this, I think keeping it as it was before is a better approach that what was introduced in this PR and in my two subsequent ones.

[robclancy]

I just checked the mail documentation. The array way was part of the mail docs for 5.2 at least. The named keys I'm sure was also part of mail docs at some point (on my phone so just did a quick look). However the latest docs show a fluent way of doing it instead, that's where the main confusion has come from. So the mail docs (not just notification) should reference the array method.
Also since the way latest mail works is fluent like this PR I think the original idea here should also be done, just in a backwards compatible way.

The reason we knew how to do text emails is because this is originally how mail just worked so naturally we just did it on notification views.

[driesvints]

@robclancy check. We can definitely look at also implementing a fluent way that covers the existing use cases. If anyone wants to try a PR for that that keeps the current tests passing we can merge that.

[stayallive]

Thanks @driesvints, there are definitely possibilities to have the old and the new work together but this was a bit too bluntly (nl: kort door de bocht) like this.

Thanks for reverting. And thank you @robclancy for explaining it clearly 👍