Removed the HTTP layer and started to use HTTPlug #641
Conversation
ghost
added
the
|
Wow - thanks for the contribution @Nyholm! :) I love seeing this much red in a PR:
This might take me a bit to review but I should be able to sometime next week. And I'd love to get @yguedidi's feedback on this one too since it's a bigger PR. Also - we haven't quite set up the branches for working on v6 yet, but I'll set up Thanks again for getting involved in the Facebook PHP SDK! :) |
|
@Nyholm PS: Do you mind also adding a summary of changes to the CHANGELOG? :) |
|
Of course. Sorry about that. |
|
@stevenmaguire This is the one I was telling you about. :) |
|
I've rebased this PR. |
CHANGELOG.md
Outdated
| @@ -3,6 +3,15 @@ | |||
| Starting with version 5, the Facebook PHP SDK follows [SemVer](http://semver.org/). | |||
|
|
|||
|
|
|||
| ## 6.x | |||
|
|
|||
|
|
|||
README.md
Outdated
| ``` | ||
|
|
||
| Why the extra packages? We give you the flexibility to choose what HTTP client (e.g. cURL or Guzzle) to use and what PSR-7 implementation you prefer. Read more about this at the [HTTPlug documentation](http://php-http.readthedocs.io/en/latest/httplug/users.html). |
src/Facebook/FacebookClient.php
Outdated
| $timeOut = static::DEFAULT_FILE_UPLOAD_REQUEST_TIMEOUT; | ||
| } elseif ($request->containsVideoUploads()) { | ||
| $timeOut = static::DEFAULT_VIDEO_UPLOAD_REQUEST_TIMEOUT; | ||
| } |
There was a problem hiding this comment.
By removing this, you break the intended behavior
There was a problem hiding this comment.
Yes I know. There is no way of us configure the client. This is the responsibility of the application author. He/She would change this setting if he/she notice that the file uploads are "time outing".
There was a problem hiding this comment.
We then have to think about documenting this case
There was a problem hiding this comment.
Added a note about that in FacebookVideo.fbmd
src/Facebook/FacebookClient.php
Outdated
| } elseif ($request->containsVideoUploads()) { | ||
| $timeOut = static::DEFAULT_VIDEO_UPLOAD_REQUEST_TIMEOUT; | ||
| } | ||
|
|
||
| // Should throw `FacebookSDKException` exception on HTTP client error. | ||
| // Don't catch to allow it to bubble up. |
There was a problem hiding this comment.
This comment should be removed now
tests/FacebookClientTest.php
Outdated
|
|
||
| $this->assertInstanceOf('Facebook\Tests\Fixtures\MyFooClientHandler', $httpHandler); | ||
| } | ||
|
|
||
| public function testTheHttpClientWillFallbackToDefault() |
There was a problem hiding this comment.
Please keep a test that checks that without constructor parameters, we get an instance of Http\Client\HttpClient
There was a problem hiding this comment.
@Nyholm you missed this comment :)
tests/FacebookTest.php
Outdated
| @@ -68,55 +68,26 @@ public function testInstantiatingWithoutAppSecretThrows() | |||
| } | |||
|
|
|||
| /** | |||
| * @expectedException \InvalidArgumentException | |||
| * @expectedException \Throwable | |||
There was a problem hiding this comment.
must be reverted as \Throwable is only available since PHP 7, but we support 5.6
tests/FacebookTest.php
Outdated
|
|
||
| public function testGuzzleHttpClientHandlerCanBeForced() | ||
| /** | ||
| * @expectedException \Throwable |
|
Thank you for the review. I've addressed all of your comments above. |
|
I've rebased this PR. Not sure why Travis wont run. They had some issues yesterday.. it may run eventually. |
src/Facebook/FacebookClient.php
Outdated
| $rawResponse->getHeaders() | ||
| $psr7Response->getBody(), | ||
| $psr7Response->getStatusCode(), | ||
| $psr7Response->getHeaders() |
There was a problem hiding this comment.
This change will break FacebookResponse as PSR getHeaders() will return an array of values for each header, while our implementation GraphRawResponse returns a string for each header. FacebookResponse must be updated
composer.json
Outdated
| "psr/http-message": "^1.0", | ||
| "php-http/client-implementation": "^1.0", | ||
| "php-http/httplug": "^1.0", | ||
| "php-http/discovery": "^1.0", | ||
| "paragonie/random_compat": "^2.0" | ||
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "~4.0", | ||
| "mockery/mockery": "~0.8", |
There was a problem hiding this comment.
Can you please update thoses version constraint to use ^ too? for consistency
There was a problem hiding this comment.
They are updated to the lastest minor versions
tests/FacebookClientTest.php
Outdated
|
|
||
| $this->assertInstanceOf('Facebook\Tests\Fixtures\MyFooClientHandler', $httpHandler); | ||
| } | ||
|
|
||
| public function testTheHttpClientWillFallbackToDefault() |
There was a problem hiding this comment.
@Nyholm you missed this comment :)
| @@ -57,25 +59,22 @@ public function send($url, $method, $body, array $headers, $timeOut) | |||
| private function respondStart() | |||
| { | |||
| if ($this->respondWith == 'FAIL_ON_START') { | |||
| return new GraphRawResponse( | |||
| "HTTP/1.1 500 OK\r\nFoo: Bar", | |||
| return new Response(500, ['Foo'=>'Bar'], | |||
There was a problem hiding this comment.
Please fix CS issue, this should be:
return new Response(
500,
['Foo' => 'Bar'],
'...'
);You can check this build for all CS issues
|
@Nyholm Why |
|
The A Foo PSR-7 implementation may have created their own factories. A user that use the Foo PSR-7 will not need the So in practice, yes. Everybody will need |
|
This PR has stalled for almost a year. I would be happy to see it merged. Do I have maintainers support on HTTPlug? Btw, Since this PR was originally made HTTPlug has been downloaded a million times. |
|
@Nyholm I support HTTPlug :) |
|
Thank you for a quick response. I'll be happy to work on this PR. I will update it shortly. |
|
I've rebased this PR and reviewed it once again. The tests fails on HHVM because of invalid travis config. (See #798 for the fix). I think this is good to merge. Let me know if I can do anything to help. |
Yes, most of those can be found here.
I agree! :)
The Facebook PHP SDK already has an injectable HTTP client that solves this exact issue. :)
They can already do that with the injectable HTTP client. :) I've talked about this issue a lot in person, on a podcast and in GitHub issues as well as watched others discuss the issue and the only real problem HTTPlug solves is allowing the user to choose their own HTTP client for which the Facebook PHP SDK already has a solution. If HTTPlug was supposed to be a permanent standard that addressed this issue and the whole community agreed to back it, then I might be more open to switching to the standard. But it's not a standard, it's more of a "future-facing polyfill/shim" before PSR-15 and PSR-17 become standardized. That's why switching to it now is not pragmatic. Guzzle is our de-facto standard until the FIG releases some standards surrounding HTTP Clients. So implementing HTTPlug for the Facebook PHP SDK seems overly disruptive as we'd have to switch to a pseudo-standard before the real standard is finalized. It would be less disruptive (and require a lot fewer dependencies) to stick with the current convention of using Guzzle until the standard is finalized. If people don't have Guzzle, they just simply inject their HTTP client of choice like they've always done since the release of v5 of the PHP SDK. :) Perhaps my biggest issue with this implementation is that it requires 6 dependencies just to work properly - you have the 4 in the composer.json, the one for your HTTP client lib and the one for the adapter to make it work with HTTPlug. And the documentation also suggests adding 3 more dependencies on top of that when you install the PHP SDK with @Nyholm - what you're doing it great and it is absolutely solving a problem for many libraries and I applaud your hard work! And you also seem like a great person and I'd love to have you on the podcast again sometime soon! :) I don't think the Facebook PHP SDK suffers from the same problems as the other libraries because of the existing injectable HTTP client. TL;DR: Ultimately, I think this effort should be put into making the existing injectable HTTP client even more dev-friendly and better documented and choosing Guzzle 6 as the default HTTP client. That's just my 2 cents. :) PS: I'll post on twitter to see if we can get any more general feedback. :) |
|
Thanks for explaining your points. I will try to answer them and explain how I see them, but I understand your concerns and I think they are valid.
Actually it's not just that. The aformentioned Guzzle 5 vs 6 dependency issue causes a lot of trouble in the community. This is one of the biggest disadvantages of being the "the de-facto HTTP client for PHP" instead of being a standard. (BTW Guzzle is not an accepted standard either, it just happened to become the most popular HTTP client for PHP. Not speaking against Guzzle, since I am maintainer, just wanted to raise the point).
When we started HTTPlug we wanted to have a decent interface which we can work with. Instead of proposing a standard to the FIG and starting years of debates and countless rewrites we worked on the interface for a year then we started using it. There were a few major issues which we had to solve, but we were able to do so without breaking BC. We plan to submit HTTPlug as a PSR (Tobias already worked on it a lot), but we wanted to test it first. If it ever becomes a PSR, we would like it to maintain it's current form, so that early adopters of HTTPlug will be compatible with a possible PSR too. The other difference between HTTPlug and PSRs is that we intend to maintain HTTPlug even if it becomes a PSR.
This is certainly something that we had a lot of thinking about, but we simply couldn't find a better way to remain as flexible as possible. Although that's quite a number of dependencies, think about the case when you have 4 different clients and 2 different PSR-7 implementations installed. To sum up: I understand your concerns and understand why HTTPlug may not be a fit for the SDK. I tried to address your concerns, maybe they help to make the decision. @Nyholm do you have anything to add? |
|
Thanks @sagikazarmark! @SammyK, from a maintainer to a maintainer ;)
True, but it's a client you need to develop and maintain in your project that is only dedicated to Facebook SDK.
Sure, but as said above: with a custom development.
Not exactly, it allows the user to choose their own HTTP client that will be used across all the libraries that support HTTPlug.
Why you use vendors and depend on them? Because you don't want to reinvent the wheel, and because project dedicated to one thing will surely implement it and test it better than you can do.
I would like to hear your points about many dependancies, what is exactly the issues? Especially when now composer handle everything for you! The two issues I see are:
But HTTPlug is a pretty stable package, tested since more than a year (still at version one), and should not conflict with other package as it should be the only HTTP client abstraction you need ;) Let me know if you still don't want HTTPlug for Facebook PHP SDK, I'll think more and harder to convince you! |
Yes, and HTTPlug is not a standard either. :) At least with Guzzle, it's a lot more widely used than HTTPlug which is more reason for staying with Guzzle 6 and giving more love the the injectable HTTP client handler that we already have.
That's certainly a tradeoff - either you have to pull in extra adopters from a 3rd party with potentially lots of extra cruft and maintain the periodic update cycle with HTTPlug, or you have a few extra lines of code you maintain in your code to inject the HTTP client you already have. Both require maintenance and I'd personally rather maintain a few lines of of glue between libs vs pulling in and maintaining lots of different libraries. And I already have some ideas of making the injectable HTTP client even easier for the end developer to make it even more trivial to setup and maintain. :)
That'd be awesome if HTTPlug was used as widely as Guzzle, but taking a look at the HTTPlug home page, there are only a handful of projects that have adopted it and even fewer official SDK's which would put the Facebook PHP SDK in the early-adopter category. The PHP SDK would also be hands-down the most installed & used library to adopt HTTPlug so far; not necessarily a bad thing, but kinda kills the "across all the libraries that support HTTPlug" argument. :) My guess is that the vast majority of the people using the Facebook PHP SDK haven't installed one of the few libraries that implements HTTPlug and this PR would over-complicate and add lots more cruft all for an ideology instead of cleaning up the simple solution we have for this problem already. :)
I 100% agree for non-trivial things like routers, ORM's and console libs, but we're talking about gluing libraries together here. Think of all those extra libraries we're pulling in just to glue this library to another. The more I think about it, the more I'm convinced this is a bad idea for the Facebook PHP SDK. :)
Other than the points you brought up, in this particular situation, you have to pull in upwards of 6 or more dependencies and not all of them are from the same vendor. That's great that HTTPlug has been at 1.0 for a year, but what about the other libs? And with the extra dependencies you have to install from the command line outside of what exists in the composer.json - how are we supposed maintain those? I might be missing something here and perhaps we don't have to worry about it, but the whole developer experience of installing and maintaining the Facebook PHP SDK from composer seems to get cloudy and overly complicated. I hope that clarifies some points. I'm pretty opposed to adding it at this point and we didn't get a whole lot of feedback from twitter, so I suggest we do the following:
What are your thoughts of doing something like that? :) |
|
I suggest something like:
Change this PR so it "just works" when installing. Ie, no extra step for the user. We require Guzzle6 (or php-http/curl-client to avoid guzzle5/6 conflict). Then later, but before releasing version 6, we could decide: A) Remove "just works" experience to be more flexible and "technically correct" The two changes A & B is quite small in comparison to this large PR. If the maintainers are happy with this suggestion I'll make the updates tonight. |
|
I really like your proposition @Nyholm!! I don't really care about Guzzle6 or cURL as it's for the development only. |
|
So @SammyK, what's your pont on the new version? :) |
README.md
Outdated
| @@ -15,9 +15,11 @@ This repository contains the open source PHP SDK that allows you to access the F | |||
| The Facebook PHP SDK can be installed with [Composer](https://getcomposer.org/). Run this command: | |||
|
|
|||
| ```sh | |||
| composer require facebook/graph-sdk | |||
| composer require facebook/graph-sdk php-http/curl-client guzzlehttp/psr7 php-http/message | |||
There was a problem hiding this comment.
@Nyholm You should update the doc here :)
|
Wrappers should just replace the entire client, and not use the setter, tbh
…On 5 Jul 2017 17:54, "Sammy Kaye Powers" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Facebook/FacebookClient.php
<#641 (comment)>
:
> */
- public function setHttpClientHandler(FacebookHttpClientInterface $httpClientHandler)
+ public function setHttpClient(HttpClient $httpClient)
The setter is good for those who use the Facebook PHP SDK via some kind of
wrapper lib that doesn't give them access to the constructor injection.
There's really no harm in keeping it. :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#641 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakCEft8bh0aElEEgxYUR3H3kpHL2tks5sK7GggaJpZM4Js3aC>
.
|
|
@yguedidi It's certainly better since it's now choosing a default HTTP client for them and they don't have to do extra steps to install an HTTP client, but now we're in the same spot as we would be if went the default Guzzle 6 install with injectable HTTP client. Only with this PR we're we're forcing the user to pull in 6 extra dependencies on every install. So I'm still very much against. Sorry! :) I'll try to carve out some time this week to do a quick PR for Guzzle 6 default install just to get a comparison. :) |
I added the last commit to allow us to merge this PR and move forward. To postpone the decision of HTTPlug or not. The changes from this PR to "hard couple to Guzzle" will be minimal (compared to move to Guzzle without this PR). |
|
@SammyK so now it looks like your only remaining point is the number of dependendies. Personaly, I don't considere this an argument because:
If you have any cons, let me know! ;) |
I am guessing you were talking about these dependencies.
It's a dependency of Guzzle as well and only required separately because because the interfaces are used in this package.
This is a virtual package, satisfied by the guzzle6-adapter.
This is an interface package, should be very lightweight.
I am guessing this is required in order to allow discovering an installed client/factory. It helps zero configuration, but optional, can be removed (along with the feature).
I don't think this is related to HTTPlug.
This is the adapter for Guzzle 6.
I am guessing this is necessary for using the Message factories. This is probably the "biggest" one, but still quite small. If you wish, you can use a PSR-7 package directly instead, IMO it's less important than letting the user choose the client. Also, once PSR-17 is out (and PSR-7 packages itselves provide the factories) there will be another dependency on that interface package. So as far as I can see, we are talking about 2-3 mandatory dependencies (on top of Guzzle; one of them is an interface package), not 6. Just to make things clear, you might still consider this too much, that's out of scope for this comment. |
|
Many people (not only here) are concerned about the number of dependency packages. What one should be more concern about is the number of dependency classes and lines of code. The more classes you have the more likely will you have bugs and breaking changes. It will also be harder to maintain larger packages. Some statistics:
|
Nope. :) My other arguments against it are still valid:
And of course the issue of the number of dependencies all to glue two libraries together. @yguedidi I'm with you that composer takes care of everything and what's wrong with pulling in a few extra dependencies? But we're talking about glue; six dependencies you need just to glue an HTTP client to your lib; it's overly complicated. That's just a big smell to me when we already have a simple solution in place. :) Sorry folks - with each post I think I become even more against it! Hehe. :) Again - I'll try to get some time to play with this PR sometime this week or next and give some final feedback. :) |
It solves a problem we have: sending HTTP requests. Otherwise we would not have implement a too simple solution haha. But then we have the problem to maintain it, while HTTPlug do it for use, we then can just focus on the SDK.
But Guzzle is?
Used by who? libraries? business project developers? |
Also, the API surface on the dependency being used is much smaller now. Popularity shouldn't be taken over good design: if someone still wanted to use guzzle, they could by using the php-http guzzle adapters. Php-http is not a standard, but at least it's a huge improvement in interface segregation. |
|
Disclaimer: I restrained myself to write about the HTTPlug bundle as this library is not a Symfony integration and the features I will describe are not yet finalized (but in really good progress). I'm currently working on a new awesome feature which aims to provide Symfony profiler integration for any 3rd party library using HTTPlug without any configuration and without any integration bundle. As Symfony is a widely used framework, this would provide a huge added value for developers helping them to understand what they are doing, why things are not working. You can learn more about this upcoming feature here: php-http/HttplugBundle#109 |
😸 |
|
I don't really have anything new to add. I still don't think this is the right direction for the Facebook PHP SDK, but I relinquish the ultimate decision to my friend @yguedidi. Your call buddy! I'll support whatever decision you make. :) |
|
Thank you @yguedidi. |
|
Thank you for merging! |
|
Thank you @Nyholm for your work and you patience! |
This will fix #605, it is also related to #586.
This is a PR in order to better support PSR7 in this library by using HTTPlug abstraction. By using HTTPlug a lot of code can be removed and the maintenance will be more focus on the graph API and not the HTTP transport layer (as you can see the diff, a lot of code is no longer necessary).
Advantages
End user choice: The end user will have the choice of the http client implementation, which will be, for a majority of user, the guzzle6 library. However some others librairies can be chosen, like Zend/Http or CakePHP one which are already available in their respective framework.
By not couple us to Guzzle6 we allow people to use this SDK with Guzzle5 (or any other adapter).
Less maintenance on this library: HTTPlug offer a large feature set (with the usage of the client-common package) which does not need to be rewrite in this library: Retry failed requests, Load Balancing (Client Pool), Failure detection, logging, caching etc ...
Better integration with frameworks (Symfony ATM, Laravel in progress): HTTPlug offer an integration to existing frameworks for library using this layer. In example, for Symfony all requests will be logged and traced when in dev mode (time, memory, diff at each modification, etc ....).
Shared work: Many libraries already implement HTTPlug or are on the way, this mean, we can better profit for feature offered / wanted by other libraries.
Future work
One could go further with PSR-7. I've chosen to stop here for this PR. But possible future work could be:
FacebookResponseandFacebookRequestRequestBodyMultipartand replace it with MultipartStreamBuilderRequestBodyUrlEncodedandRequestBodyInterfaceFacebookClient::getHttpClient