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

Implement PSR-17 + PSR-18 #207

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
language: php

php:
- 5.6
- 7.0
- 7.1
- 7.2
- 7.3
Expand All @@ -24,9 +22,9 @@ env:

matrix:
include:
- php: 5.6
- php: 7.1
env: setup=lowest symfony="^2.1"
- php: 5.6
- php: 7.1
env: setup=lowest symfony="^3"
- php: 7.1
env: setup=lowest symfony="^4"
Expand Down
15 changes: 9 additions & 6 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,15 @@
"classmap": ["src/Omnipay.php"]
},
"require": {
"php": "^5.6|^7",
"php-http/client-implementation": "^1",
"php-http/message": "^1.5",
"php-http/discovery": "^1.2.1",
"php": "^7.1",
"moneyphp/money": "^3.1",
"php-http/discovery": "^1.6",
"psr/http-client": "^1",
"psr/http-client-implementation": "^1",
"psr/http-factory": "^1",
"psr/http-factory-implementation": "^1",
"symfony/http-foundation": "^2.1||^3||^4",
"moneyphp/money": "^3.1"
"nyholm/psr7": "^1"
},
"require-dev": {
"omnipay/tests": "^3",
Expand All @@ -53,7 +56,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "3.0.x-dev"
"dev-master": "3.1.x-dev"
}
},
"suggest": {
Expand Down
80 changes: 62 additions & 18 deletions src/Common/Http/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,47 +2,78 @@

namespace Omnipay\Common\Http;

use function GuzzleHttp\Psr7\str;
use Http\Client\HttpClient;
use Http\Discovery\HttpClientDiscovery;
use Http\Discovery\MessageFactoryDiscovery;
use Http\Message\RequestFactory;
use Http\Discovery\Psr17FactoryDiscovery;
use Http\Discovery\Psr18ClientDiscovery;
use Omnipay\Common\Http\Exception\NetworkException;
use Omnipay\Common\Http\Exception\RequestException;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\StreamFactoryInterface;
use Psr\Http\Message\StreamInterface;
use Psr\Http\Message\UriInterface;
use Psr\Http\Client\ClientInterface as HttpClientInterface;
use Psr\Http\Message\RequestFactoryInterface;

class Client implements ClientInterface
{
/**
* The Http Client which implements `public function sendRequest(RequestInterface $request)`
* Note: Will be changed to PSR-18 when released
*
* @var HttpClient
* @var HttpClientInterface
*/
private $httpClient;

/**
* @var RequestFactory
* @var RequestFactoryInterface
*/
private $requestFactory;

public function __construct($httpClient = null, RequestFactory $requestFactory = null)
/**
* @var StreamFactoryInterface
*/
private $streamFactory;

public function __construct($httpClient = null, $requestFactory = null, $streamFactory = null)
Copy link
Member

Choose a reason for hiding this comment

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

Should these all be type-hinted? I'm not sure if a later PHP breaks this by requiring the typehint to be optional e,g. ?StreamFactoryInterface and that's the reason for leaving optional parameters unhinted? Just discovering this myself.

Suggested change
public function __construct($httpClient = null, $requestFactory = null, $streamFactory = null)
public function __construct(HttpClientInterface $httpClient = null, RequestFactoryInterface$ requestFactory = null, StreamFactoryInterface $streamFactory = null)

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason they are not typehinted, is that current versions use HttpPlug instead of PSR Http Client, so that would be a breaking change.

{
$this->httpClient = $httpClient;
$this->requestFactory = $requestFactory;
$this->streamFactory = $streamFactory;
}

private function getHttpClient() : HttpClientInterface
{
if (!$this->httpClient) {
$this->httpClient = Psr18ClientDiscovery::find();
}

return $this->httpClient;
}

private function getRequestFactory() : RequestFactoryInterface
{
$this->httpClient = $httpClient ?: HttpClientDiscovery::find();
$this->requestFactory = $requestFactory ?: MessageFactoryDiscovery::find();
if (!$this->requestFactory) {
$this->requestFactory = Psr17FactoryDiscovery::findRequestFactory();
}

return $this->requestFactory;
}

private function getStreamFactory() : StreamFactoryInterface
{
if (!$this->streamFactory) {
$this->streamFactory = Psr17FactoryDiscovery::findStreamFactory();
}

return $this->streamFactory;
}

/**
* @param $method
* @param $uri
* @param array $headers
* @param string|array|resource|StreamInterface|null $body
* @param string|null|StreamInterface $body
* @param string $protocolVersion
* @return ResponseInterface
* @throws \Http\Client\Exception
* @throws NetworkException|RequestException
*/
public function request(
$method,
Expand All @@ -51,23 +82,36 @@ public function request(
$body = null,
$protocolVersion = '1.1'
) {
$request = $this->requestFactory->createRequest($method, $uri, $headers, $body, $protocolVersion);
$request = $this->getRequestFactory()
->createRequest($method, $uri)
->withProtocolVersion($protocolVersion);

if ($body !== null && $body !== '') {
if (is_string($body)) {
$body = $this->getStreamFactory()->createStream($body);
}
$request = $request->withBody($body);
}

foreach ($headers as $name => $value) {
$request = $request->withHeader($name, $value);
}

return $this->sendRequest($request);
}

/**
* @param RequestInterface $request
* @return ResponseInterface
* @throws \Http\Client\Exception
* @throws NetworkException|RequestException
*/
private function sendRequest(RequestInterface $request)
{
try {
return $this->httpClient->sendRequest($request);
return $this->getHttpClient()->sendRequest($request);
} catch (\Http\Client\Exception\NetworkException $networkException) {
throw new NetworkException($networkException->getMessage(), $request, $networkException);
} catch (\Exception $exception) {
} catch (\Throwable $exception) {
throw new RequestException($exception->getMessage(), $request, $exception);
}
}
Expand Down
84 changes: 53 additions & 31 deletions tests/Omnipay/Common/Http/ClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,18 @@
use Http\Client\Exception\NetworkException;
use Mockery as m;
use GuzzleHttp\Psr7\Request;
use Http\Client\HttpClient;
use Http\Message\RequestFactory;
use Omnipay\Common\Http\Exception\RequestException;
use Omnipay\Tests\TestCase;
use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\RequestFactoryInterface;
use Psr\Http\Message\RequestInterface;

class ClientTest extends TestCase
{
public function testEmptyConstruct()
public function testSendGet()
{
$client = new Client();

$this->assertAttributeInstanceOf(HttpClient::class, 'httpClient', $client);
$this->assertAttributeInstanceOf(RequestFactory::class, 'requestFactory', $client);
}

public function testSend()
{
$mockClient = m::mock(HttpClient::class);
$mockFactory = m::mock(RequestFactory::class);
$mockClient = m::mock(ClientInterface::class);
$mockFactory = m::mock(RequestFactoryInterface::class);
$client = new Client($mockClient, $mockFactory);

$request = new Request('GET', '/path');
Expand All @@ -33,9 +26,6 @@ public function testSend()
$mockFactory->shouldReceive('createRequest')->withArgs([
'GET',
'/path',
[],
null,
'1.1',
])->andReturn($request);

$mockClient->shouldReceive('sendRequest')
Expand All @@ -44,13 +34,54 @@ public function testSend()
->once();

$this->assertSame($response, $client->request('GET', '/path'));
}

public function testSendPostJson()
{
$mockClient = m::mock(ClientInterface::class);
$mockFactory = m::mock(RequestFactoryInterface::class);
$client = new Client($mockClient, $mockFactory);

$request = new Request('POST', '/path');
$response = new Response();

$mockFactory->shouldReceive('createRequest')->withArgs([
'POST',
'/path',
])->andReturn($request);

$mockClient->shouldReceive('sendRequest')
->with(m::on(function (RequestInterface $request) {

if ($request->getMethod() !== 'POST') {
return false;
}

if ($request->getHeader('Content-Type') !== ['application/json']) {
return false;
}

if ((string) $request->getBody() !== '{foo:bar}') {
return false;
}

return true;
}))
->andReturn($response)
->once();

$this->assertSame($response, $client->request(
'POST',
'/path',
['Content-Type' => 'application/json'],
'{foo:bar}'
));
}

public function testSendException()
{
$mockClient = m::mock(HttpClient::class);
$mockFactory = m::mock(RequestFactory::class);
$mockClient = m::mock(ClientInterface::class);
$mockFactory = m::mock(RequestFactoryInterface::class);
$client = new Client($mockClient, $mockFactory);

$request = new Request('GET', '/path');
Expand All @@ -59,9 +90,6 @@ public function testSendException()
$mockFactory->shouldReceive('createRequest')->withArgs([
'GET',
'/path',
[],
null,
'1.1',
])->andReturn($request);

$mockClient->shouldReceive('sendRequest')
Expand All @@ -76,8 +104,8 @@ public function testSendException()

public function testSendNetworkException()
{
$mockClient = m::mock(HttpClient::class);
$mockFactory = m::mock(RequestFactory::class);
$mockClient = m::mock(ClientInterface::class);
$mockFactory = m::mock(RequestFactoryInterface::class);
$client = new Client($mockClient, $mockFactory);

$request = new Request('GET', '/path');
Expand All @@ -86,9 +114,6 @@ public function testSendNetworkException()
$mockFactory->shouldReceive('createRequest')->withArgs([
'GET',
'/path',
[],
null,
'1.1',
])->andReturn($request);

$mockClient->shouldReceive('sendRequest')
Expand All @@ -103,8 +128,8 @@ public function testSendNetworkException()

public function testSendExceptionGetRequest()
{
$mockClient = m::mock(HttpClient::class);
$mockFactory = m::mock(RequestFactory::class);
$mockClient = m::mock(ClientInterface::class);
$mockFactory = m::mock(RequestFactoryInterface::class);
$client = new Client($mockClient, $mockFactory);

$request = new Request('GET', '/path');
Expand All @@ -113,9 +138,6 @@ public function testSendExceptionGetRequest()
$mockFactory->shouldReceive('createRequest')->withArgs([
'GET',
'/path',
[],
null,
'1.1',
])->andReturn($request);

$exception = new \Exception('Something went wrong');
Expand Down