Framework: Notification doesn't add Mailables to the queue

[hanspagel]

• Laravel Version: 5.7.27
• PHP Version: 7.3.2
• Database Driver & Version: Postgres 10.3

Description:

I want to delay emails sent from a notification, so I tried to add ->delay() but emails get send out instantly.

Steps To Reproduce:

Add this to your notification:

    public function toMail($notifiable)
    {
        return (new NewCommentMail($notifiable, $this->comment))
            ->delay(now()->addMinutes(10));
    }

I use Redis as a queue driver and Horizon as the queue worker. The Mailable implements the ShouldQueue contract.

Am I on the wrong track or is there any workaround?

[driesvints]

I'll need more info and/or code to debug this further. Please post relevant code like models, jobs, commands, notifications, events, listeners, controller methods, routes, etc. You may use https://paste.laravel.io to post larger snippets or just reply with shorter code snippets. Thanks!

[hanspagel]

Hey Dries! Thanks for checking in! Here is the relevant notification class and an example mailable class. Hope that helps!

app/Notifications/NewComment.php

<?php

namespace App\Notifications;

use Illuminate\Bus\Queueable;
use App\Mail\NewComment as NewCommentMail;
use Illuminate\Notifications\Notification;
use Illuminate\Contracts\Queue\ShouldQueue;

class NewComment extends Notification implements ShouldQueue
{
    use Queueable;

    /**
    * Get the mail representation of the notification.
    *
    * @param  mixed  $notifiable
    * @return \Illuminate\Notifications\Messages\MailMessage
    */
    public function toMail($notifiable)
    {
        return (new NewCommentMail($notifiable))
            ->delay(now()->addMinutes(10));
    }
}

app/Mail/NewComment.php

<?php

namespace App\Mail;

use App\Models\User;
use Illuminate\Bus\Queueable;
use Illuminate\Mail\Mailable;
use Illuminate\Queue\SerializesModels;
use Illuminate\Contracts\Queue\ShouldQueue;

class NewComment extends Mailable implements ShouldQueue
{
    use Queueable,
        SerializesModels;

    public $comment;

    /**
     * Create a new message instance.
     *
     * @return void
     */
    public function __construct(User $user)
    {
        $this->user = $user;
    }

    /**
     * Build the message.
     *
     * @return $this
     */
    public function build()
    {
        return $this
            ->to($this->user->email)
            ->subject("New Comment")
            ->view('emails.new-comment');
    }
}

[hanspagel]

Digging a little deeper: The MailChannel takes a Mailable and executes send() on it [1], but send() destructs the object in its details (path to template, data and so on) [2]. So it can't take into account that the ShouldQueue Contract is implemented.

[1] Illuminate\Notifications\Channels\MailChannel

if ($message instanceof Mailable) {
    return $message->send($this->mailer);
}

[2] Illuminate\Mail\Mailable

public function send(MailerContract $mailer)
{
    $this->withLocale($this->locale, function () use ($mailer) {
        Container::getInstance()->call([$this, 'build']);

        $mailer->send($this->buildView(), $this->buildViewData(), function ($message) {
            $this->buildFrom($message)
                    ->buildRecipients($message)
                    ->buildSubject($message)
                    ->runCallbacks($message)
                    ->buildAttachments($message);
        });
    });
}

If I understand the problem correctly, the solution is to extend the MailChannel [1] to check for the ShouldQueue Contract. What do you think?

[driesvints]

I believe you're right. If you send in a fix for this then please also add a test.

[ghost]

Shouldn't you use ->delay () on notify instead of in the toMail function, like in the docs?

$when = now()->addMinutes(10);

$user->notify((new InvoicePaid($invoice))->delay($when));

https://laravel.com/docs/5.7/notifications#queueing-notifications

[driesvints]

ah I believe @GeraldPK is right. @hanspagel can you try this instead?

[hanspagel]

But that's a different use case. What I want to archieve is that a user gets the (database) notification instantly, but the email delayed. So I can cancel the email when the user is online and has read the (database) notification already. Makes sense?

[driesvints]

Yes, that actually makes sense 👍

If you send in a PR definitely add this to your explanation as an example.

[designvoid]

But that's a different use case. What I want to archieve is that a user gets the (database) notification instantly, but the email delayed. So I can cancel the email when the user is online and has read the (database) notification already. Makes sense?

I am facing the same issue - is this intended behaviour? I don't want to queue the Notification, just the Mailable by implementing ShouldQueue on it

[driesvints]

I'm going to close this because I feel this is more of a feature request than an actual bug. If anyone's willing to whip up a PR you're free to send one in. Thanks.

[heikokrebs]

@driesvints i don't think this is a feature request but really a bug.
When using mailables from a toMail() in a notification, it is not possible to queue them like it is implemented in the mailable class, to have the ShouldQueue interface respected.

[pmatisko]

I also believe this is a bug. Here is a workaround that seems working fine:

$job = function() use ($user) { $user->notify((new SendNewMessage($user, 'message'))->onQueue('emails')); }; dispatch($job);