laravel-notification-channels / onesignal

OneSignal notifications channel for Laravel
MIT License
283 stars 119 forks source link

Pass the notification object to the routeNotificationFor method #64

Closed sateshpersaud closed 6 years ago

sateshpersaud commented 6 years ago

This allows the developer to access the notification here:

public function routeNotificationForOneSignal(Notification $notification)
{
    // access $notification here
}
LKaemmerling commented 6 years ago

Could you please rebase this on the refactor-to-traits branch?

@timacdonald how usefully do you find this?

timacdonald commented 6 years ago

It looks like this functionality is new in 5.6: https://github.com/laravel/framework/pull/22289

I think if it is supported in core it makes sense to add it. I personally don't have a need for it, but obviously it is something that could be handy in some instances.

This is also relevant: https://laracasts.com/discuss/channels/laravel/how-to-route-notifications-based-on-the-notification-being-sent

LKaemmerling commented 6 years ago

Okay, this leads us to the problem that we must drop support < 5.6 for this... i think this isn't a good solution @Lloople what do you think?

timacdonald commented 6 years ago

I'm not 100% that it will be a breaking change.

<5.6 is expecting one argument - the $driver 5.6+ is expecting 2 arguments $driver and an optional $notification.

In PHP if a function has explicit parameters - like these do, but is passed more arguments than it is expecting it will just ignore the additional arguments.

As an example - start a tinker sessions and enter camel_case('foo bar', 'baz'). It will work fine and output fooBar and just ignore baz as it only expects one argument.

Correct me if I'm wrong - I may well be missing something.

Lloople commented 6 years ago

@LKDevelopment Yeah, maybe we should create a tag just after your trait refactoring and stop 5.5 compatibility there.

I mean, maybe next releases should be >5.6 only