bug #38670 [RateLimiter] Be more type safe when fetching from cache (Nyholm)

chalasr · chalasr · commit 753932527eff · 2020-10-22T10:40:20.000+02:00
This PR was merged into the 5.x branch. Discussion ---------- [RateLimiter] Be more type safe when fetching from cache | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | This is a super minor thing. A `$cacheItem` can be a hit, but it does not contain a `LimiterStateInterface`. Also, PSR-6 specifies that if the `$cacheItem` is not a hit, it must return null. Commits ------- 4795756 [RateLimiter] Be more type safe when fetching form cache
diff --git a/src/Symfony/Component/RateLimiter/Storage/CacheStorage.php b/src/Symfony/Component/RateLimiter/Storage/CacheStorage.php
@@ -42,11 +42,12 @@ public function save(LimiterStateInterface $limiterState): void
     public function fetch(string $limiterStateId): ?LimiterStateInterface
     {
         $cacheItem = $this->pool->getItem(sha1($limiterStateId));
-        if (!$cacheItem->isHit()) {
-            return null;
+        $value = $cacheItem->get();
+        if ($value instanceof LimiterStateInterface) {
+            return $value;
         }
 
-        return $cacheItem->get();
+        return null;
     }
 
     public function delete(string $limiterStateId): void
diff --git a/src/Symfony/Component/RateLimiter/Tests/Storage/CacheStorageTest.php b/src/Symfony/Component/RateLimiter/Tests/Storage/CacheStorageTest.php
@@ -55,6 +55,18 @@ public function testFetchExistingState()
         $this->assertEquals($window, $this->storage->fetch('test'));
     }
 
+    public function testFetchExistingJunk()
+    {
+        $cacheItem = $this->createMock(CacheItemInterface::class);
+
+        $cacheItem->expects($this->any())->method('get')->willReturn('junk');
+        $cacheItem->expects($this->any())->method('isHit')->willReturn(true);
+
+        $this->pool->expects($this->any())->method('getItem')->with(sha1('test'))->willReturn($cacheItem);
+
+        $this->assertNull($this->storage->fetch('test'));
+    }
+
     public function testFetchNonExistingState()
     {
         $cacheItem = $this->createMock(CacheItemInterface::class);

Original file line number	Diff line number	Diff line change
`@@ -42,11 +42,12 @@ public function save(LimiterStateInterface $limiterState): void`
`42`	`42`	`public function fetch(string $limiterStateId): ?LimiterStateInterface`
`43`	`43`	`{`
`44`	`44`	`$cacheItem = $this->pool->getItem(sha1($limiterStateId));`
`45`		`- if (!$cacheItem->isHit()) {`
`46`		`- return null;`
	`45`	`+ $value = $cacheItem->get();`
	`46`	`+ if ($value instanceof LimiterStateInterface) {`
	`47`	`+ return $value;`
`47`	`48`	`}`
`48`	`49`
`49`		`- return $cacheItem->get();`
	`50`	`+ return null;`
`50`	`51`	`}`
`51`	`52`
`52`	`53`	`public function delete(string $limiterStateId): void`