Laravel 10.41.0 notifications fail to send

[SimondeJong-VinvuSoftware]

Laravel Version

10.41.0

PHP Version

8.2.2

Database Driver & Version

SQLite 3.43.2

Description

After updating Laravel from v10.32.1 to v10.42.0, my unit tests focussing on the sending of a notification fail. It seems the notifications are not sent. The unit test passes without issues on v10.32.1. Thanks in advance for any help!

Steps To Reproduce

An example of a failing unit test is:

public function test_resend_setup_account_notification_sent(): void
{
    $actingAs = User::where('name', 'User')->first();

    NotificationSupport::fake();
    $user = User::factory()->create(['admin' => false]);
    $user->password = null;
    $user->email_verified_at = null;
    $user->save();

    $response = $this->json('GET', "api/v2/admin/users/{$user->id}/invite", [], [
        'Authorization' => 'Bearer '.$this->getBearerTokenUser($actingAs),
    ]);

    $response->assertJsonFragment([
        'status' => 'Setup account notification resent',
        'user_id' => $user->id,
        'user_name' => $user->name,
    ])->assertStatus(200);

    Notification::assertSentTo([$user], SetupAccountNotification::class);
}

And example of the controller method is as follows:

public function resendInvite(User $user) 
{
    $user->notify(new SetupAccountNotification($user));
    
    return response()->json([
                'status' => 'Setup account notification resent',
                'user_id' => $user->id,
                'user_name' => $user->name,
            ], 200); 
}

All unit tests which test the sending of notifications seem to fail.

[driesvints]

Hit @SimondeJong-VinvuSoftware. Could you maybe pinpoint the exact version which causes this for you to fail?

[SimondeJong-VinvuSoftware]

Hi @driesvints ,

Thanks for your help. It seems the issue arrises from v10.33.0 and higher

[SimondeJong-VinvuSoftware]

@driesvints ,

I found the specific commit which seems to be breaking; 4bc11bb.

The function sendNow is implemented as follows in v10.33.0:

 public function sendNow($notifiables, $notification, array $channels = null)
    {
        if (! $notifiables instanceof Collection && ! is_array($notifiables)) {
            $notifiables = [$notifiables];
        }

        foreach ($notifiables as $notifiable) {
            if (! $notification->id) {
                $notification->id = Str::uuid()->toString();
            }

            $notifiableChannels = $channels ?: $notification->via($notifiable);

            if (method_exists($notification, 'shouldSend')) {
                $notifiableChannels = array_filter(
                    $notifiableChannels,
                    fn ($channel) => $notification->shouldSend($notifiable, $channel) !== false
                );
            }

            if (empty($notifiableChannels)) {
                continue;
            }

            $this->notifications[get_class($notifiable)][$notifiable->getKey()][get_class($notification)][] = [
                'notification' => $notification,
                'channels' => $notifiableChannels,
                'notifiable' => $notifiable,
                'locale' => $notification->locale ?? $this->locale ?? value(function () use ($notifiable) {
                    if ($notifiable instanceof HasLocalePreference) {
                        return $notifiable->preferredLocale();
                    }
                }),
            ];
        }
    }

This means when the shouldSend method is not implemented the notification will never be sent when faked.
I believe the if statement empty($notifiableChannels) should be nested in the if statement above in order to resolve this issue. This would adapt the function to be as follows:

public function sendNow($notifiables, $notification, array $channels = null)
    {
        if (! $notifiables instanceof Collection && ! is_array($notifiables)) {
            $notifiables = [$notifiables];
        }

        foreach ($notifiables as $notifiable) {
            if (! $notification->id) {
                $notification->id = Str::uuid()->toString();
            }

            $notifiableChannels = $channels ?: $notification->via($notifiable);

            if (method_exists($notification, 'shouldSend')) {
                $notifiableChannels = array_filter(
                    $notifiableChannels,
                    fn ($channel) => $notification->shouldSend($notifiable, $channel) !== false
                );

                if (empty($notifiableChannels)) {
                    continue;
                }
            }

            $this->notifications[get_class($notifiable)][$notifiable->getKey()][get_class($notification)][] = [
                'notification' => $notification,
                'channels' => $notifiableChannels,
                'notifiable' => $notifiable,
                'locale' => $notification->locale ?? $this->locale ?? value(function () use ($notifiable) {
                    if ($notifiable instanceof HasLocalePreference) {
                        return $notifiable->preferredLocale();
                    }
                }),
            ];
        }
    }

[driesvints]

So I suspected that PR was the culprit as I faced a similar issue while working on laravelio/laravel.io#979

@SimondeJong-VinvuSoftware but it turns out in the end the PR was correct. If there's no channels returned from the notification then there's no notifications to be sent. In fact, the previous behaviour was a faulty one and perhaps something people like you and me relied on on a false note.

I don't know how your notifications look like but you should make sure they send back the correct channels through the via method.

[SimondeJong-VinvuSoftware]

@driesvints , Thanks for your help. You are indeed correct that I relied on the incorrect behaviour. I will update my notifications to fix the issue.