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

Add plugin documentation #30

Merged
merged 1 commit into from Nov 12, 2015
Merged

Add plugin documentation #30

merged 1 commit into from Nov 12, 2015

Conversation

joelwurtz
Copy link
Member

See php-http/plugins#3

Don't know if we need to provide documentation of each plugin here, or if we can do it in another pull request ?

@@ -1,5 +1,202 @@
# Plugin System

The plugin system will allow to manipulate requests and responses inside a `HttpClient`.
The plugin system allow to manipulate requests and responses inside a `HttpClient`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general: the correct form is aN HTTP Client.

@sagikazarmark
Copy link
Member

I think plugin docs can go into this PR.

@joelwurtz
Copy link
Member Author

Ok this PR is based on a branch directly on this repo, if some people want to help to contribute on one of the plugin documentation you are welcome :)

@sagikazarmark
Copy link
Member

Will do

@@ -1,5 +1,202 @@
# Plugin System

The plugin system will allow to manipulate requests and responses inside a `HttpClient`.
The plugin system allow to manipulate requests and responses inside an `HttpClient`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/allow/allows/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though "manipulate" is a bit misleading, as they are immutable. should we say "to look at requests and responses and replace them if needed"?

@dbu
Copy link
Contributor

dbu commented Nov 12, 2015

very interesting to read how the plugins work! i commented a lot of details, hope you don't mind joel.

process: i suggest we wrap up the main section and then merge without each plugin description already in. otherwise we might wait for quite a while until making this available. will also be easier to review the individual plugin docs in separate pull requests

plugins: how would a cache plugin work? it would simply not call $next but return the response as a promise, right?
what about something like the redirect plugin, it has to call $first, right? if a plugin wants to work with the response, it has to wait on the promise, right? and if it cares about exceptions (e.g. logging) it has to try-catch as well and rethrow?

@joelwurtz
Copy link
Member Author

very interesting to read how the plugins work! i commented a lot of details, hope you don't mind joel.

Quite the opposite thanks for the remarks.

process: i suggest we wrap up the main section and then merge without each plugin description already in. otherwise we might wait for quite a while until making this available. will also be easier to review the individual plugin docs in separate pull requests

👍

plugins: how would a cache plugin work? it would simply not call $next but return the response as a promise, right?

Exactly, or return the exception as the promise if an error happens

what about something like the redirect plugin, it has to call $first, right?

Yes that exaclty what it's done actually see : https://github.com/php-http/plugins/blob/master/src/RedirectPlugin.php

if a plugin wants to work with the response, it has to wait on the promise, right?

No it's just call the then method to add behavior when the response is resolved, see the CookiePlugin for example : https://github.com/php-http/plugins/blob/master/src/CookiePlugin.php

However there is some case when you need to wait for the promise when you have an underlying request by example (like in the RetryPlugin or RedirectPlugin)

and if it cares about exceptions (e.g. logging) it has to try-catch as well and rethrow?

No it's simply define the second callable of the then method which allow to handle the exception, but you must rethrow the exception inside this callable (and not return, otherwise it will consider the exception like it was the Response and put in the next onFulfilled callable)

@dbu
Copy link
Contributor

dbu commented Nov 12, 2015

ah, still new to promises. makes a lot of sense. can you maybe integrate your answers into the documentation to make things more clear? i guess i am not the only one not familiar with promises, so having more details could help. maybe we should move the "write your own plugin" into its own chapter inside the plugins/ folder. in a way, its the least important, coming after using the existing plugins. it would be cool to show the core code of plugins to explain cases:

  • alter a request
  • alter a response
  • prematurely end a request with a synthetic response (cache, vcr, mocking)
  • start a new request and return that result instead
  • abort a request prematurely with an error
  • abort a response with an error (Error plugin, or for example server validation fails)

@sagikazarmark
Copy link
Member

I am new to promises as well, so expect a few dumb questions @joelwurtz 😜

@joelwurtz
Copy link
Member Author

So all things should be good unless there is still remarks ?


## Order of plugins

When you inject an array of plugins into the `PluginClient`, the order of the plugins, injected into the `PluginClient`, matters.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> When you inject an array of plugins into the PluginClient, the order of the plugins matters.

otherwise the phrase is repeating itself

@dbu
Copy link
Contributor

dbu commented Nov 12, 2015

i have a few more grammatical comments, otherwise this looks ready to merge to me.
i will try to do #31, looking at the existing plugins and if necessary asking you questions. then i will learn about promises...

joelwurtz added a commit that referenced this pull request Nov 12, 2015
@joelwurtz joelwurtz merged commit cffbd09 into master Nov 12, 2015
@joelwurtz joelwurtz mentioned this pull request Nov 13, 2015
@ddeboer ddeboer deleted the feature/plugins branch January 2, 2016 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants