Use http based promise in emulated trait and specs #42
Conversation
| @@ -12,7 +12,7 @@ | |||
| ], | |||
| "require": { | |||
| "php": ">=5.4", | |||
| "php-http/httplug": "^1.0", | |||
| "php-http/httplug": "^1.1.0", | |||
There was a problem hiding this comment.
^1.1 would be fine here
There was a problem hiding this comment.
Yes, it's better, thanks.
joelwurtz
force-pushed
the
fix/switch-to-http-promise
branch
from
80d58d6 to
07dc655
Compare
|
Ok, so what happens when a non-http exception gets thrown? |
joelwurtz
force-pushed
the
fix/switch-to-http-promise
branch
from
25debfb to
0cf1adb
Compare
|
When a non-httplug exception is thrown we abort the plugin chain and the exception bubble up like normal. Only httplug exception will respect the plugin chain. |
|
This will break other libraries though. |
|
I don't think so. If you used a plugin with rejection callbacks broke your library too. This was a bug IMO, which has been solved. Also, this bug only affects you if you use plugins. |
| @@ -24,7 +24,7 @@ | |||
| * @param callable $next Next middleware in the chain, the request is passed as the first argument | |||
| * @param callable $first First middleware in the chain, used to to restart a request | |||
| * | |||
| * @return Promise | |||
| * @return Promise Resolves a PSR-7 Response or fails with an Http\Client\Exception (The same as HttpAsyncClient). | |||
There was a problem hiding this comment.
Is this a BC break ? (For me no, it's more a clarification but i dont know if this interface is already used but other packages than one we have under control ?)
There was a problem hiding this comment.
Could we improve this doc block somehow even more?
Maybe describe what the callables will return?
There was a problem hiding this comment.
Don't we have already this on the documentation ? if so would just add a link with @see
There was a problem hiding this comment.
We don't but we should. Write
@see http://php-http.readthedocs.io/en/latest/plugins/introduction.html
There was a problem hiding this comment.
From my POV, it's not. Http Promises still fullfill the Promise interface.
There was a problem hiding this comment.
Add "@see http://docs.php-http.org/en/latest/plugins/build-your-own.html". I've added some lines about exceptions (php-http/documentation#151)
Is this a BC break ?
No, this is not a BC break.
|
I added some explanation here: https://github.com/php-http/httplug/releases/tag/v1.1.0 |
joelwurtz
force-pushed
the
fix/switch-to-http-promise
branch
from
0cf1adb to
cabbf75
Compare
|
Thank you @joelwurtz. Is this ready for merge? |
|
Please gimme a note when this can be merged and tagged because I would like to do some other things too. |
|
For me all the work is done here, so feel free to merge if it's ok to you (not a fan of merging my own PR in a collaboration project, it feels presumptuous...) |
|
Thank you for this PR. Good work! |
See issue #34 and discussion in #35 also for more information