[18:28:53] dr0ptp4kt: I forgot one thing: if anyone thinks they are not ready for AuthManager (I'm thinking mobile apps, but wouldn't hurt to tell everyone at SoS), they should add a blocker to https://phabricator.wikimedia.org/T135504 ASAP [18:43:57] Still blocked on https://phabricator.wikimedia.org/T135872 with running unit tests. I could work around it by creating the table manually but I can't imagine that's what other people do. [18:44:15] I never enabled CentralAuth, it's presumably required by the other roles, which lots of people use. How is this supposed to work? [18:45:26] If it's supposed to use a central db in vagrant, then we can create that by default with that role? If we prefer using the main wiki database for it, then we can create the table with puppet and run the script there. Either way it should work fine by default. [18:56:55] Krinkle: globaluser is created by the vagrant role; maybe you had an error while provisioning and ended up in an inconsistent state? [18:57:19] um, I mean the centralauth database os created [18:57:40] Interesting. [18:57:45] Was that added recently? [18:58:17] I don't think so [18:58:36] probably the unit tests use a different config [18:59:35] at a wild guess I would just try to set $wgCentralAuthDatabase = 'centralauth' somewhere where tests will pick it up and see if that improves things [19:01:24] tgr: I'm looking into PHPSessionHandlerTest::testSessionHandling() some more and I'm even more baffled than I was before. It looks like deleteObjectsExpiringBefore doesn't actually do anything on any BagOStuff implementations except SqlBagOStuff, which means that the following is true: [19:01:55] tgr: The problem is that MediaWikiTestCase doesn't clone tables in databases other than the main $wgDBname database, so CentralAuth's shared tables don't get cloned and it winds up blowing up unit tests. CA has a hack for its own tests, but if some other test hits one of CA's hooks the hack isn't in place. CA also has another hack for our CI environment. [19:01:56] - Your test doesn't actually confirm that PHPSessionHandler::gc() gets called [19:02:27] - The test would pass even if you took out ini_set( 'session.gc_divisor', 1 ); ini_set( 'session.gc_probability', 1 ); [19:03:40] it's the natural expiration of the items in the bagostuff implementation that is causing it to pass [19:06:34] anomie: yeah I just found the code. I think a hacky but functional way for Krinkle to get the unit tests working would be to just comment out the table-dropping code in CentralAuthTestCaseUsingDatabase::tearDownAfterClass, then run some CA unit test to set up the test tables [19:07:25] that's pretty misleading, because the way the test reads is: configure PHP's behavior such that it will end up calling the session handler's GC function on sesion_start(), and then confirm that the result of the session handler's GC function did the right thing. [19:08:33] ori: HashBagOStuff checks expiration when you try to get the data [19:08:45] yes, that's what i'm saying [19:08:53] that is the reason the test passes [19:09:04] the fact that the gc() function is called does not matter [19:09:24] tgr: then it changes from wiki.unittest_globaluser not existing to Error: 1146 Table 'centralauth.unittest_globaluser' doesn't exist (127.0.0.1) [19:09:27] and the fact that the gc() function calls $this->store->deleteObjectsExpiringBefore() doesn't matter [19:09:34] It still has the prefix in it unfortunately. [19:09:58] Krinkle: yeah, sorry, that was bad advice, see later comment [19:10:08] or were you referring to that? [19:10:47] Not yet :) [19:10:50] I'll try that. [19:11:04] I think the db/tables are fine, the wiki is fine. [19:11:08] (web access) [19:11:12] It's just tests. [19:14:03] you can easily verify this by changing ini_set( 'session.gc_divisor', 1 ) to ini_set( 'session.gc_divisor', 10000000 ), the test will pass [19:16:45] it seems to me that if you wanted to properly test this, you would have a BagOStuff implementation that does *not* check expiry on gets, and that *does* implement a real deleteObjectsExpiringBefore [19:17:30] OK. I think it's because flow -> echo -> centralauth. I'll disable flow and hopefully return to a usable vagrant. [19:17:42] tgr: The test hack didn't work. Errors somewhere later down the line. [19:23:44] ori, uh, you are right [19:36:32] tgr: That tokens patch is the last thing I want to land to master + REL1_27 before I rc.0 [19:37:38] ostriches: give me a minute to test and I'll +2 [19:37:50] Okie dokie thx [20:50:27] anomie: is it intentional that there is no grant for changing user rights? [20:53:00] tgr: I don't recall any reason we specifically didn't create one, more likely it's just we couldn't think of any case where someone would need it in an OAuth client. Why, does JsonConfig or zerowiki need it? [20:53:49] no, just wanted to use it to test T122056 and was surprised it's missing [20:53:50] T122056: Old tokens are remaining valid within a new session - https://phabricator.wikimedia.org/T122056 [21:14:49] anomie: do you think the token reset should be applied to bot logins? [21:15:12] that seems to be the one code path that does not do it but I can't think of any way that could be abused [22:36:26] I'm a little stumped with the traceback on https://phabricator.wikimedia.org/T136650. All the titles created by Skin::preloadExistence() should be legit, and I don't see a way that Title::getDBkey() could return a non-string value? [23:04:15] legoktm: SkinPreloadExistence hook handler modifying &$titles ? [23:04:48] https://github.com/wikimedia/mediawiki-extensions-SandboxLink/blob/398ff8d876b4439ff3edcb4f722c8391faaafa66/SandboxLinkHooks.php#L80 [23:05:07] SandboxLink isn't installed on mw.o, and the usage there looked safe anyways [23:09:00] I'm thinking of adding debug logging to LinkBatch::add()? Because that's the only thing that touches $this->data [23:18:18] sounds sane