-
Notifications
You must be signed in to change notification settings - Fork 192
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
add support for guzzle 6 #84
add support for guzzle 6 #84
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
5915d0d
to
6a62d46
Compare
82c72ba
to
3de62fd
Compare
@dwsupplee Have you seen #81? |
I have! HTTP Plug is pretty interesting, it is definitely something to keep an eye on as it evolves and gains traction with the community. Keep me posted! :) |
return $resp->getHeader(self::FLAVOR_HEADER) == 'Google'; | ||
$resp = $httpHandler( | ||
new Request('GET', $checkUri), | ||
['timeout' => 0.3] |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
6be1859
to
069a011
Compare
@Nyholm I imagine since this is PSR-7 compatible and uses middlewares, PHP-HTTP will be able to play nicely? |
…for-guzzle-6 refactors test skipping and fixes remaining tests
This PR replicates half of the things I've done in #81.
Im not too sure you understand Guzzle on the other hand, is also great but it is not a standard. (Though, it is popular) By using the power of PSR-7 we can remove the dependency from Guzzle. Which is good because you should not rely on a implementation, that's not SOLID. So, we need an interface to rely on that Guzzle implements, say a |
I think we understand what HTTP Plug is trying to achieve - the issue is it seems we are not comfortable relying on the implementation at this point in time.
This code does adhere to PSR-7 and you are able to override the transport layer with whatever implementation you choose (including php-http's!): use Google\Auth\Credentials\GCECredentials;
use Http\Adapter\Guzzle5HttpAdapter;
use Psr\Http\Message\RequestInterface;
$adapter = new Guzzle5HttpAdapter();
$isOnGCE = GCECredentials::onGce([$adapter, 'sendRequest']);
HttpPlug is one way of achieving this goal. I genuinely appreciate where you guys are coming from and am interested to see where it goes. |
This is now merged, but because I rebased, it doesn't realize it's merged. |
Hey guys, HTTPlug developer here. Although I am not a fan of passing around callables, I have to admit it works and usually provides great flexibility (which is not always an advantage), like in the case above. I like the implementation, but to be completely fair, I would like to correct some things about our interface: It is HTTPlug, not HTTP Plug. We played with the words a bit. 😄
HTTPlug is a set of interfaces, there is no implementation in it. We PROVIDE some implementations. I would be happy to hear some feedback: are there any actual issues you see or just felt that the callable/middleware way is more flexible (which is possibly true)? From #81
We are talking about one dependency: the contract package itself. No other dependencies are required for consumer packages. I guess you meant actual implementations, which are indeed not just single dependencies, but neither Guzzle is. |
Thank you for the info and clarification @sagikazarmark. Really this comes down to the amount of time to work on this, and the demand of the community. Our main goal is to get this working and useful. Guzzle6/PSR7 are the fastest ways to accomplish this. Also, I'm sure you'll agree, PR #81 would have required a tremendous amount of work to get into shape. The tests were failing and even smaller things like whitespace issues made it not a viable option. That being said, this is a community project, and we can open an issue for HTTPlug support, to see how many |
@bshaffer it wasn't my intention to promote the library or force using it. I don't believe it is a standard (although some PSR could be depend on it), and I definitely don't think there is only one possible solution for problems (that's one of the idea of HTTPlug actually). Just wanted to clear some things, which I believed to be easily misunderstandable. I agree that at the current state of the library it requires a big amount of work to make it actually work, and the interfaces are not 100% stable yet, so any code depending on it should not be considered production ready. |
Notable changes:
Google\Auth\GCECredentials
is now found under theGoogle\Auth\Credentials\GCECredentials
namespaceThe code is functional but please keep in mind there are still a few
@todos
which need to be addressed.