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

Replace Guzzle with Http #90

Closed
wants to merge 3 commits into from
Closed

Replace Guzzle with Http #90

wants to merge 3 commits into from

Conversation

arondeparon
Copy link

@arondeparon arondeparon commented Mar 26, 2024

What does this PR do?

  • Replaced Guzzle with Laravel's Http class in SlackWebhookChannel

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.

@arondeparon arondeparon changed the title Remove Guzzle requirement and use Laravel HTTP Replace Guzzle with Http Mar 26, 2024
@taylorotwell
Copy link
Member

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

@arondeparon
Copy link
Author

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

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