search

laravel-notification-channels / webhook

Webhook notifications channel for Laravel
https://laravel-notification-channels.com
MIT License
170 stars 61 forks source link

Channel Name #30

Closed gizburdt closed 4 years ago

gizburdt commented 5 years ago

Hi,

Thanks for taking care of the issues and PR's! I'm wondering for a long time; why is the channel name capitalized?

https://github.com/laravel-notification-channels/webhook/blob/master/src/WebhookChannel.php#L35

All other packages (and core packages) are using lowercase names.

Thanks :)

atymic commented 4 years ago

Good question!

I'm not sure why it's capitalized, but it's passed to Str:Studly() so it shouldn't matter anyway

https://github.com/laravel/framework/blob/1bbe5528568555d597582fdbec73e31f8a818dbc/src/Illuminate/Notifications/RoutesNotifications.php#L42-L44

Feel free to PR an update + a test if possible to cover it

gizburdt commented 4 years ago

I don't think it will break anything (https://github.com/laravel-notification-channels/webhook/blob/master/src/WebhookChannel.php#L35)

But maybe it's safe to - when we change it - bump the major version?

The reason I submitted this issue is because we save the notification route in the database (in our project) and later we check to which channels we need to push the notification. The only notification channel which is capitalized is this one. So we need an extra if statement to make it work properly.

atymic commented 4 years ago

Closed in #35, and will tag a new major version :)