Page MenuHomePhabricator

ObjectFactory needs a way to inject configuration settings into objects.
Open, MediumPublic5 Estimated Story Points

Description

When constructing an object via ObjectFactory based on an object spec, we can currently pass in literal values, and specify services. We can't however specify configuration variables. That would be needed to allow e.g. REST route handlers to depend on configuration.

The alternative is to request the MainConfig instance to be injected. This seems undesirable, because it constitutes a dependency on all configuration, where only specific configuration is needed/desired.

The mechanism for referencing config variables could be similar to the mechanism used to reference service instances.

Context: this came up when trying to make the cache duration for T245675 configurable.

Event Timeline

daniel created this task.Feb 27 2020, 8:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 27 2020, 8:42 PM
Anomie added a subscriber: Anomie.Feb 28 2020, 2:26 PM

Note that ObjectFactory is not MediaWiki-specific, it's an external library that works with any PSR-11 container interface. Any mechanism for injecting configuration should not assume MediaWiki's Config interface or ServiceOptions class (OTOH, we could librarize those and then assume them) and certainly shouldn't assume the config service is named "MainConfig".

Also of note is that MediaWiki for some reason has separate Config objects for core configuration versus extension configuration for some reason. An object being instantiated from an extension might need settings from both of these.

Note that ObjectFactory is not MediaWiki-specific, it's an external library that works with any PSR-11 container interface. Any mechanism for injecting configuration should not assume MediaWiki's Config interface or ServiceOptions class (OTOH, we could librarize those and then assume them) and certainly shouldn't assume the config service is named "MainConfig".

Yes, the ObjectFactory library would specify an interface for config access, and MediaWiki would implement an adapter that maps that interface to Config (or ConfigFactory, see below).

Also of note is that MediaWiki for some reason has separate Config objects for core configuration versus extension configuration for some reason.

The intent was to not have all settings in a single namespace, allowing extensions to have their settings in a single array, or a separate file, or a database table, or whatever. But few extensions use this option.

An object being instantiated from an extension might need settings from both of these.

Could be done by providing access to one more level of indirection. i.e. ConfigFactory (via some adapter).

WDoranWMF set the point value for this task to 5.Feb 28 2020, 5:07 PM
eprodromou added a subscriber: eprodromou.

Disconnecting this from the parent.

DannyS712 updated the task description. (Show Details)Oct 7 2020, 10:33 PM
DannyS712 added a comment.EditedOct 7 2020, 11:09 PM

Note that ObjectFactory is not MediaWiki-specific, it's an external library that works with any PSR-11 container interface. Any mechanism for injecting configuration should not assume MediaWiki's Config interface or ServiceOptions class (OTOH, we could librarize those and then assume them) and certainly shouldn't assume the config service is named "MainConfig".

Yes, the ObjectFactory library would specify an interface for config access, and MediaWiki would implement an adapter that maps that interface to Config (or ConfigFactory, see below).

Alternatively, can I suggest that the same ContainerInterface that is already a dependency for the object factory library be used for the config?
Per https://www.php-fig.org/psr/psr-11/, ContainerInterface needs to have get and has methods
In core, the Config interface specifies exactly those two methods

Currently, the object factory provides the extraArgs specified in the options, then the services, then the args. Perhaps, using the existing dependencies, config can be added after the services as an array of values retrieved from a specified service

$spec = [
	'class' => 'FooBar',
	'services' => [
		'ExampleService',
	],
	'config' => [
		'service' => 'MainConfig',
		'values' => [
			'ConfigurationOption',
			'AnotherConfigurationOption'
		],
	],
];

This would result in the ObjectFactory requesting the MainConfig service from the ServiceContainer, and then for each of the configuration values specified, retrieving it from that MainConfig. The service specified should implement ContainerInterface to ensure that it has a get method for the configuration (or, as suggested above, a new interface is added for that)

The resulting call would be along the lines of

$configService = $options['serviceContainer']->get( $spec['config']['service'] );
$configOptions = [];
foreach( $spec['config']['value'] as $option ) {
	$configOptions[$option] = $configService->get( $option );
}

and then the FooBar class would be created with the ExampleService as its first parameter, and an associative array is the second parameter mapping configuration options to the values.

Alternatively, just like $options['serviceContainer'] is used as the source for services, we could add $options['configContainer'] (param name tbd). This would simplify the spec:

$spec = [
	'class' => 'FooBar',
	'services' => [
		'ExampleService',
	],
	'config' => [
		'ConfigurationOption',
		'AnotherConfigurationOption'
	],
];

for the creation of instances of ObjectFactory which then use the non-static method createObject, the config source would be injected into the constructor as well. Thoughts?

Will do once platform engineering makes a decision on

  • Whether the configuration source should be specified in the spec, or in the options
  • Whether a new interface for configuration sources should be added, or ContainerInterface also used as the interface for the configuration source
Restricted Application added a project: User-DannyS712. · View Herald TranscriptOct 13 2020, 5:51 PM
DannyS712 moved this task from Unsorted to Later on the User-DannyS712 board.Oct 13 2020, 5:51 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:01 PM

Move back to inbox to reflect need for platform engineering to approve implementation:

Will do once platform engineering makes a decision on

  • Whether the configuration source should be specified in the spec, or in the options
  • Whether a new interface for configuration sources should be added, or ContainerInterface also used as the interface for the configuration source
