benwilkins / laravel-fcm-notification

Laravel FCM (Firebase Cloud Messaging) Notification Channel
MIT License
213 stars 91 forks source link

Make $notification available in routeNotificationForFcm #26

Closed chimit closed 5 years ago

benwilkins commented 5 years ago

Why do you need $notification here? The notifiable entity should just be determining where the notification is going, which shouldn't be dependent on the notification itself. Also, this would present breaking changes.

chimit commented 5 years ago

Because $notification is available in other delivery channels. In my case, I need it to filter recipients:

class Group extends Model
{
    use Notifiable;

    public function users()
    {
        return $this->belongsToMany('App\Models\User')
                    ->withTimestamps();
    }

    public function routeNotificationForFcm($notification)
    {
        return $this->load(['users' => function ($query) use ($notification) {
            // Do not send push notification to the author
            $query->where('users.id', '<>', $notification->user->id);
        }, 'users.devices'])
            ->users
            ->pluck('devices')
            ->collapse()
            ->pluck('fcm_token')
            ->all();
    }
}

I send notifications to groups of users instead of users to prevent multiple requests to the Firebase. Related to this issue: https://github.com/benwilkins/laravel-fcm-notification/issues/2

benwilkins commented 5 years ago

I'll approve it. It will have to be version 3.0 since it introduces breaking changes.