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.

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
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ The change log describes what is "Added", "Removed", "Changed" or "Fixed" betwee
### Changed

- `ApiLimitExceedException::__construct` has a new second parameter for the remaining API calls.
- First parameter of `Github\Client` has changed type from `\Http\Client\HttpClient` to
`Github\HttpClient\Builder`. A factory class was also added. 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));
$github = Github\Client::createWithHttpClient($httpClient);
```


## 2.0.0-rc4

Expand Down
2 changes: 1 addition & 1 deletion doc/api_version.md
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($httpClient), 'v2');
echo $client->getApiVersion(); // prints "v2"
```
10 changes: 6 additions & 4 deletions doc/customize.md
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 = Github\Client::createWithHttpClient(new Http\Adapter\Guzzle6\Client());
```

### Configure the HTTP client
Expand All @@ -37,7 +36,10 @@ class CustomUserAgentPlugin implements Plugin
}
}

$githubClient->addPlugin(new CustomUserAgentPlugin());
$httpBuilder = new Github\HttpClient\Builder(new Http\Adapter\Guzzle6\Client());
$httpBuilder->addPlugin(new CustomUserAgentPlugin());

$client = new Github\Client($httpBuilder);
```

### Run Test Suite
Expand Down
3 changes: 2 additions & 1 deletion doc/security.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ 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');
$builder = new Github\HttpClient\Builder(new GuzzleClient());
$github = new Github\Client($builder, 'machine-man-preview');

$jwt = (new Builder)
->setIssuer($integrationId)
Expand Down
212 changes: 52 additions & 160 deletions lib/Github/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,15 @@
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 +93,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
* @var Builder
*/
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
*/
private $headers = [];
private $httpClientBuilder;

/**
* @var History
Expand All @@ -146,33 +105,46 @@ class Client
/**
* Instantiate a new GitHub client.
*
* @param HttpClient|null $httpClient
* @param string|null $apiVersion
* @param string|null $enterpriseUrl
* @param Builder|null $httpClientBuilder
* @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

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

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);
}
}

/**
* Create a Github\Client using a HttpClient.
*
* @param HttpClient $httpClient
*
* @return Client
*/
public static function createWithHttpClient(HttpClient $httpClient)
{
$builder = new Builder($httpClient);

return new self($builder);
}

/**
* @param string $name
*
Expand Down Expand Up @@ -313,15 +285,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 +303,12 @@ public function authenticate($tokenOrLogin, $password = null, $authMethod = null
*/
private function setEnterpriseUrl($enterpriseUrl)
{
$this->removePlugin(Plugin\AddHostPlugin::class);
$this->removePlugin(PathPrepend::class);
$builder = $this->getHttpClientBuilder();
$builder->removePlugin(Plugin\AddHostPlugin::class);
$builder->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;
}

/**
* 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 +319,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 +327,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 +355,6 @@ public function __call($name, $args)
}

/**
*
* @return null|\Psr\Http\Message\ResponseInterface
*/
public function getLastResponse()
Expand All @@ -469,20 +363,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