laravel / slack-notification-channel

Slack Notification Channel for laravel.
https://laravel.com/docs/notifications#slack-notifications
MIT License
864 stars 59 forks source link

Argument #1 ($message) must be of type Illuminate\Notifications\Slack\SlackMessage #68

Closed aromka closed 1 year ago

aromka commented 1 year ago

Slack Notification Channel Version

3.0

Laravel Version

10.4

PHP Version

8.1

Database Driver & Version

No response

Description

In the latest update it seems that return type for toSlack() changed from \Illuminate\Notifications\Messages\SlackMessage to Illuminate\Notifications\Slack\SlackMessage which results in

TypeError: Illuminate\Notifications\Slack\SlackChannel::buildJsonPayload(): Argument #1 ($message) must be of type Illuminate\Notifications\Slack\SlackMessage, Illuminate\Notifications\Messages\SlackMessage given, called in /vendor/laravel/slack-notification-channel/src/Slack/SlackChannel.php on line 38

This should probably be documented as a breaking change for the upgrade.

Steps To Reproduce

Upgrade slack notifications to v3.0

jbrooksuk commented 1 year ago

@aromka I think this is an application issue. The toSlack method is defined within your application with a return type, but I guess you've now switched to Block Kit and need to update the return type too.

zeshan77 commented 1 year ago

Hi @jbrooksuk , I'm also facing the same issue. I couldn't clearly understand your explanation, with all due respect :)

The documentation says that toSlack() should return Illuminate\Notifications\Slack\SlackMessage but in the code it expects Illuminate\Notifications\Messages\SlackMessagehttps://github.com/laravel/slack-notification-channel/blob/9408050fb7fdb3ba45acb7a27a92c5cce98637de/src/Channels/SlackWebhookChannel.php#L52

jbrooksuk commented 1 year ago

@zeshan77 there are two ways to build Slack notifications:

You only need to change the toSlack return type if you're rebuilding your notification to send with Block Kit. Hope that helps.

chrisloftus commented 1 year ago

I'm also getting this error. v3.0.

Laravel v9.51.0

PHP 8.1

Notification class:

<?php

namespace App\Notifications;

use Illuminate\Notifications\Notification;
use Illuminate\Notifications\Messages\SlackMessage;
use Illuminate\Notifications\Messages\SlackAttachment;

class ItemNotification extends Notification
{
    public function __construct(
        public string $link,
        public string $imageUrl,
        public string $action = 'Added'
    ) {
        //
    }

    /**
     * Get the notification's delivery channels.
     *
     * @param  mixed  $notifiable
     * @return array
     */
    public function via($notifiable)
    {
        return ['slack'];
    }

    /**
     * Get the Slack representation of the notification.
     *
     * @param  mixed  $notifiable
     * @return \Illuminate\Notifications\Messages\SlackMessage
     */
    public function toSlack($notifiable)
    {
        return (new SlackMessage)
            ->content("{$this->action}: {$this->link}")
            ->attachment(function (SlackAttachment $attachment) {
                $attachment->image($this->imageUrl);
            });
    }
}

How I'm calling it:

Notification::route('slack', config('logging.channels.slack.url'))
    ->notify(new ItemNotification("{$subUrl}/{$newId}", $imageUrl));
nhaynes commented 1 year ago

If anyone else is encountering this and can't figure out why, you may still be supplying an incoming webhook url as the notification route.

You may be doing this via the routeNotificationFor method on a Notifiable:

class User extends Authenticatable
{
    use Notifiable;

    /**
     * Route notifications for the Slack channel.
     *
     * @param  \Illuminate\Notifications\Notification  $notification
     * @return string
     */
    public function routeNotificationForSlack($notification)
    {
        return 'https://hooks.slack.com/services/...';
    }
}

or you may be doing it as an AnonymousNotifiable:

Notification::send(Notification::route('slack', "https://hooks.slack.com/services/..."), new MySlackNotification())

To resolve this error, you need to replace these webhook urls with channel names:

class User extends Authenticatable
{
    use Notifiable;

    /**
     * Route notifications for the Slack channel.
     *
     * @param  \Illuminate\Notifications\Notification  $notification
     * @return string
     */
    public function routeNotificationForSlack($notification)
    {
        return '#my-channel';
    }
}

or if you are using an AnonymousNotifiable:

Notification::send(Notification::route('slack', "#my-channel"), new MySlackNotification())

The determineChannel method in SlackNotificationRouterChannel uses the format of the route to determine which SlackMessage to use. See https://github.com/laravel/slack-notification-channel/blob/3.x/src/SlackNotificationRouterChannel.php

sedwardsgt commented 2 months ago

OK, so I'm running into this exact issue as well, and I have a few questions for some in here.

First, I have created a Slack app and published it, starting back with Laravel 9 using nathanheffley/laravel-slack-blocks. We are using incoming webhooks, and the Block Kit API to build blocks via json. Using OAuth, we call https://slack.com/oauth/v2/authorize, and the user is prompted to select the channel they wish to send notifications to (this is important later). Then, as you can see here

        $response = $slack->getAccessToken('authorization_code', [
            'code' => $request->input('code'),
        ]);

        $accountModel->slackWebhookUrl = $response['incoming_webhook']['url'];

we get the incoming_webhook and stash in the db record for the notifiable, and get the webook in the model

    public function routeNotificationForSlack($notification): ?string
    {
        return $this->getAttribute('slackWebhookUrl');
    }

I am now upgrading our app to Laravel 11, and am now attempting to use this to send notifications, since the nathanheffley library has been archived and does not support Laravel 11 (because of this functionality). On a side note, the nice thing about the nathanheffley library was that there was one SlackBlock class, and you didn't have to use to a separate class depending on the block type. Sigh.

I am getting the error that everyone else is getting, and looking at all the comments, I have a few questions:

What am I missing?

sedwardsgt commented 2 months ago

OK, continuing working through this, I am already getting a bot token via OAuth, and the response includes the selected channel name, so I'm stashing the token and channel value, and per the docs, my routeNotificationForSlack implementation looks like this

    /**
     * @param  $notification
     * @return mixed
     */
    public function routeNotificationForSlack($notification): mixed
    {
        return SlackRoute::make($this->getAttribute('slackChannelName'), $this->getAttribute('slackBotToken'));
    }

However, now, I get the following error

500 Slack API call failed with error [missing_scope].

My app has the incoming_webhooks scope defined, which, per the Slack docs, is what I need (and all I have needed in the past).