Page MenuHomePhabricator

session_write_close failure in PHPSessionHandlerTest under php8.3
Open, Needs TriagePublic

Description

Running the PHPSessionHandlerTest with php8.3 gives following error:

3) MediaWiki\Session\PHPSessionHandlerTest::testSessionHandling with data set #0 ('php')
session_write_close(): Failed to write session data using user defined save handler. (session.save_path: /var/lib/php/sessions, handler: write)

/workspace/src/tests/phpunit/includes/session/PHPSessionHandlerTest.php:246

4) MediaWiki\Session\PHPSessionHandlerTest::testSessionHandling with data set #1 ('php_binary')
session_write_close(): Failed to write session data using user defined save handler. (session.save_path: /var/lib/php/sessions, handler: write)

/workspace/src/tests/phpunit/includes/session/PHPSessionHandlerTest.php:246

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

That sounds bad. Session data should be written to MediaWiki's session backend, not files - the default PHP session handler should be disabled as soon as Setup.php runs. Maybe it's related to tests being run in CLI mode, but if PHP also fails to set up PHPSessionHandler-based session handling in PHP 8.3 for web requests, that would be rather problematic.

The error comes from \Wikimedia\PhpSessionSerializer::decode - it seems the serialization format may have changed.

I wonder if the PHPSessionHandler infrastructure is still necessary these days—as I understand it, it was meant to be a compatibility layer between the new Session abstraction and extensions still using the PHP global session_* functions. These days, the ecosystem should be basically fully migrated to the former, so perhaps there's no longer a need to maintain a session handler?

It appears to me that the immediate failure here is that the session directory at /var/lib/php/sessions is not writable (ie it doesn't exist and the "nobody" user in CI isn't allowed to create it).

The broader issue is indeed that something is using PHP's default session handler at all, instead of MediaWiki's.

As for why php8.3 fails and the earlier versions not, hard to say. The image provision is in the integration/config.git repository. We generally don't create or change directories in the image, much less vary them between versions.

That leaves either:

  • our own code having a conditional somewhere that goes down this path only under newer PHP versions (while it might sounds unlikely, this is rather easy to do, eg a deprecation warning is likely to be limited to newer versions, which then triggers error handlers and printers, which in turn may produce output or run session-related code).
  • the session directory varying between PHP versions. Our images are public and the "docker run" command from CI will work locally as well. You can run it with "--rm -i -t --entrypoint /bin/bash" to have a look around at the INI files, and/or the relevant directories.

It appears to me that the immediate failure here is that the session directory at /var/lib/php/sessions is not writable (ie it doesn't exist and the "nobody" user in CI isn't allowed to create it).

I thought that to be the issue initially, but that's not the case—setting an override for that in the test still does not fix it on PHP 8.3. After some debugging, it turns out that PHP 8.3 changed the behavior of unserialize such that it now emits a warning for trailing data[1] while it previously simply discarded the leftovers. wikimedia/php-session-serializer relies on the old behavior and so it fails due to the new warning (running its tests locally reproduces the issue too).

We could probably update the error handler there to ignore this particular warning—the function is a mirror of PHP's own internal PS_SERIALIZER_DECODE_FUNC which also does not care about trailing input.


[1] https://wiki.php.net/rfc/unserialize_warn_on_trailing_data

That sounds bad. Session data should be written to MediaWiki's session backend, not files - the default PHP session handler should be disabled as soon as Setup.php runs.

The test is testing PHPSessionHandler's integration with the session_* functions so it calls them explicitly.

I meant session.save_path should never be touched since PHPSessionHandler doesn't use it. But probably this is just a poorly phrased PHP error message that and the actual error has nothing to do with that path.

Filed T362324: Disable PHPSessionHandler in Wikimedia production about getting rid of PHPSessionHandler entirely.