Page MenuHomePhabricator

Allow extensions to register handlers with factories without the use of global variables
Closed, DeclinedPublic

Description

Problem:
Currently, extensions use global variables to register handler classes (or object specs or instantiators) to define ContentHandlers, SpecialPages, APIModules, etc.

Proposal:
Make ExtensionRegistry a service. Give it a getHanderSpecs( $type ) name. In ServiceWiring, use a call like e.g. $extensionRegistry->getHandlerSpecs( 'SpecialPages' ) when constructing SpecialPageFactory. Internally, ExtensionRegistry can map the $type parameter to global variables, e.g. 'SpecialPages' to $wgSpecialPages. In future versions, extension.json can support a 'handlers' key that would bypass the global variables.

This removes the need for global variables, and avoids the need for factory services to offer methods for registering handler specs at runtime.

Alternative:
SlotRoleRegistry already allows extensions to register handlers by calling defineRole, but this is cumbersome since it requires the extension to use the MediaWikiServices hook to then register a service manipulator callback.

Event Timeline

daniel created this task.Jan 20 2020, 2:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 20 2020, 2:34 PM
Legoktm edited subscribers, added: Legoktm; removed: Kunal.Jan 21 2020, 3:33 AM

Make ExtensionRegistry a service.

Last time I tried doing this (never posted to Gerrit) there was some bootstrapping problem where the ExtensionRegistry is needed right away in LocalSettings.php, but that's too early to invoke MediaWikiServices. See rMW69aecc2eeae0: Don't initialize MediaWikiServices before extensions have been loaded as well.

In ServiceWiring, use a call like e.g. $extensioneregistry->getHandlerSpecs( 'SpecialPages' ) when constructing SpecialPageFactory. Internally, ExtensionRegistry can map the $type parameter to global variables, e.g. 'SpecialPages' to $wgSpecialPages. In future versions, extension.json can support a 'handlers' key that would bypass the global variables.

I think this is unnecessary, the existing attributes infrastructure should be able to handle it fine. We would convert SpecialPages to be an attribute internally, and whatever needs that calls $registry->getAttribute( 'SpecialPages' );. The only reason this hasn't been done yet is to avoid breaking cases where people are inspecting the $wgSpecialPages global (rare, but it does happen).

daniel updated the task description. (Show Details)Jan 21 2020, 11:34 AM
daniel added a project: TechCom.

Tagging TechCom for discussion, since this seems like a cross-cutting concern.

Is there very much difference between global configuration arrays and configuration arrays wrapped in a global service?

I think this is unnecessary, the existing attributes infrastructure should be able to handle it fine. We would convert SpecialPages to be an attribute internally, and whatever needs that calls $registry->getAttribute( 'SpecialPages' );. The only reason this hasn't been done yet is to avoid breaking cases where people are inspecting the $wgSpecialPages global (rare, but it does happen).

For handling extension addition of things like special pages, this sounds like a better path to me too.

We may still want to keep the configuration "global"[1] though, to allow for overriding things in LocalSettings.php (e.g. 359e528d29).

[1]: Scare quotes because it's already supposed to be abstracted behind Config, which doesn't necessarily have to be GlobalVarConfig.

Catrope updated the task description. (Show Details)Jan 21 2020, 9:18 PM

Tagging TechCom for discussion, since this seems like a cross-cutting concern.

This step seems premature.

This step seems premature.

Note that I didn't tag this as an RFC. That would indeed be premature.

Catrope moved this task from Inbox to In progress on the TechCom board.Jan 22 2020, 9:35 PM
daniel added a comment.EditedJan 23 2020, 10:12 PM

Last time I tried doing this (never posted to Gerrit) there was some bootstrapping problem where the ExtensionRegistry is needed right away in LocalSettings.php, but that's too early to invoke MediaWikiServices. See rMW69aecc2eeae0: Don't initialize MediaWikiServices before extensions have been loaded as well.

We may need to split things up more to make this work. Code called from LocalSettings can't be a service. It would need to stash information in global state. But that could be a private static field in a class, that stashes the list of extensions.

The code that actually applies the extension registration can be a service, I think. Currently, this is a "push" process, by which information from extension.json is put into global arrays and such. My idea is to make this a "pull" operation, where the appropriate wiring code asks the extension registration for the neccessary info.

I think this is unnecessary, the existing attributes infrastructure should be able to handle it fine. We would convert SpecialPages to be an attribute internally, and whatever needs that calls $registry->getAttribute( 'SpecialPages' );.

For backwards compatibility, we'll have to decide at which point infortmation added programmatically to the then-deprecated $wgSpecialPages would be merged in, and what code would be responsible for this.

The only reason this hasn't been done yet is to avoid breaking cases where people are inspecting the $wgSpecialPages global (rare, but it does happen).

We can still, for now, export the information from ExtensionRegistry to $wgSpecialPages. SpecialPageFactory doesn't have to use it.

I'd try to fix any code that directly interacts with globals, though. Removing the need for global variables is indeed the motivation for this patch.

daniel added a comment.EditedJan 23 2020, 10:23 PM

Is there very much difference between global configuration arrays and configuration arrays wrapped in a global service?

The global service can easily be reset for testing.

