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

Guzzle6/PSR-7 support #83

Closed
bshaffer opened this issue Nov 13, 2015 · 9 comments
Closed

Guzzle6/PSR-7 support #83

bshaffer opened this issue Nov 13, 2015 · 9 comments
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@bshaffer
Copy link
Contributor

The lack of support for Guzzle6 in this library is causing problems for our users (see googleapis/google-api-php-client#720). We have a few options:

  1. Implement handlers, and use the handler for the appropriate guzzle version.
    • Pros: There is a precedent for it
    • Cons: Additional classes, and it may not work for more fancy things such as subscribers
  2. Check which version is being used in the OAuth2 and GCECredentials, as these are the only places where the Guzzle Client is called directly! This still requires adding Middleware support, but this could be added in a single class (similar to AuthTokenFetcher) specifically for Guzzle6/PSR7.
    • Pros: Minimal change required - two files + 1 new file. Should then work for all PSR-7-compatible HTTP Clients, and may be able to play nicely with the third option (see below)
    • Cons: May be trickier to implement with Middleware than expected
  3. Use an HTTP abstraction library (see Removing dependency on Guzzlehttp #81)
    • Pros: Users can add Google Authentication onto whatever HTTP client they are using, we do not force them to use Guzzle
    • Cons: Adds another dependency. Adds yet another abstraction layer. It's hard to say if these libraries will actually be supported (similar to ORM problem). The library mentioned might not be stable enough.

Some additional thoughts:

  • The most important thing is for this library to work with at least one HTTP Client
  • The Google Client library can be opinionated with Guzzle (because of its complexity)
  • If this library can work with any PSR-7-compatible HTTP library, that would be a huge community win.
@bshaffer bshaffer changed the title Guzzle6 support Guzzle6/PSR-7 support Nov 13, 2015
@behunin
Copy link

behunin commented Nov 13, 2015

So its either PSR-7 + Guzzle or PSR-7 + Php-http?

@bshaffer
Copy link
Contributor Author

@behunin yes, or maybe more appropriately PSR-7 + Guzzle 5 or PHP-HTTP, with both defaulting to Guzzle 6

@behunin
Copy link

behunin commented Nov 13, 2015

I think we need a poll taken here but, my vote is Guzzle6. I like the way it handles async and batching for the Shopping API Project I am working on. Haven't taken the time to really look into PHP-HTTP though I am not really wanting to be dependent on another project that is in beta or alpha. I need this working before the big shopping boom.

@dwsupplee
Copy link
Contributor

@bshaffer

I'm currently working on a PR with these goals:

  • Remove dependencies on the subscriber interface to allow classes currently implementing it to be used in more generic scenarios.
  • Provide subscriber/middleware implementations so we support idiomatic ways of dealing with guzzle 5/6.
  • Implement basic http handlers for guzzle 5/6 and tie them to an interface that accepts a PSR7 request and returns a PSR7 response. Ideally users would be able to write their own handlers if they would like to use another http layer. This is similar in scope to #81 but will not require any outside dependencies. It looks like the PHP-HTTP team is working to make their library a standard but that remains to be seen. As implementations continue to be created, perhaps more of a standard will arise but I don't think we are quite there yet.

Let me know what you think,
Dave

@bshaffer
Copy link
Contributor Author

So it sounds like option #2 in a nutshell. We should support a PSR-7 middleware, and use Guzzle for internal requests and as the default HTTP.

We could leave the subscriber interfaces as they are, and reimplement each of them as PSR7 middlewares. So each SubscriberInterface has a corresponding Middleware.

This would be my suggestion, since the actual logic in each SubscriberInterface is pretty minimal. I'm not sure we need to make them "more generic".

@dwsupplee
Copy link
Contributor

I agree the logic is pretty minimal, but ideally the subscriber wouldn't be worrying about caching. If we pull that type of logic out it's one less thing to have to duplicate in the middleware implementation. To me, the subscriber should be as simple as,

public function onBefore(BeforeEvent $event)
{
    $event->getRequest()->setHeader('Authorization', 'Bearer ' . $this->fetcher()->fetchAuthToken());
}

@bshaffer
Copy link
Contributor Author

SGTM

@Nyholm
Copy link
Contributor

Nyholm commented Nov 19, 2015

I've just updated #81. It supports PSR7 and PHP-HTTP. There are 4 "FIXME"'s in the comments and I have to rewrite a lot of the tests. Any input and suggestions are appreciated.

FYI. The PHP-HTTP is aiming on tagging a stable release this year. The current state of the library is "pretty stable".

@bshaffer
Copy link
Contributor Author

resolved - guzzle 6 is now in v0.5

@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

No branches or pull requests

5 participants