Page MenuHomePhabricator

Inject IContextSource into ViolationMessageRenderer
Closed, ResolvedPublic

Description

@Ladsgroup pointed out in a comment on I154d9cdebe that ConstraintParameterRenderer and ViolationMessageRenderer could get an IContextSource injected (or, more generically, any MessageLocalizer?) and use its msg function instead of using new Message() and wfMessage() directly. I’m not sure if that has any concrete advantages, but it sounds good to me.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 20 2018, 3:33 PM

So the reason I made MessageLocalizer is that we wanted to inject RequestContext somewhere just to use the msg method and @daniel pointed out that this class is a huge kitchen sink that are dragging around. That interface is an attempt to decouple RequestContext and message localization. In my ideal world, core would implement a proper RequestBasedMessageLocalizer and we would only inject that but for now I think injecting RequestContext instance using MessageLocalizer typehint (so we can easily mock it in tests, etc.) would be enough for me.

@Lucas_Werkmeister_WMDE The rationale for changing code to use narrow interfaces where possible is top remove circular dependencies to enable modularization. Currently, MW core is a tight knot of cross-dependencies, and kitchen sinks like IContextSource and "smart" records like User and Title are the main reason for that.

So, changing from IContextSource to MessageLocalizer contributes general code hygiene. The advantage is mid-term improvements of code quality.

@Ladsgroup: I don't see a need for RequestBasedMessageLocalizer. It's fine to use a RequestContext as the concrete instance, the code just shouldn't bind to that class (or the IContextSource) any more. RequestContext can be deprecated and replaced when nothing hints against it any more.

Change 422161 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Inject MessageLocalizer into message renderers

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

Change 422162 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/WikibaseQualityConstraints@master] Use qqx language for tests

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

Change 422161 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Inject MessageLocalizer into message renderers

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

Change 422162 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Use qqx language for tests

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

Lucas_Werkmeister_WMDE closed this task as Resolved.Mar 28 2018, 12:50 PM
Lucas_Werkmeister_WMDE claimed this task.

Done :)