[03:51:58] legoktm: I'm going to bug you one last time about https://github.com/wikimedia/composer-merge-plugin/pull/67 and then give up and self-merge [03:52:09] * bd808 knows this is unfair [04:06:33] bd808: bah sorry, looking now [04:07:30] bd808: what code coverage rule is $this->inputOutput->write($message); ignoring? [04:07:54] * bd808 looks at the patch [04:08:42] oh. its the back compat code for old versions of composer before writeError was introduced [04:09:01] so why does phpcs need to ignore it? [04:09:16] so I get 100% coverage :) [04:09:39] its not code sniff ignore its code cover report ignore [04:10:04] ohhh [04:10:08] I misread [04:10:14] oops :P [04:12:15] my hero! Now I can submit the next follow up patch ;) [04:12:26] heh [04:25:50] legoktm: https://gerrit.wikimedia.org/r/#/c/248681/ [04:29:38] legoktm: this one should be an easy review -- https://github.com/wikimedia/composer-merge-plugin/pull/79 [04:29:46] bd808: I already finished reviewing it :P [04:29:59] so fast [15:24:38] anybody knows why hook arguments - e.g. https://www.mediawiki.org/wiki/Manual:Hooks/TitleMoveComplete - are defined as by-ref? [15:33:46] SMalyshev: In the case you link, probably old PHP4 compat where objects were cloned when passed into functions unless passed as references, and now we can't change it because it would break everything using the hook. In some other cases, another possible reason is that the parameter is intended to be replaceable by the hook function (such as $error in https://www.mediawiki.org/wiki/Manual:Hooks/AbortMove). [15:34:42] anomie: would it actually break if you don't put & in functions? [15:34:52] I mean in hook implementations [15:35:37] AbortMove is fine, that makes sense, but TitleMoveComplete doesn't [15:40:25] SMalyshev: If the hook function is declared to take a reference but the hook call doesn't pass one, PHP and HHVM both issue a notice and PHP additionally doesn't run the function at all (HHVM runs it, but it doesn't get a reference to the original variable). If the hook function doesn't declare a reference then behavior depends on the PHP version, but no warnings in any case: In 5.3.10 it gets a reference anyway and so might accidentally modify [15:40:26] the input, while in 5.6.14 (and HHVM 3.6.5) it doesn't get a reference. [15:41:10] anomie: I'm talking about the opposite case, so that function implementation should not be declared to take a reference [15:41:33] SMalyshev: I covered both cases in my reply. [15:42:16] anomie: yes, so I wonder why hook description still lists by-ref parameters and whether it shouldn't be changed [15:42:40] Because the hook gets passed by-ref parameters. [15:43:33] anomie: I'm not sure I understand this. The hook, as it seems, does not _need_ by-ref parameters, so it should not be requiring them [15:44:30] Except that for historical reasons some implementations of the hook *do* need by-ref parameters, so we need to keep passing them, and then in some versions of PHP all hook functions will *get* by-ref even if they didn't ask for it. [15:45:09] anomie: you're saying some implementations actually change title and user when calling TitleMoveComplete hook? And we're supporting such scenario? [15:45:39] SMalyshev: No. "If the hook function is declared to take a reference but the hook call doesn't pass one, PHP and HHVM both issue a notice and PHP additionally doesn't run the function at all" [15:46:03] Stray notices are bad enough, and not calling the function really sucks. [15:46:13] anomie: ok, so the question is - *why* we are prescribing that the hook function should be declared to take reference? [15:47:02] New hooks shouldn't. But this hook (presumably) existed back when we still supported PHP4 where by-ref objects were necessary to not suck performance-wise. [15:47:28] (and to function for mutable objects, for that matter) [15:47:29] anomie: but PHP 4 support is long gone. Why it is still defined this way, even in new extensions? [15:47:50] in fact, the docs say you must define it this way in https://www.mediawiki.org/wiki/Manual:Hooks/TitleMoveComplete [15:47:55] SMalyshev: I already answered that: backwards compatibility. Enough going round and round the mulberry bush. [15:48:18] anomie: backwards compatibility with what? defining it without refs does not break anything [15:48:33] * anomie ignores further discussion on this topic, as it has already been explained. [15:48:48] ok, thanks a lot for your help, I got the idea [16:09:54] SMalyshev: to your compatibility with what question, I think the answer is that MW tends to be very conservative about backward compatibility breaks. There could be an unknown number of users of that hook (or any hook) in the deployed MW base worldwide (and beyond! NASA sends MW into space!) [16:10:11] maybe we are too conservative (quite probable) [16:10:39] and we are still cleaning up random PHP4 code in core all over the place [16:10:46] bd808: I'm not sure what BC break that would be though. I.e., if we say that we do not require refs on new implementation of that hook, what kind of BC break that would be? [16:11:23] the doc could be updated probably but the call to the hooks would still need to pass-by-ref wouldn't they? [16:11:27] bd808: I mean right now Wikibase defines that hook without refs, and nobody seems to minf [16:11:29] mind [16:11:39] bd808: the call yes, of course [16:11:58] *nod* That may have been where you and Brad were talking past each other [16:12:01] bd808: but I'm talking about fixing the docs and the existing instances, so that we do not propagate the insanity further [16:13:22] +1. I haven't looked at the specific example you are talking about but we should be cleaning up the docs at least for things where we don't want the hook to be able to substitute a new object [16:13:28] Except if you "fix" the docs, then they don't reflect what's really passed to the hook. And things might accidentally break in PHP 5.3 when the function isn't expecting a by-ref but gets one anyway. [16:14:00] * bd808 wants php 5.5 to be min supported version ASAP [16:14:08] * anomie would like that too [16:14:30] bd808: yep that was my intent, right now the docs pretty much say "you must use references" and of course people do that even if in fact the hook does not use them [16:15:13] bd808: but if that's fixed then at least for new code people stop using them and then eventually all uses of refs one would hopefully die out [16:15:26] I'll channel me inner Reedy and say {{SOFIXIT}} :) [16:16:14] Or at least file some bugs that someday may get fixed [16:16:40] bd808: I can definitely make the patch for the code (and will, once I am back from Germany probably and nobody beats me to it). Not sure what is the protocol for editing the hooks docs... [16:18:16] nothing special really, just another gerrit patch [16:18:33] getting reviews ... well that's always the trick around this project [16:18:34] bd808: right now i think https://gerrit.wikimedia.org/r/#/c/244044/ broke some stuff because it removed &s... but that's why I got the original question of why it needs refs [16:19:22] that does look like it could break things [16:19:42] yeah we can fix cirrus easily but there are more [16:19:55] I'd just put the refs back [16:20:00] they don't hurt anything [16:20:48] bd808: Better find a way to bribe opsen to get the work machines upgraded ASAP :: [16:21:00] Joe is working on it now! [16:21:11] tin/terbium [16:21:44] and I think the dump servers are close; there's an hhvm bug that is still causing bzip stream issues [16:21:47] we were looking earlier why mod_dav was enabled on them both [16:21:53] That's not a blocker though [16:22:01] They're on newer PHP, just not hhvm [16:22:03] I saw that. I'm guessing for the lulz [16:24:19] SMalyshev: you really should just do a partial revert of that patch that puts the refs back rather than try to play whack-a-mole with all the hook implementations it might be breaking [16:26:50] https://phabricator.wikimedia.org/T87036#1757899 [16:27:01] Should we MOTD on tin saying people should be using mira? [16:28:10] bd808: dcausse probably would do that but it just doesn't look right in general [16:28:11] Oh, is master co-sync working? :/ [16:30:33] not quite yet Reedy, but maybe soon [16:32:06] :) [16:33:34] I suspect if tin is sorted, terbium shouldn't be too bad... It's not gonna be full of unpuppetised cronjobs this time around [16:34:53] that's a very hopeful POV Sam ;) [16:35:08] You remember fenari, right? ;) [16:36:00] it was being decommed as I was hired (and for the whole first year I was here) [16:36:07] I never actually logged into it [16:57:33] legoktm: I love that you are working to turn a security bug POC into a feature for CI. :) [17:05:22] hehe :) [17:24:39] bd808: Are you rebasing my EP patch or should I? [17:24:41] ++<<<<<<< c419ff0b6a4841205ff4fa58048671baa6cdb736 [17:24:41] + * @param string|null $html Linktext of the link as raw html [17:24:41] ++======= [17:24:41] + * @param null|string $html [17:24:41] ++>>>>>>> Move ORM classes to EducationProgram extension [17:24:58] Reedy: I just tweaked the commit message [17:25:08] I'll rebase and fix the conflict then :) [17:26:48] heh, I fixed something in that patch that Aaron had done in a recent commit [17:26:49] https://github.com/wikimedia/mediawiki-extensions-EducationProgram/commit/345059cc393880f1001f07b4c9ef030e8fb6c1d3 [17:37:43] bd808, legoktm: where do packagist-admin@wikimedia.org emails go? [17:38:13] I think we had an alias setup. I think I get them... [17:38:50] I definitely get them [17:38:53] bd808: do you mind if a add it to a wikimedia wordpress account so i can add a gravatar? you'll get an e-mail with a link asking you to confirm ownership of the account [17:39:05] oh yeah that would be awesome [17:40:11] done, you should have received an e-mail [17:40:50] I suppose you need that link [17:41:10] PM'd [17:42:57] nice done [17:44:06] SMalyshev: you might want to look at https://gerrit.wikimedia.org/r/#/c/246306/ [18:04:38] csteipp: a +2 on https://gerrit.wikimedia.org/r/#/c/247923/ would certainly be welcome [18:05:03] ori: the config setting you mentioned was auto_eject_hosts, right? [18:20:56] AaronSchulz: http://git.legoktm.com/legoktm/base-convert waiting on the gerrit repo to be created [18:26:24] looking [18:29:50] nice [18:32:33] tgr: yes [18:33:20] tgr: I had a hunch that it was the cause of the silver nutcracker alerts we kept seeing, so I removed it from silver's config in https://gerrit.wikimedia.org/r/#/c/248603/ . It's too early to say definitively, but I haven't seen any alerts since I merged it. [18:34:53] auto_eject_hosts was set for the production cluster at one point and since then other users of nutcracker have copied it, but it is interesting to note that it is not set by default [19:41:15] ori: wikimedia/running-stat is merged. It took zuul foreeeevaaar to finish it [19:41:22] bd808: \o/ thank you! [20:05:16] hmmm...can't vent on the #wikimedia-staff channel about my DMV saga. Can I vent here? :-) [20:08:37] Can't allow that. We're never offtopic in here... [20:08:44] * Reedy coughs loudly [20:14:50] :-) [20:15:08] * robla scurries off to create the #robla-venting-about-dmv channel [20:15:47] csteipp: want to take a quick gander at https://phabricator.wikimedia.org/T109723#1759237 and tell me if it makes sense? [20:16:14] as in, whether the description is comprehensible. I know that actually auditing it is more work than that. [20:22:12] dapatrick: or you! (I almost forgot about you! :P) [20:52:45] who can send to mediawiki-announce? [20:55:45] a handful of people ;) [20:57:04] excellent answer, Reedy [20:58:22] ikr? [20:58:32] tgr: I know I could at one point, so presumably I still can [20:58:41] I think I had the password at some point [20:58:52] ostriches would be the most recent person to send anything via it though I guess [20:59:28] But Tim and Chris for 2 more should be able to [21:00:43] Me and Chris both have the password. [21:01:44] I guess that's the thing [21:01:46] Anyone can send to it [21:01:50] Only a few people can approve it [21:01:57] /disable the drop all [21:02:51] Yeah, we have drop all turned on. [21:03:20] Well, plus everyone's mod bit it set so even when I disable drop all it'd get held for moderation. [21:03:34] Yeah, that sounds familiar [21:10:16] AaronSchulz: https://phabricator.wikimedia.org/T116485 Echo was using the LinksUpdateAfterInsert hook + $wgUser to send out notifications...is there any way to get the user who triggered the change now that it's happening in the job queue? [21:11:05] legoktm: this is for forward link updates ? [21:12:03] forward? Echo notifies you if a page you created was linked somewhere. Except we've started notifying people who make the link because $wgUser isn't set [21:13:33] so, yes, foward links it seems [21:14:12] does it care about ones added via backlinks or just ones added by editing? [21:14:20] just editing [21:14:49] thanks [21:15:00] ostriches: can you approve the mail I just sent there? [21:15:22] tgr: It'll have been > /dev/null [21:15:45] You'll need to get him to turn the drop all filter, send it again, approved, then he can turn the drop all on again [21:16:11] uh [21:16:23] okay, can you do that then? [21:16:41] or I can just send you the text of the email if that's easier [21:17:39] legoktm: if the user was injected into LinksUpdate and getAsJobSpecification() set the user as a flag, then the RefreshLinks class could know about it and pass it to LinksUpdateAfterInsert [21:19:01] ok, I'll try that [21:20:57] bd808: lol @ interface_exists() making up fully 15.41% of load on api cluster [21:21:05] https://performance.wikimedia.org/xenon/svgs/daily/2015-10-27.api.reversed.svgz [21:21:08] seriously? [21:21:08] maybe add LinksUpdate::setTriggeringUser() or such [21:21:38] yep [21:21:53] that's crazy [21:22:24] it's just over double the time we spend in mysqli::hh_real_query [21:22:54] I wonder what effect the new guard will have? [21:22:59] tgr: what e-mail are you sending from? [21:23:08] hopefully a lot [21:33:55] ostriches: gtisza@wikimedia.org [21:36:41] try again [21:41:20] ostriches: sent [21:42:03] ori: Do you think that monolog patch is worth rushing to get or can/should we wait for the next tagged release? [21:42:21] (the microseconds thing) [21:42:55] tgr: https://lists.wikimedia.org/pipermail/mediawiki-announce/2015-October/000183.html [21:43:09] thanks! [21:44:19] yw [21:51:29] AaronSchulz: is https://gerrit.wikimedia.org/r/249302 heading in the right direction? I haven't tested it yet [21:58:37] bd808: why don't we simply make a local modification to the monolog code in mediawiki/vendor? [21:58:50] it'd get clobbered the next time the lib is updated, but that's what we want in this case [21:59:56] I suppose that would work. Composer shouldn't mess with the existing files in vendor as long as the package version isn't changing. [22:00:27] it's not a huge deal, we could just not worry about it [22:00:40] worry about getting it in a week sooner than it would naturally or whatever [22:00:53] what is the release cadence like? [22:01:05] erratic :) [22:01:31] TimStarling: reminder re: https://gerrit.wikimedia.org/r/#/c/249025/. From some ad-hoc logging it looks like some modules (infobox especially) take hundreds of milliseconds [22:02:16] bd808: I'll submit a patch to vendor [22:02:23] yeah, but remember that you are including callbacks into the parser in your profiling [22:02:35] argument expansion, preprocessing [22:02:52] the built-in profiler doesn't count that [22:03:40] ori: I think we have fallen way behind. I would think that bump to 1.17.2 + my patch would be a reasonable update. The train has rolled so we would get a good shot of testing on the beta cluster [22:04:09] bd808: cool [22:04:48] I can actually make the patch for that if you'd like [22:04:49] ori: it might be interesting to use the CPU time from LuaSandbox::getCPUUsage() [22:05:09] TimStarling: Oh, I was just about to ask if that is available to PHP -- I guess it is! [22:05:12] bd808: please do! [22:05:52] we use pauseUsageTimer() to discount time spent in PHP callbacks [22:06:44] Cool. I'll update the Scribunto patch to use that. I'm hoping to use the code in https://gerrit.wikimedia.org/r/#/c/249025/ for sampling, tho [22:10:34] Oh wow. I had completely forgotten about LuaSandbox::getProfilerFunctionReport() too [22:13:07] but since the HHVM migration we only enable the profiler if forceprofile is true [22:14:26] well, getCPUUsage is good enough, certainly an improvement over the microtime( true ) sandwich [23:22:17] ori: Sorry. I just saw your message. Did csteipp already get back tyou? [23:22:22] *to you [23:22:39] no, I think he had to take the afternoon off [23:23:13] I'm back... just working on pci stuff, so want to strangle myself [23:23:27] What was this about? [23:23:46] https://phabricator.wikimedia.org/T109723#1759237 [23:24:22] i think it's OK to close the task unless you see something alarming in that description [23:25:02] Nope, that looked ok [23:25:13] it would benefit from a security audit, but in the general way that almost everything could [23:25:36] Yeah, I'd love to spend the time doing a full audit, but totally not possible right now. [23:26:51] cool, so i'll close it [23:27:26] I'm just adding some comments, and I'll close it [23:27:34] cool, thanks [23:29:20] whenever anyone says "working on pci stuff" the first image that comes into my head is scraping your hands trying to extract a stuck nic from some dusty atx desktop case [23:29:34] So many cuts. [23:29:44] it's not that different I'm sure [23:29:44] haha. Done that one :) [23:29:55] And lost jumpers. [23:30:57] More like pull out 500 pci boards, each with multiple screws, then plug them all back in just because someone thought it would be prettier if they were in a different order. [23:35:46] and needed a vendor logo silk screened on them [23:36:07] * bd808 should really take PCIDSS stuff off of his resume [23:36:34] ori: was https://tessera.wikimedia.org/dashboards/6/ciphers?from=-1h moved as part of this? [23:37:28] I'm having trouble resolving it, but it could just be Comcast messing up. [23:37:46] I think tessera got killed [23:38:18] Oh, okay. Was it replaced with something else? [23:38:38] Probably this dashboard -- https://grafana.wikimedia.org/dashboard/db/tls-ciphers [23:39:21] grafana won the graphite dashboard wars (for now) [23:39:26] yes [23:39:28] what bd808 said [23:40:19] Ah, swell. Thanks.