Page MenuHomePhabricator

Refactor DifferenceEngine
Open, Needs TriagePublic

Description

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.

Related Objects

StatusSubtypeAssignedTask
OpenBUG REPORTNone
OpenNone
StalledNone
OpenNone
OpenNone
DuplicateNone
OpenFeatureNone
OpenFeatureNone
DuplicateNone
ResolvedNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
ResolvedNone
ResolvedNone
OpenFeatureNone
OpenNone
ResolvedHalfak
OpenNone
OpenNone
OpenFeatureNone
StalledNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
Resolvedtstarling
OpenNone
DeclinedNone
ResolvedBUG REPORTJdlrobson
ResolvedNone
OpenNone

Event Timeline

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.