Disable Redis serialisation

[s7anley]

Hi,

By default \Redis will serialise initial value as i:1; and increment will silently fail afterwards.

I think it would be a good improvement if documentation mention that Redis serialisation has to be disabled otherwise it will not work e.g. $redis->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_NONE); and maybe raise an exception in case of increment failed.

I can help you with the change, just wanted to confirm that I'm not missing something.

[nikolaposa]

I would be surprised if Redis extension does not take care of that on its own under the hood. I would expect that incr() always behave as it should regardless of the serialization. Do you know whether that option is enabled by default? Official docs is quite deficient.

[s7anley]

When I was testing it locally (PHP 7.0.32 and redis ext 3.1.4), a default was SERIALIZER_PHP and thus the rate limiting was not working at all.

I will upgrade extension and try it out again.

[s7anley]

I just tested it with extension redis-4.2.0 for redis:3.0.6-alpine and redis:4.0.11-alpine it is not working too.

[s7anley]

https://github.com/phpredis/phpredis/issues?utf8=%E2%9C%93&q=is%3Aissue+SERIALIZER_PHP

[nikolaposa]

Ok, that confirms it. Can you create a PR that changes README with a suggestsion that serialization should be disabled?

[nikolaposa]

... or perhaps we should enforce it here: https://github.com/nikolaposa/rate-limit/blob/master/src/RedisRateLimiter.php#L29, via setOption().

[s7anley]

It's not safe to change the configuration on the passed instance, because other parts of the application could rely on serialisation.

I will prepare MR for README and think a little bit how to check it on the fly.

[nikolaposa]

Yeah, I thought about that afterwards, too, but perhaps something like checking whether flag is on + throw meaningful exception would is the most appropriate approach in my opinion.

[s7anley]

Isn't that BC if we throw an exception now?

[s7anley]

Anyway, the option on \Redis client could be changed on runtime, so there is case when this check will not prevent wrong behaviour anyway.

[nikolaposa]

What do you think would be the most sensible solution to this?