[10:21:47] !bash "perfection is reached not when there is no more code to add, but when there is no more code to take away" [10:21:47] Lucas_WMDE: Stored quip at https://bash.toolforge.org/quip/8kWuJpsB8tZ8Ohr0K2Vo [12:11:29] Amir1: RE https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1218314 - Just in case... the way to cherry-pick a stack in Gerrit is bottom-up, and then for the 2nd/3rd etc to specify the sha1 of the proposed parent. The cherry-pick dialog has a field to set parent to another cherry-pick instead of origin/wmfXYZ itself. [12:12:31] this works even for unmerged parents, quite nice, but takes a few clicks to do, by copying the sha1 of the patch set of the proposed cherry-pick [12:24:01] JustHannah: I restored the experimental php85 job job for less.php https://gerrit.wikimedia.org/r/1218742 [12:24:09] result at https://gerrit.wikimedia.org/r/1217553 [12:52:20] Krinkle, thank you! [12:52:55] Filed T412804 [12:52:56] T412804: Phan/CodeBase.php:590 [8192] Method SplObjectStorage::attach() is deprecated since 8.5 - https://phabricator.wikimedia.org/T412804 [13:02:08] xSavitar: https://github.com/phan/phan/commit/25d7e8d185a4c78e7423c188fb28bba5dbde20c1 and https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/+/1203134 [13:04:18] Nice, thanks for the pointers [13:16:52] Krinkle, thank you! Seen! [19:16:29] tgr_: where does the account validation logic from the CA side vary by RIGOR_NONE/VALID? From reading https://phabricator.wikimedia.org/T377588#10242800 and https://phabricator.wikimedia.org/T372880 it sounds like there are two different branches. [19:16:57] However, looking at CentralAuthUser::getInstanceByName today this does not appear to be the case. I don't see how these two tasks are about differnet entyrpoints/rigor modes. [19:25:50] it doesn't, CentralAuthUser calls core username validation with RIGOR_VALID [19:27:24] the entry points were different, one was about Special:Contributions which takes the username from the request, the other about userOptions.php iterating through user rows [19:28:34] using RIGOR_NONE seems scary, that means the username isn't a valid title, who knows what assumptions that would violate down the line [19:30:17] tgr_: yeah, I'm not suggesting we change the rigor mode. The explainations on task made me expect that CentralAuthUser::getInstance takes a $rigor and that these two tasks differ in which rigor they use. I don't see that. [19:30:52] I see now that the T372880 failed via the onUserGetRights hook, and the fix was to avoid instantiating CentralAuthUser in the first place. [19:30:53] T372880: CentralAuthUser::getInstanceByName with RIGOR_NONE throws exception when username contains colon (Aug 2024) - https://phabricator.wikimedia.org/T372880 [19:31:10] so what is the relation to rigor? [19:32:00] I suppose you mean that due to userOPtions using RIGOR_NONE it is able to create a User object, and we're failing here on hooks/methods via the User class, as opposed to plainly from an untrusted string [19:33:00] although both tasks actually involve Special:Contributions as the web request [19:34:08] the fixed task was SpecialContributions/User::isHidden/User::isAllowed/…/CentralAuthHooks->onUserGetRights/CentralAuthUser [19:34:16] I think I just meant it would be nice to not have to validate the username in CentralAuthUser (except on creation) - if it exists in the DB, accept it, if not, the object won't really do anything [19:34:44] but it would probably break something somewhere expecting $cu->getName() to be a valid username [19:34:54] something outside CA I mean [19:35:18] the new task is SpecialContributions/User::isHidden/User::getBlock/extensions-GlobalBlocking/CentralAuthIdLookup/CentralAuthUser::getInstanceByName(string) [19:35:29] there are lots of entry points, so it's a bit of a whackamole [19:37:21] throwing an exception when the user exists but has an invalid name seems fine though, those cases do need manual cleanup [19:42:44] the design in core seems to be to allow login and general account usage and limit this pattern only to creation. following that in CA as well would be consistnet. [19:43:41] It's hard when the interface is a string, but I suppose we could start by skipping validation when the argument is a registered UserIdentity (has user ID). [20:00:57] yeah maybe we should bite the bullet and do it [20:01:24] I imagine it would cause some errors in components that use CentralAuth usernames, though [20:02:05] basically anything that takes a value from CentralAuthUser::getName() and then turns it into a user or title would have to adapt [20:07:33] I suppose that's the same as User->getName already [20:08:01] And CentralAuthUser::getName today, if created via CentralAuthUser::newFromId or newFromRow [20:08:09] * Krinkle tests [20:09:07] I'm fairly sure newFromId() today just turns the ID into a username and then follows the username code path [20:16:33] $ mwscript eval.php svwiki [20:16:33] $u = User::newFromId(331568); [20:16:33] > echo $u->getName(); [20:16:33] Nr: 135 [20:16:33] > echo $u->getId(); [20:16:33] 331568 [20:16:33] > return $u->getRegistration(); [20:16:34] 20130522145154 [20:24:08] $ mwscript eval.php svwiki [20:24:08] > $res = MW::srv()->get('CentralAuth.GlobalUserSelectQueryBuilderFactory')->newGlobalUserSelectQueryBuilder()->whereGlobalUserIds(27824779)->fetchCentralAuthUsers(); [20:24:08] > $cu = iterator_to_array($res)[0]; [20:24:09] > return $cu->getName(); [20:24:09] Nr: 135 [20:24:29] taavi: This one doesn't :) [20:24:38] (via newFromRow) [20:25:41] the User::newFrom* methods rely on User::load() which will query the user with a WHERE part depending on the from flag, then pass it to newFromRow [20:25:54] except that newFromName also does username validation [20:26:07] the other entry points don't [20:26:22] GlobalUserSelectQueryBuilder::whereUserNames validates as well. [20:26:32] normalizes/validates implicitly [20:26:53] via default $rigor RIGOR_VALID [20:36:23] I complained about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuth/+/1217825 being written with AI, which I'm still pretty sure is the case, but the code actually seems decent [20:36:40] I wonder what tool was used