Page MenuHomePhabricator

Introduce MessageLocalizer interface to remove the need to depend on IContextSource for creating Message objects
Closed, ResolvedPublic

Description

Code that needs to construct Message objects from message keys currently tends to rely on IContextSource::msg, or the global function wfMsg(). To avoid reliance on global state, and dragging in the kitchen sink that is IContextSource, a narrow interface called MessageLocalizer should be introduced, exposing just the msg() function from IContextSource. The IContextSource interface can then extend MessageLocalizer. Calling code that only needs the msg() method can then start using MessageLocalizer instead of IContextSource, without further refactoring being required.

This allows testing and re-use of such components without the need to construct or get an IContextSource from global state.

Event Timeline

Change 355879 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/core@master] Intorduce MessageLocalizer interface for exposing msg() method

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

WMDE-leszek triaged this task as Medium priority.
WMDE-leszek moved this task from Review to Doing on the Wikidata-Former-Sprint-Board board.

Change 357353 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/core@master] [POC] Add LanguageMessageLocalizer implementing MessageLocalizer

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

daniel renamed this task from Introduce MessageLocalizer interface to remove dependency on IContextSource in DiffView and friends. to Introduce MessageLocalizer interface to remove the need to depend on IContextSource for creating Message objects.Jun 9 2017, 4:15 PM
daniel updated the task description. (Show Details)

To avoid reliance on global state, and dragging in the kitchen sink that is IContextSource, a narrow interface called MessageLocalizer should be introduced [..]
Calling code that only needs the msg() method can then start using MessageLocalizer instead of IContextSource [..]
This allows testing and re-use of such components without the need to construct or get an IContextSource from global state.

I think this exaggerates the problem a bit. Creating a Message object requires two things: An interface language, and a title. (Defaults from global User and wgTitle)

When a class doesn't (and shouldn't) involve a context source, you construct Message directly (or wfMessage). When the class has sensible values for these two requirements (language, title), one can create a convenience wrapper by defining a local msg method.

I doubt anyone would extend ContextSource for a class just to get a msg() method. That seems rather unnecessary. I would not consider that acceptable for code in MediaWiki core.

I don't particularly mind a MessageLocalizer interface existing, but want to make sure the rationale is not "Avoiding global state" or "Avoiding context source". The rationale is standardising the method name and parameters with a centralised interface.

I am skeptical of these kinds of interfaces as it usually doesn't end up being used. We'd specify the interface whenever we add such method, but never use it anywhere. It would help to have an example use case where a method would use MessageLocalizer as parameter type, or in what scenario one would want to instanceof-check for it?

Change 357353 abandoned by Ladsgroup:
[POC] Add LanguageMessageLocalizer implementing MessageLocalizer

Reason:
It's better to go with I1864afb8bcc641698689828914949a06506d8f3a, restore if you want to pick this up and work on it

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

I doubt anyone would extend ContextSource for a class just to get a msg() method. That seems rather unnecessary. I would not consider that acceptable for code in MediaWiki core.

Well, using subclassing for this kind of thing shouldn't be done anyway. So in Wikibase, we use composition - that is, we inject a factory service. Defining an interface for that factory is what this ticket is about.

The Message object itself relies heavily on global state. If we want to fix that at some point, we should avoid direct constructor calls - if all Messages come from a factory, we can easily start injecting things. The global function essentially acts as a constructor, so it does not help us to get rid of global state.

I am skeptical of these kinds of interfaces as it usually doesn't end up being used. We'd specify the interface whenever we add such method, but never use it anywhere. It would help to have an example use case where a method would use MessageLocalizer as parameter type, or in what scenario one would want to instanceof-check for it?

We have several services in Wikibase that currently require an IContextSource in the constructor, solely for constructing messages. Having a more narrow interface to inject there was the original motivation for this ticket. We originally planned to put the interface into Wikibase, but we then figured it would be nice to have in core, too.

I think narrow interfaces should be preferred over one-covers-all things like IContextSource. If I Was King, we'd get rid of the ContextSource base class and IContextSource asap, and replace it by a bunch of narrow interfaces, and composition/injection. This ticket is a small first step in that direction.

If you feel that MessageLocalizer should not be in core, we can just introduce it to Wikibase for now.

I doubt anyone would extend ContextSource for a class just to get a msg() method. That seems rather unnecessary. I would not consider that acceptable for code in MediaWiki core.

Well, using subclassing for this kind of thing shouldn't be done anyway. So in Wikibase, we use composition - that is, we inject a factory service. Defining an interface for that factory is what this ticket is about.

I see now. The point being that you can inject a class with a msg method into something without that class needing to know what language to be used, or why (e.g. content/user/arbitrary language) and pre-configuring other parameters to Message (useDatabase, title, interface, etc.).

Change 355879 merged by jenkins-bot:
[mediawiki/core@master] Introduce MessageLocalizer interface for exposing msg() method

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