-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RateLimiter] Fix delete method of the cache storage #38661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Thank you. This is correct.
Could you add a small unit test for this too?
I'm not sure how to write such a test, I'd rather not try to make something up here. |
Do you mind if I push to your branch? |
Not at all, I'd be happy to take your suggestion on this :-) |
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.
I added a test.
Im happy with this PR.
|
||
public function testDelete() | ||
{ | ||
$this->pool->expects($this->once())->method('deleteItem')->with(sha1('test'))->willReturn(true); |
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 line uses $this->pool
which is a mock created in setUp()
.
We say that we "expect" that "once" the "method deleteItem
" is called with sha1('test')
. We also return true.
If what we expect does not happen, then the test will fail
{ | ||
$this->pool->expects($this->once())->method('deleteItem')->with(sha1('test'))->willReturn(true); | ||
|
||
$this->storage->delete('test'); |
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 is just calling the storage
Thank you @GregOriol. |
This PR fixes a small issue with RateLimiter's cache storage and the delete method: all getItems are called with a sha1 of the id, but not the one for delete, which makes it miss the deletion.