Skip to content

Fix broken HttpMethodsClient with PSR RequestFactory #202

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 4 commits into from
Jul 19, 2020
Merged

Fix broken HttpMethodsClient with PSR RequestFactory #202

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0cacffa
Bumped version
GrahamCampbell Jul 18, 2020
a95df4a
Fix broken HttpMethodsClient with Psr\Http\Message\RequestFactoryInte…
GrahamCampbell Jul 18, 2020
9d1d462
Fixed syntax error
GrahamCampbell Jul 18, 2020
e907ad7
Allow the empty string through without a stream factory
GrahamCampbell Jul 19, 2020
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
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "2.2.x-dev"
"dev-master": "2.3.x-dev"
}
}
}
70 changes: 63 additions & 7 deletions src/HttpMethodsClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
use Psr\Http\Message\RequestFactoryInterface;
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;

final class HttpMethodsClient implements HttpMethodsClientInterface
{
Expand All @@ -22,19 +25,29 @@ final class HttpMethodsClient implements HttpMethodsClientInterface
*/
private $requestFactory;

/**
* @var StreamFactoryInterface|null
*/
private $streamFactory;

/**
* @param RequestFactory|RequestFactoryInterface
*/
public function __construct(ClientInterface $httpClient, $requestFactory)
public function __construct(ClientInterface $httpClient, $requestFactory, StreamFactoryInterface $streamFactory = null)
{
if (!$requestFactory instanceof RequestFactory && !$requestFactory instanceof RequestFactoryInterface) {
throw new \TypeError(
sprintf('%s::__construct(): Argument #2 ($requestFactory) must be of type %s|%s, %s given', self::class, RequestFactory::class, RequestFactoryInterface::class, get_debug_type($requestFactory))
);
}

if (!$requestFactory instanceof RequestFactory && null === $streamFactory) {
@trigger_error(sprintf('Passing a %s without a %s to %s::__construct() is deprecated as of version 2.3 and will be disallowed in version 3.0. A stream factory is required to create a request with a string body.', RequestFactoryInterface::class, StreamFactoryInterface::class, self::class));
Copy link
Contributor

Choose a reason for hiding this comment

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

i assume that in next major, we might want to only support the psr factory, to move forward? then we could simply make the stream factory a required argument... (no need to change anything here, just a thought ;-) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too. I am thinking once the old interfaces are all marked as deprecated in the future (https://github.com/php-http/message-factory/blob/master/src/ResponseFactory.php doesn't have a deprecation annotation yet), we can come back here to this constructor and also add a futher deprecation warning for if a psr request factory is not passed. Then yeh, in the next major version make all prams typed with psr interfaces, and all required.

}

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

public function get($uri, array $headers = []): ResponseInterface
Expand Down Expand Up @@ -79,12 +92,55 @@ public function options($uri, array $headers = [], $body = null): ResponseInterf

public function send(string $method, $uri, array $headers = [], $body = null): ResponseInterface
{
return $this->sendRequest($this->requestFactory->createRequest(
$method,
$uri,
$headers,
$body
));
if (!is_string($uri) && !$uri instanceof UriInterface) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is not a BC break, even if the caller didn't have strict typing and expected stuff to be cast to a string at runtime, because this file itself has strict typing and was previously forwarding the parameter as is, so the result would still be a type error later, but just a more obscure error message. Including an early guard here will help callers correct their error, and also checking the uri and body are the correct types here mean our logic later on within the private createRequest function is a little simpler than it would have otherwise been.

throw new \TypeError(
sprintf('%s::send(): Argument #2 ($uri) must be of type string|%s, %s given', self::class, UriInterface::class, get_debug_type($uri))
);
}

if (!is_string($body) && !$body instanceof StreamInterface && null !== $body) {
throw new \TypeError(
sprintf('%s::send(): Argument #4 ($body) must be of type string|%s|null, %s given', self::class, StreamInterface::class, get_debug_type($body))
);
}

return $this->sendRequest(
self::createRequest($method, $uri, $headers, $body)
);
}

/**
* @param string|UriInterface $uri
* @param string|StreamInterface|null $body
*/
private function createRequest(string $method, $uri, array $headers = [], $body = null): RequestInterface
{
if ($this->requestFactory instanceof RequestFactory) {
return $this->requestFactory->createRequest(
$method,
$uri,
$headers,
$body
);
}

if (is_string($body) && null === $this->streamFactory) {
throw new \RuntimeException('Cannot create request: A stream factory is required to create a request with a string body.');
}

$request = $this->requestFactory->createRequest($method, $uri);

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

if (null !== $body) {
$request = $request->withBody(
is_string($body) ? $this->streamFactory->createStream($body) : $body
);
}

return $request;
}

public function sendRequest(RequestInterface $request): ResponseInterface
Expand Down