-
Notifications
You must be signed in to change notification settings - Fork 16
Adding response mutator. #48
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
Changes from 5 commits
a0192c0
8abb28a
63f438f
465c0e4
f62329e
a968e8f
884c87c
0e5159f
fef55cf
e1e4bec
c29b205
3edb6df
ce5224d
27e053b
c5e2ac2
d0f2494
699d45e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?php | ||
|
||
namespace Http\Client\Common\Plugin\Cache\Mutator; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
|
||
/** | ||
* Adds a header if the response came from cache. | ||
* | ||
* @author Iain Connor <[email protected]> | ||
*/ | ||
class AddHeaderResponseMutator implements ResponseMutator | ||
{ | ||
/** @var string */ | ||
private $headerName; | ||
|
||
/** | ||
* @param string $headerName | ||
*/ | ||
public function __construct($headerName = 'X-From-Php-Http-Cache') | ||
{ | ||
$this->headerName = $headerName; | ||
} | ||
|
||
/** | ||
* Mutate the response depending on the cache status. | ||
* | ||
* @param ResponseInterface $response | ||
* @param bool $cacheHit | ||
* | ||
* @return string | ||
*/ | ||
public function mutate(RequestInterface $request, ResponseInterface $response, $cacheHit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any more information that we could pass in instead of the boolean? like the $cacheItem or such? then this could be used to extract additional debugging information if wanted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion, updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record, I'm going to keep the |
||
{ | ||
return $response->withHeader($this->headerName, $cacheHit); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
|
||
namespace Http\Client\Common\Plugin\Cache\Mutator; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
|
||
/** | ||
* An interface for a mutator of a possibly cached response. | ||
* | ||
* @author Iain Connor <[email protected]> | ||
*/ | ||
interface ResponseMutator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i am not happy with this name. every plugin has the possibility to change a response. this thing is specifically about the cache. but unfortunately i don't have a great name either. the class doc could be "XXX is called by the cache plugin with information on the cache status to change information on the response based on whether it was a cache hit or a miss." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't think of anything significantly better. Will rename this for the time being and see how it feels. |
||
{ | ||
/** | ||
* Mutate the response depending on the cache status. | ||
* | ||
* @param RequestInterface $request | ||
* @param ResponseInterface $response | ||
* @param bool $cacheHit | ||
* | ||
* @return string | ||
*/ | ||
public function mutate(RequestInterface $request, ResponseInterface $response, $cacheHit); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
use Http\Client\Common\Plugin\Exception\RewindStreamException; | ||
use Http\Client\Common\Plugin\Cache\Generator\CacheKeyGenerator; | ||
use Http\Client\Common\Plugin\Cache\Generator\SimpleGenerator; | ||
use Http\Client\Common\Plugin\Cache\Mutator\ResponseMutator; | ||
use Http\Message\StreamFactory; | ||
use Http\Promise\FulfilledPromise; | ||
use Psr\Cache\CacheItemInterface; | ||
|
@@ -59,6 +60,7 @@ final class CachePlugin implements Plugin | |
* @var array $methods list of request methods which can be cached | ||
* @var array $respect_response_cache_directives list of cache directives this plugin will respect while caching responses | ||
* @var CacheKeyGenerator $cache_key_generator an object to generate the cache key. Defaults to a new instance of SimpleGenerator | ||
* @var ResponseMutator $response_mutator an object to mutate the response based on the results of the cache check. Defaults to nothing | ||
* } | ||
*/ | ||
public function __construct(CacheItemPoolInterface $pool, StreamFactory $streamFactory, array $config = []) | ||
|
@@ -129,7 +131,11 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
$method = strtoupper($request->getMethod()); | ||
// if the request not is cachable, move to $next | ||
if (!in_array($method, $this->config['methods'])) { | ||
return $next($request); | ||
return $next($request)->then(function (ResponseInterface $response) use ($request) { | ||
$response = $this->handleResponseMutator($request, $response, false); | ||
|
||
return $response; | ||
}); | ||
} | ||
|
||
// If we can cache the request | ||
|
@@ -141,7 +147,10 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
// The array_key_exists() is to be removed in 2.0. | ||
if (array_key_exists('expiresAt', $data) && (null === $data['expiresAt'] || time() < $data['expiresAt'])) { | ||
// This item is still valid according to previous cache headers | ||
return new FulfilledPromise($this->createResponseFromCacheItem($cacheItem)); | ||
$response = $this->createResponseFromCacheItem($cacheItem); | ||
$response = $this->handleResponseMutator($request, $response, true); | ||
|
||
return new FulfilledPromise($response); | ||
} | ||
|
||
// Add headers to ask the server if this cache is still valid | ||
|
@@ -154,14 +163,14 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
} | ||
} | ||
|
||
return $next($request)->then(function (ResponseInterface $response) use ($cacheItem) { | ||
return $next($request)->then(function (ResponseInterface $response) use ($request, $cacheItem) { | ||
if (304 === $response->getStatusCode()) { | ||
if (!$cacheItem->isHit()) { | ||
/* | ||
* We do not have the item in cache. This plugin did not add If-Modified-Since | ||
* or If-None-Match headers. Return the response from server. | ||
*/ | ||
return $response; | ||
return $this->handleResponseMutator($request, $response, false); | ||
} | ||
|
||
// The cached response we have is still valid | ||
|
@@ -171,7 +180,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
$cacheItem->set($data)->expiresAfter($this->calculateCacheItemExpiresAfter($maxAge)); | ||
$this->pool->save($cacheItem); | ||
|
||
return $this->createResponseFromCacheItem($cacheItem); | ||
return $this->handleResponseMutator($request, $this->createResponseFromCacheItem($cacheItem), true); | ||
} | ||
|
||
if ($this->isCacheable($response)) { | ||
|
@@ -196,7 +205,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
$this->pool->save($cacheItem); | ||
} | ||
|
||
return $response; | ||
return $this->handleResponseMutator($request, $response, false); | ||
}); | ||
} | ||
|
||
|
@@ -343,6 +352,7 @@ private function configureOptions(OptionsResolver $resolver) | |
'methods' => ['GET', 'HEAD'], | ||
'respect_response_cache_directives' => ['no-cache', 'private', 'max-age', 'no-store'], | ||
'cache_key_generator' => null, | ||
'response_mutator' => null, | ||
]); | ||
|
||
$resolver->setAllowedTypes('cache_lifetime', ['int', 'null']); | ||
|
@@ -357,6 +367,7 @@ private function configureOptions(OptionsResolver $resolver) | |
|
||
return empty($matches); | ||
}); | ||
$resolver->setAllowedTypes('response_mutator', ['null', 'Http\Client\Common\Plugin\Cache\Mutator\ResponseMutator']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but I was following the style of the other lines in this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets use it here and we fix it elsewhere. (but as saying below, i think this should be an array, while we are at it. |
||
|
||
$resolver->setNormalizer('respect_cache_headers', function (Options $options, $value) { | ||
if (null !== $value) { | ||
|
@@ -441,4 +452,22 @@ private function getETag(CacheItemInterface $cacheItem) | |
} | ||
} | ||
} | ||
|
||
/** | ||
* Call the reponse mutator, if one is set. | ||
* | ||
* @param RequestInterface $request | ||
* @param ResponseInterface $response | ||
* @param bool $cacheHit | ||
* | ||
* @return ResponseInterface | ||
*/ | ||
private function handleResponseMutator(RequestInterface $request, ResponseInterface $response, $cacheHit) | ||
{ | ||
if (null !== $this->config['response_mutator']) { | ||
$response = $this->config['response_mutator']->mutate($request, $response, $cacheHit); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this should be a list of mutators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion, will update. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though, not totally familliar with the resolver... is there any better way of stating that the array must be of generic type |
||
} | ||
|
||
return $response; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a semi-official way to mark cache hits is to use "X-Cache: HIT" (resp. MISS). can we find a different example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, updated.