Page MenuHomePhabricator

Make all SessionManager tests pass with PHPSessionHandler disabled
Closed, ResolvedPublic

Description

  • Set $wgPHPSessionHandling = 'disable' in your LocalSettings
  • Find any tests for SessionManager or other session-adjacent features that fail under that configuration
    • If the test primarily tests the PHPSessionHandler class, make it pass using $this->overrideConfigValue( ... )
    • If the test is more generic, find a way to make it pass regardless of the config, or split it into two tests (one with $wgPHPSessionHandling set to 'disable', another with it set to 'enable')

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
matmarex updated the task description. (Show Details)
matmarex added a subscriber: Hokwelum.

So I tried doing this locally by adding $wgPHPSessionHandling = 'disable'; to my localsettings and running: ./vendor/bin/phpunit tests/phpunit/includes/session/.

it returned:

OK (138 tests, 1280 assertions)

Yeah, my bad. I actually saw the error in warn mode, not disable mode:

1) MediaWiki\Tests\Session\SessionBackendTest::testResetIdOfGlobalSession
Use of $_SESSION was deprecated in MediaWiki 1.27. [Called from session_write_close in (internal function)]

/vagrant/mediawiki/includes/debug/MWDebug.php:386
/vagrant/mediawiki/includes/debug/MWDebug.php:357
/vagrant/mediawiki/includes/debug/MWDebug.php:238
/vagrant/mediawiki/includes/GlobalFunctions.php:787
/vagrant/mediawiki/includes/session/PHPSessionHandler.php:331
/vagrant/mediawiki/includes/session/SessionBackend.php:271
/vagrant/mediawiki/tests/phpunit/includes/session/SessionBackendTest.php:1014

Apparently the test overrides the enable flag (so it doesn't break when you disable PHP session handling) but doesn't override the warn flag.

But just to be sure, you could check the normal CI pipeline: create a patch where you change the default value of $wgPHPSessionHandling to disable, label it as a test patch (we usually prefix the commit message with something like [DNM] or [POC]), create an empty CentralAuth patch that depends on it (I think CentralAuth is the only Wikimedia extension that heavily interacts with session handling), upload them to Gerrit and see if they pass CI.

Change #1180221 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] [DNM] set $wgPHPSessionHandling to disable

https://gerrit.wikimedia.org/r/1180221

Change #1180225 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/extensions/CentralAuth@master] [DNM] test disabled $wgPHPSessionHandling

https://gerrit.wikimedia.org/r/1180225

From the test patch:

There were 2 failures:

1) MediaWiki\Tests\Api\ApiLoginTest::testDeprecatedUserLogin with data set "Bot passwords enabled" (true)
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     'result' => 'Success'
-    'lguserid' => 1
-    'lgusername' => 'TestUser 31a86d.27e'
+    'lguserid' => 2
+    'lgusername' => 'UTSysop'
 )

/workspace/src/tests/phpunit/includes/api/ApiLoginTest.php:110


2) MediaWiki\Tests\Api\ApiLoginTest::testDeprecatedUserLogin with data set "Bot passwords disabled" (false)
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     'result' => 'Success'
-    'lguserid' => 1
-    'lgusername' => 'TestUser 31a86d.280'
+    'lguserid' => 2
+    'lgusername' => 'UTSysop'
 )

/workspace/src/tests/phpunit/includes/api/ApiLoginTest.php:110

That's a scary error. I guess an existing session is somehow being reused in ApiTestCase::doApiRequest()?

@matmarex, were you able to reproduce the failed test locally?

To replicate the errors, I had to do $this->overrideConfigValue( CAMainConfigNames::CentralAuthEnableSul3, false ); in APiLoginTest.

Yes, I can reproduce it locally. I was testing with CentralAuth not being installed, since it interferes with many of the core auth/session tests (since it intentionally changes the behavior that they are testing).

Change #1182208 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] Fix failing test when PHPSessionHandler is set to disable

https://gerrit.wikimedia.org/r/1182208

Change #1182208 merged by jenkins-bot:

[mediawiki/core@master] Fix failing test when PHPSessionHandler is set to disable

https://gerrit.wikimedia.org/r/1182208

For the record, I manually tested the same workflow as in the failing test with $wgPHPSessionHandling = 'disable' (logging in with the legacy action=login API while logged in as another user) and it works correctly.

I did some debugging and it looks like ApiLogin reads out the user data from a different Session object than the one which AuthManager writes to. I haven't dug deep enough to figure out why this happens (probably some incomplete mocking/resetting somewhere), or why enabling PHPSessionHandler makes it work.

Change #1180225 abandoned by Hokwelum:

[mediawiki/extensions/CentralAuth@master] [DNM] test disabled $wgPHPSessionHandling

Reason:

Not needed!

https://gerrit.wikimedia.org/r/1180225