search

laravel / slack-notification-channel

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

Replace Guzzle with Http #90

Closed arondeparon closed 7 months ago

arondeparon commented 7 months ago

What does this PR do?

Why?

Short version: testability.

Longer version:

This issue was the trigger for me: when running tests that use an old webhook-style integration, you will be presented with Error using Illuminate\Notifications\Slack\SlackMessage buildJsonPayload errors.

In my particular case, I do not have the ability to overrride routeNotificationFor because I am not triggering my notification from a model, but am doing it as so:

            Notification::route('slack', config('services.slack.url'))
                ->notify(new MyAwesomeNotification($foo, $bar, $baz));

I tried overriding the config in the tests to point to https://hook.slack.com but then we are presented with a 404 page, without an easy ability to mock this URL.

Hence, this is why using the native Http client from Laravel is preferred: we can just mock this request from our tests and be done, and all is well again in the world.

Tests

Tests have been updated to reflect this change.

taylorotwell commented 7 months ago

Sorry - we can't change the entire return type on a patch release.

arondeparon commented 7 months ago

No worries, that makes sense. Would it be worth considering this for a major release?