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

Comments

Projects
None yet
4 participants
@SammyK
Collaborator

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

This comment has been minimized.

dcelasun commented May 1, 2014

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

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented May 1, 2014

👍

@SammyK

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

gfosco commented May 16, 2014

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

@SammyK

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

gfosco commented May 16, 2014

Yes you do.

@SammyK

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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