[Cache] Added RedisAdapter #17441
[Cache] Added RedisAdapter #17441
Conversation
|
Have 0 idea why it failed on HHVM... Maybe someone could inspect with HHVM knowledge? |
| */ | ||
| protected function doClear() | ||
| { | ||
| return $this->redis->flushDB(); |
There was a problem hiding this comment.
Should this consider the namespace? I mean like this it would flush all keys.
There was a problem hiding this comment.
Redis does not support delete with prefix or etc... I could do get keys and when do foreach but you know...
P.S. APCU does also clear all cache...
|
I would have said that php-cache/integration-tests@e851dc1 should have been the required change to for HHVM to call the destructor. @nicolas-grekas Do you have any idea? |
|
@xabbuh no, and in fact I skipped this test on HHVM... |
|
Sorry, do you mean no as "no idea" or as "no that commit should not fix it"? |
|
As "no idea" :-) |
|
Maybe someone could help as I don't have HHVM on any machine to try debug it... 😞 |
| * @param string $namespace | ||
| * @param int $defaultLifetime | ||
| */ | ||
| public function __construct($redisConnection, $namespace = '', $defaultLifetime = 0) |
There was a problem hiding this comment.
Why not typehinting the $redisConnection argument with \Redis type?
There was a problem hiding this comment.
Did you read my pull request description?
There was a problem hiding this comment.
Oops, sorry my bad! I didn't read the description...
There was a problem hiding this comment.
phpredis, PRedis, ReactPHP Redis they all use same function names but there is no general Interface for them all that's why I left it open... I know it's bad but what I could I do... Maybe create some kind of wrapper which adds interface I don't know 😞
There was a problem hiding this comment.
The type of this argument in the constructor docblock must reflect that you can pass different objects (by either listing all of them or by documenting what a passed object must be able to do).
There was a problem hiding this comment.
Sorry, but I do not want to include all 3rd party libraries just to show possible types, but documentation is a better thing I think. Or I will be creating a library which will provide unified interface and wrapper which would wrap those objects and provide interface
There was a problem hiding this comment.
Well you can just set the expected type to object and then add a sentence describing the requirement for an object being passed here.
There was a problem hiding this comment.
@GCDS I think you should include all the possible types in the docblock if you intend to support all of them. PHPDoc is the right place to do it imo. Furthermore all the possible implementations should be tested. Otherwise we cannot really say we support them.
|
I'd like to know the exact list of targeted Redis client that should work with this adapter. |
|
Just tried predis on test case the interface is ok but returns and logic does not work the same as phpredis... If the choose only to support phpredis we can't hardcode \Redis as type because then we will be unable to pass FakeRedis to constructor. |
|
Working with a PHPUnit Mock instead of the FakeRedis should work though? |
|
@dmaicher I guess yeah, but the thing that class does not exists, and to mock fully working stuff its bit hardcore |
|
@GCDS What about copy/pasting this: https://github.com/ukko/phpredis-phpdoc? |
|
I don't think that tests run against the IMO we need an integration test for each of supported implementations. An integration test that would use the real redis client, not a fake/stub/mock. |
|
@jakzal We had this discussion before and I totally agree. We must have integration tests with Redis here as we have some for LDAP for instance. |
|
@GCDS redis.so is available by default on travis so we can easily use it in the test suite. Are you still ok to work on this PR? Do you need help? |
|
@GCDS Do you have time to finish this PR by adding some tests? |
|
I will finish. If I understand correctly I just need update tests to use real Redis? |
|
That's correct, tests must use a real Redis. |
|
Ok, I will come back home and finish the PR 👍 |
|
I changed to use real Redis Instance also enabled Redis inside Travis config file. could you check if everything is ok? @fabpot |
|
@GCDS Can you rebase on current master and fix the tests. Since you began the PR, the cache component evolved and it seems that your code is not accurate anymore. Thanks. |
This PR was merged into the 3.1-dev branch. Discussion ---------- [Cache] Redis adapter | Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17441 | License | MIT | Doc PR | - Commits ------- 6b7a1fc [Cache] Finish Redis adapter 4893cbc Added RedisAdapter
I have added RedisAdapter. I haven't hardcoded that it would use phpredis extension version client because there is many other clients which provide same API so to support them I left open. For test purpose I wrote a FakeRedis client implementation which acts like real just stores everything in array.
If we choose to use extension version I will remove FakeRedis and add variable requirement for
\Redistype. But we will need redis instance on tests... Or somehow find way to mock it (We will need extend all cache/integration-test tests because of mocking)