Conversation
| * @param \Psr\Http\Client\ClientInterface|\Http\Client\HttpClient|null $httpClient | ||
| * @param \Psr\Http\Message\RequestFactoryInterface|\Http\Message\RequestFactory|null $requestFactory | ||
| */ | ||
| public function __construct($httpClient = null, $requestFactory = null) |
There was a problem hiding this comment.
Alternatively we could broaden them, so ?RequestFactory|RequestFactoryInterface`
There was a problem hiding this comment.
Pull request overview
Updates Omnipay’s HTTP client layer to support PSR-18/PSR-17 discovery and introduce a new PSR-typed client while keeping compatibility paths for the legacy HTTPlug factory.
Changes:
- Added
AbstractClient+PsrClientand switched the default gateway HTTP client to PSR-based discovery. - Refactored legacy
Clientto extend the new shared implementation and expanded request-body handling behavior. - Updated CI/test matrix and added/expanded unit tests for request construction and body normalization.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Common/Http/PsrClientTest.php | Adds coverage for PSR-typed client request creation/body handling and exception wrapping. |
| tests/Common/Http/ClientTest.php | Expands coverage for the legacy client to match new body handling behavior. |
| tests/Common/AbstractGatewayTest.php | Updates assertions to match the new default HTTP client type. |
| src/Common/Message/AbstractRequest.php | Removes direct dependency on the legacy Client class import. |
| src/Common/Http/PsrClient.php | Introduces a PSR-typed client wrapper around the shared implementation. |
| src/Common/Http/Client.php | Deprecates legacy client and delegates to AbstractClient. |
| src/Common/Http/AbstractClient.php | New shared implementation: PSR-18 discovery, PSR-17 request building, body normalization, exception wrapping. |
| src/Common/AbstractGateway.php | Switches default HTTP client from legacy Client to PsrClient. |
| composer.json | Updates platform/dependencies to support PSR factories and adjusts deprecated message-factory dependency placement. |
| .github/workflows/phpunit.yml | Updates CI PHP matrix and lowest-deps job configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "require-dev": { | ||
| "omnipay/tests": "^4.1", | ||
| "php-http/mock-client": "^1.6", | ||
| "php-http/guzzle7-adapter": "^1", | ||
| "squizlabs/php_codesniffer": "^3.8.1", | ||
| "http-interop/http-factory-guzzle": "^1.1" | ||
| "http-interop/http-factory-guzzle": "^1.1", | ||
| "php-http/message-factory": "^1.1" | ||
| }, |
There was a problem hiding this comment.
composer.json moves php-http/message-factory to require-dev, but production code (e.g., src/Common/Http/AbstractClient.php and src/Common/Http/Client.php) still references Http\Message\RequestFactory in imports and type declarations. If users install without require-dev, PHP will fatal when loading these classes. Either keep php-http/message-factory in require, or remove the hard type dependency (e.g., accept an untyped legacy factory and only interact with it behind an interface_exists/class_exists check).
| "php": "^8", | ||
| "php-http/client-implementation": "^1", |
There was a problem hiding this comment.
The PR description suggests this is not a big BC break, but composer.json now requires PHP ^8 (dropping ^7.2|^8). If this is intentional, it should be called out explicitly as a BC break (and aligned with the project’s release/UPGRADE notes). If not intentional, restore the previous PHP constraint.
| public function testRequestWithInvalidBodyType() | ||
| { | ||
| $mockClient = m::mock(HttpClient::class); | ||
| $client = new Client($mockClient, MessageFactoryDiscovery::find()); | ||
|
|
||
| $this->expectException(\InvalidArgumentException::class); | ||
| $this->expectExceptionMessage('Invalid resource type: array'); | ||
|
|
||
| $client->request('POST', '/path', [], ['invalid' => 'array']); | ||
| } |
There was a problem hiding this comment.
ClientTest expects exception messages 'Invalid resource type: ...', but AbstractClient throws 'Invalid body type: ...'. These assertions will fail; update the expected messages (or align the production exception message) so Client and PsrClient behave consistently.
| public function testRequestWithInvalidObjectBody() | ||
| { | ||
| $mockClient = m::mock(HttpClient::class); | ||
| $client = new Client($mockClient, MessageFactoryDiscovery::find()); | ||
|
|
||
| $object = new \stdClass(); | ||
|
|
||
| $this->expectException(\InvalidArgumentException::class); | ||
| $this->expectExceptionMessage('Invalid resource type: object'); | ||
|
|
||
| $client->request('POST', '/path', [], $object); | ||
| } |
There was a problem hiding this comment.
ClientTest expects exception messages 'Invalid resource type: ...', but AbstractClient throws 'Invalid body type: ...'. These assertions will fail; update the expected messages (or align the production exception message) so Client and PsrClient behave consistently.
|
|
||
| class Client implements ClientInterface | ||
| /** | ||
| * @deprecated use Psr18Client instead |
There was a problem hiding this comment.
The @deprecated tag says 'use Psr18Client instead', but this PR introduces Omnipay\Common\Http\PsrClient (and there is no Psr18Client class in src/). Update the deprecation message to reference the actual replacement to avoid misleading users.
| * @deprecated use Psr18Client instead | |
| * @deprecated use PsrClient instead |
| use Http\Message\RequestFactory; | ||
| 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\StreamInterface; | ||
|
|
||
| class Client implements ClientInterface | ||
| /** | ||
| * @deprecated use Psr18Client instead | ||
| */ | ||
| class Client extends AbstractClient | ||
| { | ||
| /** | ||
| * The Http Client which implements `public function sendRequest(RequestInterface $request)` | ||
| * Note: Will be changed to PSR-18 when released | ||
| * | ||
| * @var HttpClient | ||
| */ | ||
| private $httpClient; | ||
|
|
||
| /** | ||
| * @var RequestFactory | ||
| */ | ||
| private $requestFactory; | ||
|
|
||
| public function __construct($httpClient = null, ?RequestFactory $requestFactory = null) | ||
| { | ||
| $this->httpClient = $httpClient ?: HttpClientDiscovery::find(); | ||
| $this->requestFactory = $requestFactory ?: MessageFactoryDiscovery::find(); | ||
| } | ||
|
|
||
| /** | ||
| * @param $method | ||
| * @param $uri | ||
| * @param array $headers | ||
| * @param string|array|resource|StreamInterface|null $body | ||
| * @param string $protocolVersion | ||
| * @return ResponseInterface | ||
| * @throws \Http\Client\Exception | ||
| */ | ||
| public function request( | ||
| $method, | ||
| $uri, | ||
| array $headers = [], | ||
| $body = null, | ||
| $protocolVersion = '1.1' | ||
| ) { | ||
| $request = $this->requestFactory->createRequest($method, $uri, $headers, $body, $protocolVersion); | ||
|
|
||
| return $this->sendRequest($request); | ||
| } | ||
|
|
||
| /** | ||
| * @param RequestInterface $request | ||
| * @return ResponseInterface | ||
| * @throws \Http\Client\Exception | ||
| */ | ||
| private function sendRequest(RequestInterface $request) | ||
| { | ||
| try { | ||
| return $this->httpClient->sendRequest($request); | ||
| } catch (\Http\Client\Exception\NetworkException $networkException) { | ||
| throw new NetworkException($networkException->getMessage(), $request, $networkException); | ||
| } catch (\Exception $exception) { | ||
| throw new RequestException($exception->getMessage(), $request, $exception); | ||
| } | ||
| parent::__construct($httpClient, $requestFactory); |
There was a problem hiding this comment.
Client.php imports and typehints Http\Message\RequestFactory. If php-http/message-factory is no longer a required dependency, this hard reference can trigger fatal errors when loading the class. Either keep the dependency in require or remove/loosen this typehint to avoid a mandatory runtime dependency on the abandoned package.
|
|
||
| public function __construct( | ||
| $httpClient = null, | ||
| RequestFactoryInterface|RequestFactory $requestFactory = null, |
There was a problem hiding this comment.
AbstractClient has a hard type reference to Http\Message\RequestFactory in the union type. If php-http/message-factory is not a required dependency, this will fatal at load time. Consider removing the union type (accept an untyped legacy factory) and only using the legacy path when the interface exists, or keep php-http/message-factory as a required dependency.
| RequestFactoryInterface|RequestFactory $requestFactory = null, | |
| RequestFactoryInterface|object $requestFactory = null, |
| /** | ||
| * @param RequestInterface $request | ||
| * @return ResponseInterface | ||
| * @throws \Http\Client\Exception | ||
| */ | ||
| private function sendRequest(RequestInterface $request) | ||
| { | ||
| try { | ||
| return $this->httpClient->sendRequest($request); | ||
| } catch (NetworkExceptionInterface $networkException) { | ||
| throw new NetworkException($networkException->getMessage(), $request, $networkException); | ||
| } catch (\Exception $exception) { | ||
| throw new RequestException($exception->getMessage(), $request, $exception); | ||
| } |
There was a problem hiding this comment.
sendRequest()’s docblock says it throws \Http\Client\Exception, but the implementation catches and wraps exceptions into Omnipay’s NetworkException/RequestException (and also depends on PSR-18 NetworkExceptionInterface). Update the docblock to reflect the actual thrown exceptions to avoid misleading implementers.
Fixes #262
So I don't think this is a (big) BC break:
The RequestInterface is slightly different, but the method name is the same, just the number of parameters has changed. But the new way should work for both. The typehint allows both versions , so should not be a BC break, unless maybe autowiring? Not sure if I can avoid that though..A new optional parameter for the StreamInterfaceChain the methods instead of calling directly