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

[4.x] Add a default cURL HTTP client #1589

Merged
merged 22 commits into from
Oct 18, 2023
Merged

[4.x] Add a default cURL HTTP client #1589

merged 22 commits into from
Oct 18, 2023

Conversation

cleptric
Copy link
Member

@cleptric cleptric commented Sep 20, 2023

Work in Progress

This adds a cURL-based HTTP client and simplifies the setup when using a custom Transport or HttpClient.
As mentioned in #1586, this removes the /store endpoint entirely. All events are now sent to the /envelope endpoint.

Fix PHP 7.2 - 7.3 tests

Fix CS

Remove .relay folder
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

As I've already said in #1586 (comment), I'm against writing our own HTTP client but, since I recognize I don't have a final say in this, I'm still giving my review.

I would at least suggest leveraging PSR-18 (HTTP client) and so PSR-7 (request/response) with it, but we could ditch PSR-17 (factories) to keep it simple.

src/ClientBuilder.php Show resolved Hide resolved
src/ClientBuilder.php Show resolved Hide resolved
src/HttpClient/HttpClient.php Show resolved Hide resolved
src/HttpClient/HttpClient.php Outdated Show resolved Hide resolved
src/HttpClient/HttpClient.php Show resolved Hide resolved
src/HttpClient/HttpClient.php Outdated Show resolved Hide resolved
src/HttpClient/HttpClient.php Outdated Show resolved Hide resolved
src/HttpClient/Response.php Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

ste93cry commented Sep 21, 2023

I agree with everything already said by @Jean85: first of all, I don’t understand the reason to have our own HttpClientInterface instead of just using the PSR-18 one. There is no benefit I can think of, it would be just another layer that needs to be implemented by the end user. Second, and more importantly, I don’t understand why we are reimplementing a full cURL client, which we have to maintain and for which there is no benefit because it’s a lot simpler and less customizable than any other out of here. Moreover, what’s happening here is that we are reinventing the wheel: I can understand that this SDK has to ship with a default client, even though I don’t agree, but at least choose one reliable client instead of reinventing the wheel. We had our own HTTP client in the past, and it was full of issues. Given that the SDK is integrated with both Laravel and Symfony, it would be wise to choose symfony/http-client which is common for both

src/Transport/ResultStatus.php Show resolved Hide resolved
src/HttpClient/Response.php Outdated Show resolved Hide resolved
@cleptric cleptric marked this pull request as ready for review October 16, 2023 21:09
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Overal this is starting to look great, mostly because it's a heck of a lot simpler.

src/Client.php Show resolved Hide resolved
src/HttpClient/HttpClient.php Outdated Show resolved Hide resolved
src/HttpClient/HttpClient.php Show resolved Hide resolved
src/Client.php Show resolved Hide resolved
src/ClientBuilder.php Show resolved Hide resolved
src/HttpClient/HttpClient.php Show resolved Hide resolved
src/Options.php Show resolved Hide resolved
src/Options.php Show resolved Hide resolved
src/Util/Http.php Outdated Show resolved Hide resolved
src/Util/Http.php Outdated Show resolved Hide resolved
@cleptric cleptric mentioned this pull request Oct 17, 2023
@cleptric cleptric added this to the 4.0 milestone Oct 18, 2023
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

👍🚀

@cleptric cleptric mentioned this pull request Oct 18, 2023
5 tasks
@cleptric
Copy link
Member Author

Merging now to unblock other features. The remaining ToDos are documented in #1595

@cleptric cleptric merged commit 83d6d01 into 4.x Oct 18, 2023
32 of 33 checks passed
@cleptric cleptric deleted the curl-http-client branch October 18, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants