Page MenuHomePhabricator

Replace PhabricatorMarkupInterface with a better-suited API/abstraction
Closed, ResolvedPublic

Description

The remarkup rendering pipeline is legitimately complex: it involves caching and multiple rendering stages to try to batch lookups while respecting policies.

A while ago I wrote PhabricatorMarkupInterface to try to hide this complexity, but I think my architecture was not very good. Some of the ideas I had at the time (about objects having multiple formal remarkup fields) haven't panned out in practice.

Today, interacting with Remarkup APIs feels very heavyweight and cumbersome, and the interface itself is an awkward mess. Most new callsites just use PhabricatorMarkupOneOff, since it's a million times easier than implementing PhabricatorMarkupInterface and the cost of missing the parsing cache on a single field on the page is small.

I think this interface should probably be a wrapper object around the block instead, with semantics roughly like this:

// This object is configurable and we can add cache information.
$description_field = new RemarkupView($raw_text);

$engine->addView($description_field);
$engine->process();

return $description_field->render();

All the cache management and ruleset configuration can live on RemarkupView as setters.

Objects which still desire to provide standard access to cachable fields can expose methods like getSummaryRemarkupView() in lieu of implementing PhabricatorRemarkupInterface.

Callsites which just want to render a field in an ad-hoc way can use a much simpler API, and maybe even get ad-hoc access to caching with some setter.

We can make this object an AphrontView so it doesn't need to be explicitly rendered. We may be able to make processing lazy and handle engine pools implicitly: this seems to have been very successful in the recent update to the handle APIs.

Even without lazy processing, aligning the API to how it's actually used should be able to simplify essentially every callsite without any reduction in power or flexibility.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Remarkup.
epriestley updated the task description. (Show Details)
epriestley added a subscriber: epriestley.

I remember trying to make something implement PhabricatorMarkupInterface a while back for a custom app, and it was so painful and I couldn't figure out what was going on so I ended up just using PhabricatorMarkupOneOff instead, even though it didn't feel like the right way to do things.

Given CORGI, would this be nice to pursue sooner than later? That is, I'd rather write all CORGI copy with Remarkup, and there is a lot of it.

Yeah, that makes sense. I think this is straightforward and won't create a big ball of mess or anything, so I'll see if I can slip it in somewhere.