We could also have two different instances of that serve, allowing us to (potentially, eventually) to construct services that act on different wikis, with the extensions of the target wiki enabled. The global instance is just the default, not the only possible instance.

I think this is unnecessary, the existing attributes infrastructure should be able to handle it fine. We would convert SpecialPages to be an attribute internally, and whatever needs that calls $registry->getAttribute( 'SpecialPages' );.

For handling extension addition of things like special pages, this sounds like a better path to me too.

As long as the information is managed inside the registry and not via global state, I don't care too much about the details.

We may still want to keep the configuration "global"[1] though, to allow for overriding things in LocalSettings.php (e.g. 359e528d29).

But this isn't configuration. It's wiring. It's similar, but not the same.

How about allowing an inline extension.json:

wfLoadExtensionSpec( {
  'name' => 'Dummy',
  'APIModules' => [
    'imagerotate' => 'ApiDisabled'
  ]
});

Nice and consistent, and no need for globals.

Anomie added a comment.EditedJan 24 2020, 3:32 PM

Is there very much difference between global configuration arrays and configuration arrays wrapped in a global service?

The global service can easily be reset for testing.

A global configuration value can also easily be reset for testing. Possibly more easily.

We could also have two different instances of that serve, allowing us to (potentially, eventually) to construct services that act on different wikis, with the extensions of the target wiki enabled. The global instance is just the default, not the only possible instance.

We could accomplish the same by replacing GlobalVarConfig with something else for one or both of the multiple instances.

And being able to actually do this seems a very long way off and seems likely to involve other more fundamental bootstrapping issues.

But this isn't configuration. It's wiring. It's similar, but not the same.

As far as I can tell, either

  • Wiring is configuration, or
  • "Wiring" is the process of turning configuration into live data structures. And in that case what we're talking about here isn't actually "wiring", it's the configuration that "wiring" transforms.

How about allowing an inline extension.json:

wfLoadExtensionSpec( {
  'name' => 'Dummy',
  'APIModules' => [
    'imagerotate' => 'ApiDisabled'
  ]
});

Nice and consistent,

Seems like a false consistency to me. You're wrapping what's needed in unnecessary boilerplate and introducing implicit ordering issues.

Providing a function in LocalSettings.php to supply values for ER attributes may not be a bad way to do things, but let's skip the extra boilerplate and do it so that attributes set via this method always override attributes from extension.json regardless of how ER might order processing of multiple extensions.

and no need for globals.

As I mentioned earlier, you still have globals, they're just hidden as ExtensionRegistry::$instance->queued[???][...] instead of being in $GLOBALS.

@daniel, have you already reviewed the "attributes" functionality of extension.json and found it to be insufficient? Either I'm not understanding what you're looking for, or you're describing functionality that is already implemented.

We may still want to keep the configuration "global"[1] though, to allow for overriding things in LocalSettings.php (e.g. 359e528d29).

But this isn't configuration. It's wiring. It's similar, but not the same.

It's both configuration and wiring. Most uses of it are wiring in that it maps special page name to class name.

However, $wgSpecialPages['SpecialFoo'] = 'SpecialDisabled'; is *configuration* albeit through wiring.

if we are intent on splitting the two, we'd need to introduce some $wgDisabledSpecialPages['Foo'] = true;, that then internally wires to the \SpecialDisabled class.

How about allowing an inline extension.json:

wfLoadExtensionSpec( {
  'name' => 'Dummy',
  'APIModules' => [
    'imagerotate' => 'ApiDisabled'
  ]
});

Nice and consistent, and no need for globals.

No, we're very much not doing this.

Site configuration should imho be statically readable and loadable from disk. The abstraction of the Config classes, and the move from extension default settings from PHP to JSON both set us on that path and gave it momentum.

For WMF production, as example, settings overrides are also slowly becoming more machine readable in a structued JSON (or YAML) format. Where the only conditional code is the wiki ID. Something like config.DisabledSpecialPages or config.SpecialPages can be easily declared in such format.

Executable code like wfLoadExtensionSpec( … ) that injects it at run-time is essentially not possible to declare in such format.

I think an easy way to think about this is to consider the current wiki and its configuration as being pure and injectable as-is. It doesn't warrant further decoupling or injecting within itself. The service wiring gives us an injected Conf object for overrideable parts, with the defaults coming from extension attributes. Merging these seems like a good approach going forward and allows both to be read at run-time (ExtensionRegistry for the full set of available/provided values, and Config layer for any site-specific overrides. We could potentially limit this to only being able to disable things to avoid any edge cases from slipping in).

+1 to wgDisabledSpecialPages.

Krinkle renamed this task from Allow extensions to register handlers with factories without the use of global variables. to Allow extensions to register handlers with factories without the use of global variables.Jan 29 2020, 4:58 PM
Krinkle triaged this task as Medium priority.
Tgr added a subscriber: Tgr.Feb 10 2020, 7:53 AM
Legoktm closed this task as Declined.Jul 24 2020, 11:18 PM

@daniel, have you already reviewed the "attributes" functionality of extension.json and found it to be insufficient? Either I'm not understanding what you're looking for, or you're describing functionality that is already implemented.

As far as I'm aware the requested functionality is already available. Please re-open if that's not the case.

Aklapper removed a subscriber: Anomie.Fri, Oct 16, 5:28 PM