Raise error on invalid serializer. #431
Conversation
- Raise error on invalid serializer. Co-authored-by: Go Kudo <[email protected]>
thanks @nikic Co-authored-by: Nikita Popov <[email protected]>
| @@ -0,0 +1,11 @@ | |||
| --TEST-- | |||
There was a problem hiding this comment.
This seems to be redundant and the various other tests already assert this notice isn't emitted for valid serializers
| --TEST-- | ||
| Serializer: invalid pattern. | ||
| --SKIPIF-- | ||
| <?php require_once(dirname(__FILE__) . '/skipif.inc'); ?> |
There was a problem hiding this comment.
this should also skip if extension_loaded('igbinary');, since users may have installed and enabled igbinary before running this test
| @@ -1218,6 +1218,9 @@ PHP_APCU_API zend_bool apc_cache_defense(apc_cache_t *cache, zend_string *key, t | |||
| PHP_APCU_API void apc_cache_serializer(apc_cache_t* cache, const char* name) { | |||
| if (cache && !cache->serializer) { | |||
| cache->serializer = apc_find_serializer(name); | |||
| if (strcmp(name, "default") != 0 && !cache->serializer) { | |||
| php_error_docref(NULL, E_WARNING, "apc_cache_serializer: serializer \"%s\" is not supported", name); | |||
There was a problem hiding this comment.
Not a maintainer but I'm somewhat familiar with the codebase
not supported or not enabled, maybe. There's also the question of whether it should fall back to the "php" serializer instead, given that the actual default ini value is "php" (see linked ticket)
There was a problem hiding this comment.
Indeed. The current behavior is complicated and cannot be understood without following the implementation. Also, the situation where the fallback behavior that occurs when it is not present and the flow when php is explicitly specified are separated is probably not desirable.
I have added a comment to your Issue.
APCu works with the default serializer, even if the value set in apc.serializer is invalid.
But, this is unfriendly if you are using a third party serializer such as igbinary. For example, if igbinary is installed before APCu is installed, igbinary will not support APCu. The current behavior of working with the default serializer without warning may produce unintended behavior.
This fix changes it to give an E_WARNING equivalent warning if an unavailable serializer is specified.