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

Plugins planned #3

Closed
10 of 14 tasks
joelwurtz opened this issue Nov 8, 2015 · 58 comments
Closed
10 of 14 tasks

Plugins planned #3

joelwurtz opened this issue Nov 8, 2015 · 58 comments

Comments

@joelwurtz
Copy link
Member

plugins planned (new or migrate from Ivory):

  • Authentication (replaces BasicAuth by @sagikazarmark)
  • Cookie (Journal interface #35, migrate)
  • History (migrate)
  • Logger (migrate)
  • Redirect (migrate)
  • Retry (migrate)
  • Tape Recorder (PHP-VCR like by @jeromegamez, see tape recorder php-vcr style #4)
  • Stopwatch (migrate)
  • Convert error responses to exceptions (throw exception when status is 4xx / 5xx)
  • Base URI (to complete incomplete requests, see also Dependencies #37)
  • Validator (validate http request based on protocol (1.0 / 1.1)
  • Sanitizer (add http request headers based on available informations, like Host header, Content-Length header, ...)
  • Encoding : Deflate, Chunk, ... (Add header to request to say that we support compression, and decorate stream for deflate / chunk if needed)
  • Cache (integration with doctrine / cache, psr 6 ? )
@jeromegamez
Copy link

Are you okay with me implementing the Tape Recorder part :)? Unless someone else is already at it, of course!

@joelwurtz
Copy link
Member Author

No problem go ahead :)

@sagikazarmark
Copy link
Member

@jeromegamez it's good to see you here. 😄

No problem at all, it is your baby anyway.

I was looking at VCR. Do you think there is a possibility to join forces with them? Honestly, I never used these tools too much, just played with them. However, if they are the same, why couldn't we team up with them?

I don't insist on it, I leave this to you, but it would be good to investigate what the possibilities are.

@sagikazarmark
Copy link
Member

What's the difference between history and logger?

@dbu
Copy link
Contributor

dbu commented Nov 8, 2015

@jeromegamez awesome! i created #4 to discuss this in its own thread.

@joelwurtz
Copy link
Member Author

What's the difference between history and logger?

Logger log request / response / exception to a psr/log implementation
History keep trace of http calls (request response couple) in a journal

The real distinction comes from the "couple" thing, in logger plugin there a just some events which may or may be not related, in history the trace is always with the couple

@sagikazarmark
Copy link
Member

I see. Where is history data kept?

@joelwurtz
Copy link
Member Author

The default implementation from ivory http adapter kept data in memory (with a variable), see https://github.com/egeloen/ivory-http-adapter/blob/master/src/Event/History/Journal.php

@dbu
Copy link
Contributor

dbu commented Nov 8, 2015

so the history one would be a meta plugin for other tools (like a symfony toolbar integration) to extract what happened during this request?

@joelwurtz
Copy link
Member Author

so the history one would be a meta plugin for other tools (like a symfony toolbar integration) to extract what happened during this request?

Like a data collector thing yes

@sagikazarmark
Copy link
Member

That sounds good. @dbu can it somehow be added to the HttplugBundle?

@dbu
Copy link
Contributor

dbu commented Nov 8, 2015 via email

@joelwurtz
Copy link
Member Author

Updated the list :

Compression and Chunk should be in one plugin (Encoding)
Add Cache Plugin (i have see many libraries using cache system to avoid doing to many calls to an api endpoint)

@sagikazarmark
Copy link
Member

Cache is something that is experimental even in guzzle, would be nice to have. Not sure though whether we need cache if we have VCR or not.

@joelwurtz
Copy link
Member Author

I think they all depend on a mutual dependancy like the Formatter / Normalizer thing (which can also be used in the Log plugin), so we can serialize a http request in another format. But despite from that workflow and usage is not the same.

@dbu
Copy link
Contributor

dbu commented Nov 9, 2015 via email

@jeromegamez
Copy link

VCR even doesn't cache. In VCR's domain, I'm asking you: what is a cache? Has this something to do with those hipster people strolling through the woods searching for treasures? I just know cassettes, racks and my recorder 😇

@jeromegamez
Copy link

But seriously, I also think that the VCR shouldn't be more than that: the notion of a TTL would already be oit of scope.

@dbu
Copy link
Contributor

dbu commented Nov 9, 2015 via email

@sagikazarmark
Copy link
Member

@jeromegamez you probably spent more time with this than me, so I guess you know better, it was just an idea.

However this raises a question: VCR has a storage layer, doesn't it?

I agree, that caching is something else.

@sagikazarmark
Copy link
Member

Plugins repo is probably on the right way. I think we should not finish all the plugins prior to the stable release. Partly because some components (like caching and formatter) require more planning/duscussion, partly because I don't want to hurry just because stable release, partly because we have other work to do: polishing discovery, message factory, documentation, not to mention the packages used by plugins (authentication, cookie).

We probably have enough plugins implemented for the stable release. There are a few "would be nice to have"s, like Base URI, Sanitizer, Encoding, but I think none of them is requirement.

This repo should also be reviewed. I've already found minor CS errors, I will provide a PR for those.

@joelwurtz @dbu what's the situation with plugin docs? Have anybody written any docs?

@dbu
Copy link
Contributor

dbu commented Nov 9, 2015 via email

@joelwurtz
Copy link
Member Author

Will try to begin some documentation on this tomorrow will do that directly on a branch so you can edit yourself

@joelwurtz
Copy link
Member Author

Ok to remove the Timer / Formatter Plugin, they were not plugin in ivory http adapter, just class to add information on the Logger / Journal Plugin ?

@dbu
Copy link
Contributor

dbu commented Nov 13, 2015 via email

@joelwurtz
Copy link
Member Author

I think with the two plugins already in PR, next work will be on the BaseUri plugin and History plugin, after that we can safely release a first / alpha version of plugin-client (BaseUri is the most needed one i think)

@sagikazarmark
Copy link
Member

Agree. I would concentrate on BaseURI and cleaning up/proofreading existing code. History plugin can wait I think.

@dbu
Copy link
Contributor

dbu commented Nov 18, 2015 via email

@sagikazarmark
Copy link
Member

Hm, I remember something about this discussion, but I can't recall where we discussed it.

Also, host is only required for 1.1

@sagikazarmark
Copy link
Member

OAuth v2 authentication

Absolutely, but shouldn't it be an authentication component instead?

https://github.com/php-http/authentication

Page iterator

I would turn it upside down. Since you actually need the iterator, not the client itself, I would use Pagerfanta with a custom adapter. Although I don't think such a plugin/adapter is easy: you need insights about the content, it's format, etc which doesn't seem to be a responsibility of the HTTP part of your code.

@RomeroMsk
Copy link

Absolutely, but shouldn't it be an authentication component instead?

Yes, I think, it's better to make it inside Authentication. So, you are planning to implement this soon? It is worth to be included in plugins TODO list then.

which doesn't seem to be a responsibility of the HTTP part of your code

You're right, will think about additional utility for this.

@ddeboer
Copy link

ddeboer commented Dec 29, 2015

@RomeroMsk There’s now Bearer support in php-http/message. Is that sufficient for your needs?

@RomeroMsk
Copy link

@ddeboer, not it's not enough. Adding the bearer header to request is not a big problem. We need to fetch the token from auth endpoint first and refetch it in case of 401 HTTP Status.

@Nyholm
Copy link
Member

Nyholm commented Dec 30, 2015

@ddeboer, not it's not enough. Adding the bearer header to request is not a big problem. We need to fetch the token from auth endpoint first and refetch it in case of 401 HTTP Status.

How you fetch and refresh a token is very specific to the API. I think it is hard to try to standardize in a http client/plugin.

@RomeroMsk
Copy link

How you fetch and refresh a token is very specific to the API. I think it is hard to try to standardize in a http client/plugin.

No, OAuth v2 - is standard. Token is fetching by HTTP request with strictly defined parameters. So it can be standardized in plugin.

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

You’re right, @RomeroMsk, the standard OAuth2 flows should be done inside a plugin. Do you have time to work on an OAuth2 plugin?

@Nyholm
Copy link
Member

Nyholm commented Dec 30, 2015

Yes, OAuth v2 is standard. But the implementation of it differ.
Example with a few I've worked with.

Vendor Description
LinkedIn They have no refresh functionality, you simply have to reauthenticate. Uses POST request to get token. Uses a grant_type parameter.
Facebook Does not have a refresh, Uses GET request to get token.
HH.ru Using a POST request to get and refresh the token. Uses a grant_type parameter.
Superjob.ru Using a GET request to get and refresh the token

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

Right, these are (mostly) different implementations of the same RFC 6749 standard.

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

We could have a look at https://github.com/commerceguys/guzzle-oauth2-plugin for inspiration.

@RomeroMsk
Copy link

Do you have time to work on an OAuth2 plugin?

Sorry, have no time to work on it now.

We could have a look at https://github.com/commerceguys/guzzle-oauth2-plugin for inspiration.

Or at this: https://github.com/thephpleague/oauth2-client
Please, note that commerceguys' package is not supporting Guzzle 6 now (commerceguys/guzzle-oauth2-plugin#19).

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

Agreed on integration with the phpleague component, as that’s the direction commerceguys seems to take as well.

@sagikazarmark Do we need a separate plugin or only a php-http/message Authentication component?

@sagikazarmark
Copy link
Member

Hard question, which I am trying to answer for some time. I think the problem is that we can't authenticate and send the original request in one request, which means we need to start at least one new request, which means we need a separate plugin for this.

Another solution would be some kind of Authentication interface which allows doing some extra logic apart from authenticating the request itself.

About league oauth: it looks promising. It would allow us to implement an Authentication method in a nice way. However it uses guzzle 6 internally, which itself is not a problem, but then (at least for now) Guzzle 6 would be a requirement to use our OAuth authentication method. We could probably ask them if they want to use httplug in the future.

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

I think the problem is that we can't authenticate and send the original request in one request, which means we need to start at least one new request, which means we need a separate plugin for this.

True, but the request would be sent by thephpleague/oauth2-client, so either through Guzzle 6 (currently) or later through HTTPlug. As long as our plugin acts on the correct headers (so doesn’t act on OAuth authentication requests), we’re good.

Is the separate oauth2-client dependency a reason to put the OAuth2 plugin in a separate repo or do we already have plugins with optional (yuck) dependencies?

@sagikazarmark
Copy link
Member

OK, let's do this. Having it as an authentication method is nicer than having a separate plugin for it.

@ddeboer
Copy link

ddeboer commented Dec 30, 2015

Ok, so add it to php-http/message and see how far we get?

@sagikazarmark
Copy link
Member

Yup.

Do we need at least an authorization code in advance? In most cases we can't confirm access on an authorization page, can we?

@joelwurtz
Copy link
Member Author

I think oauth2 will be better integrated with an Oauth2Client implementing HttpClient / HttpAsyncClient and decorating an HttpClient / HttpAsyncClient with the PluginClient for the AuthenticationPlugin

WDYT ?

@sagikazarmark
Copy link
Member

Why? I think with the plugin system we wanted to avoid such decorations whenever possible.

Also, if we plan to use league oauth, a client wrapper seems to be an overkill.

@joelwurtz
Copy link
Member Author

Because Oauth2 is not something which change the request / workflow behavior and is more a standard around http. It will do multiple calls and i find it cleaner as a decorator than a plugin.

@sagikazarmark
Copy link
Member

Because Oauth2 is not something which change the request

This is not true, it changes the requests (adds the token).

It will do multiple calls

That's true, but this is why there is a $first callback, isn't it?

Also, if we plan to use league oauth, a client wrapper seems to be an overkill.

What about this one?

@dbu
Copy link
Contributor

dbu commented Dec 31, 2015

lets try to build that plugin and see if it feels iffy. and talk to the league maintainers of oauth to see if they can be interested in httplug.

@sagikazarmark
Copy link
Member

lets try to build that plugin

You mean authentication method?

@dbu
Copy link
Contributor

dbu commented Dec 31, 2015

sorry, got confused by the issue title and all. yes, authentication method.

@joelwurtz
Copy link
Member Author

Closing this, as i think Sanitizer plugin is not useful (separated into other plugins, with the ContentLengthPlugin as an example)

and validator plugin not really a indispensable one, as it does not make sens to have this at runtime IMO

@joelwurtz joelwurtz mentioned this issue Jan 26, 2016
@guillemcanal
Copy link

Regarding the TapeRecorder plugin (PHP-VCR alike), I did an implementation some time ago. The public API is a big wacky and it need some documentation, but, here you go : https://github.com/eleven-labs/http-replay-plugin

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

No branches or pull requests

8 participants