Skip to content

Conversation

@kenguest
Copy link
Contributor

@kenguest kenguest commented Nov 28, 2022

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()

kenguest and others added 4 commits November 28, 2022 16:51
This will fix deprecation warning of passing null to parameter yiisoft#1 of type string to unserialize function (PHP 8.1)
@what-the-diff
Copy link

what-the-diff bot commented Nov 28, 2022

  • 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
Copy link
Contributor

rob006 commented Nov 28, 2022

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
Copy link
Member

Related to #4497?

@kenguest
Copy link
Contributor Author

kenguest commented Nov 30, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor

rob006 commented Nov 30, 2022

Related fixes in Yii 2:
yiisoft/yii2-redis#247
yiisoft/yii2#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).

@marcovtwout
Copy link
Member

After investigation by @wtommyw we're only applying the fix to getValue() specifically for CRedisCache, as @rob006 noted the other fix might hide errors in other cache implementations.
@kenguest Thanks for contributing!

@marcovtwout marcovtwout merged commit 1c6f6fe into yiisoft:master Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants