Skip to content

Add client builder #480

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 16 commits into from
Dec 9, 2016
Merged

Add client builder #480

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
86d9d00
Added HTTP Client Builder
Nyholm Dec 4, 2016
51ba284
Bugfixes
Nyholm Dec 4, 2016
7153101
Added changelog
Nyholm Dec 4, 2016
f2b1a77
Removed support for default headers in the builder
Nyholm Dec 4, 2016
02b38a2
minor
Nyholm Dec 4, 2016
3fe4dbc
Making builder final
Nyholm Dec 4, 2016
bd2dd41
removed final
Nyholm Dec 4, 2016
461f27b
Updating docs for new constructor
Nyholm Dec 5, 2016
fa4d1d9
Updated docs
Nyholm Dec 5, 2016
a8f60a1
Style fix
Nyholm Dec 6, 2016
511775a
Added http client to constructor
Nyholm Dec 6, 2016
c915187
Fixed tests and docs
Nyholm Dec 6, 2016
3160b9c
Adding a factory class
Nyholm Dec 6, 2016
38ceb97
Updated changelog
Nyholm Dec 6, 2016
0efb593
Renamed to createWithHttpClient
Nyholm Dec 7, 2016
d7cde53
Updated changelog
Nyholm Dec 7, 2016
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
15 changes: 15 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,21 @@ The change log describes what is "Added", "Removed", "Changed" or "Fixed" betwee

## 2.0.0-rc5 (UNRELEASED)

### Changed
Copy link
Collaborator

