Notification Middleware is called twice

[kieranbrown]

• Laravel Version: v6.11.0
• PHP Version: 7.4.0
• Database Driver & Version: n/a

Description:

SendQueuedNotifications contains a middleware property and a middleware method through the Queueable trait, causing middleware to be called twice.

The NotificationSender will dispatch an instance of SendQueuedNotifications and pass the notification middleware with the through method. The through method exists on the Queueable trait, which adds the middleware to the property.

framework/src/Illuminate/Notifications/NotificationSender.php

Lines 193 to 204 in 8e189a8

	$this->bus->dispatch(
	(new SendQueuedNotifications($notifiable, $notification, [$channel]))
	->onConnection($notification->connection)
	->onQueue($notification->queue)
	->delay($notification->delay)
	->through(
	array_merge(
	method_exists($notification, 'middleware') ? $notification->middleware() : [],
	$notification->middleware ?? []
	)
	)
	);

The snippet below fires the middleware then eventually handles the sending of the notification. If you take a look at line 80, it's merging a middleware() method with a middleware property. This merge is what's actually causing duplication of middleware. Because through was called earlier on the SendQueuedNotifications class the property contains the middleware, but a middleware() method also exists which returns that property's data. Causing the array merge to contain an array of duplicates which then essentially means they'll be called twice.

framework/src/Illuminate/Queue/CallQueuedHandler.php

Lines 77 to 86 in 8e189a8

	protected function dispatchThroughMiddleware(Job $job, $command)
	{
	return (new Pipeline($this->container))->send($command)
	->through(array_merge(method_exists($command, 'middleware') ? $command->middleware() : [], $command->middleware ?? []))
	->then(function ($command) use ($job) {
	return $this->dispatcher->dispatchNow(
	$command, $this->resolveHandler($job, $command)
	);
	});
	}

Hopefully this makes sense, you should be able to follow the steps to replicate to see this issue.

Steps To Reproduce:

Create a Notification class, add a middleware() method that returns an example middleware. In the handle method simply put dump('called'). Notify a user and you should see this output twice.

I'm happy to submit a PR but really can't think of a nice solution for this, any thoughts?

[driesvints]

Why would you define/need both the property and method? Can you share some code?

[kieranbrown]

@driesvints thanks for the reply. I'm only defining the middleware method, not the property. The steps to replicate are rather simple, you should be able to see the middleware called twice.

[kieranbrown]

@driesvints Apologies I didn't see you asked for an example.

In this example, the handle method of SomeMiddleware is called 4 times rather than 2. The middleware should be called once per channel, but it's being called twice per channel.

It's a tricky issue to explain so apologies I'm not doing a very good job. Try out the notification middleware feature for yourself, put a dump('called') in the handle method and you should see the issue. Might make more sense if you see it for yourself.

<?php

declare(strict_types=1);

namespace App\Notifications;

use Illuminate\Bus\Queueable;
use Illuminate\Notifications\Notification;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Notifications\Messages\MailMessage;

class SomeClass extends Notification implements ShouldQueue
{
    use Queueable;

    /**
     * Get the middleware the job should be dispatched through.
     *
     * @return array
     */
    public function middleware()
    {
        return [new SomeMiddleware()];
    }

    /**
     * Get the mail representation of the notification.
     *
     * @param User $notifiable
     *
     * @return MailMessage
     */
    public function toMail($notifiable)
    {
        //
    }

    /**
     * Get the notification's delivery channels.
     *
     * @param User $notifiable
     *
     * @return array
     */
    public function via($notifiable)
    {
        return ['mail', 'database'];
    }
}

[ankurk91]

Are you using a queue or horizon or similar?

[kieranbrown]

Are you using a queue or horizon or similar?

I think this is an issue across all queue drivers, but at the moment i'm just using sync. Laravel horizon is not installed.

[driesvints]

A fix for this was merged.