Provide a PSR-16 decorator for zend-cache storage adapters #152

weierophinney · 2018-04-18T15:40:14Z

This patch serves as an alternative to #126, based on a comment by @marc-mabe to that patch.

The patch adds a new class, Zend\Cache\Psr\SimpleCacheDecorator, which accepts a zend-cache storage instance to its constructor, and then proxies to it via the methods that implement the PSR-16 CacheInterface. The approach is simpler than that in #126 as it does not require testing against every adapter; we can test the decorator against a mock of the StorageInterface, validating its behavior. The tests provided also ensure that any exceptions thrown by the underlying storage operations are re-cast to PSR-16 implementations.

This approach may miss a few details; each of @marc-mabe, @kynx, and @boesing raised a few issues in #126 that I am uncertain if this approach covers. I'd love feedback before I merge this.

TODO

Validate keys to ensure they do not contain reserved characters
Validate keys to ensure they do not exceed maximum length
Detect whether the zend-cache adapter requires serialization; if so, serialize values before storage, and deserialize on retrieval.
Implement TTL capabilities per PSR-16:
- If a negative or zero TTL is provided, have set() and setMultiple() remove the item(s) from the cache (instead of storing).
- If the adapter does not support TTL semantics, have set() and setMultiple() return boolean false for non-null, >0 TTL values.
Implement integration tests for as many adapters as make sense.

weierophinney · 2018-04-18T15:40:39Z

Pinging @kynx and @boesing for review.

thomasvargiu · 2018-04-18T16:17:30Z

@weierophinney Can we also add this on composer.json?

    "provide": {
        "psr/cache-implementation": "1.0",
        "psr/simple-cache-implementation": "1.0"
    }

weierophinney · 2018-04-18T16:27:49Z

@thomasvargiu Done!

kynx

Only had a quick scan but looks good!

The thing that made the PSR-6 adapter such a pain was handling the different limitations of the underlying storage and the different values they can store.

I’m wondering if it would be better to serialise everything up front to avoid those.

kynx · 2018-04-18T17:52:29Z

src/Psr/SimpleCacheDecorator.php

+    /**
+     * {@inheritDoc}
+     */
+    public function set($key, $value, $ttl = null)


Yeah, same here

Oops darn mobile interface are my comment: the spec forbids some character (like @) in the key. These should be caught early.

Oh, okay - I'll review the spec and add some checks here and in setMultiple() then.

I've added a validation method that does the following:

Checks for any characters reserved by the PSR-16 specification, raising an exception if any are detected.

Checks the key length; anything over 64 characters results in an exception.

Both set() and setMultiple() now call this before doing any other logic.

weierophinney · 2018-04-18T19:38:07Z

The thing that made the PSR-6 adapter such a pain was handling the different limitations of the underlying storage and the different values they can store.

I’m wondering if it would be better to serialise everything up front to avoid those.

I just turned to that section of the specification, and it looks like it may be identical to PSR-6. As such, I'm going to implement the logic found in the PSR-6 adapter with regards to:

determining if serialization is necessary
de/serializing values during set/get(Multiple) operations

The only other consideration is what to do about adapters that do not support per-item TTL, as this is a soft requirement of PSR-16 (see: requirements on set() and setMultiple() with regards to $ttl arguments). If no $ttl argument is sent, then any adapter is valid. However, if one is sent, PSR-16 is clear that the adapter should honor it.

The options I see are:

We raise an exception during instantiation if the adapter cannot support TTL, like we do in the PSR-16 adapter. The downside of this approach is that if the consumer is not using TTL values, then they are excluded from using that adapter for no reason.
We return boolean false from the set() and setItems() operations if a $ttl is provided, but the adapter is not capable of it. A boolean false indicates a failure to cache, which would be valid in cases where we cannot set a TTL.

Considering the idea behind PSR-16 was to make caching simple and to prevent raising exceptions for common operations, I think the second approach likely would be more in the spirit of PSR-16 support, so I'm going to proceed in that direction.

thomasvargiu · 2018-04-18T20:41:44Z

src/Psr/SimpleCacheDecorator.php

