[16:29:39] <DanielK_WMDE_>	 anomie: hi! will you get around to anotehr round of reviews on the RevisionStore patch today?
[16:29:56] <anomie>	 DanielK_WMDE_: We'll see.
[16:31:52] <DanielK_WMDE_>	 if anyone else wants to havea look at the first major step towards MCR...
[16:31:55] <DanielK_WMDE_>	 https://gerrit.wikimedia.org/r/#/c/374077/
[19:01:10] <anomie>	 bd808: Am I correct in thinking that still no one has created a phab task for the "move entry points into a webroot directory" idea?
[19:01:38] <Reedy>	 I'd suggest not based on his comment referencing another closed task
[19:01:49] <bd808>	 I haven't seen it. There was some discussion about it in the other "move all the things" ticket
[19:02:04] <anomie>	 Ok then, T180394
[19:02:05] <stashbot>	 T180394: MediaWiki entry points should not be in the base repo directory - https://phabricator.wikimedia.org/T180394
[19:02:05] <bd808>	 the organization of core is ... poor
[19:02:17] <anomie>	 Feel free to edit.
[19:03:08] <bd808>	 anomie: nice. You got the main drawback too I think
[19:03:39] <bd808>	 the current core is organized on the "unpack a tarball in docroot" pattern
[19:03:57] <bd808>	 and changing that means other changes for new deploys
[19:04:40] <bd808>	 I don't think it is a stretch to say the current pattern followed has been found to be an anti-pattern
[19:05:52] <bd808>	 this slides into the sticky tarpit of shared hosting support pretty quickly :/
[19:09:38] <anomie>	 I suppose we could maintain stub index.php and so on in the root directory if we want to keep that extremely simple installation process. Or point out that if all else fails you can create your own stubs in /var/www/whatever that just `require` the corresponding entry points wherever they're located.
[19:13:41] <legoktm>	 I think we'd need the root stubs regardless for back-compat
[19:15:45] <anomie>	 legoktm: Well, one release with them logging deprecation warnings, then remove them. ;)
[19:17:21] <legoktm>	 I don't think that would really be a sufficient deprecation period, but regardless removing them doesn't make much of a difference right? if people point the webserver at webroot/ then the root entry points are just ignored
[19:24:19] <anomie>	 Yeah. The actual tricky part is that we currently can have arbitrary stuff in extensions/ and skins/ that needs to be served, but ideally without serving all the other stuff in there too. Local file uploads (images/) and possibly some things in resources/ too, but those are probably easier to deal with.
[19:28:53] <legoktm>	 excluding debug mode and images, all CSS/JS should go through load.php and don't need to be exposed to the web
[19:29:20] <legoktm>	 maybe we need a similar proxy for image assets?
[20:02:34] <RoanKattouw>	 Does anyone know if we have any conventions or documentation officially deprecating/discouraging the use of extract() in PHP code?
[20:03:01] <Reedy>	 I think so
[20:03:06] <RoanKattouw>	 I'm writing a CR comment referring to someone's use of extract() saying it's evil and they shouldn't use it, but I can't find anything to back that up offhand
[20:03:49] <Reedy>	 Hmm.. Just my old task saying it sucks
[20:03:49] <Reedy>	 https://phabricator.wikimedia.org/T28496
[20:03:56] <MatmaRex>	 RoanKattouw: it is evil, ut we have no rules against it
[20:04:08] <MatmaRex>	 RoanKattouw: fun fact, some of Parser code relies on it
[20:04:13] <RoanKattouw>	 hah
[20:04:20] <RoanKattouw>	 I know some API code uses it, or maybe used to
[20:04:41] <RoanKattouw>	 I think we might have cleaned up all the usages of extract( $this->extractRequestParams() );
[20:05:07] <Reedy>	 There's less than 50 usages in all of our php repos
[20:05:13] <Reedy>	 11 in core
[20:05:18] <Reedy>	 10 being in parser
[20:06:57] <Reedy>	 Let's attempt to make it official :P
[20:06:58] <Reedy>	 https://phabricator.wikimedia.org/T180398
[20:07:54] <RoanKattouw>	 Also, lol, of course $tags = null ; ... if ( !$tags ) { $tags[] = 'foo'; } is completely fine in PHP and doesn't even cause a notice
[20:10:32] <Reedy>	 Guess we should really document it on https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP
[20:13:47] <Krinkle>	 RoanKattouw: https://wiki.php.net/rfc/forbid_dynamic_scope_introspection
[20:14:19] <Krinkle>	 And many PHP7 RFCs also mentioned extract() as a doorn in het oog
[20:14:23] <RoanKattouw>	 Whoa
[20:14:36] <RoanKattouw>	 BTW that proposed setting doesn't forbid what it sounds like
[20:14:42] <RoanKattouw>	 It doesn't forbid calling extract() directly
[20:15:03] <RoanKattouw>	 It forbids stufff like $func = 'extract';  ... call_user_func_array( $func, [ ... ] );
[20:15:06] <Krinkle>	 Also https://externals.io/message/100637
[20:15:11] <Krinkle>	 "Deprecate extract() in PHP 7.3"
[20:15:12] <RoanKattouw>	 Or $func( ... ); etc
[20:15:13] <Krinkle>	 maybe
[20:15:45] <RoanKattouw>	 At first I was alarmed about them seeming to want to deprecate useful things like func_get_args(), but then I saw they were only talking about dynamic calls
[20:15:46] <Reedy>	 all steps in the right direction :P
[20:16:22] <RoanKattouw>	 And then I thought "whoa calling extract() and friends using call_user_func() or array_map() is super evil, I hadn't even realized that was possible"
[20:17:16] <RoanKattouw>	 Also lol @ all the things in the "Unclear behavior" section
[20:17:41] <Krinkle>	 And array_map is userland in HHVM
[20:17:43] <Krinkle>	 Fun detail
[20:17:48] <Krinkle>	 doesn't do what you think it does in that case
[20:17:57] <Krinkle>	 (mentioned in the RFC)
[20:18:24] <RoanKattouw>	 Yeah
[20:18:51] <RoanKattouw>	 And as they point out, what if you know that the var name it uses internally is $fn, and do array_map( 'extract' ,  [... ] ) where one of the array keys is 'fn'
[20:18:58] <RoanKattouw>	 You could change the function to a different function halfway through
[20:20:03] <RoanKattouw>	 Oh this was merged last year, ncie