Page MenuHomePhabricator

RfC: Linker refactor
Closed, ResolvedPublic

Description

Static methods are widely used throughout the MediaWiki code base. This introduces procedural code, global state and makes it quite a challenge to do good unit tests of the MediaWiki code. It is difficult to isolate pieces of code and test each piece, each class, each method and is not possible to mock the static methods of another class.

One of the most egregious examples is in the Linker class, whose static methods are used nearly 500 times throughout MediaWiki. Especially heavy use occurs in special pages, ChangesList / Enhanced Changes, Difference Engine, etc., making them difficult to test fully.

MediaWiki should move away from using static functions, and a first step or trial of this would be to replace the Linker utility class with service objects. Then other classes using Linker can mock out those methods and isolate their own methods for testing.

Another issue with Linker is that it mixes many different functionalities that are not necessarily directly about "linking". Linker includes code that handles formatting comments, such as in recent changes pages. This should be separated into distinct value and service class (Comment/Summary + formatters). Linker also contains code for formatting the table of contents in pages. This could be split into its own formatter class.

Any refactoring of the Linker class needs to keep performance in mind and how the code interacts with the LinkCache. Methods such as Linker::link() can be quite inefficient, especially if called numerous times. (e.g. on watchlist) In refactoring this code, may also be able to look into how to improve that

More details: mw:Requests_for_comment/Linker_refactor

Details

Related Gerrit Patches:

Event Timeline

Qgil created this task.Sep 22 2014, 9:35 AM
Qgil raised the priority of this task from to High.
Qgil updated the task description. (Show Details)
Qgil added a project: Architecture.
Qgil changed Security from none to None.
Qgil moved this task from Inbox to Ready to Go on the Architecture board.
Qgil added a subscriber: Qgil.
Qgil closed this task as Resolved.Sep 22 2014, 9:57 AM
Qgil claimed this task.

This RfC was discussed and approved on 2014-09-18 (logs).

Qgil edited projects, added TechCom-RFC; removed Architecture.Oct 22 2014, 8:46 PM
daniel moved this task from Inbox to Declined on the TechCom-RFC board.Feb 27 2015, 3:25 PM
daniel moved this task from Declined to Implemented on the TechCom-RFC board.Feb 27 2015, 3:28 PM

@daniel moved this task to Implemented on the ArchCom-RfC workboard.

Where has this been implemented?

daniel moved this task from Implemented to (unused 2) on the TechCom-RFC board.Apr 19 2016, 3:08 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 3:08 PM
daniel reopened this task as Open.Apr 19 2016, 3:08 PM

Re-opening, since this was approved, but not implemented.

@Legoktm it was never implemented. I think Quim closed it, because it was approved, and I moved it to implemented, because it was closed... We were still experimenting with the Workboard in February 2015.

RobLa-WMF removed Qgil as the assignee of this task.Apr 19 2016, 8:13 PM
RobLa-WMF updated the task description. (Show Details)

Re-opening, since this was approved, but not implemented.

@daniel: from a workflow perspective, perhaps the RFC should be closed (since no one is currently asking for more TechCom advice on this subject), but a follow-up task of "implement T469" should be filed. Thoughts?

I'll also note that it's frustrating that the "submitter" can't be edited. @aude is the author of this RFC, but she's only now being cc'd on this. @aude: what are your thoughts on what the next steps should be? If you still believe that someone should implement this, it would be handy for you to file the task requesting that. That will make it easier to close this task off and to have a separate task to discuss implementation logistics.

@RobLa-WMF: we should decide if we want to close all approved RFCs then. Perhaps simpler, we could leave them open, and just remove the ArchCom-RFCs tag. They could be used as implementation tasks.

Currently, we have the approved RFCs on the board, and open. That's why I reopened this one.

RobLa-WMF mentioned this in Unknown Object (Event).Apr 20 2016, 6:43 AM

Change 284750 had a related patch set uploaded (by Legoktm):
[WIP] Add HtmlPageLinkRenderer (rewrite of Linker::link())

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

Change 284750 merged by jenkins-bot:
Add LinkRenderer (rewrite of Linker::link())

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

Krinkle closed this task as Resolved.Nov 4 2017, 12:38 AM
Krinkle moved this task from Untriaged to Implemented on the TechCom-RFC (TechCom-Approved) board.