Page MenuHomePhabricator

Only use GrowthExperimentsServices::getWikiConfig to request configuration variables registered in community configuration
Closed, ResolvedPublic

Description

While working on T384181: Remove the Argentina/Chile/Mexico campaign-related code, I discovered we use GrowthExperimentsServices::getWikiConfig to request variables that cannot be in community configuration, because they are not present in any of the schemas we registered. For example:

$growthServices = GrowthExperimentsServices::wrap( $services );
return new CampaignConfig(
	$growthServices->getGrowthWikiConfig()->get( 'GECampaigns' ) ?? [],
	$growthServices->getGrowthWikiConfig()->get( 'GECampaignTopics' ) ?? [],
	$services->getUserOptionsLookup()
);

with the CommunityConfiguration extension will always load GECampaigns and GECampaignTopics from the server configuration, and never from community configuration. This is because neither variable is registered in any community configuration schema, so MediaWikiConfigReader from CommunityConfiguration would never know what provider to contact:

urbanecm@wmf3345 GrowthExperiments % git grep GECampaigns
ServiceWiring.php:                      $growthServices->getGrowthWikiConfig()->get( 'GECampaigns' ) ?? [],
extension.json:         "GECampaigns": {
includes/Config/GrowthExperimentsMultiConfig.php:               'GECampaigns',
includes/Config/GrowthExperimentsMultiConfig.php:               'GECampaigns' => 'array_merge',
includes/Config/Validation/GrowthConfigValidation.php:                  'GECampaigns' => [
includes/NewcomerTasks/CampaignConfig.php: * Wrapper for the GECampaigns PHP / community configuration variable, used to retrieve
includes/NewcomerTasks/CampaignConfig.php:       * @return string|null The campaign name, which is the array key used in $wgGECampaigns.
tests/phpunit/unit/Config/GrowthExperimentsMultiConfigTest.php:                 'wiki only' => [ 'GECampaigns', null, [ 'foo' => 1 ], [ 'foo' => 1 ] ],
tests/phpunit/unit/Config/GrowthExperimentsMultiConfigTest.php:                 'global only' => [ 'GECampaigns', [ 'foo' => 1 ], null, [ 'foo' => 1 ] ],
tests/phpunit/unit/Config/GrowthExperimentsMultiConfigTest.php:                 'aray_merge' => [ 'GECampaigns', [ 'foo' => 1, 'bar' => 2 ], [ 'foo' => 2, 'baz' => 3 ],
urbanecm@wmf3345 GrowthExperiments % git grep GECampaignTopics
ServiceWiring.php:                      $growthServices->getGrowthWikiConfig()->get( 'GECampaignTopics' ) ?? [],
extension.json:                 "description": "A map of campaign ID to campaign configuration. Campaign configuration currently includes these fields:\n* pattern: a regexp matched against the \"campaign\" request parameter during signup to determine whether the user should be included in some campaign\n* signupPageTemplate: a template name to use for the \"benefits\" block of Special:CreateAccount\n* signupPageTemplateParameters: a map of parameters to pass to the template\n* topics: an array of topic IDs (which are defined in GECampaignTopics) to include on the top of the topic selector as custom topics\n\nExamples can be found at https://www.mediawiki.org/wiki/Extension:GrowthExperiments/Technical_documentation/Special:EditGrowthConfig.",
extension.json:         "GECampaignTopics": {
includes/Config/GrowthExperimentsMultiConfig.php:               'GECampaignTopics',
includes/Config/Validation/GrowthConfigValidation.php:                  'GECampaignTopics' => [
urbanecm@wmf3345 GrowthExperiments %

Using growthWikiConfig is significantly less efficient, so we should aim not doing that whenever possible.

In addition to this, MediaWikiConfigReader should probably complain if it is used to request a variable that is not registered.

Event Timeline

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

Change #1112688 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] MediaWikiConfigReader: Log when falling back

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

Change #1112689 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] Do not load GECampaigns and GECampaignTopics from the wiki config

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

Change #1112688 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] MediaWikiConfigReader: Log when falling back

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

Change #1112689 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Do not load GECampaigns and GECampaignTopics from the wiki config

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

Okay, we're going to need to do something regarding this https://gerrit.wikimedia.org/r/1112688... From testwiki:

2025-01-23 11:52:56.970489 [4d219398-8cdf-4161-af21-80c2dc2c8ef9] mw-web.eqiad.main-758f5ddf7f-wfhm2 testwiki 1.44.0-wmf.13 CommunityConfiguration INFO: MediaWiki\Extension\CommunityConfiguration\Access\MediaWikiConfigReader was unable to find GEMentorshipMinimumAge in community configuration, returning configuration from the fallback config {"key":"GEMentorshipMinimumAge"} 
[Exception RuntimeException] (/srv/mediawiki/php-1.44.0-wmf.13/extensions/CommunityConfiguration/src/Access/MediaWikiConfigReader.php:120) 
  #0 /srv/mediawiki/php-1.44.0-wmf.13/extensions/CommunityConfiguration/src/Access/MediaWikiConfigReader.php(131): MediaWiki\Extension\CommunityConfiguration\Access\MediaWikiConfigReader->getConfigByVariableName(string)
  #1 /srv/mediawiki/php-1.44.0-wmf.13/extensions/GrowthExperiments/includes/Config/MediaWikiConfigReaderWrapper.php(46): MediaWiki\Extension\CommunityConfiguration\Access\MediaWikiConfigReader->get(string)
  #2 /srv/mediawiki/php-1.44.0-wmf.13/extensions/GrowthExperiments/includes/Mentorship/Hooks/MentorHooks.php(297): GrowthExperiments\Config\MediaWikiConfigReaderWrapper->get(string)
  #3 /srv/mediawiki/php-1.44.0-wmf.13/includes/HookContainer/HookContainer.php(155): GrowthExperiments\Mentorship\Hooks\MentorHooks->onUserGetRights(MediaWiki\User\User, array)
  #4 /srv/mediawiki/php-1.44.0-wmf.13/includes/HookContainer/HookRunner.php(4348): MediaWiki\HookContainer\HookContainer->run(string, array)
  #5 /srv/mediawiki/php-1.44.0-wmf.13/includes/Permissions/PermissionManager.php(1551): MediaWiki\HookContainer\HookRunner->onUserGetRights(MediaWiki\User\User, array)
  #6 /srv/mediawiki/php-1.44.0-wmf.13/includes/Permissions/PermissionManager.php(1499): MediaWiki\Permissions\PermissionManager->getUserPermissions(MediaWiki\User\User)
  #7 /srv/mediawiki/php-1.44.0-wmf.13/includes/Permissions/PermissionManager.php(1066): MediaWiki\Permissions\PermissionManager->userHasRight(MediaWiki\User\User, string)
  #8 /srv/mediawiki/php-1.44.0-wmf.13/includes/Permissions/PermissionManager.php(516): MediaWiki\Permissions\PermissionManager->checkQuickPermissions(string, MediaWiki\User\User, MediaWiki\Permissions\PermissionStatus, string, bool, MediaWiki\Title\Title)

GEMentorshipMinimumAge is definitely supposed to be in community configuration?

Urbanecm_WMF changed the task status from Open to Stalled.Feb 4 2025, 8:26 PM
Urbanecm_WMF added a subscriber: Sgs.

Following a discussion of this with @Sgs, I'm rethinking my original suggestion. Routing all config reads through the MediaWikiConfigReader class makes more sense in the long term, as it makes it easier to add (or remove) variables from Community Configuration. Instead of merely adding the new variable to a schema (plus doing a migration), one also needs to go through all the usages, which can be time consuming. With that in thought, it might be more appropriate to rename`MediaWikiConfigReader` to MediaWikiConfigRouter, as it is essentially routing incoming requests (to an appropriate CC provider, or to MainConfig, depending on what the schemas say). MediaWikiConfigReader should return control to MainConfig fairly quickly (immediately assuming the cache is warm), so routing everything (including potentially more sensitive lookups) through it doesn't seem hurtful. During the discussion with @Sgs, we were unable to come up with any positives of keeping both config objects separate. Of course, it is possible for extensions to do that if they have a reason, but we probably don't have such a reason.

I recognise this comment is a 180° rotation on my end. Because of that, I'm marking this as stalled and starting a discussion among all Growth engineers, so that we can come to a final solution on this.

With that in thought, it might be more appropriate to rename`MediaWikiConfigReader` to MediaWikiConfigRouter, as it is essentially routing incoming requests (to an appropriate CC provider, or to MainConfig, depending on what the schemas say).

I wonder why one should replace the other, wouldn't it make sense to have both? That is, a MediaWikiConfigRouter that has access to both a MediaWikiConfigReader and a MediaWikiServices::getInstance()->getMainConfig(). If the MediaWikiConfigReader has a config, then it returns the config from there and from MediaWikiServices::getInstance()->getMainConfig() otherwise.

What do you think?

That makes sense to me on the idea level. However, I'm unsure how to separate routing and reading. Would MediaWikiConfigRouter catch the exception from MediaWikiConfigReader? I'm not sure if that would be effective or not.

Looking through logstash for the last 7 days, I see the following 5 configs showing up with the message from MediaWikiConfigReader: Log when falling back:

BabelAllowOverride
BabelCentralDb
BabelDefaultLevel
GEPersonalizedPraiseMaxReverts
CommunityConfigurationCommonsApiURL

Michael claimed this task.

We added a generic way to read config from wherever possible in T387452: CommunityConfiguration should provide a generic configuration reading object. And in refactor: throw exception if setting does not exist in community config we removed the fallback mechanism that this ticket here is about. It now throws an exception to prevent reintroduction.