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

Added feature to inject custom HTTP clients #84

Merged
merged 2 commits into from May 20, 2014

Conversation

Projects
None yet
4 participants
@SammyK
Collaborator

SammyK commented May 20, 2014

Take a deep breath...! The very much needed injectable HTTP client handler PR is here! :) This would close #4.

Usage

You can inject your own HTTP client in two steps:

  1. Code to the Facebook\FacebookHttpable interface.
  2. Inject your implementation like so:
FacebookRequest::setHttpClientHandler(new MyCustomHttpClient());

I've posted examples of Guzzle and stream wrapper implementations.

Considerations

  • I really tried to decouple the code in the FacebookRequest object in this refactor. The execute() method was especially getting out of hand. It's not perfect yet, but it's much better than before.
  • I killed the IPv6 workaround since it was broken anyways - even with the latest fix 0a34085. And it really shouldn't be the responsibility of the SDK to fix people's misconfigured servers. :)
  • This merge is going to cause some serious conflicts with #75. But I hope you can see why we don't need #75 with this PR since the stream implementation can be easily injected by the developer.
  • If you'd like to include the Guzzle and stream wrapper implementations in the SDK, that would make it easy for a developer to just inject a new implementation with just two lines of code without even having to write anything new on their own:
use Facebook\FacebookStreamHttpClient;
FacebookRequest::setHttpClientHandler(new FacebookStreamHttpClient());
  • There is a new dev dependency in the composer.json file. It pulls in mockery so that we can mock dependancies. The test suite doesn't test in isolation enough and mockery will make it easier to do that going forward. So make sure to run composer update when you pull this one in to test.

The end

Let me know if you have any questions on anything!

@ahsanity

This comment has been minimized.

Contributor

ahsanity commented May 20, 2014

Great work! Love the idea of injectable HTTP client handler :)

Here's my 2cents:

  1. the class FacebookStreamHttpClient in your examples should also be part of this PR (good to see some of the code from #75 in this class!)
  2. getHttpClientHandler() method in FacebookRequest should fall back to use FacebookStreamHttpClient if curl is not enabled in the environment (i.e. of the user didn't set her own custom class)

Reasoning:
I know your main design philosophy is that the developer can add her own custom http client class depending on the platform/environment. This is great but I believe both the curl http client and stream http client class should be added by default as this covers most of the use cases for php environments. That way the average joe developer (like me) don't need to bother with adding their own implementation as with these two classes it covers most php environments. Only when a dev has some special need (or want to use some other 3rd party lib) they can implement their own class.

Hope this is useful and makes sense :)

Cheers!

@gfosco

This comment has been minimized.

Contributor

gfosco commented May 20, 2014

I'm supportive of this and the Stream client inclusion and fallback. Great stuff Sammy.

/**
* @param FacebookCurl|null Procedural curl as object
*/
public function __construct($facebookCurl = null)

This comment has been minimized.

@yguedidi

yguedidi May 20, 2014

Collaborator

Missing type hint

/**
* Make a new curl reference instance
*
* @return void

This comment has been minimized.

@yguedidi

yguedidi May 20, 2014

Collaborator

@return null, same below, 3 times

$headerComponents = explode("\n", $rawHeader);
foreach ($headerComponents as $line) {
if (strpos($line, ':') === false)

This comment has been minimized.

@yguedidi

yguedidi May 20, 2014

Collaborator

I think it's safer to use ': ' (with a space)

if (strpos($line, ':') === false)
{
$headers['http_code'] = $line;
}

This comment has been minimized.

@yguedidi

yguedidi May 20, 2014

Collaborator

Coding standard about braces ;)

$headerSize = self::$facebookCurl->getinfo(CURLINFO_HEADER_SIZE);
// This corrects a Curl bug where header size does not account
// for additional Proxy headers.
if ( self::needsCurlProxyFix() ) {

This comment has been minimized.

@yguedidi

yguedidi May 20, 2014

Collaborator

I think stuffs about cURL's bug should go in FacebookCurl, just shortcut FacebookCurl::getinfo() when called with CURLINFO_HEADER_SIZE

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented May 20, 2014

Whaoo great work ! Much better than what I'd send ;)
Just a question, why add mockery ? You can mock with PHPUnit.
I'd also propose to move FacebookCurl to the end of FacebookCurlHtttpClient. Because it's act like a private class, should not be instanciated by the dev.

@SammyK

This comment has been minimized.

Collaborator

SammyK commented May 20, 2014

Thanks for the feedback guys! :)

@ahsanity & @gfosco Sweet - I'll add the Guzzle and stream implementations in another PR with the stream implementation fallback. That way this PR doesn't get too out of scope. :)

@yguedidi Thanks for the code review! I'll def tweak some of your callouts.

On moving FacebookCurl into the same file as FacebookCurlHtttpClient - that's funny you should mention that because that's how I had it at first! :) But when both classes are in the same file, the FacebookCurl class will be declared twice when you try to mock the FacebookCurl object for testing thus throwing an error.

On mockery - I included it because it has really become the de facto standard in mocking objects in PHPUnit and has a lot more power and flexibility when mocking an object than PHPUnit's mocking capabilities.

@SammyK

This comment has been minimized.

Collaborator

SammyK commented May 20, 2014

Done! This commit should merge without conflicts.

@yguedidi - On moving the curl bug fix to FacebookCurl, I really like that idea, but unfortunately FacebookCurl isn't tested as-is since its only reason for existing is so that we can mock the curl functionality for the tests in the implementation. It's just a procedural-to-object conversion and nothing more. And I want to be able to test the bug fix. So it makes more sense for testing purposes to keep the bug fix in FacebookCurlHtttpClient. :)

@gfosco

This comment has been minimized.

Contributor

gfosco commented May 20, 2014

Onward and upward.

gfosco added a commit that referenced this pull request May 20, 2014

Merge pull request #84 from SammyK/feature-injectable-http-client
Added feature to inject custom HTTP clients

@gfosco gfosco merged commit db3109d into facebook:master May 20, 2014

@SammyK

This comment has been minimized.

Collaborator

SammyK commented May 21, 2014

W00t, w00t! 👍

@SammyK SammyK deleted the SammyK:feature-injectable-http-client branch May 21, 2014

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