Skip to content

Commit 2bcaeb9

Browse files
jreanjrean
committed
Update CachedHttpClient.php
I used your implementation of the filesystem cache. - Delete all files under the cache directory `github-api-cache`. - Set properly your authentication method (I used token). - Run the following code once after a proper Client instantiation with cache: ``` $commits_article_1 = ResponseMediator::getContent($client->getHttpClient()->get("repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md")); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); $commits_article_2 = ResponseMediator::getContent($client->getHttpClient()->get("repos/jrean/askjong.com/commits?sha=master&path=articles/this-is-a-first-article.md")); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); ``` 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) Once this code has run the cache directory `github-api-cache` will have 6 files (3 for each queries including the `rate_limit` query + 3 `.etag` files) **The `rate_limit` should / must decrease by 2** (a call to `rate_limit` doesn't count against Github). -- - Run the same code again. 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 304, cache is used) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 304, cache is user) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) **The `rate_limit` don't move** (a call to `rate_limit` doesn't count against Github). -- - Run the following code which use your wrapping methods against Github Api (Notice we are requesting the same informations): **It should use the cache but it will not.** (I explain why just after). ``` $commits_article_1 = $client->api('repo')->commits()->all('jrean', 'askjong.com', array('sha' => 'master', 'path' => 'articles/hello-world.md')); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); $commits_article_2 = $client->api('repo')->commits()->all('jrean', 'askjong.com', array('sha' => 'master', 'path' => 'articles/this-is-a-first-article.md')); $rateLimit = ResponseMediator::getContent($client->getHttpClient()->get("rate_limit")); var_dump($rateLimit); ``` 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) Once this code has run the cache directory `github-api-cache` will have new files... **The `rate_limit` decreases by 2** (a call to `rate_limit` doesn't count against Github). It should not decrease because we should use the cache! -- - Run the same code again. 4 requests against Github Api: - 1 query to fetch the commits for the `articles/hello-world.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 1 query to fetch the commits for the `articles/this-is-a-first-article.md` file. (`Guzzle\Http\Message\Response` status code is 200) - 2 queries to fetch the rate_limit (doesn't count again the rate limit). (`Guzzle\Http\Message\Response` status code is 200) **The `rate_limit` decreases by 2 again** (a call to `rate_limit` doesn't count against Github). It should not decrease because we should use the cache! - Run the same code again and it will still decrease your `rate_limit`... -- # Solution This issue comes from the use of `$path`: `protected function createRequest($httpMethod, $path, $body = null, array $headers = array(), array $options = array())` Then here: `if ($modifiedAt = $this->getCache()->getModifiedSince($path)) {` And: `if ($etag = $this->getCache()->getETag($path)) {` `$path` doesn't take care of the `$options` array which contains the sub array for the `query` parameters! So each time `getModifiedSince($path)` and `getETag($path)` will work with an incomplete path because it doesn't know query parameters (if they exists). In that exemple `$path` value will always be `repos/jrean/askjong.com/commits` and it should be `repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md`. See my code modifications to fix the problem. (I didn't use `$request->getQuery()` because it returns query parameters but encoded and it is not acceptable because when we want to construct our own queries like `repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md` we won't encode parameters) By applying this patch we can now use the cache as it should be :) I started this issue a few days ago. KnpLabs#262
1 parent a03bae7 commit 2bcaeb9

File tree

1 file changed

+40
-4
lines changed

1 file changed

+40
-4
lines changed

‎lib/Github/HttpClient/CachedHttpClient.php

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ class CachedHttpClient extends HttpClient
2626
*/
2727
private $lastCachedResponse;
2828

29+
/**
30+
* $path + query parameter(s) if they exist.
31+
*
32+
* @var string
33+
*/
34+
private $path;
35+
2936
/**
3037
* @return CacheInterface
3138
*/
@@ -51,16 +58,18 @@ public function setCache(CacheInterface $cache)
5158
*/
5259
public function request($path, $body = null, $httpMethod = 'GET', array $headers = array(), array $options = array())
5360
{
61+
$this->formatPath($path, $options);
62+
5463
$response = parent::request($path, $body, $httpMethod, $headers, $options);
5564

5665
if (304 == $response->getStatusCode()) {
57-
$cacheResponse = $this->getCache()->get($path);
66+
$cacheResponse = $this->getCache()->get($this->path);
5867
$this->lastCachedResponse = $cacheResponse;
5968

6069
return $cacheResponse;
6170
}
6271

63-
$this->getCache()->set($path, $response);
72+
$this->getCache()->set($this->path, $response);
6473

6574
return $response;
6675
}
@@ -74,7 +83,7 @@ protected function createRequest($httpMethod, $path, $body = null, array $header
7483
{
7584
$request = parent::createRequest($httpMethod, $path, $body, $headers, $options);
7685

77-
if ($modifiedAt = $this->getCache()->getModifiedSince($path)) {
86+
if ($modifiedAt = $this->getCache()->getModifiedSince($this->path)) {
7887
$modifiedAt = new \DateTime('@'.$modifiedAt);
7988
$modifiedAt->setTimezone(new \DateTimeZone('GMT'));
8089

@@ -83,7 +92,7 @@ protected function createRequest($httpMethod, $path, $body = null, array $header
8392
sprintf('%s GMT', $modifiedAt->format('l, d-M-y H:i:s'))
8493
);
8594
}
86-
if ($etag = $this->getCache()->getETag($path)) {
95+
if ($etag = $this->getCache()->getETag($this->path)) {
8796
$request->addHeader(
8897
'If-None-Match',
8998
$etag
@@ -105,4 +114,31 @@ public function getLastResponse($force = false)
105114

106115
return ($force) ? $lastResponse : $this->lastCachedResponse;
107116
}
117+
118+
/**
119+
* Format the path and add query parameters if they exist.
120+
*
121+
* @param string $path
122+
* @param array $options
123+
* @return void
124+
*/
125+
private function formatPath($path, array $options)
126+
{
127+
$this->path = $path;
128+
129+
if (array_key_exists('query', $options) && !empty($options['query'])) {
130+
$this->path .= '?';
131+
132+
$i = 0;
133+
foreach ($options['query'] as $key => $value) {
134+
if ($i > 0) {
135+
$this->path .= '&';
136+
}
137+
138+
$this->path .= $key . '=' . $value;
139+
140+
$i++;
141+
}
142+
}
143+
}
108144
}

0 commit comments

Comments
 (0)