Proposal: Make registering serializers easier for shared session module#7715
Open
TysonAndre wants to merge 1 commit intophp:masterfrom
Open
Proposal: Make registering serializers easier for shared session module#7715TysonAndre wants to merge 1 commit intophp:masterfrom
TysonAndre wants to merge 1 commit intophp:masterfrom
Conversation
TysonAndre
added a commit
to TysonAndre/igbinary
that referenced
this pull request
Dec 4, 2021
…dule **This depends on the proposal for PHP 8.2 (php/php-src#7715) to allow safely registering native session serializers even if the session module isn't statically compiled into php (without crashing if the session module isn't loaded or is loaded in the wrong order, after the extension providing the serializer)
0204f87 to
a6ca581
Compare
TysonAndre
added a commit
to TysonAndre/igbinary
that referenced
this pull request
Dec 4, 2021
…dule **This depends on the proposal for PHP 8.2 (php/php-src#7715) to allow safely registering native session serializers even if the session module isn't statically compiled into php (without crashing if the session module isn't loaded or is loaded in the wrong order, after the extension providing the serializer)
Additionally, this PR passes the PHP reference value `PS(http_session_vars)` as an extra argument to the serializer and unserializer, so that extensions don't need to directly depend on symbols found in the session shared module (`ps_globals.http_session_vars`). (Grepping the tarballs for the top hundreds of PECL extensions by downloads, PECLs such as igbinary, msgpack, and hprose provide custom session serializers that are more space efficient than PHP's built-in serializers for most use cases. I'm a maintainer of igbinary.) Previously, when `--enable-session=shared` is used instead of `--enable-session=static`, external session serializers such as igbinary and msgpack would either 1. load session serializers because it would result in php failing to load all of that serializer's functionality if session wasn't loaded, or if session was going to be loaded after the serializer. This was based on the behavior of the now-removed `wddx` extension for php 2. Crash/fail to load if the shared library is loaded if the session module hadn't been loaded first. - php_session_register_serializer and PS(http_session_vars) would be undefined C symbols and cause the serializer shared library to fail to load. Examples of issues caused: - https://github.com/msgpack/msgpack-php/issues?q=is%3Aissue+session+is%3Aclosed for `undefined symbol` issues (ps_globals, etc) - igbinary/igbinary#116 Because many users of PHP use a package manager instead of building php themselves, and package managers often publish php-session as a shared library that must be installed separately, this means that many users are unable to use custom serialization methods with php's native session handlers if session isn't statically compiled into php. (https://www.php.net/manual/en/session.configuration.php#ini.session.serialize-handler) - e.g. https://pkgs.alpinelinux.org/package/edge/community/x86/php8-session is required by php8-pecl-msgpack because of this. - Some other OSes will statically compile session into php to avoid this (e.g. Windows, seemingly homebrew for Mac, Ubuntu). This PR exposes a new function pointer in the module globals that can be accessed through the session module in `HashTable *module_registry` if `session_module && session_module->module_started`. Existing PECL releases will be unaffected and can continue to use `php_session_register_serializer` without changes. ZTS: `((php_ps_globals*) (*((void ***) tsrm_get_ls_cache()))[TSRM_UNSHUFFLE_RSRC_ID(*session_module->globals_id_ptr)]); NTS: `session_module->globals_ptr`
a6ca581 to
e7fc20b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why: PHP's serializer is a text format, and for many use case it (1) uses more storage space for serialized data (increasing data storage needs for sessions), and (2) is slower than PECLs such as https://github.com/igbinary/igbinary#igbinary or https://github.com/msgpack/msgpack-php/ at serializing/unserializing data for most use cases. Making it easier for end users to install modules and use them as session serializers would help save space and improve performance for users.
Additionally, this PR passes the address of the PHP reference value (IS_REFERENCE)
PS(http_session_vars)as an extra argument to the serializer and unserializer,
so that extensions don't need to directly depend on symbols
found in the session shared module (
ps_globals.http_session_vars).(Grepping the tarballs for the top hundreds of PECL extensions by downloads,
PECLs such as igbinary, msgpack, and hprose provide custom session serializers
that are more space efficient than PHP's built-in serializers for most use cases.
I'm a maintainer of igbinary.)
Previously, when
--enable-session=sharedis used instead of--enable-session=static, external session serializers such as igbinary andmsgpack would either
load session serializers because it would result in php failing to load
all of that serializer's functionality if session wasn't loaded, or if session
was going to be loaded after the serializer.
This was based on the behavior of the now-removed
wddxextension for phpCrash/fail to load if the shared library is loaded if the session module
hadn't been loaded first.
C symbols and cause the serializer shared library to fail to load.
Examples of issues caused:
for
undefined symbolissues (ps_globals, etc)Because many users of PHP use a package manager instead of building php
themselves, and package managers often publish php-session as a shared library that
must be installed separately, this means that many users are unable to use
custom serialization methods with php's native session handlers if session isn't
statically compiled into php.
(https://www.php.net/manual/en/session.configuration.php#ini.session.serialize-handler)
is required by php8-pecl-msgpack because of this.
this (e.g. Windows, seemingly homebrew for Mac, Ubuntu).
This PR exposes a new function pointer in the module globals that can be
accessed through the session module in
HashTable *module_registryifsession_module && session_module->module_started.Existing PECL releases will be unaffected and can continue to use
php_session_register_serializerwithout changes.ZTS:
((php_ps_globals*) (*((void ***) tsrm_get_ls_cache()))[TSRM_UNSHUFFLE_RSRC_ID(*session_module->globals_id_ptr)]); NTS:session_module->globals_ptr`Why not user-defined serializers/unserializers
This PR is deliberately a change with no/minimal impact on existing PECL releases or userland code.
This does not propose adding a user-defined serializer. That was already proposed in https://wiki.php.net/rfc/user_defined_session_serializer and fell short of the 2/3 yes vote requirement.
https://externals.io/message/97279#97304
The other RFC had the following possible objections:
save(string $uuid, array $data)andread(string $uuid): array $dataas an alternative class to SessionHandlerInterface