Skip to content

Commit 00f5e7b

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#17661 [Cache] Fix expiries handling (nicolas-grekas)
This PR was merged into the 3.1-dev branch. Discussion ---------- [Cache] Fix expiries handling | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 064ec0b [Cache] Fix expiries handling
2 parents df99788 + 064ec0b commit 00f5e7b

File tree

9 files changed

+50
-61
lines changed

9 files changed

+50
-61
lines changed

‎src/Symfony/Component/Cache/Adapter/AbstractAdapter.php

Lines changed: 35 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,13 @@ function ($key, $value, $isHit) use ($defaultLifetime) {
4949
$this->mergeByLifetime = \Closure::bind(
5050
function ($deferred, $namespace) {
5151
$byLifetime = array();
52+
$now = time();
5253

5354
foreach ($deferred as $key => $item) {
54-
if (0 <= $item->lifetime) {
55-
$byLifetime[(int) $item->lifetime][$namespace.$key] = $item->value;
55+
if (null === $item->expiry) {
56+
$byLifetime[0][$namespace.$key] = $item->value;
57+
} elseif ($item->expiry > $now) {
58+
$byLifetime[$item->expiry - $now][$namespace.$key] = $item->value;
5659
}
5760
}
5861

@@ -166,20 +169,7 @@ public function hasItem($key)
166169
$id = $this->getId($key);
167170

168171
if (isset($this->deferred[$key])) {
169-
$item = (array) $this->deferred[$key];
170-
try {
171-
$e = null;
172-
$value = $item[CacheItem::CAST_PREFIX.'value'];
173-
$ok = $this->doSave(array($key => $value), $item[CacheItem::CAST_PREFIX.'lifetime']);
174-
unset($this->deferred[$key]);
175-
176-
if (true === $ok || array() === $ok) {
177-
return true;
178-
}
179-
} catch (\Exception $e) {
180-
}
181-
$type = is_object($value) ? get_class($value) : gettype($value);
182-
CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e));
172+
$this->commit();
183173
}
184174

185175
try {
@@ -286,44 +276,50 @@ public function saveDeferred(CacheItemInterface $item)
286276
*/
287277
public function commit()
288278
{
289-
$f = $this->mergeByLifetime;
290-
$ko = array();
279+
$ok = true;
280+
$byLifetime = $this->mergeByLifetime;
281+
$byLifetime = $byLifetime($this->deferred, $this->namespace);
282+
$retry = $this->deferred = array();
291283

292-
foreach ($f($this->deferred, $this->namespace) as $lifetime => $values) {
284+
foreach ($byLifetime as $lifetime => $values) {
293285
try {
294-
if (true === $ok = $this->doSave($values, $lifetime)) {
295-
continue;
296-
}
286+
$e = $this->doSave($values, $lifetime);
297287
} catch (\Exception $e) {
298-
$ok = false;
299288
}
300-
if (false === $ok) {
301-
$ok = array_keys($values);
289+
if (true === $e || array() === $e) {
290+
continue;
302291
}
303-
foreach ($ok as $id) {
304-
$ko[$lifetime][] = array($id => $values[$id]);
292+
if (is_array($e) || 1 === count($values)) {
293+
foreach (is_array($e) ? $e : array_keys($values) as $id) {
294+
$ok = false;
295+
$v = $values[$id];
296+
$type = is_object($v) ? get_class($v) : gettype($v);
297+
CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => substr($id, strlen($this->namespace)), 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
298+
}
299+
} else {
300+
foreach ($values as $id => $v) {
301+
$retry[$lifetime][] = $id;
302+
}
305303
}
306304
}
307305

308-
$this->deferred = array();
309-
$ok = true;
310-
311306
// When bulk-save failed, retry each item individually
312-
foreach ($ko as $lifetime => $values) {
313-
foreach ($values as $v) {
307+
foreach ($retry as $lifetime => $ids) {
308+
foreach ($ids as $id) {
314309
try {
315-
$e = $this->doSave($v, $lifetime);
310+
$v = $byLifetime[$lifetime][$id];
311+
$e = $this->doSave(array($id => $v), $lifetime);
316312
} catch (\Exception $e) {
317313
}
318-
if (true !== $e && array() !== $e) {
319-
$ok = false;
320-
foreach ($v as $key => $value) {
321-
$type = is_object($value) ? get_class($value) : gettype($value);
322-
CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
323-
}
314+
if (true === $e || array() === $e) {
315+
continue;
324316
}
317+
$ok = false;
318+
$type = is_object($v) ? get_class($v) : gettype($v);
319+
CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => substr($id, strlen($this->namespace)), 'type' => $type, 'exception' => $e instanceof \Exception ? $e : null));
325320
}
326321
}
322+
$this->deferred = array();
327323

328324
return $ok;
329325
}

‎src/Symfony/Component/Cache/Adapter/ApcuAdapter.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,6 @@ protected function doDelete(array $ids)
7070
*/
7171
protected function doSave(array $values, $lifetime)
7272
{
73-
return apcu_store($values, null, $lifetime);
73+
return array_keys(apcu_store($values, null, $lifetime));
7474
}
7575
}

‎src/Symfony/Component/Cache/Adapter/ArrayAdapter.php

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public function getItems(array $keys = array())
7878
$this->validateKey($key);
7979
}
8080

81-
return $this->generateItems($keys);
81+
return $this->generateItems($keys, time());
8282
}
8383

8484
/**
@@ -132,9 +132,9 @@ public function save(CacheItemInterface $item)
132132
$item = (array) $item;
133133
$key = $item[CacheItem::CAST_PREFIX.'key'];
134134
$value = $item[CacheItem::CAST_PREFIX.'value'];
135-
$lifetime = $item[CacheItem::CAST_PREFIX.'lifetime'];
135+
$expiry = $item[CacheItem::CAST_PREFIX.'expiry'];
136136

137-
if (0 > $lifetime) {
137+
if (null !== $expiry && $expiry <= time()) {
138138
return true;
139139
}
140140
if ($this->storeSerialized) {
@@ -149,7 +149,7 @@ public function save(CacheItemInterface $item)
149149
}
150150

151151
$this->values[$key] = $value;
152-
$this->expiries[$key] = $lifetime ? $lifetime + time() : PHP_INT_MAX;
152+
$this->expiries[$key] = null !== $expiry ? $expiry : PHP_INT_MAX;
153153

154154
return true;
155155
}
@@ -185,12 +185,12 @@ private function validateKey($key)
185185
return $key;
186186
}
187187

188-
private function generateItems(array $keys)
188+
private function generateItems(array $keys, $now)
189189
{
190190
$f = $this->createCacheItem;
191191

192192
foreach ($keys as $key) {
193-
if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key))) {
193+
if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= $now || !$this->deleteItem($key))) {
194194
$value = null;
195195
} elseif ($this->storeSerialized) {
196196
$value = unserialize($this->values[$key]);

‎src/Symfony/Component/Cache/Adapter/ProxyAdapter.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,10 @@ private function doSave(CacheItemInterface $item, $method)
121121
return false;
122122
}
123123
$item = (array) $item;
124+
$expiry = $item[CacheItem::CAST_PREFIX.'expiry'];
124125
$poolItem = $this->pool->getItem($item[CacheItem::CAST_PREFIX.'key']);
125126
$poolItem->set($item[CacheItem::CAST_PREFIX.'value']);
126-
$poolItem->expiresAfter($item[CacheItem::CAST_PREFIX.'lifetime']);
127+
$poolItem->expiresAt(null !== $expiry ? \DateTime::createFromFormat('U', $expiry) : null);
127128

128129
return $this->pool->$method($poolItem);
129130
}

‎src/Symfony/Component/Cache/CacheItem.php

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ final class CacheItem implements CacheItemInterface
2828
private $key;
2929
private $value;
3030
private $isHit;
31-
private $lifetime;
31+
private $expiry;
3232
private $defaultLifetime;
3333

3434
/**
@@ -71,9 +71,9 @@ public function set($value)
7171
public function expiresAt($expiration)
7272
{
7373
if (null === $expiration) {
74-
$this->lifetime = $this->defaultLifetime;
74+
$this->expiry = $this->defaultLifetime > 0 ? time() + $this->defaultLifetime : null;
7575
} elseif ($expiration instanceof \DateTimeInterface) {
76-
$this->lifetime = $expiration->format('U') - time() ?: -1;
76+
$this->expiry = $expiration->format('U');
7777
} else {
7878
throw new InvalidArgumentException(sprintf('Expiration date must implement DateTimeInterface or be null, "%s" given', is_object($expiration) ? get_class($expiration) : gettype($expiration)));
7979
}
@@ -87,12 +87,11 @@ public function expiresAt($expiration)
8787
public function expiresAfter($time)
8888
{
8989
if (null === $time) {
90-
$this->lifetime = $this->defaultLifetime;
90+
$this->expiry = $this->defaultLifetime > 0 ? time() + $this->defaultLifetime : null;
9191
} elseif ($time instanceof \DateInterval) {
92-
$now = time();
93-
$this->lifetime = \DateTime::createFromFormat('U', $now)->add($time)->format('U') - $now ?: -1;
92+
$this->expiry = \DateTime::createFromFormat('U', time())->add($time)->format('U');
9493
} elseif (is_int($time)) {
95-
$this->lifetime = $time ?: -1;
94+
$this->expiry = $time + time();
9695
} else {
9796
throw new InvalidArgumentException(sprintf('Expiration date must be an integer, a DateInterval or null, "%s" given', is_object($time) ? get_class($time) : gettype($time)));
9897
}

‎src/Symfony/Component/Cache/Tests/Adapter/ApcuAdapterTest.php

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,6 @@
1616

1717
class ApcuAdapterTest extends CachePoolTest
1818
{
19-
protected $skippedTests = array(
20-
'testDeferredExpired' => 'Failing for now, needs to be fixed.',
21-
);
22-
2319
public function createCachePool()
2420
{
2521
if (defined('HHVM_VERSION')) {

‎src/Symfony/Component/Cache/Tests/Adapter/ArrayAdapterTest.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ class ArrayAdapterTest extends CachePoolTest
2222
protected $skippedTests = array(
2323
'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayAdapter is not.',
2424
'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayAdapter is not.',
25-
'testDeferredExpired' => 'Failing for now, needs to be fixed.',
2625
);
2726

2827
public function createCachePool()

‎src/Symfony/Component/Cache/Tests/Adapter/DoctrineAdapterTest.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ class DoctrineAdapterTest extends CachePoolTest
2323
protected $skippedTests = array(
2424
'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayCache is not.',
2525
'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayCache is not.',
26-
'testDeferredExpired' => 'Failing for now, needs to be fixed.',
2726
);
2827

2928
public function createCachePool()

‎src/Symfony/Component/Cache/Tests/Adapter/ProxyAdapterTest.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ class ProxyAdapterTest extends CachePoolTest
2323
protected $skippedTests = array(
2424
'testDeferredSaveWithoutCommit' => 'Assumes a shared cache which ArrayAdapter is not.',
2525
'testSaveWithoutExpire' => 'Assumes a shared cache which ArrayAdapter is not.',
26-
'testDeferredExpired' => 'Failing for now, needs to be fixed.',
2726
);
2827

2928
public function createCachePool()

0 commit comments

Comments
 (0)