Skip to content

feat: PSR18 #885

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

Closed
wants to merge 1 commit into from
Closed

feat: PSR18 #885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
},
"require": {
"php": "^5.6 || ^7.0 || ^8.0",
"guzzlehttp/guzzle": "^6.0 || ^7.0",
"psr/http-client-implementation": "^1.0",
"psr/http-factory-implementation": "^1.0",
"paragonie/random_compat": "^1 || ^2 || ^9.99"
},
"require-dev": {
"guzzlehttp/guzzle": "^7.3",
"guzzlehttp/psr7": "^2.0@RC",
Copy link
Author

Choose a reason for hiding this comment

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

For tests, uses RC because this package doesn't declare providing the PSR17 factories until 2.x.

"mockery/mockery": "^1.3",
"php-parallel-lint/php-parallel-lint": "^1.2",
"phpunit/phpunit": "^5.7 || ^6.0 || ^9.3",
Expand Down
29 changes: 13 additions & 16 deletions src/Provider/AbstractProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@

namespace League\OAuth2\Client\Provider;

use GuzzleHttp\Client as HttpClient;
use GuzzleHttp\ClientInterface as HttpClientInterface;
use GuzzleHttp\Exception\BadResponseException;
use League\OAuth2\Client\Grant\AbstractGrant;
use League\OAuth2\Client\Grant\GrantFactory;
use League\OAuth2\Client\OptionProvider\OptionProviderInterface;
Expand All @@ -28,6 +25,8 @@
use League\OAuth2\Client\Tool\GuardedPropertyTrait;
use League\OAuth2\Client\Tool\QueryBuilderTrait;
use League\OAuth2\Client\Tool\RequestFactory;
use Psr\Http\Client\ClientExceptionInterface;
use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use UnexpectedValueException;
Expand Down Expand Up @@ -89,7 +88,7 @@ abstract class AbstractProvider
protected $requestFactory;

/**
* @var HttpClientInterface
* @var ClientInterface
*/
protected $httpClient;

Expand Down Expand Up @@ -126,11 +125,7 @@ public function __construct(array $options = [], array $collaborators = [])
$this->setRequestFactory($collaborators['requestFactory']);

if (empty($collaborators['httpClient'])) {
$client_options = $this->getAllowedClientOptions($options);

$collaborators['httpClient'] = new HttpClient(
array_intersect_key($options, array_flip($client_options))
);
throw new \LogicException('Must specify client');
Copy link
Author

Choose a reason for hiding this comment

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

The previous workflow no longer works, we cannot create a default client since we don't know what's available.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

There are 3 ways.

  1. No fallback
  2. Just Guzzle.
  3. https://github.com/php-http/discovery

To "find popular clients" is the job of the discovery package.

Copy link
Author

Choose a reason for hiding this comment

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

OK, so we can test for any of those and try to construct the client.

}
$this->setHttpClient($collaborators['httpClient']);

Expand Down Expand Up @@ -209,10 +204,10 @@ public function getRequestFactory()
/**
* Sets the HTTP client instance.
*
* @param HttpClientInterface $client
* @param ClientInterface $client
* @return self
*/
public function setHttpClient(HttpClientInterface $client)
public function setHttpClient(ClientInterface $client)
{
$this->httpClient = $client;

Expand All @@ -222,7 +217,7 @@ public function setHttpClient(HttpClientInterface $client)
/**
* Returns the HTTP client instance.
*
* @return HttpClientInterface
* @return ClientInterface
*/
public function getHttpClient()
Copy link
Member

Choose a reason for hiding this comment

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

Is this method really needed? But it is maybe out of scope for this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I'd keep it for now, but since this is a BC break, this will have to be 3.x anyway.

{
Expand Down Expand Up @@ -600,12 +595,14 @@ protected function createRequest($method, $url, $token, array $options)
* WARNING: This method does not attempt to catch exceptions caused by HTTP
* errors! It is recommended to wrap this method in a try/catch block.
*
* @param RequestInterface $request
* @param RequestInterface $request
*
* @return ResponseInterface
* @throws \Psr\Http\Client\ClientExceptionInterface
*/
public function getResponse(RequestInterface $request)
{
return $this->getHttpClient()->send($request);
return $this->getHttpClient()->sendRequest($request);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add logic here to follow redirects?

}

/**
Expand All @@ -619,8 +616,8 @@ public function getParsedResponse(RequestInterface $request)
{
try {
$response = $this->getResponse($request);
} catch (BadResponseException $e) {
$response = $e->getResponse();
} catch (ClientExceptionInterface $e) {
throw $e;
Copy link
Author

Choose a reason for hiding this comment

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

There's no equivalent here.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

$parsed = $this->parseResponse($response);
Expand Down
122 changes: 0 additions & 122 deletions src/Tool/ProviderRedirectTrait.php
View file
Open in desktop

This file was deleted.

28 changes: 24 additions & 4 deletions src/Tool/RequestFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

namespace League\OAuth2\Client\Tool;

use GuzzleHttp\Psr7\Request;
use Psr\Http\Message\RequestFactoryInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\StreamFactoryInterface;

/**
* Used to produce PSR-7 Request instances.
Expand All @@ -23,6 +25,16 @@
*/
class RequestFactory
{
/**
* @var RequestFactoryInterface
*/
protected $requestFactory;

/**
* @var StreamFactoryInterface
*/
protected $streamFactory;

Comment on lines +28 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these properties injected? I think I'm missing a constructor here.

/**
* Creates a PSR-7 Request instance.
*
Expand All @@ -32,7 +44,7 @@ class RequestFactory
* @param string|resource|StreamInterface $body Message body.
* @param string $version HTTP protocol version.
*
* @return Request
* @return RequestInterface
*/
public function getRequest(
$method,
Expand All @@ -41,7 +53,15 @@ public function getRequest(
$body = null,
$version = '1.1'
) {
return new Request($method, $uri, $headers, $body, $version);
$request = $this->requestFactory->createRequest($method, $uri)
->withProtocolVersion($version)
->withBody($this->streamFactory->createStream($body));

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

return $request;
}

/**
Expand Down Expand Up @@ -70,7 +90,7 @@ protected function parseOptions(array $options)
* @param null|string $uri
* @param array $options
*
* @return Request
* @return RequestInterface
*/
public function getRequestWithOptions($method, $uri, array $options = [])
{
Expand Down
Loading