8000 bug #46699 [Cache] Respect $save option in all adapters (jrjohnson) · symfony/symfony@fa4a7a4 · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit fa4a7a4

Browse files
< 8000 div class="d-flex flex-column gap-2 width-full">
nicolas-grekasnicolas-grekas
committed
bug #46699 [Cache] Respect $save option in all adapters (jrjohnson)
This PR was merged into the 4.4 branch. Discussion ---------- [Cache] Respect $save option in all adapters | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | I was working with a cache chain which included the array adapter. When we have a cache miss we need to consolidate all of the misses so we can do a single DB query for all values. So first we look in the cache, then and then we can pull the data as a big group, hydrate the results, and then cache them. Because we search for these values a bunch of times in the same request I chained the array and Redis adapter together. Looks like: ``` $hitsAndMisses = array_map( fn(mixed $id) => $this->cache->get($id, function (ItemInterface $item, bool &$save) { $save = false; return false; }), $ids ); $hits = array_filter($hitsAndMisses); //cut: compare arrays to get missed Ids $hydrated = $this->hydrate($missedIds); //very expensive to do individually $missedValues = array_map(fn($obj) => $this->cache->get( $obj->id, function (ItemInterface $item) use ($obj) { return $obj; } ), $hydrated); return array_values([...$hits, ...$missedValues]); ``` Unfortunately neither the `ArrayAdapter` nor the `ChainAdapter` respect the $save reference. I was able to fix the `ArrayAdapter` in this PR, but I'm at a loss for how to do the same for the `ChainAdapter`, but I was able to create a generic failing test. Commits ------- b14aa77 [Cache] Respect $save option in ChainAdapter 5e8de1e [Cache] Respect $save option in ArrayAdapter
2 parents 5990514 + b14aa77 commit fa4a7a4

File tree

4 files changed

+40
-2
lines changed

4 files changed

+40
-2
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@ public function get(string $key, callable $callback, float $beta = null, array &
6060
// ArrayAdapter works in memory, we don't care about stampede protection
6161
if (\INF === $beta || !$item->isHit()) {
6262
$save = true;
63-
$this->save($item->set($callback($item, $save)));
63+
$item->set($callback($item, $save));
64+
if ($save) {
65+
$this->save($item);
66+
}
6467
}
6568

6669
return $item->get();

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,17 @@ static function ($sourceItem, $item, $sourceMetadata = null) use ($defaultLifeti
9191
*/
9292
public function get(string $key, callable $callback, float $beta = null, array &$metadata = null)
9393
{
94+
$doSave = true;
95+
$callback = static function (CacheItem $item, bool &$save) use ($callback, &$doSave) {
96+
$value = $callback($item, $save);
97+
$doSave = $save;
98+
99+
return $value;
100+
};
101+
94102
$lastItem = null;
95103
$i = 0;
96-
$wrap = function (CacheItem $item = null) use ($key, $callback, $beta, &$wrap, &$i, &$lastItem, &$metadata) {
104+
$wrap = function (CacheItem $item = null, bool &$save = true) use ($key, $callback, $beta, &$wrap, &$i, &$doSave, &$lastItem, &$metadata) {
97105
$adapter = $this->adapters[$i];
98106
if (isset($this->adapters[++$i])) {
99107
$callback = $wrap;
@@ -107,6 +115,7 @@ public function get(string $key, callable $callback, float $beta = null, array &
107115
if (null !== $item) {
108116
($this->syncItem)($lastItem = $lastItem ?? $item, $item, $metadata);
109117
}
118+
$save = $doSave;
110119

111120
return $value;
112121
};

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,31 @@ public function testRecursiveGet()
9898
$this->assertSame(1, $cache->get('k2', function () { return 2; }));
9999
}
100100

101+
public function testDontSaveWhenAskedNotTo()
102+
{
103+
if (isset($this->skippedTests[__FUNCTION__])) {
104+
$this->markTestSkipped($this->skippedTests[__FUNCTION__]);
105+
}
106+
107+
$cache = $this->createCachePool(0, __FUNCTION__);
108+
109+
$v1 = $cache->get('some-key', function($item, &$save){
110+
$save = false;
111+
return 1;
112+
});
113+
$this->assertSame($v1, 1);
114+
115+
$v2 = $cache->get('some-key', function(){
116+
return 2;
117+
});
118+
$this->assertSame($v2, 2, 'First value was cached and should not have been');
119+
120+
$v3 = $cache->get('some-key', function(){
121+
$this->fail('Value should have come from cache');
122+
});
123+
$this->assertSame($v3, 2);
124+
}
125+
101126
public function testGetMetadata()
102127
{
103128
if (isset($this->skippedTests[__FUNCTION__])) {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ class PhpArrayAdapterTest extends AdapterTestCase
2424
{
2525
protected $skippedTests = [
2626
'testGet' => 'PhpArrayAdapter is read-only.',
27+
'testDontSaveWhenAskedNotTo' => 'PhpArrayAdapter is read-only.',
2728
'testRecursiveGet' => 'PhpArrayAdapter is read-only.',
2829
'testBasicUsage' => 'PhpArrayAdapter is read-only.',
2930
'testBasicUsageWithLongKey' => 'PhpArrayAdapter is read-only.',

0 commit comments

Comments
 (0)
0