[08:15:56] tgr|away: Yeah, that sounds like it. I though of that as well but wasn't so sure. It makes sense since the constructor always gets called upon creation of an object of that class. So yeah, this is good, thanks :) [08:16:04] *thought [08:16:50] This can happen for classes with a constructor but what if the class has no constructor tgr|away? [08:18:54] For example; like for cases where all the methods are static. Apparently, MWMessagePack class has no constructor [15:49:14] xSavitar: for static methods you need to add the deprecation marker to the method [15:51:07] tgr: Okay, so meaning in this case where the class has just 1 method, adding a deprecation marker to that single method is good enough to deprecate the class? Including the @deprecated annotation above the class? [15:51:38] yeah, add @deprecated and call wfDeprecated at the beginning of the method [15:52:35] tgr: That sounds neat. Now what of a class with many methods that are all static? Well, I'm just curious but I highly suspect such a class would have non-static methods too and also a constructor [15:52:50] But if such a case exist, it means we'll have to add a hard dep marker to all the static methods right? [16:04:52] Need review for a UBN patch relating to JobQueue and MassMessage, anyone online familiar with those? https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MassMessage/+/505806/ [16:52:08] xSavitar: yes, all static methods need a hard deprecation call regardless of whether the class is used non-statically or not [16:52:57] Krinkle: not really familiar with it but it seems to me this will regress to T59464 [16:52:58] T59464: MassMessage thinks namespaces that don't exist on the submission site are NS_MAIN - https://phabricator.wikimedia.org/T59464 [16:54:07] since $params['title'] will be reset from $title [16:54:24] hm, no, I guess only $this->params['title'] will [16:55:25] still would be nicer not to pass $title at all [17:00:10] tgr: so aftre that CR just now, I went half way throgh converting it to Params only and that requires quite a few changes, including to 'new $class' in MassMessageSubmitJob which seems like a lot of breaking changes. [17:00:13] Maybe for a separate commit? [17:00:28] I'm up for doing that, but would like to reduce risk and have that ride beta/train [17:01:07] I wonder if we are talking about the same thing, it seems like a trivial change to me [17:01:33] just remove the $title parameter from the parent constructor call, should not affect anything else [17:02:29] as far as I can see the only effect would be that $this->param remains the same as $param [17:02:30] tgr: that's the same as the breaking change I'm considering, except with silent failure instead of hard failure. [17:02:44] see last comment. [17:03:30] Given 'title' is set the cases we care about, the parent logic for setting $params and setting $this->title won't be used. [17:04:14] I'm being hestitant for more radical changes given I've now worked for a week almost full-time on job queue related UBNs and want it to be over without further risk. [17:06:46] sure, major changes should not be done as an UBN fix [17:08:01] my original comment was wrong, what would capture the old behavior (I think) is parent::__construct( 'MassMessageJob', $params ); if ( isset( $params['title'] ) ) { ... } else { $this->title = $title; } [17:09:21] agreed. [17:09:58] anyway title/namespace from $this->params does not seem to be used anywhere outside of the constructor so I think the patch is safe to merge as it is [17:10:30] it will just be confusing as the logging takes the title from there so the logs might end up showing a different page from where it actually runs [17:12:28] Krinkle: PS 2 still passes $title to the parent constructor though [17:13:44] tgr: yeah, that's intentional. I'd rather not have it be constructed differently at queue time than at execution time. Which seems confusing to break the assumption that params-only means GenericParameterJob. [17:13:49] That's what caused lots of issues past week. [17:13:54] I'll drop it all in the next commit. [17:15:48] won't it be constructed via the MassMessageJob constructor anyway? [17:16:42] Krinkle: anyway, the current code looks fine to me if it's an UBN fix. Did/wil you test it? I don't think I have MassMessage set up locally. [17:17:00] Me neither, was going to test in Beta cluster. [17:17:06] given it requires multiple wikis [17:17:16] and I could install it locally with two wikis, but not sure it will approximate it enough. [17:17:19] the tests pass though :) [17:18:15] fair enough [17:18:32] tgr: new MassMessageJob -> getParams | kafka | Job::factory() -> new MassMessageJob; Along the way the params change and stuff comes from different sources. E.g. $title can become $params, and then first dummied in factory() and replaced by real one in ctor. [17:19:00] I can only just wrap my head around it. [17:20:35] in that case, I'm not sure the patch will work [17:21:51] since AIUI the issue is that $params['title'] is ns + title which cannot be properly expressed with a Title object when the job is queued as the namespace might not exist locally [17:22:04] This logic only happens during queuing if the job is not like GenericParameterJob and doesn't have both 'namespace' and 'title' keys. [17:22:16] when you pass $title to the constructor it will override $this->params['title'], getParams will return that [17:22:23] it's not triggered here before nor after. [17:22:33] $params stays untouched. [17:22:52] this->title is set to a runtime-only dummy that our subclass overwrites and overwrites again at execution time from the same params. [17:23:34] 'namespace' isn't set, so it doesn't enact that logic [17:24:03] oh, right [17:50:15] well, wikibase certainly has a lot of CI jobs. Taking 30 minutes ahead of our patch in the pipeline. [17:50:37] including 2x the same 4 jobs for vendor-phpunit-{hhvm,php7x} [17:50:42] looks like a bug in the ci config. [17:52:48] 42 minutes and counting https://usercontent.irccloud-cdn.com/file/jjJxuacd/zuul-status.png [18:38:50] would be nice to have some concept of a priority merge [18:39:08] did not think of that during the CI consultation [18:47:17] tgr: pipelines are shared between mediawiki-* repos so that we avoid cases where two patches are both landing concurrently and pass on the current master but not when both are landed. So they are queued. However, that queue is fragmented by branch. so wmf branches for SWAT aren't blocked on master. [18:47:27] and for wmf branches in particular we also have a separate swat pipeline. [18:47:36] But I was waiting for it to get into master first for beta testing. [18:47:50] but given that there's a bunch of errors there right now, I'm staging on mwdebug instead. [18:48:04] I did confirm with quiddity that the fatal is gone, but unable to confirm it working [18:48:45] testing jobs on mwdebug sounds painful [18:59:57] tgr: indeed. I just realised that. [19:00:13] we really need a proper stag-able canary. [19:00:18] that includes jobrunners [19:00:31] and magic cross-request tracing to keep track and stay in the same group or something. [21:24:55] kostajh: odd, I can still reproduce that XWD/mwdebug1001 issue, with any entrypoint other than index.php (thumb.php, api.php, load.php) for any domain that uses standard-docroot. [21:25:11] e.g. https://en.wikinews.org/w/api.php via XWD/mwdebug1001/hhvm [21:25:36] which means all sites except wikipedia.org [21:25:58] I wonder what's different about that server, e.g. local issue or config difference. or is it just there before anywhere else. [21:48:26] Krinkle: I guess this is true now? https://www.mediawiki.org/w/index.php?title=Manual:Global_object_variables&diff=3205288&oldid=2982410 [21:48:34] $wgParser has all been removed in core [21:49:05] Thanks to simetrical, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/502996 :) [21:49:46] xSavitar: it has not been removed. some references to it in core have been removed. [21:49:54] It is still defined, it still works, and still has usage in many extensions. [21:50:02] Setup.php: $wgParser = [21:50:12] the day that is removed, it is "removed" from MediaWiki. [21:50:19] It's going to linger in extensions for ages [21:50:27] https://codesearch.wmflabs.org/deployed/?q=wgParser%5Cb&i=nope&files=&repos= [21:50:54] Actually not as bad as I thought [21:51:10] Though, wmf deployed [21:51:10] There's not really an issue with that. It does impede progress in terms of code quality on the extension maintainer's part, but using a singleton isn't an improvement there. [21:52:01] so unless there's a benefit to its removal (for which a task hasn't been started yet), I'd say we've done enough for now. But individual extensions can certainly look at adopting the service approach through injection, but that is a major refactor, not find/replace. [21:53:11] xSavitar: So in short: That task was about starting the deprecation, which required we don't use it within core anymore. That has been closed and is finished. But the migration is not over. And it has not been removed (and isn't planned yet). [21:53:13] Krinkle: Meaning that edit I made should be reverted [21:53:18] That's right :) [21:53:23] Thanks for checking. [21:54:02] Krinkle: Done, reverted the edit, thanks! [21:54:04] This entire list is deprecated, though. So we can certainly document that none of these should be used. [21:54:42] +1 to that [21:54:57] Perhaps we could merge it with [[Manual:Globals are evil]] and have a section somewhere with "Legacy variables" that exist or used to exist. [21:57:28] Krinkle: I think you can better tell me :) [21:57:38] But since both lists are partial, it makes sense to merge them right? [21:57:59] One is a list of bad ideas, the other is an explanation of why it is bad. [21:58:30] I think https://www.mediawiki.org/wiki/Manual:Globals_are_evil points to the former [21:58:33] below the page [21:59:19] But anyway, I see the pages are not overly huge, if we decide to merge them now, it will be fairly easier to manage in the long run [21:59:37] If it gets too beefed up with content, then merging is prolly going to be hard? [21:59:58] After merging, we can add {{historical}} and then never edit again. [22:00:11] It's not topic that will require further information. It's in the past. [22:00:50] Okay