@acrobat acrobat Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The changed part is already added for RC5 right under added (line 32 in your changed file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct. Thank you.


First parameter of `Github\Client` has changed type from `\Http\Client\HttpClient` to
`Github\HttpClient\Builder`. To upgrade you need to change:

```php
// Old way does not work:
$github = new Github\Client($httpClient);

// New way will work:
$github = new Github\Client(new Github\HttpClient\Builder($httpClient));

```


### Added

- Support for JWT authentication
Expand Down
2 changes: 1 addition & 1 deletion doc/api_version.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ For example:
$client = new Github\Client();
echo $client->getApiVersion(); // prints "v3"

$client = new Github\Client($httpClient, 'v2');
$client = new Github\Client(new Github\HttpClient\Builder(), 'v2');
echo $client->getApiVersion(); // prints "v2"
```
5 changes: 2 additions & 3 deletions doc/customize.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@
yourself by calling `\Github\Client::setHttpClient`. A HTTP client must implement `Http\Client\HttpClient`. A list of
community provided clients is found here: https://packagist.org/providers/php-http/client-implementation

You can inject a HTTP client through `Github\Client#setHttpClient()` method:
You can inject a HTTP client through the `Github\Client` constructor:

```php
$client = new Github\Client();
$client->setHttpClient(new Http\Adapter\Guzzle6\Client());
$client = new Github\Client(new \Github\HttpClient\Builder(new Http\Adapter\Guzzle6\Client()));
```

### Configure the HTTP client
Expand Down
2 changes: 1 addition & 1 deletion doc/security.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ The following sample code authenticates as an installation using [lcobucci/jwt](
to generate a JSON Web Token (JWT).

```php
$github = new Github\Client(new GuzzleClient(), 'machine-man-preview');
$github = new Github\Client(new Github\HttpClient\Builder(), 'machine-man-preview');

$jwt = (new Builder)
->setIssuer($integrationId)
Expand Down
199 changes: 38 additions & 161 deletions lib/Github/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,14 @@
use Github\Api\ApiInterface;
use Github\Exception\InvalidArgumentException;
use Github\Exception\BadMethodCallException;
use Github\HttpClient\Builder;
use Github\HttpClient\Plugin\Authentication;
use Github\HttpClient\Plugin\GithubExceptionThrower;
use Github\HttpClient\Plugin\History;
use Github\HttpClient\Plugin\PathPrepend;
use Http\Client\Common\HttpMethodsClient;
use Http\Client\Common\Plugin;
use Http\Client\Common\PluginClient;
use Http\Client\HttpClient;
use Http\Discovery\HttpClientDiscovery;
use Http\Discovery\MessageFactoryDiscovery;
use Http\Discovery\StreamFactoryDiscovery;
use Http\Discovery\UriFactoryDiscovery;
use Http\Message\MessageFactory;
use Nyholm\Psr7\Factory\StreamFactory;
use Psr\Cache\CacheItemPoolInterface;

/**
Expand Down Expand Up @@ -98,45 +92,9 @@ class Client
private $apiVersion;

/**
* The object that sends HTTP messages
*
* @var HttpClient
*/
private $httpClient;

/**
* A HTTP client with all our plugins
*
* @var PluginClient
*/
private $pluginClient;

/**
* @var MessageFactory
*/
private $messageFactory;

/**
* @var StreamFactory
*/
private $streamFactory;

/**
* @var Plugin[]
*/
private $plugins = [];

/**
* True if we should create a new Plugin client at next request.
* @var bool
*/
private $httpClientModified = true;

/**
* Http headers
* @var array
* @var Builder
*/
private $headers = [];
private $httpClientBuilder;

/**
* @var History
Expand All @@ -146,27 +104,26 @@ class Client
/**
* Instantiate a new GitHub client.
*
* @param HttpClient|null $httpClient
* @param string|null $apiVersion
* @param string|null $enterpriseUrl
* @param Builder|null $httpClient
* @param string|null $apiVersion
* @param string|null $enterpriseUrl
*/
public function __construct(HttpClient $httpClient = null, $apiVersion = null, $enterpriseUrl = null)
public function __construct(Builder $httpClientBuilder = null, $apiVersion = null, $enterpriseUrl = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's better to leave old constructor and create new builder inline or support both.

Copy link
Collaborator

@acrobat acrobat Dec 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe keep the old constructor indeed but deprecated it (throw deprecation warning) and use the new constructor in 3.0. Or you could say use the new constructor from now on as 2.0 stable is not yet released

Copy link
Contributor

@cursedcoder cursedcoder Dec 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

My idea is that client should always be constructed without extra manipulations, i.e.

$github = new Client();
$github->doAnything();

// or
$github = new Client($guzzle);

maybe static factory can be a possible solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right about that, it should be a simple as possible to instantiate and use the client

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not like the new constructor myself to be honest. Should I add another constructor argument with the builder?

{
$this->httpClient = $httpClient ?: HttpClientDiscovery::find();
$this->messageFactory = MessageFactoryDiscovery::find();
$this->streamFactory = StreamFactoryDiscovery::find();

$this->responseHistory = new History();
$this->addPlugin(new GithubExceptionThrower());
$this->addPlugin(new Plugin\HistoryPlugin($this->responseHistory));
$this->addPlugin(new Plugin\RedirectPlugin());
$this->addPlugin(new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri('https://api.github.com')));
$this->addPlugin(new Plugin\HeaderDefaultsPlugin(array(
'User-Agent' => 'php-github-api (http://github.com/KnpLabs/php-github-api)',
$this->httpClientBuilder = $builder = $httpClientBuilder ?: new Builder();

$builder->addPlugin(new GithubExceptionThrower());
$builder->addPlugin(new Plugin\HistoryPlugin($this->responseHistory));
$builder->addPlugin(new Plugin\RedirectPlugin());
$builder->addPlugin(new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri('https://api.github.com')));
$builder->addPlugin(new Plugin\HeaderDefaultsPlugin(array(
'User-Agent' => 'php-github-api (http://github.com/KnpLabs/php-github-api)',
'Accept' => sprintf('application/vnd.github.%s+json', $this->getApiVersion()),
)));

$this->apiVersion = $apiVersion ?: 'v3';
$this->addHeaders(['Accept' => sprintf('application/vnd.github.%s+json', $this->apiVersion)]);
$builder->addHeaders(['Accept' => sprintf('application/vnd.github.%s+json', $this->apiVersion)]);

if ($enterpriseUrl) {
$this->setEnterpriseUrl($enterpriseUrl);
Expand Down Expand Up @@ -313,15 +270,15 @@ public function authenticate($tokenOrLogin, $password = null, $authMethod = null

if (null === $authMethod && in_array($password, array(self::AUTH_URL_TOKEN, self::AUTH_URL_CLIENT_ID, self::AUTH_HTTP_PASSWORD, self::AUTH_HTTP_TOKEN, self::AUTH_JWT))) {
$authMethod = $password;
$password = null;
$password = null;
}

if (null === $authMethod) {
$authMethod = self::AUTH_HTTP_PASSWORD;
}

$this->removePlugin(Authentication::class);
$this->addPlugin(new Authentication($tokenOrLogin, $password, $authMethod));
$this->getHttpClientBuilder()->removePlugin(Authentication::class);
$this->getHttpClientBuilder()->addPlugin(new Authentication($tokenOrLogin, $password, $authMethod));
}

/**
Expand All @@ -331,64 +288,12 @@ public function authenticate($tokenOrLogin, $password = null, $authMethod = null
*/
private function setEnterpriseUrl($enterpriseUrl)
{
$this->removePlugin(Plugin\AddHostPlugin::class);
$this->removePlugin(PathPrepend::class);

$this->addPlugin(new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri($enterpriseUrl)));
$this->addPlugin(new PathPrepend(sprintf('/api/%s/', $this->getApiVersion())));
}

/**
* Add a new plugin to the end of the plugin chain.
*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin)
{
$this->plugins[] = $plugin;
$this->httpClientModified = true;
}
$builder = $this->getHttpClientBuilder();
$builder->removePlugin(Plugin\AddHostPlugin::class);
$builder->removePlugin(PathPrepend::class);

/**
* Remove a plugin by its fully qualified class name (FQCN).
*
* @param string $fqcn
*/
public function removePlugin($fqcn)
{
foreach ($this->plugins as $idx => $plugin) {
if ($plugin instanceof $fqcn) {
unset($this->plugins[$idx]);
$this->httpClientModified = true;
}
}
}

/**
* @return HttpMethodsClient
*/
public function getHttpClient()
{
if ($this->httpClientModified) {
$this->httpClientModified = false;
$this->pushBackCachePlugin();

$this->pluginClient = new HttpMethodsClient(
new PluginClient($this->httpClient, $this->plugins),
$this->messageFactory
);
}

return $this->pluginClient;
}

/**
* @param HttpClient $httpClient
*/
public function setHttpClient(HttpClient $httpClient)
{
$this->httpClientModified = true;
$this->httpClient = $httpClient;
$builder->addPlugin(new Plugin\AddHostPlugin(UriFactoryDiscovery::find()->createUri($enterpriseUrl)));
$builder->addPlugin(new PathPrepend(sprintf('/api/%s/', $this->getApiVersion())));
}

/**
Expand All @@ -399,30 +304,6 @@ public function getApiVersion()
return $this->apiVersion;
}

/**
* Clears used headers.
*/
public function clearHeaders()
{
$this->headers = array(
'Accept' => sprintf('application/vnd.github.%s+json', $this->getApiVersion()),
);

$this->removePlugin(Plugin\HeaderAppendPlugin::class);
$this->addPlugin(new Plugin\HeaderAppendPlugin($this->headers));
}

/**
* @param array $headers
*/
public function addHeaders(array $headers)
{
$this->headers = array_merge($this->headers, $headers);

$this->removePlugin(Plugin\HeaderAppendPlugin::class);
$this->addPlugin(new Plugin\HeaderAppendPlugin($this->headers));
}

/**
* Add a cache plugin to cache responses locally.
*
Expand All @@ -431,16 +312,15 @@ public function addHeaders(array $headers)
*/
public function addCache(CacheItemPoolInterface $cachePool, array $config = [])
{
$this->removeCache();
$this->addPlugin(new Plugin\CachePlugin($cachePool, $this->streamFactory, $config));
$this->getHttpClientBuilder()->addCache($cachePool, $config);
}

/**
* Remove the cache plugin
* Remove the cache plugin.
*/
public function removeCache()
{
$this->removePlugin(Plugin\CachePlugin::class);
$this->getHttpClientBuilder()->removeCache();
}

/**
Expand All @@ -460,7 +340,6 @@ public function __call($name, $args)
}

/**
*
* @return null|\Psr\Http\Message\ResponseInterface
*/
public function getLastResponse()
Expand All @@ -469,20 +348,18 @@ public function getLastResponse()
}

/**
* Make sure to move the cache plugin to the end of the chain
* @return HttpMethodsClient
*/
private function pushBackCachePlugin()
public function getHttpClient()
{
$cachePlugin = null;
foreach ($this->plugins as $i => $plugin) {
if ($plugin instanceof Plugin\CachePlugin) {
$cachePlugin = $plugin;
unset($this->plugins[$i]);

$this->plugins[] = $cachePlugin;
return $this->getHttpClientBuilder()->getHttpClient();
}

return;
}
}
/**
* @return Builder
*/
protected function getHttpClientBuilder()
{
return $this->httpClientBuilder;
}
}
Loading