Skip to content

Conversation

@Jgmdevelopers
Copy link
Contributor

Hi.
I recreated this PR from a clean branch — the previous one accidentally included unrelated changes from my Android helper work.
This version only adds the intended feature: native topic-based sending support in FcmChannel.

When FcmMessage->topic is defined, it now calls $client->send() directly instead of sendMulticast(), which allows sending notifications to FCM topics without requiring registration tokens.

Everything else remains unchanged.
Let me know if you’d prefer keeping this logic here or moving it into a dedicated FcmTopicChannel in a future update.

@dwightwatson
Copy link
Collaborator

Thanks.

I don't fully understand the topic concept, but it seems to me personally that this doesn't really belong in the same channel. It basically exits early and doesn't use any of the existing channel code.

The key point of Laravel's notification system is that you are sending to a $notifiable object (generally a User) and that object provides the ways to communicate with it (email, address, phone numbers, push tokens).

I wonder if it makes more sense to implement this as an on-demand/anonymous notification where you would provide the topic when you dispatch it? Does that make sense to you?

Notification::route(FcmTopicChannel::class, 'topic-name')
    ->notify(new InvoicePaid($invoice));

@Jgmdevelopers
Copy link
Contributor Author

Hi Dwight,

Thanks for the explanation — it really helped clarify the design intent.

I implemented topic support inside FcmChannel based on the direction from the open issue, but I understand your point about keeping topic sends aligned with Laravel’s on-demand/anonymous notification flow.

If you prefer a separate FcmTopicChannel to keep things clean, I’m happy to update the PR in that direction. Just let me know and I’ll adjust it.

Thanks again,
Gabriel

@dwightwatson
Copy link
Collaborator

I think this needs to be an additional channel because the use case seems different.

Unless I'm wrong the "topic" is dynamic and belongs to the notification itself - not the notifies. There is no Eloquent-backed notifiable.

I think to explore this for the people that need topic basic notifications it's best for them to use a specific topic channel and either pull that topic from the message itself or an anonymous notifiable.

Does this sound reasonable to you? I'm not trying to be difficult but instead find the best place for this to fit given the use-case.

@Jgmdevelopers
Copy link
Contributor Author

Yes — that makes perfect sense.
Given the use-case and the fact that topics don't belong to a notifiable, I agree that a dedicated FcmTopicChannel is the cleanest and most consistent approach.

I'm going to adjust the PR to:

introduce a separate FcmTopicChannel

support topic-based messages pulled either from the message itself or via AnonymousNotifiable

leave the existing FcmChannel untouched and fully backward-compatible

Thanks for the clarification — this direction works well for me.

- Restored original FcmChannel without topic logic
- Added new FcmTopicChannel to handle topic-based sends
- Added CouldNotSendNotification exception
- Added full PHPUnit test suite for topic channel
- No breaking changes, fully backward compatible
@Jgmdevelopers
Copy link
Contributor Author

PR updated as discussed — FcmChannel restored to original behavior,
topic handling moved to dedicated FcmTopicChannel, including tests.

@dwightwatson
Copy link
Collaborator

Thanks - I really appreciate your effort here, however I've tackled the implementation here which includes events for failed notifications, documentation and some more test coverage.

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

Successfully merging this pull request may close these issues.

2 participants