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

Sending notification to the topic without FCM registration token. #192

Open
roshanproductions opened this issue Apr 7, 2024 · 7 comments

Comments

@roshanproductions
Copy link

Hello,

Is it possible to send a notification to topics only? Currently, when I register routeNotificationForFcm in a Notifiable model and return empty string, it throws an error:

{
    "message": "No registration tokens provided",
    "exception": "Kreait\\Firebase\\Exception\\Messaging\\InvalidArgument",
    "file": "/home/forge/pashtostories.org/vendor/kreait/firebase-php/src/Firebase/Messaging/RegistrationTokens.php",
    "line": 61,
@dwightwatson
Copy link
Collaborator

No, not at this stage. Happy to review any ideas or PRs that implement this.

@mrcrg
Copy link

mrcrg commented May 2, 2024

I had the same problem and fixed by overriding send function in FcmChannel.php.

    public function send(mixed $notifiable, Notification $notification): ?Collection
    {
        // Firstly we create the message class
        /** @var \NotificationChannels\Fcm\FcmMessage */
        $fcmMessage = $notification->toFcm($notifiable);

        // If message has a topic then send it directly
        if (!is_null($fcmMessage->topic)) {
            return collect(($fcmMessage->client ?? $this->client)->send($fcmMessage));
        }

        // Otherwise collect tokens from the notifiable model
        $tokens = Arr::wrap($notifiable->routeNotificationFor('fcm', $notification));

        if (empty($tokens)) {
            return null;
        }

        return Collection::make($tokens)
            ->chunk(self::TOKENS_PER_REQUEST)
            ->map(fn ($tokens) => ($fcmMessage->client ?? $this->client)->sendMulticast($fcmMessage, $tokens->all()))
            ->map(fn (MulticastSendReport $report) => $this->checkReportForFailures($notifiable, $notification, $report));
    }

@dwightwatson Is this behaviour correct?

@dwightwatson
Copy link
Collaborator

I'm personally not familiar with sending notifications to topics without a token.

If it's as simple if checking if $fcmMessage->topic exists and then sending it and ignoring any user tokens I'm fine with that. (Although, it does almost feel like an anonymous notification at that point).

Like I said above, happy to look at any PRs that introduce this behaviour.

I think we should still do sendMulticast and check reports for failures like we do with FCM tokens as well for consistency.

@mrcrg
Copy link

mrcrg commented May 2, 2024

Checking if $fcmMessage->topic exists was a simple solution based on our specific use case.

We need to notify an indefinite number of users with the bare minimum load on the server.
In our APP every user has the option to subscribe/unsubscribe to a list of topics and then we send push notifications via the Topic model using the Notifiable trait, so it is not anonymous.

Another way to implement this could be: if routeNotificationForFcm returns a \Kreait\Firebase\Messaging\Topic class then we ignore tokens.

I can write a PR, just let me know how do you want to integrate it.

@dwightwatson
Copy link
Collaborator

Would it only ever return a single \Kreait\Firebase\Messaging\Topic or could it return a collection? Or should we use something like routeNotificationForFcmTopic to make it distinct?

Or does it make sense to have a separate channel for this - so you use FcmChannel for token based messages and FcmTopicChannel for other ones? That might be the cleanest option, but would it make sense from your point of view?

A few people liked your earlier comment - perhaps they can share their 2 cents/use cases so we can work out what a good implementation would look like.

@elceka
Copy link

elceka commented Jun 11, 2024

Hi, I solved this by creating a new channel in the form of the FcmTopicChannel class (it extending the FcmChannel class), it comes to me conceptually as a suitable solution. However, this is only solving one problem. The other problem is the actual sending within Laravel, though apparently you can't use Facade Notification (at least I haven't figured out a way) since we don't have a recipient (token accessible via notifiable) of the notification, I solved it by using the AnonymousNotifiable class.

FcmTopicChannel class

namespace Our\Project\Namespace\Notifications;

use Illuminate\Notifications\Notification;
use Kreait\Firebase\Exception\MessagingException;
use Kreait\Firebase\Messaging\Message;
use NotificationChannels\Fcm\Exceptions\CouldNotSendNotification;
use NotificationChannels\Fcm\FcmChannel;

/**
 * Class FcmTopicChannel
 *
 * @package Our\Project\Namespace\Notifications
 */
final class FcmTopicChannel extends FcmChannel
{
    /**
     * @param                                        $notifiable
     * @param \Illuminate\Notifications\Notification $notification
     *
     * @return array
     * @throws \Kreait\Firebase\Exception\FirebaseException
     * @throws \NotificationChannels\Fcm\Exceptions\CouldNotSendNotification
     */
    public function send($notifiable, Notification $notification)
    {
        $fcmMessage = $notification->toFcm($notifiable);

        if (! $fcmMessage instanceof Message) {
            throw CouldNotSendNotification::invalidMessage();
        }

        $responses = [];

        try {
            $responses[] = $this->messaging()->send($fcmMessage);
        } catch (MessagingException $exception) {
            $this->failedNotification($notifiable, $notification, $exception);
            throw CouldNotSendNotification::serviceRespondedWithAnError($exception);
        }

        return $responses;
    }
}

Sending by AnonymousNotifiable

resolve(AnonymousNotifiable::class)->notify(new OurCustomTopicNotificationClass($attribute));

@rc1021
Copy link

rc1021 commented Jun 28, 2024

I believe that for topics or tokens, a single transmission is sufficient.

    public function send($notifiable, Notification $notification)
    {
        // Get the message from the notification class
        $fcmMessage = $notification->toFcm($notifiable);

        if (! $fcmMessage instanceof Message) {
            throw CouldNotSendNotification::invalidMessage();
        }

        // send to topic or device
        $topic = $fcmMessage->getTopic();
        $token = empty($topic) ? $notifiable->routeNotificationFor('fcm', $notification) : null;

        try {

            // The target of the transmission must be a topic or a device.'
            if (empty($topic) && empty($token))
                throw new Exception('The target of the transmission must be a topic or a device.');

            $this->fcmProject = null;
            if (method_exists($notification, 'fcmProject')) {
                $this->fcmProject = $notification->fcmProject($notifiable, $fcmMessage);
            }

            $responses = [];

            if (is_array($token)) {
                // Use multicast when there are multiple recipients
                $partialTokens = array_chunk($token, self::MAX_TOKEN_PER_REQUEST, false);
                foreach ($partialTokens as $tokens) {
                    $responses[] = $this->sendToFcmMulticast($fcmMessage, $tokens);
                }
            } else {
                $responses[] = $this->sendToFcm($fcmMessage, $token);
            }
        } catch (MessagingException $exception) {
            $this->failedNotification($notifiable, $notification, $exception, $token);
            throw CouldNotSendNotification::serviceRespondedWithAnError($exception);
        }

        return $responses;
    }

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

No branches or pull requests

5 participants