Page MenuHomePhabricator

Reconsider usage of ServiceOptions
Open, Needs TriagePublic

Description

I'm catching up on MediaWiki core changes, and saw the introduction of the ServiceOptions class, which I think is alright, but could be better. I believe I read through all of the discussion on the Gerrit change, but it's possible I missed something.

  • I think it's awkward that MainConfig gets passed to the ServiceOptions constructor. I think this is backwards in that we don't want ServiceOptions and the service itself to know of all the options.
  • ServiceOptions implements basically the \Config interface, except it doesn't.
  • As explained on T224165, tests have to go through $this->overrideMwServices( new HashConfig( [ 'Sitename' => 'TestMediaWiki' ] ) ); instead of passing the Config object directly.
  • The key list is needed twice, in the builder and in the constructor. I don't have any suggestions on how to fix this though.

My proposal:

Add a Config::withSubset( Class::CONSTUCTOR_OPTIONS ) that returns a Config implementation (either HashConfig or a new FrozenConfig, which is Hash without the mutability) with only the necessary keys. This allows us to continue relying on the Config interface, making it convenient for adhoc usage and tests. Also, the class is only ever aware of the config options it wants, not extra things.

I note that ServiceOptions allows adding from an arbitrary number of Config classes/arrays, it wasn't clear to me though exactly how important that use case was.

Event Timeline

Legoktm created this task.Jul 1 2020, 5:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 1 2020, 5:36 PM

Change 614722 had a related patch set uploaded (by Ostrzyciel; owner: Ostrzyciel):
[mediawiki/core@master] ServiceOptions: implement Config interface

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

  • ServiceOptions implements basically the \Config interface, except it doesn't.

I've accidentally stumbled upon a situation where this actually matters, see this patch for more info. Implementing \Config would allow for sensible passing of options for subservices.

On that note, I also thought about making the system a little bit more strongly coupled, without relying on coding conventions. Instead of each class that uses ServiceOptions having a CONSTRUCTOR_OPTIONS constant, such class would implement an interface that would provide a static method getConstructorOptions(). This way the builder wouldn't have to know anything about service's constants, it'd just pass the class name to ServiceOptions (or something) and the rest would be done there.

  • The key list is needed twice, in the builder and in the constructor. I don't have any suggestions on how to fix this though.

That could probably be fixed with generics, if PHP was a sensible language. Something like:

class ServiceOptions<T> where T implements ServiceOptionListProvider {
  ...
}

class SomeService implements ServiceOptionListProvider {
  public static function getConstructorOptions() { ... }

  public function __construct( ServiceOptions<SomeService> $options ) { ... }
}

new SomeService( new ServiceOptions<SomeService>( $config /* or whatever */ ) );

Idk, maybe someone will find the idea interesting. Or maybe PHP will get generics some day. Or maybe it could be done with Phan annotations.

Change 614722 abandoned by Ostrzyciel:
[mediawiki/core@master] ServiceOptions: implement Config interface

Reason:
This was indeed quite misguided. I'll try a different approach.

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