-
Notifications
You must be signed in to change notification settings - Fork 39
Adding PSR package #128
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
Adding PSR package #128
Conversation
fd0fb11
to
af00486
Compare
src/HttpAsyncClient.php
Outdated
@@ -23,5 +23,5 @@ | |||
* | |||
* @throws \Exception If processing the request is impossible (eg. bad configuration). | |||
*/ | |||
public function sendAsyncRequest(RequestInterface $request); | |||
public function sendAsyncRequest(RequestInterface $request): Promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this problem when we are doing the same update for the Async HTTP client PSR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there needs to be a new major version anyways when a promise PSR is out, so we might as well do this change now to make it more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to be more permissive in the async implementation, to allow every value and exception in our promise, this would be better, after years of use it was a mistake to reduce the scope IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, so what do you suggest? I've written Promise
here, not HttpPromise
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it's in another package, we would need to update the promise lib and then allow 2.0 here, but maybe it will not be bc break for this package, so nothing to change here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(We would still need to release a 2.1 with the package dependency upgrade to allow 1.0 || 2.0 for the promise lib)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact what do you think of using https://amphp.org/amp/promises/ interface ? there is an adapter for react and we could drop support of async for guzzle (as it's not really async in fact, it's only parallel)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. Im not sure at all...
On one hand: Do as little change as possible. That will make it easier for everyone.
On the other hand: Can we make the "correct" decision now so there is no need for v3 when we got a PSR for Promise...? Probably not, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how soon is a PSR Promise to be expected? i would assume that will take a couple of years, so if its a clear and good upgrade, i would be ok to have version 2 do something different. then again, to make the PSR move for sync http client as smooth as possible, its maybe better to not limit ourselves here and avoid a BC break
src/Exception/HttpException.php
Outdated
@@ -42,7 +42,7 @@ public function __construct( | |||
* | |||
* @return ResponseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in our projects we started the rule that when @return
just repeats the return type declaration and has no valuable information, we remove the @return
line.
(in that vein "Returns the response" is redundant to the method name. either there is more details to explain or there should be no comment...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. 👍
I will remove these redundant comments.
src/Exception/HttpException.php
Outdated
* @return ResponseInterface | ||
*/ | ||
public function getResponse() | ||
public function getResponse(): ResponseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break: https://3v4l.org/LOOiv
Is this something we want to avoid in this update to make it as smooth as possible?
src/Exception/HttpException.php
Outdated
*/ | ||
public static function create( | ||
RequestInterface $request, | ||
ResponseInterface $response, | ||
\Exception $previous = null | ||
) { | ||
): self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Im done with this PR. I suggest merging it now and start prepare for a 2.0 release. Releasing 2.0 is obviously blocked by a stable release of psr/http-client |
I removed HHVM because it failed because of a PHPSpec error (I think) https://travis-ci.org/php-http/httplug/jobs/409431649 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! so indeed the transition path looks quite sane.
as for merging, i guess as long as we don't release a version, we should be fine even if it turns out that PSR-18 still changes somehow.
please adjust the branch-alias in composer.json to 2.0-dev
CHANGELOG.md
Outdated
@@ -1,5 +1,24 @@ | |||
# Change Log | |||
|
|||
## 2.0.0 - UNRELEASED | |||
|
|||
This version contains is technically a BC break because we drop PHP5 and add return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping PHP5 support is no BC break. it just prevents people on an obsolete stack from updating
how do you mean "technically"? i think we should say: "This version is no BC break for consumers using HTTPlug. However, HTTP clients that implement HTTPlug need to adjust because we add return type declarations."
CHANGELOG.md
Outdated
|
||
### Added | ||
|
||
- Support for PSR-18 (Http client). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http or HTTP? (i don't really care but we should be consistent in our doc and consistent with how fig documents write it)
CHANGELOG.md
Outdated
|
||
### Changed | ||
|
||
- [BC Break] `HttpClient::sendRequest(RequestInterface $request)` has a return type annotation. The mew |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/mew/new/ ;-)
Thank you for the review. I've corrected the PR accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, happy with it like this!
i'll wait a bit if @sagikazarmark has input as well
Looks good to me. 👍 |
I've changed base to a new 2.x branch. |
Thank you for the reviews. |
Yes, This PR is very premature. I just want to make sure we all agree (and look forward to) that this is eventually going to be merged.
This will fix #51