[go: nahoru, domu]

Page MenuHomePhabricator

MessageValue and Message should share a common interface; MediaWiki\Message\Converter should be unnecessary
Open, Needs TriagePublic

Description

When passing data between older code and newer code, we often have to convert from MessageValue to Message, or the other way around, which is implemented in MediaWiki\Message\Converter.

This is silly. Both of them just represent a message key and parameters. They should share a common interface – we could reuse MessageSpecifier for this – and be able to be constructed from another MessageSpecifier, so we wouldn't need explicit conversion.

(Similarly to how e.g. Title and TitleValue implement LinkTarget, and can be constructed from each other using newFromLinkTarget().)

Ideally the internal formats would also be close enough that we wouldn't need so much code to convert between them.

There's also another benefit to making MessageValue use the MessageSpecifier interface: MessageSpecifier is a read-only view of the data, so this could guarantee that functions that are supposed to print messages can't accidentally modify them.

Event Timeline

Change 1007693 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Make Message and MessageValue compatible

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

The change to implement this is now ready for review: Make Message and MessageValue compatible.

Adding the MessageSpecifier interface to the MessageValue class seems to be pretty uneventful. I only had to make some updates to the StatusValue class, which previously accepted both MessageSpecifier and MessageValue and converted between them, and some of that code would go down the wrong path after the change.

The bigger change is changing how the Message class stores parameters internally to match the MessageValue class: previously it used assoc arrays, e.g. [ 'num' => 123 ], which are replaced by MessageParam value objects. Code that uses public methods like ->numParams() won't be affected by this change, but I found a few places in core and extensions that assumed the previous behavior. I updated them: https://gerrit.wikimedia.org/r/q/topic:message-param but it's worth noting that this may affect other code that wasn't covered by tests that'd let me find them. The old behavior was never documented, so I don't consider this to be a breaking change to a public interface.

A few other patches have been split off, and merged already:

Thanks to everyone who reviewed those!