Framework: Illuminate\Mailer (or facade) should drop `to()` function due to misleading mutable behavior

[icaroscherma]

• Laravel Version: 5.x or newer
• PHP Version: Any

Description:

I recently ran into an issue that multiple emails were sent to the same people and I couldn't predict that by just looking into my code. The Mailer object mutates the Mailable object when you set the recipients To, CC, BCC, etc.
In the example below I assigned one task to multiple users, and for each user, I sent the mailable object through the mailer class without re-creating each time the mailable object, the result was the first user got the $user->count() of emails, 2nd got $user->count() - 1 of emails and the last user only got one email, but all of them were into the To: field on the last email.
I believe that we should drop the to() function of the mailer or make it immutable in a way that the to() will only apply the To: field when sending the current one, not changing the mailable object.

Steps To Reproduce:

$task = App\Task::find(1);

// Undesired behaviour
$mailable = new App\Mail\TaskCreated($task);
foreach($task->users as $user) {
    Illuminate\Support\Facades\Mail::to($user->email)
        ->send($mailable);
}

// Desired behaviour using the current way
foreach($task->users as $user) {
    $mailable = new App\Mail\TaskCreated($task)
        ->to($user->email);
    Illuminate\Support\Facades\Mail::send($mailable);
}

[driesvints]

Did you actually test this on 6.x or 7.x?

[icaroscherma]

Yes, I ran into this issue with Laravel v6.18 and PHP 7.3.

[Lloople]

I ran into this a few weeks ago too. In my opinion the to property should be cleaned up after each send to preserve backward compatibility

[icaroscherma]

I ran into this a few weeks ago too. In my opinion the to property should be cleaned up after each send to preserve backward compatibility

This is an easy approach / fix, but IMHO the $mailable object should be immutable otherwise we can't predict any behaviour. Or at least on the Laravel Docs, the to() function should be addressed on the $mailable not the \Mail or Mailer object, because it's easier to predict an object change if we run a function on the object itself. I didn't even thought about this because I was using Mail::to($email)->send($mailable) not something like Mail::send($mailable->to($email));

[driesvints]

I just tried this out with your example and the emails are all sent separately, no duplicate sends. If you can provide full steps to reproduce or a repo with an example (please commit custom changes separately) then we can have another look.

[Lloople]

@driesvints The sending is not duplicated, but if you have this array

$mails = ['[email protected]', '[email protected]', '[email protected]'];

Then the user b can see that a received the same email, and c can see both a and b received the same email, but they are not receiving it again.

[driesvints]

@Lloople can you please provide full steps to reproduce?

[Lloople]

@driesvints Here you go. I also attach a screenshot of my mailtrap output with the received mails.

Psy Shell v0.10.2 (PHP 7.3.14 — cli) by Justin Hileman
>>> $mails = ['[email protected]', '[email protected]', '[email protected]'];
=> [
     "[email protected]",
     "[email protected]",
     "[email protected]",
   ]
>>> $mailable = new App\Mail\DefaultMail("subject", "this is the content");
=> App\Mail\DefaultMail {#4085
     +subject: "subject",
     +content: "this is the content",
     +locale: null,
     +from: [],
     +to: [],
     +cc: [],
     +bcc: [],
     +replyTo: [],
     +view: null,
     +textView: null,
     +viewData: [],
     +attachments: [],
     +rawAttachments: [],
     +diskAttachments: [],
     +callbacks: [],
     +mailer: null,
     +connection: null,
     +queue: null,
     +chainConnection: null,
     +chainQueue: null,
     +delay: null,
     +middleware: [],
     +chained: [],
   }
>>> foreach ($mails as $mail) { Mail::to($mail)->send($mailable); }
>>> 

[driesvints]

I just realized what the problem is. You're creating the mailable outside the for loop which will indeed append new recipients. This is expected.

Try the following:

foreach($task->users as $user) {
    $mailable = new App\Mail\TaskCreated($task);

    Illuminate\Support\Facades\Mail::to($user->email)
        ->send($mailable);
}

We can't change this because imagine you're explicitly expecting this to happen:

$mailable = new App\Mail\TaskCreated($task)

foreach($task->users as $user) {
    $mailable->to[] = $user->email;
    Illuminate\Support\Facades\Mail::send($mailable);
}

That wouldn't work anymore if you reset the recipients each time.

[Lloople]

Yes, we are aware of what the solution is, but it's not the desired behavior.

Then naming the method addTo would be more convenient because the name of the method is not explicit enough to understand what's happening under the hood.

Also, since the to is used in the Mail facade, it's not evident that it will be stored inside the $mailable object.

Having to and addTo would be great to cover both cases.

[taylorotwell]

I do find this behavior a bit unexpected. I would entertain a PR to PendingMail@fill to explore options for making this a behave a bit more as one would expect.

[driesvints]

I'll re-open this for now.

[RakibRizan]

Hi I have an issue about verification mail sending laravel 7x

[driesvints]

I've tried re-exploring this but I believe there isn't any sensible way to solve this properly. We'll add a note to the docs that mailables will need to be created fresh every time.

Anyone's free to send in a PR if they feel they can solve this in a non-breaking way. Thanks.

[driesvints]

Here's the pr: https://github.com/laravel/docs/pull/5994

[Lloople]

Anyone's free to send in a PR if they feel they can solve this in a non-breaking way. Thanks.

Why has it to be in a non-breaking way? I’m completely fine if this is fixed in Laravel 8 or 9

[driesvints]

Because we still need to support recipients being set inside the mailable itself as well. Imagine you're setting a bcc address in the build method.

[Lloople]

I’m talking without knowing much of the internal behavior right now. But a possible solution would be resetting both (Facade and Mailable) at the end of the send method.

[driesvints]

@Lloople definitely feel free to attempt a pr with that if you believe that's possible.