Skip to content

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jul 30, 2018
Merged

Adding PSR package #128

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
af00486
Adding PSR package
Nyholm Aug 18, 2017
cbc903a
Added return types and dropped PHP5
Nyholm Jul 23, 2018
de1c010
Removed redundant comments
Nyholm Jul 24, 2018
307c7fe
Removed return type
Nyholm Jul 24, 2018
58d0ada
Removed return types
Nyholm Jul 24, 2018
7d9f4ec
Make sure our exception implements the correct interfaces
Nyholm Jul 29, 2018
f217324
Make it more clear what interface we are implementing
Nyholm Jul 29, 2018
310b13f
minor
Nyholm Jul 29, 2018
b45b737
Use version 0.1
Nyholm Jul 29, 2018
f5b7e91
Remove HHVM
Nyholm Jul 29, 2018
b1f4ee6
Added change log
Nyholm Jul 29, 2018
1b29077
Small fixes
Nyholm Jul 30, 2018
b16582b
Updated branch alias
Nyholm Jul 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ cache:
- $HOME/.composer/cache/files

php:
- 5.4
- 5.5
- 5.6
- 7.0
- 7.1
- 7.2
Expand Down
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
}
],
"require": {
"php": ">=5.4",
"php": "^7.0",
"psr/http-message": "^1.0",
"psr/http-client": "^1.0",
"php-http/promise": "^1.0"
},
"require-dev": {
Expand Down
2 changes: 1 addition & 1 deletion src/Exception.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
*
* @author Márk Sági-Kazár <[email protected]>
*/
interface Exception
interface Exception extends \Throwable
{
}
4 changes: 2 additions & 2 deletions src/Exception/HttpException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function __construct(
*
* @return ResponseInterface
Copy link
Contributor

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...)

Copy link
Member Author

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.

*/
public function getResponse()
public function getResponse(): ResponseInterface
Copy link
Member Author

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?

{
return $this->response;
}
Expand All @@ -60,7 +60,7 @@ public static function create(
RequestInterface $request,
ResponseInterface $response,
\Exception $previous = null
) {
): self {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

$message = sprintf(
'[url] %s [http method] %s [status code] %s [reason phrase] %s',
$request->getRequestTarget(),
Expand Down
2 changes: 1 addition & 1 deletion src/Exception/RequestException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function __construct($message, RequestInterface $request, \Exception $pre
*
* @return RequestInterface
*/
public function getRequest()
public function getRequest(): RequestInterface
{
return $this->request;
}
Expand Down
2 changes: 1 addition & 1 deletion src/HttpAsyncClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ interface HttpAsyncClient
*
* @throws \Exception If processing the request is impossible (eg. bad configuration).
*/
public function sendAsyncRequest(RequestInterface $request);
public function sendAsyncRequest(RequestInterface $request): Promise;
Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

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)

Copy link
Member

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)

Copy link
Member Author

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?

Copy link
Contributor

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

}
5 changes: 3 additions & 2 deletions src/HttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Http\Client;

use Http\HttplugBundle\Tests\Unit\Collector\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;

Expand All @@ -12,7 +13,7 @@
* @author Márk Sági-Kazár <[email protected]>
* @author David Buchmann <[email protected]>
*/
interface HttpClient
interface HttpClient extends ClientInterface
{
/**
* Sends a PSR-7 request.
Expand All @@ -24,5 +25,5 @@ interface HttpClient
* @throws \Http\Client\Exception If an error happens during processing the request.
* @throws \Exception If processing the request is impossible (eg. bad configuration).
*/
public function sendRequest(RequestInterface $request);
public function sendRequest(RequestInterface $request): ResponseInterface;
}