Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

[Proposal] Ability to define Notifications broadcast channel name #202

Closed
mikemclin opened this issue Sep 11, 2016 · 32 comments
Closed

[Proposal] Ability to define Notifications broadcast channel name #202

mikemclin opened this issue Sep 11, 2016 · 32 comments

Comments

@mikemclin
Copy link

Currently the Notifications broadcast channel broadcasts to a dot-separated, capitalized version of the notifiable's namespace (e.g. App.User). However all of the example channel names in the docs seem to follow a lowercased basename format (e.g. user).

Obviously most people will want a consistent naming strategy for their channel names. Personally, I prefer the convention in the docs, because my client app doesn't need to know about my model paths in my API.

Anyways, it would be great if there was a hook of some sort, so we could set the channel name for a notification.

@michaeldyrynda
Copy link

michaeldyrynda commented Sep 11, 2016

You can do this by returning a custom name from the broadcastAs method in your event class.

@mikemclin
Copy link
Author

mikemclin commented Sep 11, 2016

@michaeldyrynda That is correct. But, I am talking about broadcasts from notifications, not events.

The channel is set via the Illuminate\Notifications\Events\BroadcastNotificationCreated::broadcastOn() method. Even though the method is public, we never get access to the instantiated object. The BroadcastNotificationCreated object is created by the Illuminate\Notifications\Channels\BroadcastChannel class. There are no hooks or ways in.

@michaeldyrynda
Copy link

Ah yes of course. I suppose it would be trivial to add a broadcastAs method within notifications that is the same as in events, then just update the BroadcastNotificationCreated@channelName method to check the event for the presence of a public broadcastAs method and return that.

It makes sense to have a consistent API.

@fgambino
Copy link

I created an issue for this a couple weeks ago, but I'm glad I'm not the only one who sees limitations with this design.

#162

@mikemclin
Copy link
Author

mikemclin commented Sep 12, 2016

@fgambino sorry, I missed that. I think your issue title might be poorly named, and possibly why it hasn't gained traction. "Allow custom notification type and channel names". Now, I see from the issue content, that you are actually wanting custom broadcast notification channel names, not notification channel names.

@fgambino
Copy link

@mikemclin no apology necessary. I can definitely see the confusion there. We can continue discussion in this issue!

What if a routeNotificationForBroadcast() method was added to the notifiable model/user? That would keep things consistent with how other routes are customized (email, slack, etc).

@stevethomas
Copy link

stevethomas commented Oct 8, 2016

I think the routeNotificationForBroadcast() on the notifiable model would be better as broadcastOn() is a little ambiguous on a Notification and the route..() convention has already been set for other notification types.

The biggest issue I have with not being able to specify a notification channel name is that it creates overhead for every additional channel I need to listen to; for example if I am listening to 30 widgets that's 30 channel auth callbacks.

I think a common approach would be to broadcast all/some activity onto the current User channel and delegate it from there. I'm not convinced that this is a great design pattern either, but it has to be better than dozens of auth callbacks.

@jamesgraham
Copy link

Is there a current workaround for this?

@olivernybroe
Copy link

Isn' this what the receivesBroadcastNotificationsOn() is for.

@SvenSchoene
Copy link

This is still unsolved: it is possible to overwrite the name of the channel that a Notification is broadcasting on. However, it is not possible to overwrite the name of the event that the Notification is sending.

The code for overriding this on an Event with broadcastOn() is here: https://github.com/illuminate/broadcasting/blob/master/BroadcastEvent.php#L43

The class where broadcastAs() should be overriden for a Notification should bere here: https://github.com/illuminate/notifications/blob/5.3/Events/BroadcastNotificationCreated.php#L10

In fact, if one adds the following code to the class:

    public function broadcastAs()
    {
        return 'lol';
    }

A new event with the name lol will be sent to e.g. Pusher.

I'll keep looking, but currently I see no way to configure this from the Notification.

@Uruloke: receivesBroadcastNotificationsOn() is the method that one puts on a Notifiable in order to override the name of the channel, and not the name of the event.

@olivernybroe
Copy link

You can do this by extending the broadcasters broadcast method, so you have a custom broadcaster driver which extend eg. PusherBroadcsster and overrides the data sent to the parent broadcast method. It's a few lines of codes, and works great.

@SvenSchoene
Copy link

SvenSchoene commented Feb 17, 2018

@Uruloke: You sir, are a genius! With my new custom broadcast driver I can now add a broadcastAs()-method on my Notifications. Thank you very much! 👍

@olivernybroe
Copy link

@SvenSchoene, no problem, had that same problem and spend some time decoding how the framework did it so I could customize it.

@ALL: Anyone else interested in this feature, because we should be able to quickly create a pull request for this.

@antonkomarev
Copy link

antonkomarev commented Mar 1, 2018

@SvenSchoene you can define event name since v5.6.4 just return custom event name in broadcastType method of your notification. It was implemented in PR laravel/framework#23236

I suppose this issue could be closed since all the questions which were raised here are solved.

@SvenSchoene
Copy link

@a-komarev Absolutely perfect, thx! 👍

@cadukiz
Copy link

cadukiz commented Mar 21, 2018

Hello @SvenSchoene could you send an example of this solution:

