Skip to content

Switch to Guzzle for HTTP calls #289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 17, 2021
Merged

Switch to Guzzle for HTTP calls #289

merged 2 commits into from
Mar 17, 2021

Conversation

elverkilde
Copy link
Contributor

@elverkilde elverkilde commented Mar 9, 2021 •
edited
Loading

Description

I have put this directly in the constructor instead of a new option. It's a fairly non-standard option IMO, so I think that makes sense.

This will be a breaking change, as support for 7.1 is removed and several deprecated functions have been removed. Will be released with new major version.

Related to #95 and #232.

CHANGELOG

  • [CHANGED] internal HTTP client to Guzzle
  • [ADDED] optional client parameter to constructor
  • [CHANGED] useTLS is true by default
  • [REMOVED] curl_options from options
  • [REMOVED] customer logger
  • [REMOVED] host, port and timeout constructor parameters
  • [REMOVED] support for PHP 7.1
  • [CHANGED] lower severity level of logging to DEBUG level

@elverkilde elverkilde requested a review from damdo March 9, 2021 16:24
@elverkilde elverkilde force-pushed the guzzle branch 8 times, most recently from b14048f to d481182 Compare March 9, 2021 17:24
GrahamCampbell
GrahamCampbell suggested changes Mar 9, 2021
View reviewed changes
GrahamCampbell
GrahamCampbell suggested changes Mar 9, 2021
View reviewed changes
@elverkilde elverkilde force-pushed the guzzle branch 4 times, most recently from 7df744d to 4dbf217 Compare March 10, 2021 09:34
damdo
damdo reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@damdo damdo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! 👍
A couple of minor comments

@elverkilde elverkilde mentioned this pull request Mar 11, 2021
Merged
GrahamCampbell
GrahamCampbell suggested changes Mar 11, 2021
View reviewed changes
@elverkilde elverkilde added the release-major Major version bump label Mar 17, 2021
Switch the HTTP client from plain curl to Guzzle
a4f08c0 
This is a major refactor:
* removing a lot of the curl-specific cruft
* removing the verbose logging from the tests
* lowering the severity of the logging to DEBUG level
* removing support for 7.1
* removing several deprecated parameters and options
@elverkilde elverkilde removed the release-major Major version bump label Mar 17, 2021
@elverkilde elverkilde added the release-major Major version bump label Mar 17, 2021
@elverkilde elverkilde merged commit 1a5d9d7 into master Mar 17, 2021
@elverkilde elverkilde deleted the guzzle branch March 17, 2021 08:59
@dependabot dependabot bot mentioned this pull request Mar 18, 2021
Closed
@emmadesilva
Copy link

@elverkilde Hey! Sorry for pinging you here but I just wanted to check something. Was the removal of curl_options an intended change, or just a side effect?

The reason I'm asking is because it breaks self signed certificates (which are very useful for local development) when using Laravel Broadcasting.

Since v5.0.3 no longer works on Laravel 10, this means that there is now no way to use self signed certificates when self-hosting Pusher, for example with Soketi (who documents this issue here https://docs.soketi.app/getting-started/backend-configuration/laravel-broadcasting#self-signed-certificates)

If this was an intentional removal, would you mind letting us know why it was removed? Else, would it be possible to get it added back to the package? I'm happy to try making a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@GrahamCampbell GrahamCampbell GrahamCampbell requested changes

@damdo damdo damdo approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release-major Major version bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@elverkilde @emmadesilva @GrahamCampbell @damdo @pusher-ci