From f7407bdb6daae387fc35036c71122e13d3b60701 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 16:14:51 +0100 Subject: [PATCH 1/9] Fix types for put on method signatures The put method is an alias for the PSR Cache set method and should also accept the null type. --- src/Illuminate/Cache/RedisTaggedCache.php | 2 +- src/Illuminate/Contracts/Cache/Repository.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Cache/RedisTaggedCache.php b/src/Illuminate/Cache/RedisTaggedCache.php index 58cca6311367..c231f4adfca5 100644 --- a/src/Illuminate/Cache/RedisTaggedCache.php +++ b/src/Illuminate/Cache/RedisTaggedCache.php @@ -22,7 +22,7 @@ class RedisTaggedCache extends TaggedCache * * @param string $key * @param mixed $value - * @param \DateTime|float|int|null $minutes + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return bool */ public function put($key, $value, $minutes = null) diff --git a/src/Illuminate/Contracts/Cache/Repository.php b/src/Illuminate/Contracts/Cache/Repository.php index 82fafd42aed4..1dc1505509cc 100644 --- a/src/Illuminate/Contracts/Cache/Repository.php +++ b/src/Illuminate/Contracts/Cache/Repository.php @@ -38,7 +38,7 @@ public function pull($key, $default = null); * * @param string $key * @param mixed $value - * @param \DateTimeInterface|\DateInterval|float|int $minutes + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return bool */ public function put($key, $value, $minutes); From c8ab29d6fafab342b36ee95951f427db105f23a0 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 16:45:37 +0100 Subject: [PATCH 2/9] Allow cache stores to handle null TTL At the moment our Cache implementation implements the Psr\SimpleCache\CacheInterface which states that the TTL, if null, allows for either the library or the driver to set a default. At the moment it's impossible with Laravel to allow for the drivers/stores to handle defaults on their own because the method signature doesn't allows for this. By allowing it as a valid type, developers may choose to implement a cache driver which handles a default TTL for cache items. Our current cache stores are updated to handle the new null value as well to properly return false when null is passed. This is because our current cache stores don't provide a default TTL. --- src/Illuminate/Cache/ApcStore.php | 6 +++++- src/Illuminate/Cache/ArrayStore.php | 6 +++++- src/Illuminate/Cache/DatabaseStore.php | 6 +++++- src/Illuminate/Cache/DynamoDbStore.php | 6 +++++- src/Illuminate/Cache/FileStore.php | 6 +++++- src/Illuminate/Cache/MemcachedStore.php | 6 +++++- src/Illuminate/Cache/NullStore.php | 2 +- src/Illuminate/Cache/RedisStore.php | 6 +++++- src/Illuminate/Contracts/Cache/Store.php | 2 +- tests/Cache/CacheApcStoreTest.php | 10 ++++++++++ tests/Cache/CacheArrayStoreTest.php | 8 ++++++++ tests/Cache/CacheDatabaseStoreTest.php | 12 ++++++++++++ tests/Cache/CacheFileStoreTest.php | 9 +++++++++ tests/Cache/CacheMemcachedStoreTest.php | 14 ++++++++++++++ tests/Cache/CacheRedisStoreTest.php | 7 +++++++ 15 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/Illuminate/Cache/ApcStore.php b/src/Illuminate/Cache/ApcStore.php index 6e228fddd3a4..e701c0fea67d 100755 --- a/src/Illuminate/Cache/ApcStore.php +++ b/src/Illuminate/Cache/ApcStore.php @@ -53,11 +53,15 @@ public function get($key) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + return $this->apc->put($this->prefix.$key, $value, (int) ($minutes * 60)); } diff --git a/src/Illuminate/Cache/ArrayStore.php b/src/Illuminate/Cache/ArrayStore.php index 1ced2646ca07..248770f18257 100644 --- a/src/Illuminate/Cache/ArrayStore.php +++ b/src/Illuminate/Cache/ArrayStore.php @@ -29,11 +29,15 @@ public function get($key) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $this->storage[$key] = $value; return true; diff --git a/src/Illuminate/Cache/DatabaseStore.php b/src/Illuminate/Cache/DatabaseStore.php index 7b163cdcd2e6..b0a03a794364 100755 --- a/src/Illuminate/Cache/DatabaseStore.php +++ b/src/Illuminate/Cache/DatabaseStore.php @@ -88,11 +88,15 @@ public function get($key) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $key = $this->prefix.$key; $value = $this->serialize($value); diff --git a/src/Illuminate/Cache/DynamoDbStore.php b/src/Illuminate/Cache/DynamoDbStore.php index 6adf02cebb9b..73bd60683e9a 100644 --- a/src/Illuminate/Cache/DynamoDbStore.php +++ b/src/Illuminate/Cache/DynamoDbStore.php @@ -171,11 +171,15 @@ protected function isExpired(array $item, $expiration = null) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return void */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $this->dynamo->putItem([ 'TableName' => $this->table, 'Item' => [ diff --git a/src/Illuminate/Cache/FileStore.php b/src/Illuminate/Cache/FileStore.php index 0bc164d03410..0ef7773d9e80 100755 --- a/src/Illuminate/Cache/FileStore.php +++ b/src/Illuminate/Cache/FileStore.php @@ -54,11 +54,15 @@ public function get($key) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $this->ensureCacheDirectoryExists($path = $this->path($key)); $result = $this->files->put( diff --git a/src/Illuminate/Cache/MemcachedStore.php b/src/Illuminate/Cache/MemcachedStore.php index 5fbc68ce03cc..b4718d287039 100755 --- a/src/Illuminate/Cache/MemcachedStore.php +++ b/src/Illuminate/Cache/MemcachedStore.php @@ -97,11 +97,15 @@ public function many(array $keys) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + return $this->memcached->set( $this->prefix.$key, $value, $this->calculateExpiration($minutes) ); diff --git a/src/Illuminate/Cache/NullStore.php b/src/Illuminate/Cache/NullStore.php index 2358a125782f..c787812880d1 100755 --- a/src/Illuminate/Cache/NullStore.php +++ b/src/Illuminate/Cache/NullStore.php @@ -29,7 +29,7 @@ public function get($key) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php index 0af7bff35932..06c7e425ebcd 100755 --- a/src/Illuminate/Cache/RedisStore.php +++ b/src/Illuminate/Cache/RedisStore.php @@ -84,11 +84,15 @@ public function many(array $keys) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $result = $this->connection()->setex( $this->prefix.$key, (int) max(1, $minutes * 60), $this->serialize($value) ); diff --git a/src/Illuminate/Contracts/Cache/Store.php b/src/Illuminate/Contracts/Cache/Store.php index 3db1014f0e17..25d0ebb0ca3c 100644 --- a/src/Illuminate/Contracts/Cache/Store.php +++ b/src/Illuminate/Contracts/Cache/Store.php @@ -27,7 +27,7 @@ public function many(array $keys); * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function put($key, $value, $minutes); diff --git a/tests/Cache/CacheApcStoreTest.php b/tests/Cache/CacheApcStoreTest.php index b7d4075cdd93..03a218347603 100755 --- a/tests/Cache/CacheApcStoreTest.php +++ b/tests/Cache/CacheApcStoreTest.php @@ -51,6 +51,16 @@ public function testSetMethodProperlyCallsAPC() $this->assertTrue($result); } + public function testSetMethodWithNullTTLFails() + { + $apc = $this->getMockBuilder(ApcWrapper::class)->setMethods(['put'])->getMock(); + $apc->expects($this->never()) + ->method('put'); + $store = new ApcStore($apc); + $result = $store->put('foo', 'bar', null); + $this->assertFalse($result); + } + public function testSetMultipleMethodProperlyCallsAPC() { $apc = $this->getMockBuilder(ApcWrapper::class)->setMethods(['put'])->getMock(); diff --git a/tests/Cache/CacheArrayStoreTest.php b/tests/Cache/CacheArrayStoreTest.php index c0b2c3b363d7..aed9aa161993 100755 --- a/tests/Cache/CacheArrayStoreTest.php +++ b/tests/Cache/CacheArrayStoreTest.php @@ -15,6 +15,14 @@ public function testItemsCanBeSetAndRetrieved() $this->assertEquals('bar', $store->get('foo')); } + public function testAnItemWithNoTTLCannotBeSet() + { + $store = new ArrayStore; + $result = $store->put('foo', 'bar', null); + $this->assertFalse($result); + $this->assertNull($store->get('foo')); + } + public function testMultipleItemsCanBeSetAndRetrieved() { $store = new ArrayStore; diff --git a/tests/Cache/CacheDatabaseStoreTest.php b/tests/Cache/CacheDatabaseStoreTest.php index f4cc06a05d9d..ee716dc6642f 100755 --- a/tests/Cache/CacheDatabaseStoreTest.php +++ b/tests/Cache/CacheDatabaseStoreTest.php @@ -3,6 +3,7 @@ namespace Illuminate\Tests\Cache; use Closure; +use Illuminate\Database\ConnectionInterface; use stdClass; use Exception; use Mockery as m; @@ -75,6 +76,17 @@ public function testValueIsInsertedWhenNoExceptionsAreThrown() $this->assertTrue($result); } + public function testValueIsNotInsertedWithoutTTL() + { + $connection = m::mock(ConnectionInterface::class); + $connection->shouldReceive('table')->never(); + + $store = new DatabaseStore($connection, 'table'); + + $result = $store->put('foo', 'bar', null); + $this->assertFalse($result); + } + public function testValueIsUpdatedWhenInsertThrowsException() { $store = $this->getMockBuilder(DatabaseStore::class)->setMethods(['getTime'])->setConstructorArgs($this->getMocks())->getMock(); diff --git a/tests/Cache/CacheFileStoreTest.php b/tests/Cache/CacheFileStoreTest.php index 854572df9c8f..75cdd3d7620a 100755 --- a/tests/Cache/CacheFileStoreTest.php +++ b/tests/Cache/CacheFileStoreTest.php @@ -46,6 +46,15 @@ public function testPutCreatesMissingDirectories() $this->assertTrue($result); } + public function testPutReturnsFalseForNullTTL() + { + $files = $this->mockFilesystem(); + $contents = '0000000000'; + $store = new FileStore($files, __DIR__); + $result = $store->put('foo', $contents, null); + $this->assertFalse($result); + } + public function testExpiredItemsReturnNull() { $files = $this->mockFilesystem(); diff --git a/tests/Cache/CacheMemcachedStoreTest.php b/tests/Cache/CacheMemcachedStoreTest.php index 30b60644b6f4..1a749a64a37c 100755 --- a/tests/Cache/CacheMemcachedStoreTest.php +++ b/tests/Cache/CacheMemcachedStoreTest.php @@ -74,6 +74,20 @@ public function testSetMethodProperlyCallsMemcache() Carbon::setTestNow(); } + public function testPutMethodReturnsFalseIfTTLIsNull() + { + if (! class_exists(Memcached::class)) { + $this->markTestSkipped('Memcached module not installed'); + } + + Carbon::setTestNow($now = Carbon::now()); + $memcached = $this->getMockBuilder(Memcached::class)->setMethods(['set'])->getMock(); + $store = new MemcachedStore($memcached); + $result = $store->put('foo', 'bar', null); + $this->assertFalse($result); + Carbon::setTestNow(); + } + public function testIncrementMethodProperlyCallsMemcache() { if (! class_exists(Memcached::class)) { diff --git a/tests/Cache/CacheRedisStoreTest.php b/tests/Cache/CacheRedisStoreTest.php index 7d836005085c..41602cae77a4 100755 --- a/tests/Cache/CacheRedisStoreTest.php +++ b/tests/Cache/CacheRedisStoreTest.php @@ -67,6 +67,13 @@ public function testSetMethodProperlyCallsRedis() $this->assertTrue($result); } + public function testPutMethodReturnsFalseIfTTLIsNull() + { + $redis = $this->getRedis(); + $result = $redis->put('foo', 'foo', null); + $this->assertFalse($result); + } + public function testSetMultipleMethodProperlyCallsRedis() { $redis = $this->getRedis(); From 429f33dab31f68c758916dcef03b88ef5b15ccf8 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 17:16:09 +0100 Subject: [PATCH 3/9] Fix putMany TTL on cache stores Same as previous commit. --- src/Illuminate/Cache/DynamoDbStore.php | 6 +++++- src/Illuminate/Cache/MemcachedStore.php | 6 +++++- src/Illuminate/Cache/RedisStore.php | 6 +++++- src/Illuminate/Cache/RetrievesMultipleKeys.php | 6 +++++- src/Illuminate/Contracts/Cache/Store.php | 2 +- 5 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Cache/DynamoDbStore.php b/src/Illuminate/Cache/DynamoDbStore.php index 73bd60683e9a..535e013cc3ea 100644 --- a/src/Illuminate/Cache/DynamoDbStore.php +++ b/src/Illuminate/Cache/DynamoDbStore.php @@ -200,11 +200,15 @@ public function put($key, $value, $minutes) * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param float|int $minutes + * @param float|int|null $minutes * @return void */ public function putMany(array $values, $minutes) { + if (is_null($minutes)) { + return false; + } + $expiration = $this->toTimestamp($minutes); $this->dynamo->batchWriteItem([ diff --git a/src/Illuminate/Cache/MemcachedStore.php b/src/Illuminate/Cache/MemcachedStore.php index b4718d287039..e15393484f7f 100755 --- a/src/Illuminate/Cache/MemcachedStore.php +++ b/src/Illuminate/Cache/MemcachedStore.php @@ -115,11 +115,15 @@ public function put($key, $value, $minutes) * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function putMany(array $values, $minutes) { + if (is_null($minutes)) { + return false; + } + $prefixedValues = []; foreach ($values as $key => $value) { diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php index 06c7e425ebcd..ffeeb648a466 100755 --- a/src/Illuminate/Cache/RedisStore.php +++ b/src/Illuminate/Cache/RedisStore.php @@ -104,11 +104,15 @@ public function put($key, $value, $minutes) * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function putMany(array $values, $minutes) { + if (is_null($minutes)) { + return false; + } + $this->connection()->multi(); $manyResult = null; diff --git a/src/Illuminate/Cache/RetrievesMultipleKeys.php b/src/Illuminate/Cache/RetrievesMultipleKeys.php index 5459ba063be5..41c5da0fd5bf 100644 --- a/src/Illuminate/Cache/RetrievesMultipleKeys.php +++ b/src/Illuminate/Cache/RetrievesMultipleKeys.php @@ -27,11 +27,15 @@ public function many(array $keys) * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function putMany(array $values, $minutes) { + if (is_null($minutes)) { + return false; + } + $manyResult = null; foreach ($values as $key => $value) { diff --git a/src/Illuminate/Contracts/Cache/Store.php b/src/Illuminate/Contracts/Cache/Store.php index 25d0ebb0ca3c..bca06456c959 100644 --- a/src/Illuminate/Contracts/Cache/Store.php +++ b/src/Illuminate/Contracts/Cache/Store.php @@ -36,7 +36,7 @@ public function put($key, $value, $minutes); * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function putMany(array $values, $minutes); From 4488e171e8b0fd89cb08b2d65823e670018805ed Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 17:21:59 +0100 Subject: [PATCH 4/9] Fix return type of add method The success should depend on the result of the put method call. --- src/Illuminate/Cache/Repository.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Illuminate/Cache/Repository.php b/src/Illuminate/Cache/Repository.php index 4b368140d769..ecd6113579c6 100755 --- a/src/Illuminate/Cache/Repository.php +++ b/src/Illuminate/Cache/Repository.php @@ -284,9 +284,7 @@ public function add($key, $value, $minutes) // so it exists for subsequent requests. Then, we will return true so it is // easy to know if the value gets added. Otherwise, we will return false. if (is_null($this->get($key))) { - $this->put($key, $value, $minutes); - - return true; + return $this->put($key, $value, $minutes); } return false; From 9c412460fa5982ff5f7135a5b17bbf2662eb3060 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 17:25:48 +0100 Subject: [PATCH 5/9] Allow null value for add method --- src/Illuminate/Cache/DynamoDbStore.php | 6 +++++- src/Illuminate/Cache/MemcachedStore.php | 6 +++++- src/Illuminate/Cache/RedisStore.php | 6 +++++- src/Illuminate/Cache/Repository.php | 2 +- src/Illuminate/Contracts/Cache/Repository.php | 2 +- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Cache/DynamoDbStore.php b/src/Illuminate/Cache/DynamoDbStore.php index 535e013cc3ea..7285cba370de 100644 --- a/src/Illuminate/Cache/DynamoDbStore.php +++ b/src/Illuminate/Cache/DynamoDbStore.php @@ -239,11 +239,15 @@ public function putMany(array $values, $minutes) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function add($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + try { $response = $this->dynamo->putItem([ 'TableName' => $this->table, diff --git a/src/Illuminate/Cache/MemcachedStore.php b/src/Illuminate/Cache/MemcachedStore.php index e15393484f7f..019d5f10807c 100755 --- a/src/Illuminate/Cache/MemcachedStore.php +++ b/src/Illuminate/Cache/MemcachedStore.php @@ -140,11 +140,15 @@ public function putMany(array $values, $minutes) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function add($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + return $this->memcached->add( $this->prefix.$key, $value, $this->calculateExpiration($minutes) ); diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php index ffeeb648a466..9229c9749118 100755 --- a/src/Illuminate/Cache/RedisStore.php +++ b/src/Illuminate/Cache/RedisStore.php @@ -133,11 +133,15 @@ public function putMany(array $values, $minutes) * * @param string $key * @param mixed $value - * @param float|int $minutes + * @param float|int|null $minutes * @return bool */ public function add($key, $value, $minutes) { + if (is_null($minutes)) { + return false; + } + $lua = "return redis.call('exists',KEYS[1])<1 and redis.call('setex',KEYS[1],ARGV[2],ARGV[1])"; return (bool) $this->connection()->eval( diff --git a/src/Illuminate/Cache/Repository.php b/src/Illuminate/Cache/Repository.php index ecd6113579c6..d7b30c3c8524 100755 --- a/src/Illuminate/Cache/Repository.php +++ b/src/Illuminate/Cache/Repository.php @@ -262,7 +262,7 @@ public function setMultiple($values, $ttl = null) * * @param string $key * @param mixed $value - * @param \DateTimeInterface|\DateInterval|float|int $minutes + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return bool */ public function add($key, $value, $minutes) diff --git a/src/Illuminate/Contracts/Cache/Repository.php b/src/Illuminate/Contracts/Cache/Repository.php index 1dc1505509cc..dcfb26008204 100644 --- a/src/Illuminate/Contracts/Cache/Repository.php +++ b/src/Illuminate/Contracts/Cache/Repository.php @@ -48,7 +48,7 @@ public function put($key, $value, $minutes); * * @param string $key * @param mixed $value - * @param \DateTimeInterface|\DateInterval|float|int $minutes + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return bool */ public function add($key, $value, $minutes); From 013fa4d878a0d852cad120a0af4c7c8663616e38 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 18:07:54 +0100 Subject: [PATCH 6/9] Fix handling null TTLs in cache repository The way we currently handle null TTLs in our own cache repository is that we return false and don't save anything. But according to the PSR cache repository spec there should be support for handling defaults in drivers. At the moment the null TTL isn't cascaded to the cache stores so there's no way for them to implement a default. This commit allows for the TTL to be passed to the cache stores so they may set their default TTL is they have done so. It also still makes sure that for any TTL below or equal to 0 nothing will be set. No tests were adjusted (just a really minor adjustment to a Redis test) and more tests have been added to verify the new behavior. Also fixed some really small minor things here and there. --- src/Illuminate/Cache/Repository.php | 58 ++++++++++++++++------------ tests/Cache/CacheRepositoryTest.php | 25 ++++++++++++ tests/Cache/CacheTaggedCacheTest.php | 2 +- 3 files changed, 59 insertions(+), 26 deletions(-) diff --git a/src/Illuminate/Cache/Repository.php b/src/Illuminate/Cache/Repository.php index d7b30c3c8524..510cbf8c8c5a 100755 --- a/src/Illuminate/Cache/Repository.php +++ b/src/Illuminate/Cache/Repository.php @@ -199,22 +199,22 @@ public function pull($key, $default = null) public function put($key, $value, $minutes = null) { if (is_array($key)) { - $result = $this->putMany($key, $value); - - return $result; + return $this->putMany($key, $value); } - if (! is_null($minutes = $this->getMinutes($minutes))) { - $result = $this->store->put($this->itemKey($key), $value, $minutes); + $minutes = $this->getMinutes($minutes); - if ($result) { - $this->event(new KeyWritten($key, $value, $minutes)); - } + if ($minutes !== null && $minutes <= 0) { + return false; + } + + $result = $this->store->put($this->itemKey($key), $value, $minutes); - return $result; + if ($result) { + $this->event(new KeyWritten($key, $value, $minutes)); } - return false; + return $result; } /** @@ -229,24 +229,26 @@ public function set($key, $value, $ttl = null) * Store multiple items in the cache for a given number of minutes. * * @param array $values - * @param \DateTimeInterface|\DateInterval|float|int $minutes + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return bool */ public function putMany(array $values, $minutes) { - if (! is_null($minutes = $this->getMinutes($minutes))) { - $result = $this->store->putMany($values, $minutes); + $minutes = $this->getMinutes($minutes); - if ($result) { - foreach ($values as $key => $value) { - $this->event(new KeyWritten($key, $value, $minutes)); - } - } + if ($minutes !== null && $minutes <= 0) { + return false; + } + + $result = $this->store->putMany($values, $minutes); - return $result; + if ($result) { + foreach ($values as $key => $value) { + $this->event(new KeyWritten($key, $value, $minutes)); + } } - return false; + return $result; } /** @@ -267,7 +269,9 @@ public function setMultiple($values, $ttl = null) */ public function add($key, $value, $minutes) { - if (is_null($minutes = $this->getMinutes($minutes))) { + $minutes = $this->getMinutes($minutes); + + if ($minutes !== null && $minutes <= 0) { return false; } @@ -571,18 +575,22 @@ public function offsetUnset($key) /** * Calculate the number of minutes with the given duration. * - * @param \DateTimeInterface|\DateInterval|float|int $duration + * @param \DateTimeInterface|\DateInterval|float|int|null $minutes * @return float|int|null */ - protected function getMinutes($duration) + protected function getMinutes($minutes) { - $duration = $this->parseDateInterval($duration); + if ($minutes === null) { + return null; + } + + $duration = $this->parseDateInterval($minutes); if ($duration instanceof DateTimeInterface) { $duration = Carbon::now()->diffInRealSeconds($duration, false) / 60; } - return (int) ($duration * 60) > 0 ? $duration : null; + return $duration; } /** diff --git a/tests/Cache/CacheRepositoryTest.php b/tests/Cache/CacheRepositoryTest.php index 7efc23694c94..e90eda2e32da 100755 --- a/tests/Cache/CacheRepositoryTest.php +++ b/tests/Cache/CacheRepositoryTest.php @@ -157,6 +157,31 @@ public function testAddWithDatetimeInPastOrZeroSecondsReturnsImmediately() $this->assertFalse($result); } + public function testPutWorksForNullTTL() + { + $repo = $this->getRepository(); + $repo->getStore()->shouldReceive('put')->once()->with('foo', 'bar', null)->andReturn(true); + $result = $repo->put('foo', 'bar', null); + $this->assertTrue($result); + } + + public function testPutManyWorksForNullTTL() + { + $repo = $this->getRepository(); + $repo->getStore()->shouldReceive('putMany')->once()->with(['foo' => 'bar', 'bar' => 'baz'], null)->andReturn(true); + $result = $repo->putMany(['foo' => 'bar', 'bar' => 'baz'], null); + $this->assertTrue($result); + } + + public function testAddWorksForNullTTL() + { + $store = m::mock(RedisStore::class); + $store->shouldReceive('add')->once()->with('foo', 'bar', null)->andReturn(true); + $repository = new Repository($store); + $result = $repository->add('foo', 'bar', null); + $this->assertTrue($result); + } + public function testCacheAddCallsRedisStoreAdd() { $store = m::mock(RedisStore::class); diff --git a/tests/Cache/CacheTaggedCacheTest.php b/tests/Cache/CacheTaggedCacheTest.php index 5b35c13bd2b5..a9f56c7afd45 100644 --- a/tests/Cache/CacheTaggedCacheTest.php +++ b/tests/Cache/CacheTaggedCacheTest.php @@ -112,7 +112,7 @@ public function testRedisCacheTagsPushStandardKeysCorrectly() $conn->shouldReceive('sadd')->once()->with('prefix:bar:standard_ref', 'prefix:'.sha1('foo|bar').':key1'); $store->shouldReceive('push')->with(sha1('foo|bar').':key1', 'key1:value'); - $redis->put('key1', 'key1:value'); + $redis->put('key1', 'key1:value', 0); } public function testRedisCacheTagsCanBeFlushed() From 7cce088d0f304da940993cbe5567a22e3612831c Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Mon, 14 Jan 2019 18:23:44 +0100 Subject: [PATCH 7/9] Fix styling --- tests/Cache/CacheDatabaseStoreTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Cache/CacheDatabaseStoreTest.php b/tests/Cache/CacheDatabaseStoreTest.php index ee716dc6642f..6263c037adcd 100755 --- a/tests/Cache/CacheDatabaseStoreTest.php +++ b/tests/Cache/CacheDatabaseStoreTest.php @@ -3,7 +3,6 @@ namespace Illuminate\Tests\Cache; use Closure; -use Illuminate\Database\ConnectionInterface; use stdClass; use Exception; use Mockery as m; @@ -11,6 +10,7 @@ use Illuminate\Cache\DatabaseStore; use Illuminate\Database\Connection; use Illuminate\Database\PostgresConnection; +use Illuminate\Database\ConnectionInterface; class CacheDatabaseStoreTest extends TestCase { From 231cf9cf27789e8cf2f185d34a05fe7a82bb3913 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Tue, 15 Jan 2019 13:29:03 +0100 Subject: [PATCH 8/9] Replace is_null checks --- src/Illuminate/Cache/ApcStore.php | 2 +- src/Illuminate/Cache/ArrayStore.php | 2 +- src/Illuminate/Cache/DatabaseStore.php | 2 +- src/Illuminate/Cache/DynamoDbStore.php | 6 +++--- src/Illuminate/Cache/FileStore.php | 2 +- src/Illuminate/Cache/MemcachedStore.php | 6 +++--- src/Illuminate/Cache/RedisStore.php | 6 +++--- src/Illuminate/Cache/RetrievesMultipleKeys.php | 2 +- 8 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Illuminate/Cache/ApcStore.php b/src/Illuminate/Cache/ApcStore.php index e701c0fea67d..f0e480655380 100755 --- a/src/Illuminate/Cache/ApcStore.php +++ b/src/Illuminate/Cache/ApcStore.php @@ -58,7 +58,7 @@ public function get($key) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/ArrayStore.php b/src/Illuminate/Cache/ArrayStore.php index 248770f18257..3680563f776a 100644 --- a/src/Illuminate/Cache/ArrayStore.php +++ b/src/Illuminate/Cache/ArrayStore.php @@ -34,7 +34,7 @@ public function get($key) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/DatabaseStore.php b/src/Illuminate/Cache/DatabaseStore.php index b0a03a794364..f7904afacafe 100755 --- a/src/Illuminate/Cache/DatabaseStore.php +++ b/src/Illuminate/Cache/DatabaseStore.php @@ -93,7 +93,7 @@ public function get($key) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/DynamoDbStore.php b/src/Illuminate/Cache/DynamoDbStore.php index 7285cba370de..c1aeaaa9b934 100644 --- a/src/Illuminate/Cache/DynamoDbStore.php +++ b/src/Illuminate/Cache/DynamoDbStore.php @@ -176,7 +176,7 @@ protected function isExpired(array $item, $expiration = null) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -205,7 +205,7 @@ public function put($key, $value, $minutes) */ public function putMany(array $values, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -244,7 +244,7 @@ public function putMany(array $values, $minutes) */ public function add($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/FileStore.php b/src/Illuminate/Cache/FileStore.php index 0ef7773d9e80..b3194ba9ff67 100755 --- a/src/Illuminate/Cache/FileStore.php +++ b/src/Illuminate/Cache/FileStore.php @@ -59,7 +59,7 @@ public function get($key) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/MemcachedStore.php b/src/Illuminate/Cache/MemcachedStore.php index 019d5f10807c..e3856a25eaa7 100755 --- a/src/Illuminate/Cache/MemcachedStore.php +++ b/src/Illuminate/Cache/MemcachedStore.php @@ -102,7 +102,7 @@ public function many(array $keys) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -120,7 +120,7 @@ public function put($key, $value, $minutes) */ public function putMany(array $values, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -145,7 +145,7 @@ public function putMany(array $values, $minutes) */ public function add($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php index 9229c9749118..26c7b44fdf49 100755 --- a/src/Illuminate/Cache/RedisStore.php +++ b/src/Illuminate/Cache/RedisStore.php @@ -89,7 +89,7 @@ public function many(array $keys) */ public function put($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -109,7 +109,7 @@ public function put($key, $value, $minutes) */ public function putMany(array $values, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } @@ -138,7 +138,7 @@ public function putMany(array $values, $minutes) */ public function add($key, $value, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } diff --git a/src/Illuminate/Cache/RetrievesMultipleKeys.php b/src/Illuminate/Cache/RetrievesMultipleKeys.php index 41c5da0fd5bf..c5ba0a7e4d54 100644 --- a/src/Illuminate/Cache/RetrievesMultipleKeys.php +++ b/src/Illuminate/Cache/RetrievesMultipleKeys.php @@ -32,7 +32,7 @@ public function many(array $keys) */ public function putMany(array $values, $minutes) { - if (is_null($minutes)) { + if ($minutes === null) { return false; } From 2dea9602a1d5e4ecd3d0ef7c1d112cf5dc24aa27 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Tue, 15 Jan 2019 13:31:17 +0100 Subject: [PATCH 9/9] Cleanup tests --- tests/Cache/CacheRedisStoreTest.php | 4 +--- tests/Cache/CacheRepositoryTest.php | 9 +++------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/Cache/CacheRedisStoreTest.php b/tests/Cache/CacheRedisStoreTest.php index 41602cae77a4..c899d352b581 100755 --- a/tests/Cache/CacheRedisStoreTest.php +++ b/tests/Cache/CacheRedisStoreTest.php @@ -69,9 +69,7 @@ public function testSetMethodProperlyCallsRedis() public function testPutMethodReturnsFalseIfTTLIsNull() { - $redis = $this->getRedis(); - $result = $redis->put('foo', 'foo', null); - $this->assertFalse($result); + $this->assertFalse($this->getRedis()->put('foo', 'foo', null)); } public function testSetMultipleMethodProperlyCallsRedis() diff --git a/tests/Cache/CacheRepositoryTest.php b/tests/Cache/CacheRepositoryTest.php index e90eda2e32da..bf4394e89169 100755 --- a/tests/Cache/CacheRepositoryTest.php +++ b/tests/Cache/CacheRepositoryTest.php @@ -161,16 +161,14 @@ public function testPutWorksForNullTTL() { $repo = $this->getRepository(); $repo->getStore()->shouldReceive('put')->once()->with('foo', 'bar', null)->andReturn(true); - $result = $repo->put('foo', 'bar', null); - $this->assertTrue($result); + $this->assertTrue($repo->put('foo', 'bar', null)); } public function testPutManyWorksForNullTTL() { $repo = $this->getRepository(); $repo->getStore()->shouldReceive('putMany')->once()->with(['foo' => 'bar', 'bar' => 'baz'], null)->andReturn(true); - $result = $repo->putMany(['foo' => 'bar', 'bar' => 'baz'], null); - $this->assertTrue($result); + $this->assertTrue($repo->putMany(['foo' => 'bar', 'bar' => 'baz'], null)); } public function testAddWorksForNullTTL() @@ -178,8 +176,7 @@ public function testAddWorksForNullTTL() $store = m::mock(RedisStore::class); $store->shouldReceive('add')->once()->with('foo', 'bar', null)->andReturn(true); $repository = new Repository($store); - $result = $repository->add('foo', 'bar', null); - $this->assertTrue($result); + $this->assertTrue($repository->add('foo', 'bar', null)); } public function testCacheAddCallsRedisStoreAdd()