Page MenuHomePhabricator

The SiteConfiguration class (wgConf) should not skip extraction if default is specified as null
Open, MediumPublic

Description

Example 1
$wgConf->settings = [
  
  'xSomething' => [
    'default' => false,
    'testwiki' => true,
  ]
];

What should happen on wikis that are not testwiki? It creates $xSomething with the value false, which seems expected.

Example 2
$wgConf->settings = [
  
  'xSomething' => [
    'testwiki' => true,
  ]
];

What about this one? As implicit null? Or not exist at all? Not exporting a variable in that case would allow site administrators to conditionally create a variable (for variables that are custom to the site administration and not part of the software). It would also allow conditionally overriding the software default (which is separate from the wikifarm default), and might be used as a way to avoid repeating the default in the config repository.

In any event, there isn't an obvious outcome here. If the above theoretical use cases are real, we might want to support it. Otherwise, it might be best to disallow it entirely, or to warn for it.

Lastly, the reason this task exists:

Example 3
$wgConf->settings = [
  
  'xSomething' => [
    'default' => null,
    'testwiki' => true,
  ]
];

Surely this should create $xSomething with the value null? However, it appears this currently results in the variable not being created at all - which led to the outage referenced in T225212 and documented.

Perhaps there was a reason for behaviour existing, and we may need to support it still. Or perhaps it is an accidental side-effect of isset() being used somewhere instead of array_key_exists.

Event Timeline

I think SiteConfiguration::getSetting() uses null to mean that no setting of that name exists (though it’s not mentioned in the documentation comments), that’s probably why getAll() and extractGlobalSetting() check is_null( $value ). If we still want to allow callers to distinguish between “setting is null” and “setting is missing”, then perhaps a different signature might be necessary:

protected function getSetting( string $settingName, string $wiki, array $params, &$retval ): bool

That is, “return” the value via reference and actually return a bool indicating whether it was present or not. (Yes, I know references aren’t nice, but in this case I think they’re better than a special “sentinel” return value.)

This reminds me of T192855: Remex enabled on all wikis in MW 1.30-wmf.30 exposing corruption (signatures coloring unrelated follow-up sections, etc.) on unfixed content, related IRC discussion in https://wm-bot.wmflabs.org/logs/%23mediawiki-core/20180424.txt, and the workaround in rOMWCe674799cb346: Fix wgTidyConfig expansion to not ignore `null` as value.

Regarding Example 2, that should leave other wikis at the default (from DefaultSettings.php or the like). We've probably used that in the past, and we may even be using it now (I haven't checked). In this case I guess the default would be "doesn't exist".

In Example 3 the same thing is happening with the explicit null. There may be a use case for being able to reset a wiki to the DefaultSettings.php default (in a sense the opposite of Example 3) without having to copy that default. On the other hand, it might be useful to figure out a sentinel for that other than null.

Alternative signature suggestion:

protected function getSetting( string $settingName, string $wiki, array $params, $default = null ): mixed

A caller that wants to apply a default if the key is not set, but wants to use null if the key is set to null, can simply pass the default as the last parameter. Or, if the default is not known to the caller, "sentinel" value logic can still be used at the caller's discretion.

WDoranWMF triaged this task as Medium priority.Jun 25 2019, 12:54 PM
WDoranWMF added subscribers: tstarling, WDoranWMF.

@tstarling Would you have time to review this and consider what the expected behaviours should be given @Krinkle's examples? @daniel said he would be OK making the change but would like input.

Aklapper changed the task status from Stalled to Open.Oct 19 2020, 4:31 PM

The previous comments don't explain who or what (task?) exactly this task is stalled on ("If a report is waiting for further input (e.g. from its reporter or a third party) and can currently not be acted on"). Hence resetting task status.

(Smallprint, as general orientation for task management: If you wanted to express that nobody is currently working on this task, then the assignee should be removed and/or priority could be lowered instead. If work on this task is blocked by another task, then that other task should be added via Edit Related Tasks...Edit Subtasks. If this task is stalled on an upstream project, then the Upstream tag should be added. If this task requires info from the task reporter, then there should be instructions which info is needed. If this task needs retesting, then the TestMe tag should be added. If this task is either out of scope and nobody should ever work on this, or nobody else managed to reproduce the problem described in this task, then this task should have the "Declined" status. If the task is valid but should not appear on some team's workboard, then the team project tag should be removed while the task has another active project tag.)