Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
7 changes: 5 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,20 @@
"php-http/discovery": "^1.0",
"php-http/client-implementation": "^1.0",
"php-http/client-common": "^1.3",
"php-http/cache-plugin": "^1.3"
"php-http/cache-plugin": "^1.4"
},
"require-dev": {
"phpunit/phpunit": "^4.0 || ^5.5",
"php-http/guzzle6-adapter": "^1.0",
"guzzlehttp/psr7": "^1.2",
"sllh/php-cs-fixer-styleci-bridge": "^1.3"
"sllh/php-cs-fixer-styleci-bridge": "^1.3",
"cache/array-adapter": "^0.5.0"
},
"autoload": {
"psr-4": { "Github\\": "lib/Github/" }
},
"minimum-stability": "dev",
"prefer-stable": true,
"extra": {
"branch-alias": {
"dev-master": "2.3.x-dev"
Expand Down
4 changes: 4 additions & 0 deletions lib/Github/HttpClient/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Http\Message\RequestFactory;
use Http\Message\StreamFactory;
use Psr\Cache\CacheItemPoolInterface;
use Http\Client\Common\Plugin\Cache\Generator\HeaderCacheKeyGenerator;

/**
* A builder that builds the API client.
Expand Down Expand Up @@ -181,6 +182,9 @@ public function addHeaderValue($header, $headerValue)
*/
public function addCache(CacheItemPoolInterface $cachePool, array $config = [])
{
if (!isset($config['cache_key_generator'])) {
$config['cache_key_generator'] = new HeaderCacheKeyGenerator(['Authorization', 'Cookie', 'Accept', 'Content-type']);
}
$this->cachePlugin = Plugin\CachePlugin::clientCache($cachePool, $this->streamFactory, $config);
$this->httpClientModified = true;
}
Expand Down
69 changes: 69 additions & 0 deletions test/Github/Tests/Functional/CacheTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php

namespace Github\Tests\Functional;

use Cache\Adapter\PHPArray\ArrayCachePool;
use Github\Client;
use GuzzleHttp\Psr7\Response;
use function GuzzleHttp\Psr7\stream_for;

/**
* @group functional
*
* @author Tobias Nyholm <[email protected]>
*/
class CacheTest extends \PHPUnit_Framework_TestCase
{
/**
* @test
*/
public function shouldServeCachedResponse()
{
$mockClient = new \Http\Mock\Client();
$mockClient->addResponse($this->getCurrentUserResponse('nyholm'));
$mockClient->addResponse($this->getCurrentUserResponse('octocat'));

$github = Client::createWithHttpClient($mockClient);
$github->addCache(new ArrayCachePool(), ['default_ttl'=>600]);

$github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN);
$userA = $github->currentUser()->show();
$this->assertEquals('nyholm', $userA['login']);

$userB = $github->currentUser()->show();
$this->assertEquals('nyholm', $userB['login'], 'Two request following each other should be cached.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Client should always send a request, and then see if we get a "not modified" response. We shouldn't just randomly cache for 600 seconds.

Copy link
Collaborator Author

@Nyholm Nyholm Apr 8, 2017

Choose a reason for hiding this comment

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

Client should always send a request, and then see if we get a "not modified" response

No, not always. If server says that it is okey to cache for X time, we do not need to send a request for that period.
But the logic you are referring to is in the php-http/cache-plugin.

We shouldn't just randomly cache for 600 seconds.

I set the default ttl to arbitrary number here to force the first the request to be cached (for a time longer then the tests run). Doing that I can easily verify if we make a second request or not. (Im using the MockClient, Im not actually making any HTTP calls. )

}
/**
* @test
*/
public function shouldVaryOnAuthorization()
{
$mockClient = new \Http\Mock\Client();
$mockClient->addResponse($this->getCurrentUserResponse('nyholm'));
$mockClient->addResponse($this->getCurrentUserResponse('octocat'));

$github = Client::createWithHttpClient($mockClient);
$github->addCache(new ArrayCachePool(), ['default_ttl'=>600]);

$github->authenticate('fake_token_aaa', Client::AUTH_HTTP_TOKEN);
$userA = $github->currentUser()->show();
$this->assertEquals('nyholm', $userA['login']);

$github->authenticate('fake_token_bbb', Client::AUTH_HTTP_TOKEN);
$userB = $github->currentUser()->show();
$this->assertEquals('octocat', $userB['login'], 'We must vary on the Authorization header.');
}

private function getCurrentUserResponse($username)
{
$headers = [
'Content-Type' => 'application/json',
];

$body = stream_for(json_encode([
'login' => $username,
]));

return new Response(200, $headers, $body);
}
}