Page MenuHomePhabricator

Community configuration: Add tests finding invalid configuration variables used
Open, MediumPublic

Description

In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/1017256, @Sgs accidentally did not declare JSON schema defaults for the newly-added Help panel schema (T360472). This resulted in an ugly error:

MediaWiki internal error.

Original exception: [7a2bd9f7260ee9b7668ef1ce] /wiki/Special:CommunityConfiguration/HelpPanel MediaWiki\Config\ConfigException: Key GEHelpPanelExcludedNamespaces was not found.
Backtrace:
from /var/www/html/w/extensions/CommunityConfiguration/src/Provider/WikiPageConfigProvider.php(38)
#0 /var/www/html/w/extensions/CommunityConfiguration/src/Access/WikiPageConfigReader.php(128): MediaWiki\Extension\CommunityConfiguration\Provider\WikiPageConfigProvider->get()
#1 /var/www/html/w/extensions/GrowthExperiments/includes/HelpPanel.php(131): MediaWiki\Extension\CommunityConfiguration\Access\WikiPageConfigReader->get()
#2 /var/www/html/w/extensions/GrowthExperiments/includes/HelpPanelHooks.php(159): GrowthExperiments\HelpPanel::shouldShowHelpPanel()
#3 /var/www/html/w/includes/HookContainer/HookContainer.php(159): GrowthExperiments\HelpPanelHooks->onBeforePageDisplay()
#4 /var/www/html/w/includes/HookContainer/HookRunner.php(945): MediaWiki\HookContainer\HookContainer->run()
#5 /var/www/html/w/includes/Output/OutputPage.php(2998): MediaWiki\HookContainer\HookRunner->onBeforePageDisplay()
#6 /var/www/html/w/includes/actions/ActionEntryPoint.php(162): MediaWiki\Output\OutputPage->output()
#7 /var/www/html/w/includes/MediaWikiEntryPoint.php(199): MediaWiki\Actions\ActionEntryPoint->execute()
#8 /var/www/html/w/index.php(58): MediaWiki\MediaWikiEntryPoint->run()
#9 {main}

This error was even uglier, as it only appeared following clearing wiki's internal caches (without that, it appeared to work correctly). Such a error should be definitely caught by (some) testcase – it is questionable whether it should belong to GrowthExperiments or MediaWiki-extensions-CommunityConfiguration (tagging with both momentarily), but the issue should not rely on manual detection, as it is the equivalent of taking the site fully down. Without providing such test for this, this has the potential of biting us in the future.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Looking at the stacktrace, I see that in HelpPanelHooks.php(159): GrowthExperiments\HelpPanel::shouldShowHelpPanel() we use $this->config->get( 'GEHelpPanelNewAccountEnableWithHomepage' ) without checking for ->has(...) first.

I could see a few possible ways forward here:

  • make it part of our explicit "develop with CC2.0"-guidance to never assume config exists and to always check explicitly
  • make it part of our explicit "develop with CC2.0"-guidance to always test their code (also) with CC2.0 enabled, but no config set (thus using the defaults/no-value)
  • added in edit: throw an exception if a schema is missing a default for a top-level field
  • make it so that WikiPageConfigReader::get() falls back the value in its fallback config if the CC2.0 has the config key defined, but not set
    • (and make that The Correct Way to use CC2.0 for config. (do we have a CC2.0-equivalent to "pythonic"?))

There are probably other good ideas, this is just the ones that came to mind for me when reading the task.