Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On-Demand Notifications route not respected, seems is overwritten by the via notification class method #33408

Closed
illambo opened this issue Jul 2, 2020 · 4 comments
Labels

Comments

@illambo
Copy link

illambo commented Jul 2, 2020

  • Laravel Version: 6.18.23
  • PHP Version: 7.2.22
  • Database Driver & Version: *

Description:

On-Demand Notifications route not respected, seems is overwritten by the via notification class method

Steps To Reproduce:

  • run php artisan make:notification Test
  • set via array to database and mail
<?php

namespace App\Notifications;

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

class Test extends Notification
{
    use Queueable;

    /**
     * Create a new notification instance.
     *
     * @return void
     */
    public function __construct()
    {
        //
    }

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

    /**
     * Get the mail representation of the notification.
     *
     * @param  mixed  $notifiable
     * @return \Illuminate\Notifications\Messages\MailMessage
     */
    public function toMail($notifiable)
    {
        return (new MailMessage)
                    ->line('The introduction to the notification.')
                    ->action('Notification Action', url('/'))
                    ->line('Thank you for using our application!');
    }

    /**
     * Get the array representation of the notification.
     *
     * @param  mixed  $notifiable
     * @return array
     */
    public function toArray($notifiable)
    {
        return [
            //
        ];
    }
}
  • test with php artisan (with mail driver log)
    php artisan tinker
    \Illuminate\Support\Facades\Notification::route('mail', ['[email protected]', '[email protected]'])->notify(new \App\Notifications\Test);

  • check errors -> attempt to create data to db

  • another try with empty via array

/**
     * Get the notification's delivery channels.
     *
     * @param  mixed  $notifiable
     * @return array
     */
    public function via($notifiable)
    {
        return [];
    }
  • check result log -> no message sent

Example of my current workaround:

/**
     * Get the notification's delivery channels.
     *
     * @param  mixed  $notifiable
     * @return array
     */
    public function via($notifiable)
    {
        return ($notifiable instanceof \Illuminate\Notifications\AnonymousNotifiable) ?
            array_keys($notifiable->routes) : ['database', 'mail'];
    }

Do you have any suggestions? am I wrong?

:) Thanks for the support!

@driesvints
Copy link
Member

You can't have an empty via. And when you use an on demand notification it'll limit it to the one channel you're sending.

@illambo
Copy link
Author

illambo commented Jul 2, 2020

@driesvints thanks for the reply but can u check this steps (via array set to ['database', 'mail']):

  • test with php artisan (with mail driver log)

php artisan tinker
\Illuminate\Support\Facades\Notification::route('mail', ['[email protected]', '[email protected]'])->notify(new \App\Notifications\Test);

  • check errors -> attempt to create data to db

Why try to create data to database channel if route asked is mail ?

Thanks

@driesvints
Copy link
Member

Hey @illambo. I tried that scenario and also get the error of the database channel. In general I think it would make sense for us to prevent using the database channel for anonymous notifiables. I'll try to whip up a pr for this. Thanks for reporting.

@driesvints
Copy link
Member

Sent in a PR here: #33409

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants