[16:44:50] <subbu>	 no_justification, James_F legoktm T192855 .. looks like Tidy has been replaced by RemexHtml everywhere? Is this related to the change where remex is the default on mediawiki?
[16:44:50] <stashbot>	 T192855: On several Wikipedias (ar, en, fr), badly closed markup exposes corruption (signatures coloring unrelated follow-up sections, etc.) - https://phabricator.wikimedia.org/T192855
[16:47:12] * no_justification has no skin in this game 
[16:48:16] <Krinkle>	 Wow, entering an IP in the search field defaults to directing to Special:Contributions/<IP>
[16:48:18] <Krinkle>	 Is that new?
[16:48:33] <Krinkle>	 Even if wrapped in quotes (which was unexpected, and not what I intended)
[17:00:41] <James_F>	 subbu: See my comment in -operations.
[17:03:47] <subbu>	 James_F, so much noise in there from bots and icinga .. but, your comment at 9:29 PT?
[17:04:00] <James_F>	 subbu: Yeah. no_justification can probably advise.
[17:04:36] <subbu>	 "So… if a wg* setting is set to an explicit value in InitialiseSettings it's read and set, but if it's set to null do we explicitly set it to null, or do we not set it?"
[17:04:56] <subbu>	 James_F, if that were the change, that would affect lots of things, not just Tidy / Remex right?
[17:05:00] <James_F>	 Because MW is expecting it to be explicitly set to null.
[17:05:31] <no_justification>	 You should always explicitly set something you plan to one day override
[17:05:46] <James_F>	 We do.
[17:05:52] <James_F>	 But we explicitly set it to null.
[17:05:56] <no_justification>	 Ok, and?
[17:05:57] <James_F>	 Hence my question.
[17:06:09] <subbu>	 https://phabricator.wikimedia.org/T192855#4154598
[17:06:11] <James_F>	 It seems InitialiseSettings isn't going through with that?
[17:06:22] <James_F>	 Or I'm wrong in my dianosis.
[17:06:28] <no_justification>	 We explicitly set things to null....
[17:07:46] <no_justification>	 'wgImportTargetNamespace' => [
[17:07:46] <no_justification>	 	'default' => null,
[17:07:47] <no_justification>	 eg
[17:09:04] <James_F>	 Then I'm not sure how it broken. Pre-wmf.30 MW's default value was null, and we also explicitly set the default to Remex and then over-rode back to null where we still want Tidy. Post-wmf.30 MW's default value is Remex, so our default is the same. Does the explicit over-riding to null just not do that?
[17:10:16] <no_justification>	 Probably not! It probably uses isset() if I had to guess....
[17:10:30] <James_F>	 Ha.
[17:10:34] <no_justification>	 Why not just leave the default? It's redundant, sure, but will work until this whole deal is done?
[17:10:46] <James_F>	 Leave the default in MW?
[17:11:02] <no_justification>	 Wait, is it 'default' => null that's the problem, or 'anything' => null?
[17:11:06] <James_F>	 Because you chose to branch REL1_31 and there was an RfC to change the default for REL1_31.
[17:11:22] <James_F>	 'anything' => null where MW's default is not null, perhaps?
[17:11:35] <no_justification>	 Perhaps.
[17:11:49] <no_justification>	 Chose to branch REL1_31? :p
[17:11:53] * no_justification chuckles
[17:11:54] <subbu>	 maybe switch it back to null (tidy) for now .. seems the simplest fix?
[17:12:47] <no_justification>	 Is there a ['driver' => 'Tidy'] we can use?
[17:12:52] <no_justification>	 Avoid the whole null scenario?
[17:13:00] <James_F>	 Not sure.
[17:13:17] <James_F>	 Options are (1) live with it, and ask communities to deal, (2) add a hack in mw-config to implement what we actually want, somehow, (3) revert in master but not REL1_31, (4) revert everywhere and miss the deadline.
[17:13:43] <no_justification>	 Avoiding null would be my option (5)
[17:13:44] <no_justification>	 If we can
[17:14:12] <subbu>	 (2), (3), (5) are best solutions. Let me read tidy config code to see if i can find out .. unless anomie knows this offhand wrt no_justification's qn.
[17:14:36] * anomie reads backscroll
[17:14:39] <James_F>	 Unfortunately "null" is the since-the-dawn-of-time default value because MW sucks.
[17:14:42] <subbu>	 (1) would be okay except we've been saying all year that they have time till end-June.
[17:14:52] <James_F>	 Yeah, I'd rather not do (1).
[17:15:16] <tgr>	 (3) seems pretty harmless
[17:15:17] <subbu>	 enwp seems to not mind :-) https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Tidy_no_longer_running 
[17:15:30] <tgr>	 you could even do it in the prod branches instead of master
[17:15:46] <James_F>	 tgr: It just bumps the problem down the road.
[17:16:19] <James_F>	 subbu: Yeah, but it's discourteous to put it mildly, and some wikis have pages that are actively broken, albeit mostly with rainbow discussion text from users who don't fix their HTML.
[17:16:20] <subbu>	 James_F, there i sthe immediate thing to do .. and there is the fix for this null config problem .. which is more generic.
[17:16:34] <subbu>	 James_F, no, no understood. i am not suggesting we do (1).
[17:16:36] <James_F>	 AFAICT there's no ['driver' => 'Tidy'] setting.
[17:17:04] <subbu>	 i was mostly observing that enwp doesn't seem to mind.
[17:17:17] <James_F>	 MWTidy::factory doesn't have a value for RaggettInternalHHVMorPHP.
[17:18:05] <James_F>	 Krinkle: Hey, you posted on the task, ideas?
[17:18:27] <James_F>	 We could hack it to driver => RaggettInternalHHVM given that all WMF prod is running HHVM right now, I guess?
[17:18:40] <James_F>	 But that'll break SRE's work moving to PHP7, which is not great.
[17:18:47] <anomie>	 [ 'driver' => wfIsHHVM() ? 'RaggettInternalHHVM' : 'RaggettInternalPHP' ] should do it for the WMF configuration, I'd think.
[17:18:57] <James_F>	 anomie: Yeah.
[17:19:01] <James_F>	 OK, let's do that for now.
[17:19:10] <anomie>	 Or replace wfIsHHVM() with defined( 'HHVM_VERSION' ) if GlobalFunctions haven't been loaded yet.
[17:21:14] <Krinkle>	 Wait, so do we know why it isn't using Tidy?
[17:21:37] <James_F>	 We think it's a MWMultiVersion issue with default non-null and value null.
[17:21:46] <Krinkle>	 Core uses Tidy if driverConf=null and useTidy=true
[17:22:07] <Krinkle>	 regardless of whether that works and whether we properly set useTidy=true as well for the same or all wikis, the driverConf part is failing.
[17:22:39] <Krinkle>	 James_F: Yeah my thoughts too, although the logic you're thinking of is in core (SiteConfiguration/wgConf)
[17:22:41] <James_F>	 Krinkle: Yeah, hence forcing conf.driver to the right value.
[17:22:44] <Krinkle>	 not multiversion
[17:22:48] <James_F>	 OK.
[17:22:49] <James_F>	 https://gerrit.wikimedia.org/r/428697
[17:22:51] <James_F>	 Not tested.
[17:22:58] <Krinkle>	 I'm writing a test to confirm now, but don't wait for me if there's an easier immediate fix
[17:23:07] <James_F>	 I've got to interview someone in 7 minutes' time. Can someone try that out? no_justification?
[17:23:34] <subbu>	 James_F, missing commas at the end of lines,
[17:23:42] <James_F>	 Meh.
[17:24:09] <James_F>	 Repushed.
[17:28:18] <Krinkle>	 Nope, SiteConfiguration supports null as an override value
[17:28:57] <anomie>	 SiteConfiguration::getAll() skips null values, so they don't make it to the extract() to override MW's defaults.
[17:29:43] <anomie>	 https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/SiteConfiguration.php;86cf7f19229fb0efe916a0f0f6db853b647b5002$317
[17:31:39] <subbu>	 I suppose the default change went out last week in the train ... and i guess noticed only today by different editors as caches purged?
[17:31:57] <Krinkle>	 https://gerrit.wikimedia.org/r/428701
[17:32:01] <Krinkle>	 Aye, my test was lower-level.
[17:32:06] <Krinkle>	 Nice catch
[17:34:28] <Krinkle>	 anomie: Hm.. seems like the current handling in core would also support false or empty array, which might be an easier fix
[17:34:38] <Krinkle>	 given they are also !
[17:34:40] <Krinkle>	 not
[17:35:35] <Krinkle>	 Ah, nvm. it uses !== null
[17:35:39] <anomie>	 Krinkle: For $wgTidyConfig? It doesn't look that way to me at https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/parser/MWTidy.php;86cf7f19229fb0efe916a0f0f6db853b647b5002$70
[17:36:35] <subbu>	 Is someone testing James' patch so it can be deployed? Or does it look good to go? I +1ed it.
[17:37:22] <Krinkle>	 it seems to miss the rest of the config array 
[17:37:23] <Krinkle>	 'tidyConfigFile' => $wgTidyConf,
[17:37:23] <Krinkle>	 					'debugComment' => $wgDebugTidy,
[17:37:23] <Krinkle>	 					'tidyBin' => $wgTidyBin,
[17:37:23] <Krinkle>	 					'tidyCommandLine' 
[17:37:29] <Krinkle>	 Not sure how well that goes
[17:38:48] <Krinkle>	 Yeah, that would probably fail on $this->config['tidyConfigFile'] at some point.
[17:38:55] <Krinkle>	 with E_NOTICE and probably a fatal afterwarwd
[17:39:56] <anomie>	 We could always just hack around the getAll() thing in CommonSettings.php, $wgTidyConfig = $wgConf->get( 'wgTidyConfig', $wgDBname, $dbSuffix, $confParams, $wikiTags );
[17:40:28] <anomie>	 Or maybe better, do it in $globals just after the getAll() call.
[17:40:56] <Krinkle>	 Or we can remove !is_null from getAll
[17:41:03] <subbu>	 anomie, Krinkle are you guys investigating a different solution? or a longer-term fix for the config issue?
[17:42:11] <anomie>	 Krinkle: I was thinking that too, but first I wanted to check whether anything was depending on that behavior. It doesn't look like it.
[17:43:14] <anomie>	 $wgImportTargetNamespace is null in DefaultSetting.php too so it wouldn't be affected, and $wmgLQTUserControlNamespaces isn't even set by default.
[17:43:16] <Krinkle>	 So 4 options: 1) In wmf-config  set 'wgTidyConfig' with a second $wgConf->get() that avoids the bug with null in getAll, 2) IN wmf-config substitute the logic like James did, but fix it to actually set all the keys core normally sets, 3) Fix core to not ignore null in getAll, 4) Change core to support a different false value for TidyConf, like false.
[17:45:34] <Krinkle>	 3) seems risky/untested, the others are pretty simple. 1 and 4 are easiest to write/verify.
[17:45:51] <tgr>	 3) seems like the sane option
[17:46:27] <anomie>	 I like #3 long-term. But if we think it's too risky to do quickly, either #1 or #4 work for me as a second choice.
[17:46:30] <tgr>	 add an optional reference parameter to getSetting, use that to tell whether the setting was found, use that instead of is_null in getAll
[17:48:39] <Krinkle>	 OK. Let's go with #1 so we don't need backport dance and can deal with it in core/master later, and do it with tests etc. there
[18:07:50] <Krinkle>	 anomie: Hehe, I was honestly thinking the same, that the last thing we need is for PHP to omit nulls from extract()
[18:07:53] <Krinkle>	 would not have surprised me
[18:26:47] <James_F>	 subbu: wmf.30 only went out yesterday to prod Wikipedias, so that's why it only got noticed.
[18:38:23] <James_F>	 OK, is someone planning to deploy Krinkle's fix for T192855? I'm happy to test.
[18:38:23] <stashbot>	 T192855: Remex enabled on all wikis in MW 1.30-wmf.30 exposing corruption (signatures coloring unrelated follow-up sections, etc.) on unfixed content - https://phabricator.wikimedia.org/T192855
[18:38:35] <Krinkle>	 Last I checked scap is still running
[18:39:09] <James_F>	 Oh, for the train? Meh. Such is timing. :-)
[18:40:03] <subbu>	 James_F, y'day? oh .. ok.
[18:40:33] <subbu>	 i thought trains reach their destination by thu. usually. one-off i assume.
[18:41:31] <James_F>	 subbu: They're meant to, but unfortunately there was breakage of some kind and it had to be reverted.
[18:41:44] <subbu>	 k
[19:06:53] <DanielK_WMDE>	 anomie: re RevisionSlotsUpdate and type hinting: I'm coming around to not having MutableRevisionSlots implement RevisionSlotsUpdate,  for somewhat different reasons. I'll probably play with that tomorrow, nearly done for today.
[21:11:47] <no_justification>	 Warning: Invalid argument supplied for foreach() in /srv/mediawiki/php-1.31.0-wmf.30/extensions/LiquidThreads/classes/View.php on line 507
[21:11:50] <no_justification>	 Heh
[21:11:52] <no_justification>	 Poor LQT
[21:35:25] <no_justification>	 require_once(/srv/mediawiki/docroot/m.wikipedia.org/w/../multiversion/MWMultiVersion.php): File not found in /srv/mediawiki/docroot/m.wikipedia.org/w/mobilelanding.php on line 2
[21:35:30] <no_justification>	 Well that's no bueno
[21:35:35] <no_justification>	 But we saw that before?
[21:39:30] <no_justification>	 A-ha, I broke this iirc
[21:41:54] <no_justification>	 Krinkle: https://gerrit.wikimedia.org/r/#/c/428842/
[22:25:44] <legoktm>	 subbu: James_F: did the Remex default thing get fixed?
[22:33:40] <subbu>	 legoktm, there is a patch to fix it - https://gerrit.wikimedia.org/r/#/c/428697/ - but don't think it has been deployed yet.
[22:34:03] <legoktm>	 so things are still broken?
[22:34:48] <subbu>	 James_F or Krinkle might know better about what the deployment plan for it is .. i think scap was running for the next train.
[22:35:00] <subbu>	 when they last checked a few hours back.
[22:36:10] <subbu>	 yes, remex is still live on all wikis.
[22:52:59] <legoktm>	 subbu: ok, I tested it on mwdebug1001, syncing it out right now...
[22:53:15] <subbu>	 k
[22:53:28] <legoktm>	 Not sure if it's worth spending time to invalidate the parser cache (or even how to)
[22:53:39] <subbu>	 nah, i think it is okay.
[22:54:19] <subbu>	 i'll hang around for another 15 mins to test / verify anything required.
[22:55:22] <legoktm>	 ok, should be live now
[22:56:38] <subbu>	 ok. https://ro.wikipedia.org/wiki/Otto_von_Habsburg is back to tidy behavior after i purged it.
[22:58:55] <subbu>	 so is https://en.wikipedia.org/wiki/Talk:Rise_of_Nations#???? .. 
[22:58:56] <subbu>	 lgtm.
[23:18:08] <subbu>	 thanks legoktm. off now.
[23:21:58] <legoktm>	 o/