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

[Proposal] HTTP client class & interface for extensibility #4

Closed
SammyK opened this issue Apr 30, 2014 · 8 comments
Closed

[Proposal] HTTP client class & interface for extensibility #4

SammyK opened this issue Apr 30, 2014 · 8 comments

Comments

@SammyK
Copy link
Contributor

@SammyK SammyK commented Apr 30, 2014

Let's extract the HTTP-client specific code from FacebookRequest and inject a default FacebookHttpClient class with the default curl implementation. If a developer wanted to use their own HTTP client implementation, they can code their custom class to the FacebookHttpClientInterface and inject it into FacebookRequest as a static dependency.

@dcelasun
Copy link

@dcelasun dcelasun commented May 1, 2014

+1 for this. Passing an implementation that uses Guzzle instead of plain old curl would be great.

@yguedidi
Copy link
Contributor

@yguedidi yguedidi commented May 1, 2014

👍

@SammyK
Copy link
Contributor Author

@SammyK SammyK commented May 16, 2014

Since #44 was shut down for now, let's get an injectable HTTP client handler.

But before I submit a PR, I'd like to propose that we ship the SDK with Guzzle as the default HTTP client implementation. I talked with @jeremeamia who heads up the SDK for AWS and he said the number 1 bug-causing PR-submitting functionality was their curl implementation. When they switched to Guzzle, all that headache went away.

We can see this happening right now when #38 etags were added and then started #60 having problems. The execute() function is already getting huge and impossible to maintain with features like etags.

I'd like to just rewrite all the HTTP client stuff on Guzzle with easy ways for a developer to tweak the HTTP client or inject their own handler. Just need a blessing.

@gfosco
Copy link
Contributor

@gfosco gfosco commented May 16, 2014

Not ready yet to make Guzzle the default, sorry Sammy.

@SammyK
Copy link
Contributor Author

@SammyK SammyK commented May 16, 2014

No worries. Do I have a go for refactoring the curl stuff so that it can be overwritten by setter injection?

@gfosco
Copy link
Contributor

@gfosco gfosco commented May 16, 2014

Yes you do.

@SammyK
Copy link
Contributor Author

@SammyK SammyK commented May 16, 2014

I feel like I need to give you guys some holy water since I'm always requesting a blessing... :)

@SammyK
Copy link
Contributor Author

@SammyK SammyK commented May 19, 2014

Just a heads up. The PR is coming later today and it's quite a refactor of the HTTP client stuff. But definitely take a close look at it to see why it's better (because it's hella better!) =) I'll try to throw up a gist example of how a developer could implement Guzzle with it too. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants