Added PSR-6 adapter

[kynx]

This PR address #46, adding a PSR-6 wrapper.

Some key points:

• Not all storage adapters are supported, due the PSR-6 requirement that individual cache items must support separate TTLs, set on save. This rules out Dba, Filesystem, Memory, Session and Redis < v2
• PSR-6 Error Handling requires that most exceptions thrown by the storage adapter be suppressed. I've provided an ExceptionLogger so these can be saved to a PSR-3 compatible logger. Some documentation is probably in order!
• There are some useful integration tests at https://github.com/php-cache/integration-tests. I haven't got around to incorporating those yet - only discovered them yesterday.

Happy New Year!

[Nyholm]

The implementation looks good. There were some small bugs thou.. Say you do like this:

$item = $pool->getItem('key');
$item->set('foo');
$pool->saveDeferred($item);

$item = $pool->getItem('key');
$item->set('bar');
// No save 

$item = $pool->getItem('key');
$item->get(); // should be 'foo'

I added the integration tests with this PR: https://github.com/kynx/zend-cache/pull/1

[Nyholm]

You need to check if isHit before you return anything

[kynx]

The value is set to null in the constructor if the item isn't a hit.

[marc-mabe]

Hi @kynx thanks for your work!

I have added some notes from a quick look.
What I don't understand is why did you addad the PSR-3 ExceptionLogger? In my opinion this has nothing to do with the PSR-6 implementation and we already have a storage adapter plugin you know where you can handle all thrown exceptions as you like.

[kynx]

@marc-mabe Many thanks for the feedback.

The ExceptionLogger is there for a couple of reasons:

1. The spec says: "An Implementing Library SHOULD log such errors or otherwise report them to an administrator as appropriate."
2. There's a chance a user will attach an ExceptionHandler plugin that has 'throw_exceptions' => false. That would screw up the boolean return values of most methods that talk to storage if it failed. So the ExceptionLogger will barf if it finds another ExceptionHandler plugin attached.

I didn't want to completely rule out users handling exceptions however they wanted, but I did imagine that any Service class would add the logger by default - even if it defaulted to a NullLogger. But I'll ditch it if you want.

Let me know if there's more to do with validateKey() and the clear() stuff.

[kynx]

@Nyholm Many thanks for the feedback and integration tests. I've mocked out the adapter in those now and made sure they all pass 😃

[Nyholm]

@Nyholm Many thanks for the feedback and integration tests. I've mocked out the adapter in those now and made sure they all pass 😃

You are welcome.

I see that your mock preforms better than an actual adapter. When I test this with the redis adapter I fail a few tests. The redis adapter does not preserve datatypes. Consider this test:

$item = $this->cache->getItem('key');
$item->set(5); // Integer 5
$this->cache->save($item);

$item = $this->cache->getItem('key');
$this->assertTrue(5 === $item->get(), 'Wrong data type. If we store an int we must get an int back.');

Also, this just crossed my mind. This is not a part of PSR-6 as far as I know. But what if you do like this:

$item = $this->cache->getItem('key');
$item->set('foo');
$item->expiresAfter(3600);
$this->cache->save($item);

// 10 minutes later in an other request
$item = $this->cache->getItem('key');
$item->set('bar');
$this->cache->save($item);

I'm not too sure how the storage options work. For how long will the item be saved? 3000 more seconds? 3600 seconds? Or infinity?

[kynx]

Yeah, I noticed that fail with Redis too. Do you happen to know if this is a general issue with how Redis serializes data, or if it's specific to Zend Cache?

OT, but it would be useful to provide a mechanism in the integration tests for marking certain tests skipped - maybe be just an associative array of methodName => reason that gets checked at the start of each test.

Regarding re-saving an item after TTL has been set: the default TTL configured for the storage would be used in this case. I don't think any implementation could do otherwise reliably - another process may have cleared the cache, it might be unavailable, etc.

[marc-mabe]

The ExceptionLogger is there for a couple of reasons:

The spec says: "An Implementing Library SHOULD log such errors or otherwise report them to an administrator as appropriate."
There's a chance a user will attach an ExceptionHandler plugin that has 'throw_exceptions' => false. That would screw up the boolean return values of most methods that talk to storage if it failed. So the ExceptionLogger will barf if it finds another ExceptionHandler plugin attached.

The spec makes a recommendation to log errors or report them but nobody as the calling library can actually handle errors. Also I don't see a difference where your ExceptionLogger makes it in any case simpler to log errors. zend-cache already makes it very simple to handle errors.

Yeah, I noticed that fail with Redis too. Do you happen to know if this is a general issue with how Redis serializes data, or if it's specific to Zend Cache?

Redis has no serialize feature, it simply gets requires a string and PHP converts any given value into a string at this point.

From spec

Implementing libraries MUST support all serializable PHP data types, including:

    Strings - Character strings of arbitrary size in any PHP-compatible encoding.
    Integers - All integers of any size supported by PHP, up to 64-bit signed.
    Floats - All signed floating point values.
    Boolean - True and False.
    Null - The actual null value.
    Arrays - Indexed, associative and multidimensional arrays of arbitrary depth.
    Object - Any object that supports lossless serialization and deserialization such that $o == unserialize(serialize($o)). Objects MAY leverage PHP's Serializable interface, __sleep() or __wakeup() magic methods, or similar language functionality if appropriate.

That means you have to make sure the adapter (or the serializer plugin) serializes data (reported by getCapabilities()->getSupportedDataTypes()) and if not you need to un/serialize the data by hand to match the spec.

[kynx]

I've dropped ExceptionLogger.

My holidays are over. I no longer have the time to look into the hairy details of Redis serialisation. PR is welcome.

Thanks.

Edit maybe that's a little short. If the Redis adapter doesn't handle serialising/unserialising data types correctly, isn't that a problem for the Redis adapter? I don't really see why my PRS-6 thingy should be compensating for the underlying adapter getting it wrong.

[Nyholm]

If the Redis adapter doesn't handle serialising/unserialising data types correctly, isn't that a problem for the Redis adapter? I don't really see why my PRS-6 thingy should be compensating for the underlying adapter getting it wrong.

Yes, that is possible. It depends how you want to architecture your Pool, Clients and adapters. If you decide to push that problem to the adapters I would recommend doing integration tests with all adapters instead of a mock, just to make sure the adapters work as well.

[cryptiklemur]

null is a valid value for PSR-6

[kynx]

Yes, which is why you need to check isHit() - afaik the value should be null if its not a hit.

[marc-mabe]

@kynx the redis adapter doesn't handle it wrong. It does what it suppose to do - sending data to redis in a format that redis supports and that's a string only. If you want more you have to add the serializer plugin. That's because not every one need this support and do serializing even if not required would be an useless overhead. That's how and why zend-cache is designed. So if the underlying adapter doesn't support all requirements from psr-6 this wrapper need to implement it on top.

[kynx]

@marc-mebe yes I had another l

[kynx]

Darn phone! Yes, I had another look at this and am completely wrong :(

I'm currently putting together integration tests for each adapter. Once that's done I can start working around the differences.

[Nyholm]

About skipping tests: php-cache/integration-tests#16

[marc-mabe]

Does apc.use_request_time = 1 break PSR-6 specification? So so an exception should be thrown on instantiating in this case :(

[kynx]

Oh lordy, yes it does. Should've paid more attention to that when I was cut-and-pasting the tests. I've added a check to the constructor and a test.

[kynx]

Looks like CI is going to fail until #64 is in.

[samsonasik]

setUp() function in tests should use protected modifier in favor of php-cache/integration-tests#31

[marc-mabe]

You should avoid calling getTtl if the item is already marked as isHit === false

[kynx]

A little bit of Reflection was required to test unserialization, which feels like a hack, but there we go - prophesy doesn't let me do arg-by-ref.

If a value can't be unserialized it will simply show up as a cache miss. I can't see a straightforward way of reporting the problem to the user that doesn't break PSR-6. But - unless something else is writing to the same storage adapter - I can't see it being more than a sporadic error. Let me know if you've got other ideas.

@samsonasik - thanks for the heads up. Saw your php-cache/integration-tests#31 - fixed here now too.

[kynx]

If anyone's interested in Symfony's take on this, check out symfony/symfony#17408

[samsonasik]

zendframework/zend-eventmanager ^2.6 || ^3.0 ? /cc @weierophinney

[marc-mabe]

@samsonasik That's not part of this PR - see #64

[kynx]

I've rebased against develop and the tests are passing :)

[marc-mabe]

I think this is confusing because:

• The filesystem and memory adapter supports TTL but it's based on a stored last-modification-time + runtime TTL setting
  • I'll open an issue for the Memory adapter to change this because this should be a simple change and could also improve performance but it's a BC break.
  • For the filesystem adapter this would be worst because it would be required to read the file before to know it's expired.
• The XCache adapter is not the only adapter calculating TTLs based on request time. APC/APCu also have this behavior if apc.use_request_time which is enabled be default.

EDIT: Uuups you mentioned the APC behavior already in the next paragraph

[marc-mabe]

@kynx #80 has been merged into develop now. Please can you update your PR and not allow adapters capability lockOnExpire > 0 thanks!

[samsonasik]

@kynx it needs rebase.

[kynx]

@samsonasik Yes, apologies, I've been pretty busy recently. I'll look at doing that this weekend.

[marc-mabe]

@kynx I did the following changes and merged it now:

• Disallow storages with capability "lock-on-expire" as this violates PSR-6
• calculate TTL from DateInterval with just one system call for the current time
• skip PSR integration tests on ExtensionNotLoadedException
• added PSR integration test for APCu adapter

Thanks for your work !

[kynx]

@marc-mabe Excellent! Thanks for the cleanup, and sorry I faded away. Glad to see this merged!