+        $options->setTtl($ttl);
+
+        try {
+            $result = $this->storage->setItem($key, $value);


If a negative or zero TTL is provided, the item MUST be deleted from the cache if it exists, as it is expired already.

Should it be checked?

Good catch; I'll address this when working on the TTL items I listed above.

boesing

I guess you wanted to add a %s inside the braces with an implode of self::INVALID_KEY_CHARS?

boesing · 2018-04-18T17:40:45Z

src/Psr/SimpleCacheDecorator.php

+            throw static::translateException($e);
+        }
+
+        $options->setTtl($previousTtl);


Would it be better if we put this into a finally block to ensure that even after catching an exception, the ttl is being restored?

boesing · 2018-04-18T21:15:49Z

src/Psr/SimpleCacheDecorator.php

+        $regex = sprintf('/[%s]/', preg_quote(self::INVALID_KEY_CHARS, '/'));
+        if (preg_match($regex, $key)) {
+            throw new SimpleCacheInvalidArgumentException(sprintf(
+                'Invalid key "%s" provided; cannot contain any of ()',


I guess you wanted to add a %s inside the braces with an implode of self::INVALID_KEY_CHARS?

Good catch - thanks!

thomasvargiu · 2018-04-19T13:55:15Z

@weierophinney Did you consider to test it using php-cache/integration-tests like PSR-6 tests?

weierophinney · 2018-04-19T14:02:41Z

Did you consider to test it using php-cache/integration-tests like PSR-6 tests?

I'll look into it today. I'm not sure if this will be terribly feasible, as we'd need an actual adapter (not a mock one) to easily use those. It may make more sense to wait to do those until we split the adapters into their own repos.

weierophinney · 2018-04-19T15:33:09Z

@thomasvargiu —

Regarding this statement:

I'll look into it today. I'm not sure if this will be terribly feasible, as we'd need an actual adapter (not a mock one) to easily use those. It may make more sense to wait to do those until we split the adapters into their own repos.

I've added an integration test for the APC adapter, and discovered a number of things already. One thing interesting: the integration tests are sometimes covering behavior not explicitly detailed in the spec. As examples: they test for generator/iterable arguments to the various *Multiple() methods, which are not explicitly indicated as valid values, but are not excluded, either. Additionally, I'd forgottent that DateInterval values were allowed for TTL values, so I've added functionality for addressing that, too.

As such, I think we definitely need some integration tests in play, as they're helping me refine the adapter. I'll get these in place today.

thomasvargiu · 2018-04-19T15:50:10Z

@weierophinney good, and I agree with you. I think should be better to have an integration test for every adapter like in PSR-6 tests. Then we can decide what to do when some adapter fail.
I don't know what should be better to do when someone sets a TTL on some adapter that doesn't support it.

weierophinney · 2018-04-19T17:09:11Z

Initial tests are now in place for APC and APCu adapters; APCu was running fine locally at this point, which is promising!

I also found a bug in the integration tests and reported it: php-cache/integration-tests#92

I'll hammer through the tests for the remaining adapters this afternoon; they should be fairly straight-forward, as I can use the setup logic from the PSR-6 integration tests.

@marc-mabe

This patch builds on both the approach made in zendframework#126 as well as a comment to that patch made by @marc-mabe (zendframework#126 (comment)) in order to provide a decorator for zend-cache storage adapters that fulfills the PSR-16 `SimpleCacheInterface`. Usage is as follows: ```php // $adapter is an instance of StorageInterface $cache = new SimpleCacheDecorator($adapter); ``` From there, you can then use `$cache` anywhere you would normally use a PSR-16 instance.

All completed specs are now published on the website.

PSR-16 reserves the keys `{}()@/\\`, and does not allow their usage in cache keys.

Per PSR-16, key lengths up to 64 characters are supported, but no more than that.

Extracts it to a trait so that it can be re-used with the SimpleCacheDecorator. Defines two methods: - `memoizeSerializationCapabilities()` detects serialization capabilities, and sets the properties `$serializeValues` and `$serializedFalse` if serialization is required. - the abstract function `unserialize()`, as both adapters will require this functionality, but how it is implemented will differ as they have different needs. The `CacheItemPoolAdapter` constructor was altered to call `memoizeSerializationCapabilities()` instead of the original `shouldSerialize()` (which has been removed).

weierophinney · 2018-04-19T20:26:59Z

Looks like the bulk of integration test issues are when using setMultiple() with adapters that require serialization; essentially every single one that requires serialization had unserialize() errors, which makes me think that the logic for setMultiple() may need tweaking.

None of the setMultiple* tests appear to work; they each produce `unserialize()` errors.

If the adapter requires serialization, we need to call `set()` for each ktem in the `$values` array. I'm still unclear why this is the case, but every single adapter that requires serialization was failing in the exact same way, with an `unserialize()` error when retrieving a value set via a `setMultiple()` operation. Making this change allows them to pass.

weierophinney · 2018-04-19T21:19:15Z

So, this is interesting: evidently the logic in individual adapter setItems() implementations is all quite similar, and, in the case of adapters where I needed to serialize data, it was either serializing it on its own, or otherwise altering the data in ways that setItem() does not.

The workaround I found was to detect if the adapter requires serialization, and, if so, have it call set() once per item in the $values array. This approach immediately solved problems with all adapters that displayed this issue.

This ensures that even after an exception, the adapter options are reset; particularly important when in async environments!

Also edits and re-organizes all entries for 2.8.0 to have slighly more semantic sense.

weierophinney · 2018-04-19T21:37:47Z

Okay, all feedback incorporated, and no changes at this time that affect tests, which were green on last run!

Going to merge to develop, and will release 2.8.0 next Monday. Please test, and provide feedback if you run into issues!

Close #152

marc-mabe · 2018-04-20T07:40:59Z

Hi @weierophinney,

Thanks for starting working at it!

there is a serializer plugin available that can be plugged in into any storage adapters. Maybe it’s better to throw an error if the provided storage adapter doesn’t support all required data types and let the user plugin the serializer plugin into it to make it work with the psr wrapper. I think it was done that way for psr6, too
also now there is one namespace for both psr wrappers. I think it could make sense to plit it into psr6 and psr16 namespaces
on clearing items there is also a ‘clearByNamespaceInterface’ that will remove all items of the current namespace of the storage. As the simple cache wrapper doesn’t has any knowledge about namespaces it only sees the items of the current namespace. I didn’t check the spec fully but I guess this should be preferred on clear as flush removes everything even of other namespaces
on delete there is a concurrent regression. You first check for existence and then delete but the item could be added between both calls. As removeItem return true/false based of if the item was deleted it will throw an error only in case the deletion of an existing item failed. So probably just ignoring the return value and return true in all cases

weierophinney · 2018-04-20T19:39:02Z

@marc-mabe Thanks for the feedback!

I've opened #155 to address your second point; however, the approach I took was to use Zend\Cache\Psr\CacheItemPool and Zend\Cache\Psr\SimpleCache to reflect the interfaces being implemented, versus the specification numbers.

I'll start addressing the other three points in the next few days. I'm going to open tickets for each so that others can more easily find the information.

weierophinney mentioned this pull request Apr 18, 2018

PSR-16 Simple Cache Interface implementation #126

Closed

weierophinney force-pushed the feature/psr-16-support branch from 2bffdda to e0bf3eb Compare April 18, 2018 15:50

weierophinney force-pushed the feature/psr-16-support branch from e2d3875 to 936568b Compare April 18, 2018 17:42

kynx reviewed Apr 18, 2018

View reviewed changes

weierophinney force-pushed the feature/psr-16-support branch 2 times, most recently from d4253b0 to 7a2a9bb Compare April 18, 2018 19:03

thomasvargiu reviewed Apr 18, 2018

View reviewed changes

weierophinney added enhancement WIP labels Apr 18, 2018

boesing reviewed Apr 18, 2018

View reviewed changes

weierophinney force-pushed the feature/psr-16-support branch from 32ff796 to d22f9e0 Compare April 19, 2018 15:11

weierophinney force-pushed the feature/psr-16-support branch from d22f9e0 to e09b369 Compare April 19, 2018 17:07

weierophinney force-pushed the feature/psr-16-support branch from e09b369 to 688a7a2 Compare April 19, 2018 17:58

weierophinney added 7 commits April 19, 2018 14:18

Udpates link to PSR-6 in documentation

aa52ee7

All completed specs are now published on the website.

Adds documentation covering PSR-16 support

9d3f986

Mark package as both a PSR-6 and PSR-16 provider

f0ba8db

Do not allow setting keys that are reserved in the PSR-16 specification

7c9f644

PSR-16 reserves the keys `{}()@/\\`, and does not allow their usage in cache keys.

Do not allow keys longer than 64 characters

5e10ef6

Per PSR-16, key lengths up to 64 characters are supported, but no more than that.

weierophinney force-pushed the feature/psr-16-support branch from 688a7a2 to 7d9031c Compare April 19, 2018 19:49

weierophinney added 10 commits April 19, 2018 14:57

Adds PSR-16 integration test for Memory adapter

0a33249

Creates PSR-16 integration test for Memcache adapter

44d5808

Adds PSR-16 integration test for Memcached adapter

26557cd

Adds PSR-16 integration test for MongoDb adapter

67c2d4b

Adds PSR-6 integration tests for ExtMongoDb adapter

7a2bcee

Adds PSR-16 integration test for Redis adapter

dad5806

Adds PSR-16 integration test for WinCache adapter

fd46ff9

Adds PSR-16 integration test for ZendServerDisk adapter

ce2c5f1

Adds PSR-16 integration test for ZendServerShm adapter

faaea7d

Adds PSR-16 integration test for XCache adapter

5ddc3f5

weierophinney force-pushed the feature/psr-16-support branch 2 times, most recently from e4a5945 to 2d4a69c Compare April 19, 2018 20:05

weierophinney added 2 commits April 19, 2018 15:29

Adds PSR-16 integration test for zend-session adapter

ce5843a

Adds PSR-16 integration test for Filesystem adapter

8c39032

None of the setMultiple* tests appear to work; they each produce `unserialize()` errors.

weierophinney force-pushed the feature/psr-16-support branch from 2d4a69c to 8c39032 Compare April 19, 2018 20:29

weierophinney added 2 commits April 19, 2018 16:35

Adds finally block to set, setMultiple to reset options

4044702

This ensures that even after an exception, the adapter options are reset; particularly important when in async environments!

Adds CHANGELOG entry for zendframework#152

a56b35f

Also edits and re-organizes all entries for 2.8.0 to have slighly more semantic sense.

weierophinney merged commit a56b35f into zendframework:develop Apr 19, 2018

weierophinney added a commit that referenced this pull request Apr 19, 2018

Merge branch 'feature/psr-16-support' into develop

5a79687

Close #152

weierophinney deleted the feature/psr-16-support branch April 19, 2018 21:41

This was referenced Apr 20, 2018

PSR-16 support: serialization using plugin #156

Closed

PSR-16 support: clear by namespace when appropriate #157

Closed

PSR-16 support: prevent concurrent regression when calling delete #158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a PSR-16 decorator for zend-cache storage adapters #152

Provide a PSR-16 decorator for zend-cache storage adapters #152

weierophinney commented Apr 18, 2018 •

edited

Loading

weierophinney commented Apr 18, 2018

thomasvargiu commented Apr 18, 2018

weierophinney commented Apr 18, 2018

kynx left a comment

kynx Apr 18, 2018

kynx Apr 18, 2018

weierophinney Apr 18, 2018

weierophinney Apr 18, 2018

weierophinney commented Apr 18, 2018

thomasvargiu Apr 18, 2018

weierophinney Apr 18, 2018

boesing left a comment

boesing Apr 18, 2018

weierophinney Apr 19, 2018

boesing Apr 18, 2018

weierophinney Apr 18, 2018

thomasvargiu commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

thomasvargiu commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

marc-mabe commented Apr 20, 2018

weierophinney commented Apr 20, 2018

Provide a PSR-16 decorator for zend-cache storage adapters #152

Provide a PSR-16 decorator for zend-cache storage adapters #152

Conversation

weierophinney commented Apr 18, 2018 • edited Loading

TODO

weierophinney commented Apr 18, 2018

thomasvargiu commented Apr 18, 2018

weierophinney commented Apr 18, 2018

kynx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasvargiu commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

thomasvargiu commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

weierophinney commented Apr 19, 2018

marc-mabe commented Apr 20, 2018

weierophinney commented Apr 20, 2018

weierophinney commented Apr 18, 2018 •

edited

Loading