Skip to content

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

Merged
merged 17 commits into from
Jan 29, 2018
Merged
38 changes: 38 additions & 0 deletions src/Cache/Mutator/AddHeaderResponseMutator.php
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')
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, updated.

{
$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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, I'm going to keep the cacheHit parameter as well, so the consumer doesn't have to reverse-engineer the logic regarding expiry. Checking cacheItem->isHit() won't be sufficient.

{
return $response->withHeader($this->headerName, $cacheHit);
}
}
25 changes: 25 additions & 0 deletions src/Cache/Mutator/ResponseMutator.php
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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. CacheListener is not too good, but would be more specific to what this does. can we be even more specific?

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
41 changes: 35 additions & 6 deletions src/CachePlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = [])
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)) {
Expand All @@ -196,7 +205,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
$this->pool->save($cacheItem);
}

return $response;
return $this->handleResponseMutator($request, $response, false);
});
}

Expand Down Expand Up @@ -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']);
Expand All @@ -357,6 +367,7 @@ private function configureOptions(OptionsResolver $resolver)

return empty($matches);
});
$resolver->setAllowedTypes('response_mutator', ['null', 'Http\Client\Common\Plugin\Cache\Mutator\ResponseMutator']);
Copy link
Member

Choose a reason for hiding this comment

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

We could use the ::class constant

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be a list of mutators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 CacheListener? Right now I just have $resolver->setAllowedTypes('cache_listeners', ['array']);.

}

return $response;
}
}