kenguest commented 2 years ago

Fix the issue where if CRedisCache's getValue method returns null then CCache get method may pass a null value to unserialize function causing a deprecation message to be raised. See the stack trace below.

This occurs with PHP 8.1.

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Tests pass?	✔️

2022/11/28 16:41:48 [error] [php] unserialize(): Passing null to parameter #1 ($data) of type string is deprecated (/var/www/project/vendor/yiisoft/yii/framework/caching/CCache.php:108) Stack trace:

0 /var/www/project/vendor/yiisoft/yii/framework/caching/CCache.php(108): unserialize()

1 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(3160): CRedisCache->get()

2 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(3151): UrlManager->processRules()

3 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(1104): UrlManager->init()

4 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(1393): CWebApplication->getComponent()

5 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(1716): CWebApplication->getUrlManager()

6 /var/www/project/vendor/yiisoft/yii/framework/yiilite.php(1236): CWebApplication->processRequest()

7 /var/www/project/index.php(42): CWebApplication->run()

what-the-diff[bot] commented 2 years ago

The function getValue() is modified.
If the value returned by redis server is null, it will be converted to false before returning from this method.

rob006 commented 2 years ago

This may change behavior when serializer is disabled in cache component.

Fix the issue where if CRedisCache's getValue method returns null then CCache get method may pass a null value to unserialize function causing a deprecation message to be raised. See the stack trace below.

How to reproduce this bug? If key is set by cache component, it should always be generated using serialize(), so it should never be null.

marcovtwout commented 1 year ago

kenguest commented 1 year ago

@rob006 to reproduce this, create a small yii app that caches values to redis. clear that cache etc.

I don't understand the mechanics of the CRedisCache parseResponse method, but it seems valid for it to return a null value under some circumstances which getValue would pass on to its caller.

As getValue is expected to return either a string or a boolean - not null - adding this simple guard clause will prevent this unexpected/illegal return value from being passed up the stack.

kenguest commented 1 year ago

Related to #4497?

Yes.

This could also be fixed more generally by adding a guard clause to CCache's mget and get methods but fixing it in CRedisCache's getValue method minimizes the amount of code that needs to be changed for when the cache backend is redis.

rob006 commented 1 year ago

Related fixes in Yii 2: https://github.com/yiisoft/yii2-redis/pull/247 https://github.com/yiisoft/yii2/pull/19178 (I'm not sure about this one, as it really should not happen in practice and may hide some errors in code or configuration).