"You sir, are a genius! With my new custom broadcast driver I can now add a broadcastAs()-method on my Notifications. Thank you very much!"

Because, I couldn't make it. Thanks a lot

@antonkomarev
Copy link

@cadukiz read two coments above: #202 (comment)
There is a link on a PR where changes were made. All the changes we completed with tests, so you could always look how its working in tests.

@cadukiz
Copy link

cadukiz commented Mar 21, 2018

@a-komarev Thanks for the answer, but I need to change the broadcast event name from BroadcastNotificationCreated to MyNewEvent using broadcastAs() method. But from Notification.

It was made : laravel/framework@6f28b66

But overwritten again with broadcastType().

My suggestion: keep both:
broadcastType() - for set type into event. (Already done)
broadcastAs() - for set custom Event name eg.(BroadcastNotificationCreated to MyCustomEvent):

@cadukiz
Copy link

cadukiz commented Mar 21, 2018

public function broadcastAs()
{
if (method_exists($this->notification, 'broadcastAs')) {
return $this->notification->broadcastAs();
}

   return get_class($this);  //  return default: BroadcastNotificationCreated

}

@jonyw4
Copy link

jonyw4 commented Sep 18, 2018

I have the same problem here too +1
broadcastAs is not working!

@OleksandrVladymyrov
Copy link

@taylorotwell @antonkomarev what happened?
broadcastAs is not working!

@antonkomarev
Copy link

@OleksandrVladymyrov method was renamed later laravel/framework@4227bd7

@jeanlucnguyen
Copy link

Same problem here. I'd like to change the name of the event being broadcasted:
From Illuminate\Notifications\Events\BroadcastNotificationCreated -> to MyCustomEvent

What we can do currently is only change the type in the payload of the event being broadcasted, not the name of the event itself.

When we broadcast from an Event in the app\Events folder, the name of the Event is the event itself, but there's no type in the payload unless we define it manually.
When we broadcast from a Notification, the name of the Event is always BroadcastNotificationCreated and we can only change the type in the payload.
It would be good to standardize broadcasting data being sent.

@drkwolf
Copy link

drkwolf commented Jun 24, 2019

a quick fix for this, until the patch is accepted.

exclude the current implementation then import yours

    "autoload": {
        "exclude-from-classmap": [
            "vendor/laravel/framework/src/Illuminate/Notifications/BroadcastNotificationCreated.php"
        ],
        "psr-4": {
            "App\\": "app/",
            "Illuminate\\Notifications\\": "app/libs/Illuminate/Notifications/"
        }
    },

cp vendor/laravel/framework/src/Illuminate/Notifications/BroadcastNotificationCreated.php app/libs/Illuminate/Notifications/

then add this to app/libs/Illuminate/Notifications/BroadcastNotificationCreated.php

    //patch
    public function broadcastAs()
    {
        if (method_exists($this->notification, 'broadcastAs')) {
            return $this->notification->broadcastAs();
        }
        return get_class($this); 
    }
composer dump-autoload

@infostreams
Copy link

Alternatively you can just override the 'broadcastOn' method in the Notification class.

@streamingsystems
Copy link

streamingsystems commented Jun 29, 2020

Hi All,

I spent hours yesterday trying to figure this out as well as I am trying to implement the broadcastAs in my Notification because it keeps sending down the BroadcastCreatedNotification class name but I want to customize it like I do for my other messages (that are being sent not through the notification system).

laravel/framework#33372

After looking through this thread, at this point is there any solution or workarounds?

I appreciate your response as I new to Laravel (6 months) and learning as I go.

Have a great day.

@streamingsystems
Copy link

You can do this by extending the broadcasters broadcast method, so you have a custom broadcaster driver which extend eg. PusherBroadcsster and overrides the data sent to the parent broadcast method. It's a few lines of codes, and works great.

Hi,

Do you think you might take a minute and provide a code snippet on the few lines of code that would allow me to extend the broadcaster driver? This is exactly what I need. Thanks!

@sahilxjain
Copy link

Is there any way to just use the ShouldBroadcastNow on notification since i cannot extend BroadcastNotificationCreated event ?

@Yiidiir
Copy link

Yiidiir commented Sep 8, 2020

I isn't solved yet :( , how can we extend the broadcast driver? @olivernybroe

@viaativaweb
Copy link

Damn, 4 years and we still cannot change the notification class event name from Illuminate\Notifications\Events\BroadcastNotificationCreated . The 'solution' is to just overwrite everything, lol, I'd rather do an if(payload.type == 'event') instead if it comes to that.

@marciodsousa
Copy link

So, there was a problem reported 4 years ago, it was fixed at some point, and then un-fixed and has been staying like that since...?

In what case does it make sense to send the event name as Illuminate\Notifications\Events\BroadcastNotificationCreated anywhere? This should be a really obvious thing to fix

@KenGuggelmeyer
Copy link

So, there was a problem reported 4 years ago, it was fixed at some point, and then un-fixed and has been staying like that since...?

In what case does it make sense to send the event name as Illuminate\Notifications\Events\BroadcastNotificationCreated anywhere? This should be a really obvious thing to fix

I agree that this should be reopened. What good is it to have the ability to override a Broadcast Event Name from Events but not from a Notification? Functionality should be leveraged between the two use cases.

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

No branches or pull requests