[5.7] Fix an issue when using Mail::queue to queue Mailables

[axelitus]

The issue was found on the Illuminate\Mail\Mailer::queue method. The given queue name and the queue manager where used indistinctly which caused the error when calling the Illuminate\Mail\Mailable::queue method as it expected an Illuminate\Contracts\Queue\Factory object.

Fixes #27612

[axelitus]

I was against renaming the $this->queue property, as it is used through the whole queuing system through the Illuminate\Bus\Queueable interface, so it could create other problems. Also, I did not rename the method parameter to $queueName because $queue is used in other places to stand for the queue name, so keeping it gives consistency.

[mfn]

Is it possible to come up with a test for these two cases?

[driesvints]

There's indeed a bug but I'm unsure this implementation is twtg. Can you at least provide a test like @mfn suggested which proves that the behavior was fixed?

[axelitus]

Sure, I'll look into it. I wasn't sure about it either, but since the view object is the one responsible to hand out on which queue it is supposed to be queued, this was the best way to do it that didn't involved major refactors.

As far as I can tell this is equivalent to how I was used to queue mailables on specific queues:

$mailable = (new TestMailable)
    ->onQueue('my-queue');
Mail::queue($mailable);

And it shouldn't have affected the other way of setting the queue name directly in the mailable and having it implement the ShouldQueue interface.

The thing I'm not happy about is that having the statement:

Mail::queue(new TestMailable, 'my-queue');

I wouldn't expect any side effects on the mailable (setting the queue name) and instead having the mailer do all the nuances, but I don't think is possible without changing the underlying code relating to queues or duplicating chunks of it on the mailer.

[axelitus]

On the way to the office I was thinking about this an wondered what if we match the Mailer::queue signature to the one in Mailable::queue and make the changes to set the queue name in the Mailer::onQueue method?

This way we have consistency across the mail classes that handle queueing.

So we would end with this:

/**
 * Queue a new e-mail message for sending.
 *
 * @param  \Illuminate\Contracts\Mail\Mailable  $view
 * @param  \Illuminate\Contracts\Queue\Factory|null  $queue
 * @return mixed
 *
 * @throws \InvalidArgumentException
 */
public function queue($view, Queue $queue = null)
{
    if (! $view instanceof MailableContract) {
        throw new InvalidArgumentException('Only mailables may be queued.');
    }

    return $view->queue(is_null($queue) ? $this->queue : $queue);
}

/**
 * Queue a new e-mail message for sending on the given queue.
 *
 * @param  string  $queue
 * @param  \Illuminate\Contracts\Mail\Mailable  $view
 * @return mixed
 */
public function onQueue($queue, $view)
{
    return $this->queue($view->onQueue($queue));
}

And instead of calling Mailer::queue(new TestMailable, 'my-queue'); the correct call would be Mailer::onQueue('my-queue', new TestMailable);

I think this would be better and more consistent. What do you think?

[taylorotwell]

I think the solution you have here is fine. People really shouldn't even be calling Mail::later directly. Should be using ShouldQueue on the mailable.

[axelitus]

I agree that using ShouldQueue keeps things cleaner and more contained and it should be the way to go on most cases. But also sometimes the code benefits from explicit calls to be more understandable.