Unrecognized serializer name falls back to `"default"` rather than `"php"`

[TysonAndre]

trafficstars

https://github.com/krakjoe/apcu/blob/98b26169d4145fef67df9e54e28f8af0720a5cdf/apc_persist.c#L428-L451

The "default" serializer is the absence of a serializer (affects apcu's behavior for arrays of non-objects) and there's no string entry for "default", it corresponds to a null pointer for apc_serializer_t *serializer (serializer=0x0)

i.e. there's no such thing as APC_SERIALIZER_NAME(default) or APC_UNSERIALIZER_NAME(default)

Observed: When apcu fails to find a serializer, it uses a null pointer (equivalent to apc.serializer=default) without emitting a warning about the serializer name being unrecognized Expected: Consider defaulting to "php" (the actual default) instead if a serializer is unrecognized or not loaded (e.g. igbinary not loaded)

$ gdb -args php -d apc.serializer=notjson -a
(gdb) b apc_persist
Function "apc_persist" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (apc_persist) pending.
(gdb) run
Starting program: /path/to/php-8.2.0RC3-nts-install/bin/php -d apc.serializer=notjson -a
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Interactive shell

php > apcu_store('a',  new stdClass());

Breakpoint 1, apc_persist (sma=0x7ffff3a5c9e0 <apc_sma>, serializer=0x0, orig_entry=orig_entry@entry=0x7fffffffc090)
    at /path/to/apcu/apc_persist.c:429
429                     apc_sma_t *sma, apc_serializer_t *serializer, const apc_cache_entry_t *orig_entry) {

[TysonAndre]

I didn't see https://github.com/krakjoe/apcu/pull/431 , which is related to this issue

[zeriyoshi]

+1

This is a tricky issue and often overlooked, especially when building PHP and extensions from code.

What about changing the default value of ini to default and removing php? This involves a BC Break, but since my warning about specifying an unsupported serializer is a BC Break as well, it seems appropriate to fix it in the next release.

[TysonAndre]

https://pecl.php.net/package-changelog.php?package=APCu&release=5.1.22 - I looked into this more today. I assume that the changes in 5.1.20 likely would have fixed the bugs that were encounted with apc.serializer=default (though I'm still not certain). It looks like the current approach shouldn't have bugs (though one fix for php 8.2 is not yet released).

What about changing the default value of ini to default and removing php? This involves a BC Break, but since my warning about specifying an unsupported serializer is a BC Break as well, it seems appropriate to fix it in the next release.

There are use cases where default would have better performance (but higher memory) compared to php unserialize(), e.g. unserializing values containing many small arrays. There are also use cases where php has better performance, and php has lower memory than default in most cases (default can be better when values in arrays are reused)

https://github.com/krakjoe/apcu/pull/454#issue-1419890666 see comparison of apc.serializer=default and apc.serializer=php for small arrays

[TysonAndre]

https://github.com/krakjoe/apcu/issues/323 - since I mentioned performance, there's still room for improvement for the serializer=default performance with optimizations that can be done but just aren't implemented yet

[zeriyoshi]

Thanks. I had overlooked it!