PSR-16 Simple Cache Interface implementation

[Sam-Burns]

Adds standards compliance for PSR-16: Simple Cache Interface.

[kynx]

It might be worth checking out https://github.com/php-cache/integration-tests - their goal is ensuring interop between implementations and they've just added a bunch of PSR-16 tests.

[Ocramius]

Swap the conditional to make the successful operation be the last one

[Ocramius]

Some yodaism needed, IMO

[Ocramius]

This comment is not needed

[Ocramius]

{@inheritDoc}, not just @inheritdoc (also check casing)

[Ocramius]

Comment is not needed

[Ocramius]

Default value is not contemplated by your implementation - needs to be addressed for compliance (test also needed)

[Ocramius]

TTL is not contemplated by your implementation - needs to be addressed for compliance. The correct way of doing that is to set the TTL in the options, then restore it to original value after the operation was performed.

[Ocramius]

TTL is not contemplated by your implementation - needs to be addressed for compliance. The correct way of doing that is to set the TTL in the options, then restore it to original value after the operation was performed.

[Sam-Burns]

@Ocramius do let me know if the TTL implementation was what you had in mind. Thanks for the advice.

[kynx]

Maybe I missed something, but it doesn't look like you are handling the different capabilities of the underlying adapters - for instance, some can't store objects, some store integers as string.

This was the biggest pita when I did the PSR-6 adapter last year. The simplest approach would be to serialize() everything, but of course I didn't do that ;) If not you'll need to figure out what data types you can save natively, or start dropping support for some adapters. See the src/Psr stuff on develop for examples of that.

Finally, it doesn't look like the new PSR has much to say about errors thrown by the underlying storage. PSR-6 was explicit that they shouldn't stop the calling code carrying on. That was one of the few things that really made sense to me ;) Has there been any discussion of that? What are other implementers doing?

[marc-mabe]

Hi @Sam-Burns,

thanks for your work on this but for me it doesn't look correct to just add all PSR-16 methods directly into all adapters (implementing into AbsrtactAdapter).

Lines line the following are no a good interface.

if ($ttl && ! $this instanceof TtlUsedAtWriteTimeInterface) {
    throw new BadMethodCallException('TTL not supported by cache adapter');
}

It would be better to ship wrapper for PSR-16 like it was done for PSR-6.

Also as @kynx already pointed out it's not the correct way adding an interface to mark TTL support as this interface doesn't define anything and you can (theoretically) add this feature as a storage plugin.
look here: https://github.com/zendframework/zend-cache/tree/develop/src/Psr

Something like the following should be a better approach:

namespace Zend\Cache\Psr;
use Psr\SimpleCache\CacheInterface as PsrCacheInterface;
use Psr\SimpleCache\CacheException as PsrCacheException;
use Psr\SimpleCache\InvalidArgumentException as PsrInvalidArgumentException;
use Zend\Cache\Storage\StorageInterface as ZendStorageInterface;
use Zend\Exception\InvalidArgumentException as ZendInvalidArgumentException;
use Exception;
use RuntimeException;
use InvalidArgumentException;

class SimpleCacheWrapper implements PsrCacheInterface
{
    private $storage;
    public function __construct(ZendStorageInterface $storage)
    {
        // TODO: test if the storage is suitable for matching PsrCacheInterface
        $this->storage = $storage;
    }

    public function get($key, $default = null)
    {
        try {
            $rs = $this->adapter->getItem($key, $success);
            if (!$success) {
                $rs = $default;
            }
            return $rs;
        } catch (Exception $e) {
            throw static::translateException($e);
        }
    }

    // ...

    private static function translateException(Exception $e)
    {
        $psrExceptionClass = SimpleCacheException::class;
        if ($e instanceof ZendInvalidArgumentException) {
            $psrExceptionClass = PsrInvalidArgumentException::class;
        }
        return new $psrExceptionClass($e->getMessage(), $e->getCode(), $e);
    }
}

class SimpleCacheException extends RuntimeException implements PsrCacheException
{
}

class SimpleCacheInvalidArgumentException extends InvalidArgumentException implements PsrInvalidArgumentException
{
}

Cheers

[marc-mabe]

@Sam-Burns: Btw. this PR should target the develop branch

[marc-mabe]

I found the following definitions in PSR-16 spec:

http://www.php-fig.org/psr/psr-16/#cache

If the underlying implementation does not support TTL, the user-specified TTL MUST be silently ignored.

This doesn't make sense to me but it is in the spec and would mean we can just ignore the TTL in the storage doesn't support TTL on writing.

http://www.php-fig.org/psr/psr-16/#cache

All data passed into the Implementing Library MUST be returned exactly as passed. That includes the variable type. That is, it is an error to return (string) 5 if (int) 5 was the value saved. Implementing Libraries MAY use PHP’s serialize()/unserialize() functions internally but are not required to do so. Compatibility with them is simply used as a baseline for acceptable object values.

This on the other hand needs to be handled by checking supportedDatatypes capabilities.

http://www.php-fig.org/psr/psr-16/#cacheexception

Interface used for all types of exceptions thrown by the implementing library.

For me this means the storage is allowed to throw exceptions (even on get()) - we just need to translate our own exceptions to implement this interface.

[Ocramius]

This doesn't make sense to me but it is in the spec and would mean we can just ignore the TTL in the storage doesn't support TTL on writing.

Yeah, but if the options object supports TTL, then we should set the TTL in the options anyway. Huge WTF for the spec ignoring this stuff silently.

[Ocramius]

This smells a lot... What does it say? Is it just a marker? What's the intended usage?

[Sam-Burns]

You suggested set the TTL in the options, then restore it to original value after the [write] operation was performed. I was trying to do that, but it will only work with some adapters - those that use the setting at write time, e.g. memcached. It won't work with those that use the TTL setting at read time, e.g. file cache. I'll see if I can come up with another approach for you, but any further suggestions would be very welcome.

[Ocramius]

@Sam-Burns one possible alternative is to keep TtlUsedInterface, and make it something like:

interface TtlUsedInterface
{
    public function withTtl(int $ttl, callable $callback);
}

Where the return value would be the return value of the executed callback, and an example implementation as following:

public function withTtl(int $ttl, callable $callback)
{
    $originalTtl = $this->options->getTtl();

    try {
        $this->options->setTtl($ttl);
        return $callback();
    } finally {
        $this->options->setTtl($originalTtl);
    }
}

[Ocramius]

As mentioned by @marc-mabe, this is to be handled silently.

[Ocramius]

Not really a good test solution IMO :|

[Ocramius]

Not really a good test solution IMO :|

[boesing]

Recently realized that there are issues with storage systems which does not provide auto expire ability.

We realized that the filesystem storage is aware of the ttl option BUT the ClearExpiredByFactor plugin which will ensure that expired files will be deleted, will kill files if any ttl is defined within the storage options.

Example:

$cache = Zend\Cache\StorageFactory::factory(array(
    'adapter' => array(
        'name'    => 'filesystem',
        'options' => [ 'ttl' => 1, 'cache_dir' => '/tmp' ]
    ),
));

$cache->set('foo', 'bar', 3);

for ($i=1; $i <= 4; $i++) {
     sleep(1);
    echo "Second $i: " . ($cache->getItem('foo') ?: 'not-found') . PHP_EOL;
}

This will output:

Second 1: not-found
Second 2: not-found
Second 3: not-found
Second 4: not-found

If the configuration does not set any TTL, the value would be interpreted as 0 which wont expire any file and the record remains forever, even if you set the TTL with 3.

[marc-mabe]

@boesing PSR-16 explicitly described to ignore the specified TTL if the underlying storage doesn't support it. This doesn't make sense to me but it's described that way.

For the filesystem adapter the issue is that it will handle TTL in reading the item based on current time, last modification time and defined TTL. That means the TTL defined on "set()" will be ignored and on reading it will use the TTL defined in options. So all your items gets expired after 1 second.

[boesing]

@marc-mabe I'm aware of that. However, there could be an implementation for the Filesystem storage which can ensure that expired files are not returned if stored with PSR-16.
Like for the tags, there could be an additional file where the storage will store the used TTL which is being verified.

I think there will be bug reports for the filesystem storage if someone stores data with PSR-16 interface. I know, the answer will be "PSR-16 describes that behavior" but I think there could be a solution for some adapters.
Just my 2 cents.

[marc-mabe]

@boesing I don't like the idea to special case the PSR-16 methods on the filesystem adapter. It would be weird if setItem/getItem are having different behavior then set/get.

In my opinion we should investigate to implement a plugin to add "TTL on write" support in general. This would be reusable by all storages not supporting TTL.

Then in PSR-16 throwing an exception on TTL usage is not supported no matter what the spec describes.

PS: The current TTL handling of the filesystem adapter helps to reduce disc access a lot in some cases. I would not like to rewrite it to something slower ;)

[Sam-Burns]

@marc-mabe I kind of agree with you. What you are describing is basically the intention behind the commit 0e4c9a4 .

Please note that the file caching doesn't seem to be the only one where there are problems. There are other cache adapters where the TTL is used at read time, and it doesn't matter what the setting was at write time.

I wanted to use an interface to label 'adapter where TTL is used on write'. For our purpose, that is very different from 'adapter where TTL is used on read' or 'adapter where TTL isn't implemented'. This approach was the subject of some criticism in this PR.

