Add Serialization option #411
Conversation
Serialization option is necessary for setting the type of serialization to use for redis. This is simply a new commit with all the code taken from PR#199 using the latest code of SncRedisBundle on branch 2.1
|
To save us some reviewing effort (see #297 (comment) as well), could you explain briefly the improvements over #297 and why you based on #199 instead? |
| @@ -114,6 +114,7 @@ private function addClientsSection(ArrayNodeDefinition $rootNode) | |||
| ->scalarNode('read_write_timeout')->defaultNull()->end() | |||
| ->booleanNode('iterable_multibulk')->defaultFalse()->end() | |||
| ->booleanNode('throw_errors')->defaultTrue()->end() | |||
| ->scalarNode('serialization')->defaultValue("default")->end() | |||
There was a problem hiding this comment.
String constants should be in single quotes unless using interpolation.
| "none" => 0, // \Redis::SERIALIZER_NONE, | ||
| "php" => 1, // \Redis::SERIALIZER_PHP, | ||
| "igbinary" => 2 // \Redis::SERIALIZER_IGBINARY | ||
| ); |
There was a problem hiding this comment.
Given the dependency on these constants, why can it even be invoked if phpredis is not loaded?
Also single quotes ;)
There was a problem hiding this comment.
Do you mean to say that we should use this instead?
$types = array(
'none' => 0, \Redis::SERIALIZER_NONE,
'php' => 1, \Redis::SERIALIZER_PHP,
'igbinary' => 2 \Redis::SERIALIZER_IGBINARY
);
There was a problem hiding this comment.
The comment says we're using the integers instead of the constants as they would fail if phpredis is not loaded, but in that case the serialization wouldn't work anyway, so how come we have to account for it?
|
|
||
| // allow user to pass in default serialization in which case we should automatically decide for them | ||
| if ('default' == $type) { | ||
| if (defined('HHVM_VERSION')) { |
There was a problem hiding this comment.
We dropped HHVM support so can drop this even if basing on 2.1 as far as I'm concerned.
There was a problem hiding this comment.
understood, I can remove hvvm part
| $serializationType = $extension->loadSerializationType($options['serialization']); | ||
| $this->assertTrue(is_integer($serializationType)); | ||
|
|
||
| if (defined('HHVM_VERSION')) { |
There was a problem hiding this comment.
Same here - it's a new feature so don't need to support HHVM for BC.
|
Let me close #297, the reason is that it was from an older version, and since then I made some more changes that shouldn't be in the PR. Regarding the other changes you commented I will update now. Regarding #199, it's minor:
|
|
@curry684 I made all the changes, the only thing left is your comment regarding "Given the dependency on these constants, why can it even be invoked if phpredis is not loaded?". I was unsure what you meant. |
|
I have updated the code to use Redis class constants directly as per your request. |
|
Tests failing on: It's apparently optional: phpredis/phpredis#588 Can you fix it so the code is compatible with IGBINARY support missing as it apparently is on Travis? |
|
Perhaps that was the reason why the hardcoded integer was used instead of the constant, but I think I can fix it. |
|
There are cleaner ways than hardcoding external integers out of our control ;) |
|
Merged, thanks! |
* Add Serialization option Serialization option is necessary for setting the type of serialization to use for redis. This is simply a new commit with all the code taken from PR#199 using the latest code of SncRedisBundle on branch 2.1 * Minor code style update. Remove HHVM support * Switch to using Redis Class Constants instead of hard-coded values * Fix IGBINARY Dependency * Remove ?? to support php5 (cherry picked from commit 5fca27f)
Serialization option is necessary for setting the type of serialization
to use for redis.
This is simply a new commit with all the code taken from PR#199 using
the latest code of SncRedisBundle on branch 2.1