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

[5.4] Notification Channels Array Cast #18960

Merged
merged 1 commit into from
Apr 27, 2017
Merged

[5.4] Notification Channels Array Cast #18960

merged 1 commit into from
Apr 27, 2017

Conversation

ConnorVG
Copy link
Contributor

We document that notifications can return a string from their via method but doing so causes an error. This is an issue that I see cropping up quite often with people getting stuck on 'my notification is erroring' but they're just returning the class like so:

/**
 * Get the notification's channels.
 *
 * @param  mixed  $notifiable
 * @return array|string
 */
public function via($notifiable)
{
    return MyChannel::class;
}

Which they don't expect to error because of the docblock. The same docblock also appears in the documentation.

ResetPassword notification

Custom Channel Documentation

We document that notifications can return a `string` from their `via` method but doing so causes an error. This is an issue that I see cropping up quite often with people getting stuck on 'my notification is erroring' but they're just returning the class like so:
```php
/**
 * Get the notification's channels.
 *
 * @param  mixed  $notifiable
 * @return array|string
 */
public function via($notifiable)
{
    return MyChannel::class;
}
```

Which they don't expect to error because of the docblock. The same docblock also appears in the documentation.

[ResetPassword notification](https://github.com/laravel/framework/blob/5.4/src/Illuminate/Auth/Notifications/ResetPassword.php#L32)

[Custom Channel Documentation](https://laravel.com/docs/5.4/notifications#custom-channels)
@ConnorVG
Copy link
Contributor Author

We could always change the documentation instead but I see this, in my personal opinion, as the most appropriate fix.

@taylorotwell taylorotwell merged commit de0c022 into laravel:5.4 Apr 27, 2017
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