Use If-modified-since headers to avoid X-Rate-Limit decrease #20

Merged
merged 4 commits into from Dec 21, 2012

Projects

None yet

3 participants

Member

No description provided.

@docteurklein docteurklein commented on the diff Nov 14, 2012
lib/Github/Client.php
{
- $httpClient = $httpClient ?: new Curl();
- $httpClient->setTimeout($this->options['timeout']);
- $httpClient->setVerifyPeer(false);
-
- $this->httpClient = new HttpClient($this->options, $httpClient);
+ $this->httpClient = $httpClient ?: new HttpClient($this->options);
docteurklein
docteurklein Nov 14, 2012 Member

I think this is better :)

@stloyd stloyd commented on the diff Nov 14, 2012
lib/Github/HttpClient/CachedHttpClient.php
+{
+ protected $cache;
+
+ public function __construct(array $options = array(), ClientInterface $client = null, CacheInterface $cache = null)
+ {
+ parent::__construct($options, $client);
+
+ $this->cache = $cache ?: new FilesystemCache;
+ }
+
+ public function request($path, array $parameters = array(), $httpMethod = 'GET', array $headers = array(), Response $response = null)
+ {
+ $response = parent::request($path, $parameters, $httpMethod, $headers, $response);
+
+ $key = trim($this->options['base_url'].$path, '/');
+ if ($response->isNotModified()) {
stloyd
stloyd Nov 14, 2012 Contributor

Such method not exist in Response class.

docteurklein
docteurklein Nov 14, 2012 Member

I just added it :) oops

Contributor

Why the tests actually do calls to the github api??

@stloyd stloyd commented on the diff Nov 14, 2012
lib/Github/HttpClient/CachedHttpClient.php
+ protected $cache;
+
+ public function __construct(array $options = array(), ClientInterface $client = null, CacheInterface $cache = null)
+ {
+ parent::__construct($options, $client);
+
+ $this->cache = $cache ?: new FilesystemCache;
+ }
+
+ public function request($path, array $parameters = array(), $httpMethod = 'GET', array $headers = array(), Response $response = null)
+ {
+ $response = parent::request($path, $parameters, $httpMethod, $headers, $response);
+
+ $key = trim($this->options['base_url'].$path, '/');
+ if ($response->isNotModified()) {
+ return $this->cache->get($key);
stloyd
stloyd Nov 14, 2012 Contributor

This will fail if cache was not found and response was not modified (i.e. some GC cleaned temp folder)

@stloyd stloyd commented on the diff Nov 14, 2012
lib/Github/HttpClient/HttpClient.php
@@ -139,7 +144,7 @@ public function put($path, array $parameters = array(), array $headers = array()
/**
* {@inheritDoc}
*/
- public function request($path, array $parameters = array(), $httpMethod = 'GET', array $headers = array())
+ public function request($path, array $parameters = array(), $httpMethod = 'GET', array $headers = array(), Response $response = null)
stloyd
stloyd Nov 14, 2012 Contributor

What's the reason of this change ? I look at code and can't find in what case this Response is overwritten. It seems like it's always null in acutal code.

docteurklein
docteurklein Nov 14, 2012 Member

It permits more flexibility. We had no way access to the response in precedent code. It's used in tests currently :-s
Another possibility would be to use a ResponseFactory. (The Buzz one for example).

stloyd
stloyd Nov 15, 2012 Contributor

I think that similar method to createRequest() would be enough, and more clear than this additional param =)

@stloyd stloyd merged commit 910a807 into master Dec 21, 2012

1 check passed

default The Travis build passed
Details
@stloyd stloyd deleted the since branch Dec 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment