Page MenuHomePhabricator

Add a MessageLocalizer service
Closed, DuplicatePublic

Description

While creating a UserNameUtils service in T245231: Factor username logic out of the User class, it became apparent that there is no service that allows injecting a message localizer.

When a message localizer isn't available to get a message, wfMessage is usually used. The code for that function is:

wfMessage
function wfMessage( $key, ...$params ) {
	$message = new Message( $key );

	// We call Message::params() to reduce code duplication
	if ( $params ) {
		$message->params( ...$params );
	}

	return $message;
}

I proposed that a new service be created, implementing the MessageLocalizer interface and requiring no dependencies, that simply proxies wfMessage until the message class is constructed with proper DI (T247191).

This could then be injected into services, such as the new username utils, as well as:

  • AuthManager - 20 uses of wfMessage
  • BadFileLookup - calls wfMessage within ServiceWiring to pass along the needed text
  • ConfigRepository - 1 use
  • DefaultPreferencesFactory - 10 uses
  • Language - 23 uses
  • LanguageConverter - 3 uses
  • LinkRenderer - 1 use
  • Parser - 14 uses
  • PermissionManager - 2 uses of wfMessage
  • Lots of API classes, once those have dependency injection
  • Lots of SpecialPage classes

By allowing these to be injected, it is also more likely that tests can be converted to pure unit tests and avoid use of the global state. Thoughts?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Tagging CPT per Developers/Maintainers - responsible for dependency injection

Change 577687 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add a MessageFactory service to inject a message localizer

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

Change 562388 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Provide MessageLocalizer service

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

Note that we can't call wfMessage() plainly from a service because the Message class has an implicit dependency on the global state for RequestContext User (and the user's preferred Language) and Title.

It is already unsafe to call wfMessage() in many code paths. And any code that does use it, becomes itself a mine field that is unsafe to use. When the code is used in a context without a session, the code will throw a fatal error. When it is used in code that does have a user session, there is a chance of it rendering text in the wrong language when the language isn't explicitly coming from the local context.

A service-contained message localizer would need to have a fixed dummy title, and a language that varies only by site configuration (not by user or session). But.. such a service would not be useful in most cases. And, having to call inLanguage($context->getLanguage()) everywhere we use it is likely not going to make the code and developer experience better (lots of mistakes would be made, it would be easy to miss).

Perhaps we need a factory here. I usually find such indirection a compromise I prefer to avoid, but for this case a MessageLocalizerFactory might be what we need. It would take care of the Language and Title dependencies allowing them to be passed at run-time to a method that spawns the actual MessageLocalizer. These would then be passed down as needed. For example, one instance would be passed down from load.php to ResourceLoaderContext. And another might be passed down to frontend code (SpecialPage, Action classes, OutputPage, Skin) to its main RequestContext. And another for api.php, rest.php. Anything else such as maintenance scripts and other service classes would need to decide for themselves how to access it, based on how they already decide what the appropiate language/title is.

Thoughts?

Note that in the longer term we should probably replace Message with MessageValue. See also T227447: Librarize i18n-related PHP classes in MediaWiki.

In that model the MessageValue object is newable and doesn't need any global state or factory. The use of state is pushed towards the UI layer, in that the code that wants to turn the MessageValue into a string for output would have an appropriate MessageFormatter or MessageFormatterFactory injected.

I have no opinion on the extent to which that long-term objective should block shorter-term things like this task.

Change 577687 abandoned by DannyS712:
[mediawiki/core@master] Add a MessageFactory service to inject a message localizer

Reason:

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