feat: PSR18 #885
feat: PSR18 #885
Conversation
| "paragonie/random_compat": "^1 || ^2 || ^9.99" | ||
| }, | ||
| "require-dev": { | ||
| "guzzlehttp/guzzle": "^7.3", | ||
| "guzzlehttp/psr7": "^2.0@RC", |
There was a problem hiding this comment.
For tests, uses RC because this package doesn't declare providing the PSR17 factories until 2.x.
| $collaborators['httpClient'] = new HttpClient( | ||
| array_intersect_key($options, array_flip($client_options)) | ||
| ); | ||
| throw new \LogicException('Must specify client'); |
There was a problem hiding this comment.
The previous workflow no longer works, we cannot create a default client since we don't know what's available.
There was a problem hiding this comment.
One could check if the guzzle client class exists and instantiate if it does.
That could be a smoother transition but it will be less generic and impartial.
There was a problem hiding this comment.
I agree, was thinking about extracting that logic into a factory, something like
FallbackHttpClient::discover();which could then do some heuristic to find some popular clients. WDYT?
There was a problem hiding this comment.
There are 3 ways.
- No fallback
- Just Guzzle.
- https://github.com/php-http/discovery
To "find popular clients" is the job of the discovery package.
There was a problem hiding this comment.
OK, so we can test for any of those and try to construct the client.
| } catch (BadResponseException $e) { | ||
| $response = $e->getResponse(); | ||
| } catch (ClientExceptionInterface $e) { | ||
| throw $e; |
There was a problem hiding this comment.
True, But that is a good thing =)
The ClientException will only be thrown when it is impossible to generate a response. Ie network errors or the Request is invalid (missing method) etc.
| $collaborators['httpClient'] = new HttpClient( | ||
| array_intersect_key($options, array_flip($client_options)) | ||
| ); | ||
| throw new \LogicException('Must specify client'); |
There was a problem hiding this comment.
One could check if the guzzle client class exists and instantiate if it does.
That could be a smoother transition but it will be less generic and impartial.
| @@ -222,7 +217,7 @@ public function setHttpClient(HttpClientInterface $client) | |||
| /** | |||
| * Returns the HTTP client instance. | |||
| * | |||
| * @return HttpClientInterface | |||
| * @return ClientInterface | |||
| */ | |||
| public function getHttpClient() | |||
There was a problem hiding this comment.
Is this method really needed? But it is maybe out of scope for this PR.
There was a problem hiding this comment.
Yeah, I'd keep it for now, but since this is a BC break, this will have to be 3.x anyway.
| } catch (BadResponseException $e) { | ||
| $response = $e->getResponse(); | ||
| } catch (ClientExceptionInterface $e) { | ||
| throw $e; |
There was a problem hiding this comment.
True, But that is a good thing =)
The ClientException will only be thrown when it is impossible to generate a response. Ie network errors or the Request is invalid (missing method) etc.
| */ | ||
| public function getResponse(RequestInterface $request) | ||
| { | ||
| return $this->getHttpClient()->send($request); | ||
| return $this->getHttpClient()->sendRequest($request); |
There was a problem hiding this comment.
Maybe add logic here to follow redirects?
| /** | ||
| * @var RequestFactoryInterface | ||
| */ | ||
| protected $requestFactory; | ||
|
|
||
| /** | ||
| * @var StreamFactoryInterface | ||
| */ | ||
| protected $streamFactory; | ||
|
|
There was a problem hiding this comment.
How are these properties injected? I think I'm missing a constructor here.
Closes #685. See #538.