Page MenuHomePhabricator

Refactor DifferenceEngine
Open, Needs TriagePublic


DifferenceEngine is a very convoluted class that does everything from interpreting query parameters for selecting the target revision to direct DB queries to table formatting to writing to OutputPage. To make matters worse, it is instantiated by ContentHandler and content type provider extensions are supposed to replace it with their own subclasses. It should be split up into more focused classes:

  • A DiffController which is responsible for turning query parameters such as diff=next into revision numbers, fetching the content objects and calling the diff engines and the diff formatter(s). Also detecting corner cases like suppressed revisions, intermediate revisions, diff=prev on first revision. This class should be in core and not subclassable.
  • A diff engine interface (since both DifferenceEngine and DiffEngine are reserved, maybe call it DiffGenerator) which takes two content objects and some extra parameters (slot role, rcid, diff options) and creates a representation of the diff (ideally a logical representation, but currently diff generation might shelled out to external processes which can only return HTML, so it'll have to be a HTML representation probably). The implementation should probably be selected by SlotRoleHandler.
  • A diff formatter which can render a diff representation into HTML (for now a no-op since the diff generator will already return formatted HTML), add various decoration (title, line numbers), generate headers, and compose all slot diff HTMLs into a single HTML. This might be a single core class with various hooks, or maybe an interface with implementation provided by the SlotRoleHandler.

Event Timeline

Tgr created this task.May 16 2018, 2:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 16 2018, 2:50 PM
Tgr added a comment.May 16 2018, 2:53 PM

As a semi-unrelated footnote, it would be nice to have a class that encapsulates a piece of HTML + changes in OutputPage state (ResourceLoader modules, mainly) which can be returned from formatters instead of having to pass the OutputPage to them. That could be applied to the output, cached, turned into an API response structure etc. as needed.

TheDJ added a subscriber: TheDJ.May 16 2018, 6:10 PM

HTML + changes in OutputPage state

Related to that: T60478: Improve interface for MediaHandlers to add JavaScript

Vvjjkkii renamed this task from Refactor DifferenceEngine to dvcaaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from dvcaaaaaaa to Refactor DifferenceEngine.Jul 2 2018, 4:07 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.