-
Notifications
You must be signed in to change notification settings - Fork 16
Allow a cache item to be stored forever #20
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 6 commits
a747977
e14bbdc
67f5c1a
32e1257
bd697b9
7dfeaa1
db84553
135c132
ac289f9
7c5da23
b1fee5d
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 |
---|---|---|
|
@@ -74,8 +74,8 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
|
||
if ($cacheItem->isHit()) { | ||
$data = $cacheItem->get(); | ||
// The isset() is to be removed in 2.0. | ||
if (isset($data['expiresAt']) && time() < $data['expiresAt']) { | ||
// The array_key_exists() is to be removed in 2.0. | ||
if (array_key_exists('expiresAt', $data) && ($data['expiresAt'] === null || time() < $data['expiresAt'])) { | ||
// This item is still valid according to previous cache headers | ||
return new FulfilledPromise($this->createResponseFromCacheItem($cacheItem)); | ||
} | ||
|
@@ -103,8 +103,8 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
// The cached response we have is still valid | ||
$data = $cacheItem->get(); | ||
$maxAge = $this->getMaxAge($response); | ||
$data['expiresAt'] = time() + $maxAge; | ||
$cacheItem->set($data)->expiresAfter($this->config['cache_lifetime'] + $maxAge); | ||
$data['expiresAt'] = $this->calculateResponseExpiresAt($maxAge); | ||
$cacheItem->set($data)->expiresAfter($this->calculateCacheItemExpiresAfter($maxAge)); | ||
$this->pool->save($cacheItem); | ||
|
||
return $this->createResponseFromCacheItem($cacheItem); | ||
|
@@ -120,14 +120,13 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
} | ||
|
||
$maxAge = $this->getMaxAge($response); | ||
$currentTime = time(); | ||
$cacheItem | ||
->expiresAfter($this->config['cache_lifetime'] + $maxAge) | ||
->expiresAfter($this->calculateCacheItemExpiresAfter($maxAge)) | ||
->set([ | ||
'response' => $response, | ||
'body' => $body, | ||
'expiresAt' => $currentTime + $maxAge, | ||
'createdAt' => $currentTime, | ||
'expiresAt' => $this->calculateResponseExpiresAt($maxAge), | ||
'createdAt' => time(), | ||
'etag' => $response->getHeader('ETag'), | ||
]); | ||
$this->pool->save($cacheItem); | ||
|
@@ -137,6 +136,40 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |
}); | ||
} | ||
|
||
/** | ||
* Calculate the timestamp when this cache item should be dropped from the cache. The lowest value return after | ||
* the calculation is $maxAge. | ||
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. FYI, I did not follow your suggestion here. 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. "The lowest value that can be returned is $maxAge" |
||
* | ||
* @param int|null $maxAge | ||
* | ||
* @return int|null Unix system time. @see PSR6 for caching | ||
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. sorry, 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. Perfect 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. should we mention that null means the cache item never expires and is only evicted if the cache drops items before they expire? 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 do not think so because
I think it is enough to reference the PSR-6 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. then lets replace the whole return to "This value is passed as cache lifetime to the PSR-6 cache". and not even talk about unix system time. being private does not mean it does not need to be documented. i usually don't remember such things a year later even if i coded it myself, and here contributors might try to understand the code, so good documentation is valuable even on internal things. 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. Done. Sure, private functions should be properly documented. I think one can exclude some edge cases and be a little bit shorter than on public methods. But I guess you are correct. It is a lot of numbers in this class already... |
||
*/ | ||
private function calculateCacheItemExpiresAfter($maxAge) | ||
{ | ||
if ($this->config['cache_lifetime'] === null && $maxAge === null) { | ||
return; | ||
} | ||
|
||
return $this->config['cache_lifetime'] + $maxAge; | ||
} | ||
|
||
/** | ||
* Calculate the timestamp when a response expires. After that timestamp, we need to send a | ||
* If-Modified-Since / If-None-Match request to validate the response | ||
* | ||
* @param int|null $maxAge | ||
* | ||
* @return int|null Unix system time | ||
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 explain that null means it never expires 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. Yes, agreed |
||
*/ | ||
private function calculateResponseExpiresAt($maxAge) | ||
{ | ||
if ($maxAge === null) { | ||
return; | ||
} | ||
|
||
return time() + $maxAge; | ||
} | ||
|
||
/** | ||
* Verify that we can cache this response. | ||
* | ||
|
@@ -237,12 +270,12 @@ private function configureOptions(OptionsResolver $resolver) | |
{ | ||
$resolver->setDefaults([ | ||
'cache_lifetime' => 86400 * 30, // 30 days | ||
'default_ttl' => null, | ||
'default_ttl' => 0, | ||
'respect_cache_headers' => true, | ||
'hash_algo' => 'sha1', | ||
]); | ||
|
||
$resolver->setAllowedTypes('cache_lifetime', 'int'); | ||
$resolver->setAllowedTypes('cache_lifetime', ['int', 'null']); | ||
$resolver->setAllowedTypes('default_ttl', ['int', 'null']); | ||
$resolver->setAllowedTypes('respect_cache_headers', 'bool'); | ||
$resolver->setAllowedValues('hash_algo', hash_algos()); | ||
|
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.
as null did not work as expected anyways, i think this change is ok. there only is a problem if somebody explicitly created the plugin with (null, false) - but imo that falls into the category of a bugfix that can be a BC break if you relied on the bug.
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.
Agreed