Page MenuHomePhabricator

ExtensionRegistry does not allow complete override of config variables e.g. flat arrays
Open, LowPublic

Description

It'd be really useful if there was a way to have a variable completely override an array. ie we set defaults in the extension config, but in LocalSettings (or CommonSettings et al) we can wipe that out, not use it, and use whatever we've defined there

Related incident: https://wikitech.wikimedia.org/wiki/Incident_documentation/20160810-MediaWiki

Event Timeline

Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson added a subscriber: Jdlrobson.

Not only useful, but essential. The current behaviour is highly misleading.

If extension.json defines:
wgMoo = ['a']

and in LocalSettings in production I put
wgMoo = [];

wgMoo should not be ['a'].

Jdlrobson renamed this task from Allow complete override of config variables to ExtensionRegistry does not allow complete override of config variables e.g. flat arrays.Apr 26 2017, 6:29 PM

Change 374042 had a related patch set uploaded (by EddieGP; owner: EddieGP):
[mediawiki/core@master] ExtensionRegistry: Add 'override' merge strategy

https://gerrit.wikimedia.org/r/374042

I've run into the same thing when working on T62419 and don't really want to wait for https://gerrit.wikimedia.org/r/#/c/335383/ to be merged to see this fixed, so I've pushed another patch for that.

I've run into the same thing when working on T62419 and don't really want to wait for https://gerrit.wikimedia.org/r/#/c/335383/ to be merged to see this fixed, so I've pushed another patch for that.

You can use $wgFoo = [ 'foobar' => true ]. If someone wants to remove it, they can set it to false.

I thought I had left a comment on this ticket earlier, in general I don't like encouraging full overrides. Defaults should be sane, and full overrides end up require needing more maintenance later on. If the defaults aren't sane, that's a different problem. Flat arrays have always been problematic since you need to write some amount of code to be able to remove a single item from them, and we are trying to move away from code in configuration.

Okay, I see the rationale for not introducing new flat arrays. I'll just use boolean ones for the task that drove me here then.

But still, we currently need workarounds like this one in the configuration. I'm not convinced that's a good way either.

I understand your comment as a suggestion of declining this task and abandoning the patch (correct me if that's wrong). What's the alternative for dealing with existing flat arrays then? If it is the removal all flat array config variables from all major extensions, is it realistic that all will be refactored in the foreseeable future? If we can't do that, there should probably be a way of dealing with flat arrays - without applying workarounds.

Okay, I see the rationale for not introducing new flat arrays. I'll just use boolean ones for the task that drove me here then.

But still, we currently need workarounds like this one in the configuration. I'm not convinced that's a good way either.

I understand your comment as a suggestion of declining this task and abandoning the patch (correct me if that's wrong). What's the alternative for dealing with existing flat arrays then? If it is the removal all flat array config variables from all major extensions, is it realistic that all will be refactored in the foreseeable future? If we can't do that, there should probably be a way of dealing with flat arrays - without applying workarounds.

Ideally extensions would have migrated during the conversion like what VisualEditor did. Usually we introduce a new config variable that uses key/value, deprecate the old one and map the old one to the new config, and then after a while, deprecate and remove the old config variable.

Change 374042 abandoned by EddieGP:
ExtensionRegistry: Add 'override' merge strategy

https://gerrit.wikimedia.org/r/374042

Is there some function that effectively maps boolean config variables to flat arrays internally in core? So one could have a config variable $wgVar = [ 'foo' => true, 'bar' => false, 'foobar' => true, 'baz' => 'false' ] and then call Something::getBooleanConfigAsFlatArray( 'wgVar' ) which would return [ 'foo', 'foobar' ]? I'm primarily interested in a way to simply iterate over a config variable (which works fine with flat arrays, but not with boolean ones, at least I'm not aware of a really short way) and wrote such a function for https://gerrit.wikimedia.org/r/#/c/380061/. Maybe it'd be worth to have such a helper function in core. That way one could use the boolean config variables and overwrite them in whatever way seems best, but still get config variables as flat arrays when it's necessary/easier at runtime.

You can use array_keys( array_filter( $var ) )

You can use array_keys( array_filter( $var ) )

Right, thanks! There's no need for some function then. As the preferred solution seems to be not to use flat arrays at all, I already abandoned my patch and am also closing this now.

Just a note to say I hit this again while testing T211063. It's still very unintuitive and that particular example is a great example where the default configuration may not be sane for wikis that do not put a focus on long form pages.

I ran into this bug (in my view) today and find the current behavior very misleading. Since it is possible to use flat arrays in extension.json there should be a way to threat them right. If is not wanted to use flat arrays in the configuration, which may be a good idea for reasons I cannot overview right now, then mediawiki should give at least give an error when trying to do so.

Other users also had trouble with this issue (see: https://phabricator.wikimedia.org/T262196) so maybe it is worth another look.

tstarling added a subscriber: tstarling.

Defaults should be sane, and full overrides end up require needing more maintenance later on. If the defaults aren't sane, that's a different problem.

It's a problem nobody could be bothered fixing. I think it's better to provide a merge strategy which supports flat arrays than to just leave them all broken for another 4 years. The solution in practice has just been for everyone to add little $wgExtensionFunctions closures to their LocalSettings.php to set globals after extension registration. I think that's worse in every respect than just allowing LocalSettings.php to take precedence where it makes sense to do so.

My current motivation is $wgUserMergeProtectedGroups. If someone wants to implement a fully approved solution, that's fine by me. Applying a sensible merge strategy in the interim doesn't conflict with that.

Change 684668 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/684668

Change 684669 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/UserMerge@master] For $wgUserMergeProtectedGroups use merge strategy "provide_default"

https://gerrit.wikimedia.org/r/684669

tstarling lowered the priority of this task from High to Low.May 4 2021, 4:46 AM

Change 684668 merged by jenkins-bot:

[mediawiki/core@master] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/684668

Change 693543 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_36] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/693543

Change 693544 had a related patch set uploaded (by Reedy; author: Tim Starling):

[mediawiki/core@REL1_35] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/693544

Change 693544 merged by jenkins-bot:

[mediawiki/core@REL1_35] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/693544

Change 693543 merged by jenkins-bot:

[mediawiki/core@REL1_36] Add extension.json merge strategy "provide_default"

https://gerrit.wikimedia.org/r/693543