Set ttl to specific cache key for Redis Storage Adapter

[pensiero]

• Implemented the internalTouchItem method inside Storage\Adapter\Redis that allow to touch a Redis item.
• Implemented the _ttl metadata, that return the remaining tll of a specific key

Thanks to @marc-mabe to help me understanding what to do!

Refer to #5, I have created a new PR to clean my code from other local branches that was creating troubles.

[marc-mabe]

Be be compatible with other adapters getMetadata should return false in the case the item doesn't exist.
-> from StorageInterface::getMetadata() : @return array|bool Metadata on success, false on failure where a failure means the request can't be made but it's not an error. An error would result in an exception.

[pensiero]

I'm returning false if the the item is not found (https://github.com/zendframework/zend-cache/pull/10/files#diff-8799c8aa6a52387ce614380b5db83f76R491)

Did you mean that I have to update the docblock?

[marc-mabe]

Only the one comment above.

Please let run unit tests for this adapter and check code coverage for your new code. I know the version specific lines will only be tested if you run the specific version but one of this version should be well tested and all the other new code around.

thanks!

[pensiero]

@marc-mabe I can't do also the test for specific versions.. if someone can do that, ok

[marc-mabe]

@pensiero It's very ok if you test only your version I think

[pensiero]

@marc-mabe then yes! it's ready 👍

[marc-mabe]

@pensiero but some tests failed - see continuous-integration/travis-ci/pr

[pensiero]

@marc-mabe it's strange, because in my environment the test pass... and I'm using php 5.6, that fails in Travis. Could it be for some configs? I've enabled TESTS_ZEND_CACHE_REDIS_ENABLED and started the redis server...anything else?

[Maks3w]

Could you rebase against current master? Current master have many improvements for integration tests

[pensiero]

@Maks3w done, but however test fails

[marc-mabe]

@pensiero If I look @ https://coveralls.io/builds/2883342/source?filename=src%2FStorage%2FAdapter%2FRedis.php I see the tests ran with a redis version >= 2.0 and < 2.6 with the logic:

// ...

// redis >= 2
// The command 'pttl' is not supported but 'ttl'
// The command 'ttl' returns 0 if the item does not exist same as if the item is going to be expired
// NOTE: In case of ttl=0 we return false because the item is going to be expired in a very near future
//       and then doesn't exist any more
} elseif (version_compare($redisVersion, '2', '>=')) {
    $ttl = $redis->ttl($this->namespacePrefix . $normalizedKey);
    if ($ttl <= 0) {
        return false;
    }
    $metadata['ttl'] = $ttl;
// ...

One of the tests failing looks like this:

public function testGetMetadata()
{
    $capabilities = $this->_storage->getCapabilities();
    $supportedMetadatas = $capabilities->getSupportedMetadata();
    $this->assertTrue($this->_storage->setItem('key', 'value'));
    $metadata = $this->_storage->getMetadata('key');
    $this->assertInternalType('array', $metadata); // <- failes
    // ...
}

As of the test doesn't set a specific TTL it will be the default one for infinite lifetime (TTL=0). I could think of that redis could return ttl=0 in this case, too. So a returned ttl=0 could would mean one of the following:

• item exists with infinite TTL
• item exists but item will expire in the very near future (no second based TTL left)
• item doesn't exist

If this is the case we have to check for $this->internalHasItem($normalizedKey) as well:

    if ($ttl <= 0 && !$this->internalHasItem($normalizedKey)) {
        return false;
    }

[marc-mabe]

PS: you did not rebased against master. You have merged the master into your branch.
Please use git fetch {remote} && git rebase {remote}/master ;)

[pensiero]

Ok we are making progress. Now I'm returning null if the item has no associated expire (whatever the version is) and false if the items doesn't exist.

However testGetMetadata in CommonAdapterTest is expecting an array, even if this is empty. @marc-mabe do you think I can replace the null return with an empty array?

[marc-mabe]

@pensiero From interface:

    /**
     * Get metadata of an item.
     *
     * @param  string $key
     * @return array|bool Metadata on success, false on failure
     * @throws \Zend\Cache\Exception\ExceptionInterface
     */
    public function getMetadata($key);

[pensiero]

@marc-mabe if as you said the tests are running with redis version >= 2 and < 2.6, how can this return a $metadata['ttl'] of -1 (instead of expected 0), basing on this case? thanks

[marc-mabe]

@pensiero In this case you should return an array with ttl set to NULL instead of returning null

[pensiero]

@marc-mabe it's what I did: https://github.com/pensiero/zend-cache/blob/feature/set-ttl-redis-adapter/src/Storage/Adapter/Redis.php#L531
but it seems that it's not the problem (or maybe I did not understand what you said)

[marc-mabe]

                    // ...
                    $metadata['ttl'] = null;
                }
                $metadata['ttl'] = $ttl;
                // ...

It's not working this way as the raw TTL returned by redis will overwrite the ttl 2 lines later ;)

[pensiero]

WTF! 😬 let's see if now they pass...

[pensiero]

It's seems ok, what do you think @marc-mabe ?

[marc-mabe]

Looks better now but there is the same issue with redis version >= 2.6 - see https://github.com/zendframework/zend-cache/pull/10/files#diff-8799c8aa6a52387ce614380b5db83f76R516

And please check your coding style - see https://travis-ci.org/zendframework/zend-cache/jobs/68730134

Again - please make a rebase with current master and combine all your commits into a single one
(https://help.github.com/articles/about-git-rebase/)

git fetch <upstream>
git rebase -i {upstream}/master

[pensiero]

So nice rebasing! First time used! Thanks a lot @marc-mabe (never end to learning, I'm very happy of this).

[marc-mabe]

I think it looks good now 👍
@Maks3w ping

[pensiero]

great! thanks a lot marc!

[pensiero]

Is there anything else I can do?