[11.x] Prevent unintended serialization and compression

[JeppeKnockaert]

This pull request fixes an issue when using serialization and compression combined with the increment function, introduced in #54221.

This is something used by the RateLimiter and causes it to fail since the v11.39.0.

This PR is intended to fix #54307.

[TheLevti]

Please wait with this solution. I need to verify with the phpredis maintainer. You could break the cache component once again when serialization/compression is enabled.

[TheLevti]

We can merge this one

[TheLevti]

Is it possible to contribute here or should I open a new pull request on my own?

[taylorotwell]

@TheLevti what needs to be changed?

[TheLevti]

@TheLevti what needs to be changed?

I just confirmed with the maintainers of phpredis, that this behaviour is by design. A value stored with serialization/compression, can not be incremented/decremented.

So for us, inside the rate limit component when we set the initial counter value, we need to skip serialization and store the value as is (when serialization/compression is enabled).

Solution is basically similar to what is done here. Instead of globally skipping the pack call for any Cache component usage, we should do this only when used by the RateLimiter component. (for setting the initial counter value)

[taylorotwell]

@TheLevti is there a big issue with just doing it globally in the cache component?

[TheLevti]

@TheLevti is there a big issue with just doing it globally in the cache component?

Also checked that. Theoretically possible even though it would be not correct. phpredis will return the value as is if the value can not be deserialized/uncompressed.

We can come up with a smarter solution later and use this proposal for now.

[TheLevti]

<?php

$r = new Redis;
$r->connect('localhost', 6379);

shell_exec('redis-cli set foo not-valid-serialization');

$r->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_PHP);

var_dump($r->get('foo'));

❯ php /tmp/fallback.php
string(23) "not-valid-serialization"

So it will work

[TheLevti]

I have used the following test in RedisCacheIntegrationTest to confirm the rate limiter works with this changes:

    /**
     * @param  string  $driver
     */
    #[DataProvider('redisDriverProvider')]
    public function testRedisCacheRateLimiter($driver)
    {
        $store = new RedisStore($this->redis[$driver]);
        $repository = new Repository($store);
        $rateLimiter = new RateLimiter($repository);

        $this->assertFalse($rateLimiter->tooManyAttempts('key', 1));
        $this->assertEquals(1, $rateLimiter->hit('key', 60));
        $this->assertTrue($rateLimiter->tooManyAttempts('key', 1));
    }

[vlakoff]

You could make use of is_finite():

Changing this:

return is_numeric($value) && ! in_array($value, [INF, -INF]) && ! is_nan($value);

To this:

return is_numeric($value) && is_finite($value);

[TheLevti]

You could make use of is_finite():

Changing this:

return is_numeric($value) && ! in_array($value, [INF, -INF]) && ! is_nan($value);

To this:

return is_numeric($value) && is_finite($value);

Ah nice, did not know about this function. Will try to remember on next iteration of improvements on this part ^^