Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[ExpiringCache] Fix the get() method on expired entries, some tweaks. #35

Closed
wants to merge 1 commit into from

3 participants

@vicb

According to the CacheInterface, get() should return either the cached value or null.

This is what I have implemented in the ExpiringCache (previously get would return the value if even if the entry is outdated).

It is ok ? I have noticed that the FileSystemCache and ConfigCache throw and an exception which is yet an other behavior.

@stof
Collaborator

@kriswallsmith what about this ?

@kriswallsmith

The change to get() makes sense. I don't understand the change to has().

@vicb vicb commented on the diff
src/Assetic/Cache/ExpiringCache.php
@@ -29,13 +29,12 @@ public function __construct(CacheInterface $cache, $lifetime)
public function has($key)
{
- if ($this->cache->has($key)) {
+ if ($this->cache->has($key.'.expires')) {
if (time() < $this->cache->get($key.'.expires')) {
@vicb
vicb added a note

@kriswallsmith the rationale is that you don't want to check something that is not in the cache $key and $key.expires are distinct entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vicb

... that being said, the implementation (either before or after the PR) is buggy due to race conditions.

@kriswallsmith
@stof
Collaborator

@kriswallsmith what is the status here ?

@vicb vicb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
7 src/Assetic/Cache/ExpiringCache.php
@@ -29,13 +29,12 @@ public function __construct(CacheInterface $cache, $lifetime)
public function has($key)
{
- if ($this->cache->has($key)) {
+ if ($this->cache->has($key.'.expires')) {
if (time() < $this->cache->get($key.'.expires')) {
@vicb
vicb added a note

@kriswallsmith the rationale is that you don't want to check something that is not in the cache $key and $key.expires are distinct entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
return true;
}
- $this->cache->remove($key.'.expires');
- $this->cache->remove($key);
+ $this->remove($key);
}
return false;
@@ -43,7 +42,7 @@ public function has($key)
public function get($key)
{
- return $this->cache->get($key);
+ return $this->has($key) ? $this->cache->get($key) : null;
}
public function set($key, $value)
View
50 tests/Assetic/Test/Cache/ExpiringCacheTest.php
@@ -34,7 +34,7 @@ public function testHasExpired()
$this->inner->expects($this->once())
->method('has')
- ->with($key)
+ ->with($expiresKey)
->will($this->returnValue(true));
$this->inner->expects($this->once())
->method('get')
@@ -58,7 +58,7 @@ public function testHasNotExpired()
$this->inner->expects($this->once())
->method('has')
- ->with($key)
+ ->with($expiresKey)
->will($this->returnValue(true));
$this->inner->expects($this->once())
->method('get')
@@ -98,14 +98,50 @@ public function testRemove()
$this->cache->remove($key);
}
-
- public function testGet()
+
+ public function testGetExpired()
{
+ $key = 'asdf';
+ $expiresKey = 'asdf.expires';
+ $thePast = 0;
+
+ $this->inner->expects($this->once())
+ ->method('has')
+ ->with($expiresKey)
+ ->will($this->returnValue(true));
$this->inner->expects($this->once())
->method('get')
- ->with('foo')
- ->will($this->returnValue('bar'));
+ ->with($expiresKey)
+ ->will($this->returnValue($thePast));
+ $this->inner->expects($this->at(2))
+ ->method('remove')
+ ->with($expiresKey);
+ $this->inner->expects($this->at(3))
+ ->method('remove')
+ ->with($key);
- $this->assertEquals('bar', $this->cache->get('foo'), '->get() returns the cached value');
+ $this->assertNull($this->cache->get($key), '->get() returns null if an expired value exists');
}
+
+ public function testGetNotExpired()
+ {
+ $key = 'asdf';
+ $expiresKey = 'asdf.expires';
+ $theFuture = time() * 2;
+
+ $this->inner->expects($this->once())
+ ->method('has')
+ ->with($expiresKey)
+ ->will($this->returnValue(true));
+ $this->inner->expects($this->at(1))
+ ->method('get')
+ ->with($expiresKey)
+ ->will($this->returnValue($theFuture));
+ $this->inner->expects($this->at(2))
+ ->method('get')
+ ->with($key)
+ ->will($this->returnValue('bar'));
+
+ $this->assertEquals('bar', $this->cache->get($key), '->get() returns the cached value');
+ }
}
Something went wrong with that request. Please try again.