Skip to content

Update CachedHttpClient.php #265

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 2 commits into from
May 5, 2015
Merged

Update CachedHttpClient.php #265

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bcaeb9
Update CachedHttpClient.php
jrean Apr 30, 2015
ec371af
Update CachedHttpClient.php
jrean May 1, 2015
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
44 changes: 40 additions & 4 deletions lib/Github/HttpClient/CachedHttpClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ class CachedHttpClient extends HttpClient
*/
private $lastCachedResponse;

/**
* $path + query parameter(s) if they exist.
*
* @var string
*/
private $path;
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't store this in a property. The more state is added in the class, the more complex reasonning about it is (I already think that having getLastResponse in the HttpClientInterface is a big mistake).

Copy link
Contributor

Choose a reason for hiding this comment

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

and btw, the naming is wrong: the value stored in this property is not the path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof

please don't store this in a property.

I added it in a property because it is called in two methods ! request() and createRequest(). What do you suggest so? You prefer a private method that returns something called into request() and createRequest() methods? Properties exists to be used and in that class only 2 already exist (plus eventually mine) then 4 for the parent class. This is acceptable don't you think?

"and btw, the naming is wrong: the value stored in this property is not the path"

$path is implemented everywhere and in other places it is called $id...
When I choose $this->path it was to respect what as already been implemented. The original variable was $path as you can see.
How do you want to call $path + query (if query parameters exist)?
By suggesting this pull request I'm not fighting for naming but simply trying to fix something that doesn't work as it should be. I'm totally open for constructive suggestions. What do you suggest so?


/**
* @return CacheInterface
*/
Expand All @@ -51,16 +58,18 @@ public function setCache(CacheInterface $cache)
*/
public function request($path, $body = null, $httpMethod = 'GET', array $headers = array(), array $options = array())
{
$this->formatPath($path, $options);

$response = parent::request($path, $body, $httpMethod, $headers, $options);

if (304 == $response->getStatusCode()) {
$cacheResponse = $this->getCache()->get($path);
$cacheResponse = $this->getCache()->get($this->path);
$this->lastCachedResponse = $cacheResponse;

return $cacheResponse;
}

$this->getCache()->set($path, $response);
$this->getCache()->set($this->path, $response);

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

if ($modifiedAt = $this->getCache()->getModifiedSince($path)) {
if ($modifiedAt = $this->getCache()->getModifiedSince($this->path)) {
$modifiedAt = new \DateTime('@'.$modifiedAt);
$modifiedAt->setTimezone(new \DateTimeZone('GMT'));

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

return ($force) ? $lastResponse : $this->lastCachedResponse;
}

/**
* Format the path and add query parameters if they exist.
*
* @param string $path
* @param array $options
* @return void
*/
private function formatPath($path, array $options)
{
$this->path = $path;

if (array_key_exists('query', $options) && !empty($options['query'])) {
$this->path .= '?';

$i = 0;
foreach ($options['query'] as $key => $value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use http_build_query($options['query'], '', '&') instead of reimplementing it in an incomplete way

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the userland implementation of this code (my code does not handle prefixing numeric keys as I don't need to emulate this case here):

$queryParts = array();
foreach ($options['query'] as $key => $value) {
    $queryParts[] = urlencode($key).'='.urlencodd($value);
}
$query = implode('&', $queryParts);

Your own code is missing the urlencoding (which can create the same cache keys for different URLs if my value is foo&bar=baz and you don't encode it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want to encode/urlencode? I still don't get it.

How do you manage it when $path contains only what is given with the custom query syntax $client->getHttpClient()->get("repos/jrean/askjong.com/commits?sha=master&path=articles/hello-world.md")? It already contains query string parameters not encoded.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you build them without url encoding them, it looks like a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

you cannot urlencode $path. It would break it. You need to urlencode the parts of this path when building it (each query parameter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof I finally got my own error. I used $client->getHttpClient()->get(...); the wrong way!
The signature is of course public function get($path, array $parameters = array(), array $headers = array()) so, I and everybody have to pass query parameters to the array $parameters if they exist.

It simplifies the problem now.

For both custom and regular queries Guzzle will take care of encoding query parameters if they exist as it already does in the original code. So in any case we can fetch them by using the $request->getQuery() method.

We just have to update the $path variable and concatenate the encoded query parameters if they exist. No need to extra method...
You should be happy ;)

Let me update the code and give me your feedback again.

Hope we will be able to merge then.

if ($i > 0) {
$this->path .= '&';
}

$this->path .= $key . '=' . $value;

$i++;
}
}
}
}