AMooney triaged this task as Medium priority.Mon, Nov 2, 3:19 PM
AMooney moved this task from Inbox to Tech Planning Review on the Platform Engineering board.
Pchelolo added a subscriber: Pchelolo.EditedMon, Nov 16, 11:50 PM

Whether the configuration source should be specified in the spec, or in the options

The 'options' are the options of the ObjectFactory, not the object the factory is creating. The configuration source should be in the options, while the configuration properties should be in the spec.

Whether a new interface for configuration sources should be added, or ContainerInterface also used as the interface for the configuration source

On one hand, I would be rather against reusing the interfaces. What if we at some point need to change one and not the other? However, ContainerInterface is documented as a container to obtain objects and parameters , so technically both Config and ConfigSource could qualify to be ContainerInterface..

Alternatively, just like $options['serviceContainer'] is used as the source for services, we could add $options['configContainer'] (param name tbd).

This looks good to me. Maybe call it 'ConfigurationSource'? And in has to be it's own interface, ConfigurationSource. The ConfigurationSource can have the same interface as core's ServiceOptions class.

The big question is whether we want to and how would we support potential existence of multiple configs in MW/Extensions. As @daniel suggests, we could inject and adapter for ConfigFactory or simply inject and adapter for Config. I guess the latter will not work out - we do have a ConfigFactory feature, so even though it's not widely used we should support it.

So, TLDR in my opinion, the spec would looks somewhat like this:

[
	'class' => 'FooBar',
	'services' => [
		'ExampleService',
	],
        'configSource' => 'main', # main is the default
	'config' => [
		'ConfigurationOption',
		'AnotherConfigurationOption'
	],
]

We will need 2 interfaces in ObjectFactory:

interface Config {
  public function get( string $name );
  public function has( string $name );
}

interface ConfigSource {
  public function makeConfig( string $name ): Config;
}

and two adapters in core: ConfigAdapter and ConfigSourceAdapter wrapping Config and ConfigFactory respectively. I propose to have adapters instead of forcing MW entities implement the respective interfaces since it looks more flexible.

DannyS712 added a comment.EditedMon, Nov 16, 11:59 PM

Whether the configuration source should be specified in the spec, or in the options

The 'options' are the options of the ObjectFactory, not the object the factory is creating. The configuration source should be in the options, while the configuration properties should be in the spec.

Okay, source of configuration will go in the options

Whether a new interface for configuration sources should be added, or ContainerInterface also used as the interface for the configuration source

I would be rather against reusing the interfaces. What if we at some point need to change one and not the other. These entities represent very different concerns, so they should be modeled by different interfaces.

ContainerInterface is not something we can change, since it is part of PSR-11. Per https://www.php-fig.org/psr/psr-11/meta/ one of the motivations behind the PSR-11 standard was using configuration values.

Alternatively, just like $options['serviceContainer'] is used as the source for services, we could add $options['configContainer'] (param name tbd).

This looks good to me. Maybe call it 'ConfigurationSource'? And in has to be it's own interface, ConfigurationSource. The ConfigurationSource can have the same interface as core's ServiceOptions class.

I don't think we need to support the assertRequiredOptions, and just like with the recent handling of optional services, we should probably be able to handle optional configuration values, so I suggest using the same interface as core's Config interface... which has the same exact methods as ContainerInterface, just with different exceptions being thrown

The big question is whether we want to and how would we support potential existence of multiple configs in MW/Extensions. As @daniel suggests, we could inject and adapter for ConfigFactory or simply inject and adapter for Config. I guess the latter will not work out - we do have a ConfigFactory feature, so even though it's not widely used we should support it.

So, TLDR in my opinion, the spec would looks somewhat like this:

[
	'class' => 'FooBar',
	'services' => [
		'ExampleService',
	],
        'configSource' => 'main', # main is the default
	'config' => [
		'ConfigurationOption',
		'AnotherConfigurationOption'
	],
]

One of the intentions here, I believe, was to avoid needing to rely on ConfigFactory, and instead provide a single configuration source. How are the specs supposed to know the names of the different configuration sources?

We will need 2 interfaces in ObjectFactory:

interface Config {
  public function get( string $name );
  public function has( string $name );
}

This is the same as the ContainerInterface

interface ConfigSource {
  public function makeConfig( string $name ): Config;
}

and two adapters in core: ConfigAdapter and ConfigSourceAdapter wrapping Config and ConfigFactory respectively. I propose to have adapters instead of forcing MW entities implement the respective interfaces since it looks more flexible.

Pchelolo added a comment.EditedTue, Nov 17, 12:23 AM

One of the intentions here, I believe, was to avoid needing to rely on ConfigFactory, and instead provide a single configuration source. How are the specs supposed to know the names of the different configuration sources?

Different config sources can be defined by different extensions. The extension relying on it's own config source will define all the object specs that are relying on it's config source. Pretty much nothing will change - you should really fetch a config for extension in another extension. Same thing with object specs.

ContainerInterface is not something we can change, since it is part of PSR-11. Per https://www.php-fig.org/psr/psr-11/meta/ one of the motivations behind the PSR-11 standard was using configuration values.

Yeah, I wasn't proposing to change ContainerInterface, but was rather wondering what if we would want to change config or config source interface. But after reading that document I am indeed in doubt about if we could just reuse the interface. It does say "a container to obtain objects and parameters"...

Change 642445 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/libs/ObjectFactory@master] Allow injecting configuration values

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