#156 PSR-6 / PSR-16: use serialization with storage plugin #161
Conversation
thomasvargiu
force-pushed
the
feature/156
branch
from
4e3ce04 to
92c810d
Compare
|
|
||
| foreach ($requiredTypes as $type) { | ||
| // 'object' => 'object' is OK | ||
| // 'integer' => 'string' is not (redis) | ||
| // 'integer' => 'integer' is not (memcache) | ||
| if (! (isset($types[$type]) && in_array($types[$type], [true, 'array', 'object'], true))) { | ||
| $shouldSerialize = true; | ||
| break; | ||
| return true; |
There was a problem hiding this comment.
For anybody stumbling across this patch and wondering why it works...
If an adapter does not support one of the required types, getCapabilities() will return a boolean false for that data type. However, if the Serializer plugin is attached to the adapter, it overrides getCapabilities() and returns a list of supported data types that indicates true for each type. As such, we only need to test that the adapter supports serialization at instantiation of the decorator.
| @@ -304,6 +290,13 @@ public function commit() | |||
| */ | |||
| private function validateStorage(StorageInterface $storage) | |||
| { | |||
| if ($this->isSerializationRequired($storage)) { | |||
| throw new CacheException(sprintf( | |||
| 'Storage %s requires a serializer plugin', | |||
There was a problem hiding this comment.
We should expand on this to indicate what the user will need to do to make the storage adapter work; I'd include a link to https://docs.zendframework.com/zend-cache/storage/plugin/#quick-start, as the very first example specifically demonstrates adding a serialization plugin.
| $this->memoizeSerializationCapabilities($storage); | ||
| if ($this->isSerializationRequired($storage)) { | ||
| throw new SimpleCacheException(sprintf( | ||
| 'Storage %s requires a serializer plugin', |
There was a problem hiding this comment.
Same comment here regarding exception message.
| @@ -25,6 +26,39 @@ class CacheItemPoolDecoratorTest extends TestCase | |||
| public function testStorageNotFlushableThrowsException() | |||
| { | |||
| $storage = $this->prophesize(StorageInterface::class); | |||
|
|
|||
| $capabilities = new Capabilities($storage->reveal(), new \stdClass(), $this->defaultCapabilities); | |||
| @@ -19,6 +20,7 @@ class FilesystemIntegrationTest extends TestCase | |||
| public function testAdapterNotSupported() | |||
| { | |||
| $storage = StorageFactory::adapterFactory('filesystem'); | |||
| $storage->addPlugin(new Serializer()); | |||
There was a problem hiding this comment.
With this change, will the filesystem adapter work as a PSR-6 adapter? If so, we need to have it extend the PSR-6 integration test case.
There was a problem hiding this comment.
I don't think it's supported, it was just to skip that check. Probably it's better to move the required serialization check after all others checks in the validateStorage() method.
| @@ -129,6 +130,27 @@ public function invalidatingTtls() | |||
| ]; | |||
| } | |||
|
|
|||
| /** | |||
| * @expectedException \Zend\Cache\Psr\SimpleCache\SimpleCacheException | |||
There was a problem hiding this comment.
Please use expectException() calls directly before the statements that will raise them. This ensures that an unexpected exception of the same type is not caught, causing a false positive.
…SR-6 and PSR-16 support.
weierophinney
force-pushed
the
feature/156
branch
from
92c810d to
057eed2
Compare
|
@thomasvargiu I've rebased from develop, and made updates based on the feedback I provided. Once Travis is green, I'll merge. Thanks! |
|
Thanks, @thomasvargiu! |
#156 Removed serialization. Throw exception when storage adapter require serialization