-
Notifications
You must be signed in to change notification settings - Fork 53
Fixes cache entry deletion directly after creation #184
Conversation
See #183 Only set TTL if there is a value which is not null!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change requires unit tests, please, to demonstrate that no TTL is present in the item if none was provided.
We should use TTL from options when it is not provided on set/setMultiple methods of SimpleCacheDecorator.
@weierophinney I've added tests to cover changes and prove the issue. Instead of mocking storage and options, I've implemented storage and logged TTL when setting the item in cache. Then in assertion I am checking what value of TTL was set for the item. |
Actually using mocking it will be also quite simply: public function testUseTtlFromOptionsOnSetMocking()
{
$this->options->getTtl()->willReturn(40);
$this->options->setTtl(40)->will([$this->options, 'reveal']);
$this->options->setTtl(null)->shouldNotBeCalled();
$this->storage->getOptions()->will([$this->options, 'reveal']);
$this->storage->setItem('foo', 'bar')->willReturn(true);
self::assertTrue($this->cache->set('foo', 'bar'));
}
public function testUseTtlFromOptionsOnSetMultipleMocking()
{
$this->options->getTtl()->willReturn(40);
$this->options->setTtl(40)->will([$this->options, 'reveal']);
$this->options->setTtl(null)->shouldNotBeCalled();
$this->storage->getOptions()->will([$this->options, 'reveal']);
$this->storage->setItems(['foo' => 'bar', 'boo' => 'baz'])->willReturn([]);
self::assertTrue($this->cache->setMultiple(['foo' => 'bar', 'boo' => 'baz']));
} But not sure if it is clear enough? Line $this->options->setTtl(null)->shouldNotBeCalled(); in both tests is not really needed. It says explicitly that we do not allow that cal, but as we are mocking options anyway, we are saying what calls are allowed (and all others are not). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are easily understandable, and the solution looks reasonable. 🚢
Fixes cache entry deletion directly after creation
Thanks, @alexz707! |
See
#183
Only set TTL if there is a value which is not null!