I'm more than happy to make any changes you can suggest to this PR for your project - I'm just not sure what I should do. I guess a few of the options are as follows:

• Use inheritance: with two abstract child classes of AbstractAdapter, which adapters can then inherit from, called AdapterWithTtlSupport and AdapterWithNoTtlSupport.
• Use an interface label: empty interface used as a label, and the AbstractAdapter has to have if ($this instanceof ThingWhereTtlIsImplementedInAWayWhichIsHelpfulRightNow) in it
• Use a protected method: adapters have to have protected function doesTtlWorkInThisImplementationOrShallWeJustSilentlyIgnoreIt() : bool in them.
• Use traits: adapters have a trait called Psr16WhereTtlIsSupported and Psr16WhereTtlIsntGoingToWorkSorry

On your point about throwing an exception: the PR was doing that a while ago, instead of silently ignoring the TTL if it's not going work. I can put it back if that is what you want.

If you, @Ocramius or anyone else would like to tell me if any of those approaches are something you would like to see in this PR, I'm very happy to take your guidance on how to implement this. Please let me know.

[boesing]

I've added some logic a few days ago but I thought that it's not useful since it was just a case I implemented for the use case we are using in our business.

I've created some interfaces about a year ago (because of lack in zend-cache):

interface TtlPerValueCapableInterface extends StorageInterface
{

    /**
     * Sets the given value with a specific ttl
     *
     * @param string       $key
     * @param mixed        $value
     * @param null|integer $timeout
     *
     * @return boolean
     *
     * @triggers setItem.pre(PreEvent)
     * @triggers setItem.post(PostEvent)
     * @triggers setItem.exception(ExceptionEvent)
     */
    public function setItemWithTtl($key, $value, $timeout = null);
}

As you can see, this is kinda PSR-16 behavior. As I implemented that, I did not even realized that there is any PSR like that.
However, I've extended some Adapters like Redis, Filesystem and Memcached.
(NOTE: Redis and Memcached support auto expire)

We did not used Filesystem cache that often but as we started to use it, we realized that there is no auto expire at all. After that, I dig into the Adapter and realized: of course, how could the adapter know what TTL was used as the item was stored.

Therefore, I've implemented a new interface for our module today:

interface WithoutTtlAutoExpiring extends TtlPerValueCapableInterface
{

    /**
     * @param string $key
     *
     * @return boolean
     */
    public function isExpiredByTtl($key);
}

I've implemented that to the Filesystem adapter with the same logic as tags are being stored:

    /**
     * @inheritdoc
     */
    public function setItemWithTtl($key, $value, $timeout = null)
    {
        // handleTtlPerValueCall is a trait which does the same thing this PR does in `set` method
        $stored = $this->handleTtlPerValueCall($this, $key, $value, $timeout);

        if ($stored === false || $timeout === null) {
            return $stored;
        }

        $this->normalizeKey($key);
        $ttlFile = $this->getTtlFileName($key);

        // delete related ttl file (if present)
        $this->unlink($ttlFile);

        $wouldblock = null;
        $this->putFileContent($ttlFile, $timeout, true, $wouldblock);
        if ($wouldblock) {
            $this->putFileContent($ttlFile, $timeout);
        }

        return true;
    }

    /**
     * @inheritdoc
     */
    public function isExpiredByTtl($key)
    {
        $this->normalizeKey($key);

        if (!$this->internalHasItem($key)) {
            return false;
        }

        $options = $this->getOptions();
        $ttlFile = $this->getTtlFileName($key);

        if (!file_exists($ttlFile)) {
            if ($options->getTtl() === 0) {
                return false;
            }

            return true;
        }

        $ttl = $this->getFileContent($ttlFile);

        $currentTtl = $options->getTtl();
        $options->setTtl($ttl);

        // Let base logic execute the filemtime stuff, e.g.
        $expired = !$this->internalHasItem($key);
        $options->setTtl($currentTtl);

        return $expired;
    }

    /**
     * Creates the filename for the ttl information
     *
     * @param string $normalizedKey
     *
     * @return string
     */
    private function getTtlFileName(&$normalizedKey)
    {
        return $this->getFileSpec($normalizedKey) . '.ttl';
    }

This actually works as expected, since I've added a new Plugin I've created:

class DynamicTtlCacheClear extends AbstractPlugin
{

    /**
     * @inheritdoc
     */
    public function attach(EventManagerInterface $events, $priority = 1)
    {
        $this->listeners[] = $events->attach('hasItem.pre', [$this, 'hasItem'], $priority);
        $this->listeners[] = $events->attach('hasItems.pre', [$this, 'hasItems'], $priority);
        $this->listeners[] = $events->attach('getItem.pre', [$this, 'getItem'], $priority);
        $this->listeners[] = $events->attach('getItems.pre', [$this, 'getItems'], $priority);
    }

