laravel-notification-channels / fcm

Firebase Cloud Messaging (FCM) notifications channel for Laravel
https://laravel-notification-channels.com/
MIT License
495 stars 127 forks source link

Fix support for push notifications to topics and conditions #160

Closed gdebrauwer closed 1 year ago

gdebrauwer commented 1 year ago

Currently, sending push notifications to a topic or conditions is impossible.

Notification::route(FcmChannel::class, null)->notify(new SomePushNotification());

// -----

class SomePushNotification extends Notification
{
    public function via() : array
    {
        return [FcmChannel::class];
    }

    public function toFcm()
    {
        return FcmMessage::create()
            ->setNotification(
                \NotificationChannels\Fcm\Resources\Notification::create()
                    ->setTitle('hello world!')
                    ->setBody('this is a push notification sent from the server to a firebase topic')
            )
            ->setTopic('foo');
    }
}

This is caused by the fact that the FcmChannel class immediately aborts if there are no tokens: https://github.com/laravel-notification-channels/fcm/blob/fa5e63d167f2d4a6e8ce01a2c8ea58553681e84e/src/FcmChannel.php#L46-L52

This PR fixes that.

dwightwatson commented 1 year ago

While I get this in principle this isn't really a bugfix, rather a change in functionality. The early exit for having no tokens is over 3 years old.

I feel also that this is unconventional compared to other notification drivers. Even Laravel's Vonage driver exits early without a destinaton.

I appreciate your use-case but I wonder if perhaps this should be a different channel altogether - FcmMessageChannel and FcmTopicChannel (?) or instead just code you keep in your app.

gdebrauwer commented 1 year ago

I feel also that this is unconventional compared to other notification drivers. Even Laravel's Vonage driver exits early without a destination.

I understand that, but when using a topic or condition you just don't have a destination so that makes it a bit different than normal drivers.

The changes in this PR also do not cause any breaking changes. If you do not provide a destination and also no topic or condition, then it will still do nothing and just return an empty array

dwightwatson commented 1 year ago

I understand it's not a breaking change, but there was work in #146 to greatly simplify this method and always use multicasting. I think it's weird that this happens first, regardless of if there are tokens or not. I'd be happy to consider this as a separate channel so that it's use can be explicit.