Page MenuHomePhabricator

Add interface ComponentRegistryContext
Closed, ResolvedPublic

Description

Currently components are given the entire skin. An interface ComponentRegistryContext is proposed to limit what context should be available to components.

It should adapt a Skin context.

  • Add the ComponentRegistryContext
  • Implement SkinComponentRegistryContext
  • Pass SkinComponentRegistryContext rather than Skin to SkinComponentRegistry

Developer notes

interface ComponentRegistryContext {
    public function getConfig(): Config;
   function getTitle(): Title;
}
class SkinComponentRegistryContext implements ComponentRegistryContext {
  private Skin $skin;

  public function __construct( Skin $skin ) {
    $this->skin = $skin;
  }
  public function getConfig() {
}

Event Timeline

Some thoughts (I'll look more in-depth tomorrow):

  • Implementing MessageLocalizer is almost certainly needed
  • Implementing getUser is likely going to be needed by some use cases; using UserIdentity (or what have you) would be a good idea to prevent tying us down to User
  • Offering access to IContext would allow uncoupling the connection with Skin, as it moves the responsibility away to the request context, but still allow access to context that may not be offered by ComponentRegistryContext (like the skin, for those rare cases that might actually need it)

I've taken a look at MediaWiki-skins-Mirage to see what might be needed further.

I was correct in my initial assessment; offering access to IContextSource through a getter would make things easier without direct coupling; otherwise, you'll end up implementing practically the whole interface.

Change 767583 had a related patch set uploaded (by Clare Ming; author: Clare Ming):

[mediawiki/core@master] Introduce interface ComponentRegistryContext

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

Change 767583 merged by jenkins-bot:

[mediawiki/core@master] Introduce interface ComponentRegistryContext

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

@Mainframe98 do you think this is good enough for now to resolve this?

I'm interested in hearing what the use cases are for components knowing about the skin it's running in (my feeling is that data should be skin-neutral) and what else could be accessed via IContextSource, but perhaps that could be the subject of a follow-up ticket, as we build out the component system further (in particular adding the ability for skins and extensions to add new components). Does that work for you?

I'm missing a method msg. Access to the message localizer is great, but calling $this->msg() is a staple pattern in MediaWiki. Otherwise, the only case (for MediaWiki-skins-Mirage, anyway) where access to a Skin instance is necessary is to call makeListItem on it. That isn't specific to the skin it is called on, however, so this is good enough as far as I'm concerned.

I'm missing a method msg. Access to the message localizer is great, but calling $this->msg() is a staple pattern in MediaWiki.

Have you got some code that shows how you'd like to use this? The SkinComponentRegistryContext is expected to be internal. On the component side, SkinComponentSearch is currently our only component that uses localization, but it creates a msg function wrapping the localizer. I imagine in future it may be useful have a SkinComponentLocalizable to setup a lot of this.

where access to a Skin instance is necessary is to call makeListItem on it. That isn't specific to the skin it is called on, however, so this is good enough as far as I'm concerned.

We're planning to componentize the menus next. https://phabricator.wikimedia.org/T302116 One of the implications of this I think is that the logic for makeListItem will be moved into SkinComponentMenuItem and will hopefully be suffiiciently abstracted in futute in such a way that skins can simply define menus in JSON without having to call makeListItem. General feedback is welcome on T272624 !

, however, so this is good enough as far as I'm concerned.

Closing!

I'm missing a method msg. Access to the message localizer is great, but calling $this->msg() is a staple pattern in MediaWiki.

Have you got some code that shows how you'd like to use this? The SkinComponentRegistryContext is expected to be internal. On the component side, SkinComponentSearch is currently our only component that uses localization, but it creates a msg function wrapping the localizer. I imagine in future it may be useful have a SkinComponentLocalizable to setup a lot of this.

I understand this as a substitution for existing code in skins, if it is internal to MediaWiki then the point is moot. In that case, it can be left to the future.