[Cache] Fixed incorrect usage of UNLINK with PHPRedis with Redis < 4.0 #39298
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
5bca313 to
7d7fcfc
Compare
|
Thank you very much for working on this issue. Because the 5.2 branch is affected by this bug, please base your PR on the 5.2 branch. |
cbe7247 to
1e7f16f
Compare
There was a problem hiding this comment.
From the Redis documentation this is available since Redis 4.0 https://redis.io/commands/unlink
I believe the fix should also be applied to Predis. Right?
Looks like the Predis\Command\Redis\UNLINK does not exists. /cc @jonashrem
|
Here's the pull request that introduced these changes: #37993. The UNLINK command is a pending pull request to Predis (see predis/predis#666) and not yet available, but might be in the future if/when it's approved. There's a check to see if that class exists or it will fall back to the DELETE-command, so there currently aren't any negative side effects in keeping it as far as I can see. These changes will also need to be applied to the following two files:
I'll implement those corrections soon. |
|
Which version of phpredis are you using? |
There was a problem hiding this comment.
Looking at @jderusse's comment at #39280 (comment), we should handle the case where the call returns false instead.
11d1bb9 to
4907be1
Compare
4907be1 to
9363f3b
Compare
|
Thank you @Wickex. |
Currently, deleting cache keys is broken for users using PhpRedis with Redis-server < 4.0.0. The current implementation expects PhpRedis to throw an exception if the 'unlink'-function is used but unavailable, after which it's supposed to switch to using the 'del'-function. Using the 'unlink'-function on lower Redis-server versions seems to not throw an exception, but instead it appears to silently fail.
This pull request changes this behavior and checks the Redis-server version instead. If the version is 4.0 or higher, it uses the unlink function. If not, it uses the del-function.
Also see https://redis.io/commands/unlink > "Available since 4.0.0".
(Footnote: this is one of my first times contributing to an open-source project and my first time contributing to Symfony. I've tried following the guidelines, but please let me know if I missed anything. I'm unsure how I would go about unit testing this specific bugfix due to it being dependent on the Redis version, so I omitted it. Please let me know if a unit test is indeed required for this and if so, let me know if you have any suggestions on how to go about that.)