laravel / slack-notification-channel

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

[3.0] Support BlockKit / WebAPI-based bot notifications #64

Closed claudiodekker closed 1 year ago

claudiodekker commented 1 year ago

Re-submit of https://github.com/laravel/slack-notification-channel/pull/63 using a clean commit history, as requested.

My reasoning for removing the v2 API & considering this a full v3 is pretty simple: Although Incoming Webhooks still work today, they have been marked as deprecated in favor of the WebAPI that this PR (in part) implements: https://api.slack.com/legacy/custom-integrations/messaging/webhooks

Since this constitutes a breaking change, I've also taken the opportunity to remove deprecated/no-longer-supported Laravel and PHP versions. Those who still rely on the now-deprecated legacy webhooks can lock the composer dependency to ^2.0 until this API is eventually removed by Slack altogether.

taylorotwell commented 1 year ago

Should be a separate, new channel to avoid breaking change.

driesvints commented 1 year ago

@taylorotwell can't we instruct people to use this package's v2 version if they need the old version?

taylorotwell commented 1 year ago

I don't see any reason to not just make it a new channel. It's just so easy to avoid the breaking change entirely by doing so. We already take a similar approach to our SES and SES 2.0 drivers for mail.

claudiodekker commented 1 year ago

Sounds good to me & shouldn't be too difficult.

I'll probably pick it up tomorrow morning, as I don't have time for it today. I'll ping you when it's done. 👍

taylorotwell commented 1 year ago

Thanks! Just mark as ready for review when done and I'll take a look.

taylorotwell commented 1 year ago

Can you copy of your usage examples into this PR and update them as necessary since it is a new channel? Thanks.

claudiodekker commented 1 year ago

@taylorotwell The usage remains unchanged!

The internal channel (Webhook vs. WebAPI) is determined based on whether the routeNotificationFor method is an URL string/PSR URL instance. If it is, it'll be routed to a Webhook, and if it's not, it'll be routed to the new WebAPI channel.

Part of the reason for this, has to do with the fact that the v2 version uses the Illuminate\Notifications namespace directly. With this "v3" version, I had placed everything in the Illuminate\Notifications\Slack namespace, but since we want to maintain backwards compatibility, this simply meant placing the new version in a subfolder/sub-namespace.

The reason for this has to do with that I wasn't sure which one deserves the "slack" channel type more, and only one channel (I think?) can have it at a time. With the webhooks being legacy, changing them would break backwards compatibility.

alies-dev commented 1 year ago

@claudiodekker Thanks for this great PR!

I started to use the new major version on my small project and had some issues with tests that I solved. I just wanty to share my experience ti save someones time.

SLACK_WEBHOOK_URL .env variable needed to route Slack webhook-based notifications to SlackWebhookChannel (previous implementation of slack notification channel that co-exist with a new one), see \Illuminate\Notifications\SlackNotificationRouterChannel::determineChannel() for details. But non-empty SLACK_WEBHOOK_URL also enables SlackWebhookChannel to use a real HTTP client that sends a real HTTP request to Slack (not what we need from tests, right?). To avoid it, you need to mock or fake something, e.g., SlackWebhookChannel:

$this->app->singleton(SlackWebhookChannel::class, function () {
    $httpClientHandler = new MockHandler([
        new \GuzzleHttp\Psr7\Response(200),
    ]);
    $handlerStack = HandlerStack::create($httpClientHandler);
    return new SlackWebhookChannel(new \GuzzleHttp\Client(['handler' => $handlerStack]));
});

But in my case it was even simpler: I just needed to fake Event dispatcher because I send Slack notification from an Event Listener:

$fakeEvent = \Illuminate\Support\Facades\Event::fake();
...
$fakeEvent->assertDispatched(OrderPaid::class);
jameshulse commented 1 year ago

The reason for this has to do with that I wasn't sure which one deserves the "slack" channel type more, and only one channel (I think?) can have it at a time. With the webhooks being legacy, changing them would break backwards compatibility.

@claudiodekker, great PR. Unfortunately I think backwards compatibility was broken anyway - or at least the upgrade path isn't totally clear. We are returning Illuminate\Notifications\Messages\SlackMessage from our toSlack function, but after the 3.x upgrade we get:

Illuminate\Notifications\Slack\SlackChannel::buildJsonPayload(): Argument #1 ($message) must be of type Illuminate\Notifications\Slack\SlackMessage, Illuminate\Notifications\Messages\SlackMessage given

It looks like buildJsonPayload expects the new Slack\SlackMessage. Could some other part of our configuration be incorrect which means our 2.x objects are being passed to the 3.x behaviour?

For now we are down-grading to 2.5 to ensure this is working until we can commit to the conversion to the block API.