    /**
     * @param Event $event
     *
     * @return boolean
     */
    public function hasItem(Event $event)
    {
        $storage = $event->getStorage();
        if (!$storage instanceof WithoutTtlAutoExpiring) {
            return true;
        }

        $expired = $storage->isExpiredByTtl($event->getParam('key'));

        // If key is expired, stop propagation and return false
        if ($expired === true) {
            $event->stopPropagation();

            return false;
        }

        return true;
    }

    /**
     * @param Event $event
     *
     * @return void
     */
    public function hasItems(Event $event)
    {
        // Trigger getItems so we can trigger item deletion if the key is expired
        $this->getItems($event);
    }

    /**
     * @param Event $event
     *
     * @return void
     */
    public function getItem(Event $event)
    {
        $storage = $event->getStorage();
        if (!$storage instanceof WithoutTtlAutoExpiring) {
            return;
        }

        $key = $event->getParam('key');
        if (!$storage->isExpiredByTtl($key)) {
            return;
        }

        $storage->removeItem($key);

        return;
    }

    /**
     * @param Event $event
     *
     * @return void
     */
    public function getItems(Event $event)
    {
        $keys = $event->getParam('keys');
        $storage = $event->getStorage();

        foreach ($keys as $key) {
            $pre = new Event('getItem.pre', $storage, new \ArrayObject([
                'key' => $key,
            ]));
            $this->getItem($pre);
        }
    }
}

Not quite sure if this is the best way of how to do this, but actually this fixed our problem with the Filesystem adapter which does not support auto expire.
Probably this could be useful for this PR, any ideas welcome aswell.

[marc-mabe]

@Sam-Burns
That's not was I mean with

Then in PSR-16 throwing an exception on TTL usage is not supported no matter what the spec describes.

What I mean is using the wrapper approach (as described in #126 (comment)) and throw an exception on constructor if the given storage doesn't support TTL on write.

Also adding an interface for TTL support is a no go if we want to implement this feature configurable by plugin for all storage adapters because a plugin can't add an interface.
-> What we have is $storage->getCapabilities()->getMinTtl() and $storage->getCapabilities()->getStaticTtl():

• A min. TTL of 0 means the item never expires as TTL is not supported
• Static TTL defined if the time of expiration is fixed as it's stored on write.

@boesing
The plugin should not need any new methods on storage adapter level!
-> My basic idea would be to modify the item on write by adding the time of expiration to it. Then compare time of expiration on read and restore the original item before return.
-> Similar what the serializer plugin does.

[boesing]

@marc-mabe Well, I dont like the idea that the plugin has to read tons of bytes which are probably not used because of TTL expiration.
Assume there are megabytes of data stored in a cache which should use the plugin you intend. Just to see if these megabytes are fresh, all of those megabytes need to get loaded into memory just to determine if its still fresh. I actually don't like that idea, even for performance purposes.

I still don't get where the difference is between tags support and TTL support with the Filesystem adapter. Tags aren't supported natively aswell, but there is a logic implemented to store those tags into a separate file. So why not storing TTL information in a different file aswell?

[marc-mabe]

@boesing

Well, I dont like the idea that the plugin has to read tons of bytes which are probably not used because of TTL expiration.

If you analyze normal cache use-cases you will notice that a cache should have at least a hit rate of 80%. That means you will only read unused cached data for max. 20% but you will not make additional IO for reading TTL in 80%.
Additionally most people will store less data as 1MB in a cache item and in PHP it should be muss less as it's used for HTTP most of the times. And for small amount of data it should be faster even if you don't think about filesystem block size etc.

Of course both ways would be possible, storing expiration time in data file or as an own file. Without real world benchmaks nobody can tell ;)

But even on going the way to store as separate file you shouldn't add weird public exposed methods to the filesystem adapter for making a pligin work. It should all be done within the plugin.

[marc-mabe]

@boesing
Btw. Tagging is a totally different feature used for different purposes.
For example you can add a tag for each user on different items and then clean all items of a specific user.

[weierophinney]

I really like the idea of this patch, and fully support implementing PSR-16. However, I also agree with @marc-mabe that we should do so via a decorator class instead, as he detailed in a previous comment. That approach is simpler to test, and more in line with how we have supported standards in our existing packages in the past (PSR-6 support in this package, PSR-3 support in zend-log, etc.).

(The approach also allows users to continue using zend-cache as-is, and then decorate it to use as a PSR-16 dependency for 3rd party code.)

[weierophinney]

Closing in favor of #152, which provides an approach consistent with existing PSR-6 support.