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.

In T246377#5926491, @Anomie wrote:

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).

Disconnecting this from the parent.

In T246377#5926903, @daniel wrote:

In T246377#5926491, @Anomie wrote:

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?

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

In T246377#6539635, @DannyS712 wrote:

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

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.

In T246377#6626082, @Pchelolo wrote:

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.

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

up for code review now, dropping from the tech planning meeting inbox.

Change 676348 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/libs/ObjectFactory@master] Move under new Wikimedia\ObjectFactory namespace

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

Change 676348 merged by jenkins-bot:

[mediawiki/libs/ObjectFactory@master] Move under new Wikimedia\ObjectFactory namespace

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

Hi All! Is this resolved?

In T246377#7095646, @AMooney wrote:

Hi All! Is this resolved?

No - we moved the code to a new namespace, but the patch to actually add the config injection is still pending (and I need to revisit it, from what I recall the ball is in my court)

Change 642445 had a related patch set uploaded (by Daniel Kinzler; author: DannyS712):

[mediawiki/libs/ObjectFactory@master] Allow injecting configuration values

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

Moving back to "code review in progress" after Petr found the relevant patch.

Sorry about that, I guess I accidentally unlinked this task at one point. If I recall there were some issues with the patch that I'll try to find time to work on

In T246377#7138005, @DannyS712 wrote:

Sorry about that, I guess I accidentally unlinked this task at one point. If I recall there were some issues with the patch that I'll try to find time to work on

I think it was just a typo :)

Anyway - it's really a question of how complex we want to make this. You proposal isn't bad at all, it just seems a lot of infrastructure for relatively little benefit. The question to me is: can we do this more simply? Maybe a callback is all we need?

If I was to re-design this from scratch, I'd probably just go for symbolic names (references) in parameters, using some kind of magic prefix, perhaps like yaml tags: ObjectFactory only supports the syntax, MW registers handlers for prefixes, e.g. '!service!NamespaceUtils' resolves to the NamespaceUtils service object, '!config!ExtraNamespaces' resolves to the ExtraNamespaces config setting, etc.

But that's a big change, and not easy to transition to. And you'd need a way to override or escape the prefixes... Anyway, just an idea for the future, probaly not what we want/need right now.

For now, injecting the MainConfig works OK, as long as the constructor only uses it to get config variables, and doesn't hold on to the Config object itself.

Change 769585 had a related patch set uploaded (by Reedy; author: DannyS712):

[mediawiki/libs/ObjectFactory@2.x] Move under new Wikimedia\ObjectFactory namespace

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

Change 769585 merged by jenkins-bot:

[mediawiki/libs/ObjectFactory@2.x] Move under new Wikimedia\ObjectFactory namespace

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