[02:38:45] TimStarling: if you have a bit of time, could you have a look at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/718414 - I implemented your idea with __destruct. Wanna make sure I got it correcly [02:44:26] a multi-author patchset? [02:49:57] I think it's a bit dodgy to put the register_shutdown_function() call in the constructor, I would put it in Setup.php [02:50:13] other than that, it's fine for a +1, but are you looking for a +2? [02:55:05] ok, will move into setup.php [02:55:27] I'll +2 it tomorrow after talking with someone who's doing the 1.37 release [02:55:38] just wanted a +1 [02:57:57] > a multi-author patchset? - yeah, Danny said they tried and didn't quite get it, so I did it myself. [04:34:06] I see some potentially unintended UserOptionsLookup-refactor artefacts being multiplied at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/721420/ [04:34:26] This is calling $user->saveOptions() after only interacting with UserOptionsLookup->setOption [04:34:57] It seems like UserOptionsLookup->saveSettings() would suffice, but I don't see much or anything do that. [04:35:53] Might be worth a small sprint to revisit some of the updated callers and use that when possible since it seems to do a lot less, but I don't know if that's sufficient. [04:36:08] The @internal flag suggests no, since it bypasses the CAS check [04:36:11] Makes sense :) [04:37:54] nvm [04:39:50] Each one hook caller calling saveSettings synchronously still seems awkward though [04:45:02] I vaguely recall a discussion to create a hook specifically for feeding non-default settings on a new account which would batch these. But I can't seem to find any trace of it now. I do note that saveSettings() is literally called on the line after getHookRunner()->onLocalUserCreated() so at least in theory most existing hook callers could be changed to simply not call saveSettings(). [04:45:14] Does anyone know more about a potential plan around this? [04:48:24] ah, and of course inevitably some people do exactly as they are told in the OptionsLookup->setOption doc block which says to call saveOptions afterwards: [04:48:25] https://gerrit.wikimedia.org/g/mediawiki/extensions/LanguageSelector/+/15a694ecbc17eabb2d49290a384ba12c2766bcb2/LanguageSelectorHooks.php#293 [04:49:05] Not an unreasonable thing to think given the push away from User.php and the method existing [04:49:52] maybe a bit too clean of a footgun, possibly worth renaming to saveSettingsInternal unlike for other internal methods that one wouldn't be as likely to call by accident [21:30:17] Krinkle: re user options - the future is that saveOptions will be sufficient and it will not do CAS. We've recently made it so that editing a single option doesn't touch other options, so we don't need a CAS anymore, but we still need to update user_touched and look at all the hook usages and move things from one hook to another hook in all extensions [21:30:32] I'm going to do this all, but never had the time to finish it up [21:32:15] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/705150 - need to review all users of UserSaveSettings hook cause it's not going to be called on options saving anymore [21:35:10] Pchelolo: right, the hook could be moved as-is though, that way it still gets called both ways (User::save or Options::save) since User::save calls Options::save [21:35:19] would need a User object cast for compat [21:35:56] could then introduce a new nicer-name hook that doesn't offer User to slowly migrate to over an LTS release, like with revision hooks [21:36:40] Krinkle: there's already a new hook, just for user options saving. But some extensions might expect that old hook will be called on user options saving. [21:36:42] What is the plan for other changes to the User object thouhg, like the other fields of the user row? Stay in User or into some other UserStore? [21:37:04] If there is going to be a UserStore, it might make sense to keep the touched and purge logic etc in there, and have UserLookup notify that store so as to keep UserOptions light [21:37:05] I'll look again at this tomorrow. [21:38:34] although if we do that, I wouldn't be surprised if long-term UserStore would gain set/getOption methods and turn UserOptions into an internal -only service. [21:39:08] we're still debating about other user fields - user table is a collection of completely random and quite unrelated stuff. So we're thinking to hide it all behind more interfaces [21:39:21] all of that stuff is very rarely needed [21:39:31] like email, or various tokens [21:40:10] perhaps for the short term we can rename UserOptions::save and restore the one or two mistaken callers outside core since presumbaly those are broken in some minor way. Or were those intentionally bypassing User? [21:40:31] e.g. Transalte calling UserOptionsLookup->saveSettings instead of User->saveSettings [21:40:51] until if/when have a safe public method [21:43:32] I'll just finish making the UserOptionsManager version tomorrow. [21:43:37] fell of my radar [21:44:03] and then we can replace saveSettings with saveOptions where possible