Remarkup rule for rendering PHIDs as handles
ClosedPublic
Actions

Authored by avivey on Mar 16 2016, 11:15 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

Restricted Diffusion Commit
rP2b9d4f70ba3a: Remarkup rule for rendering PHIDs as handles

Summary

adds the {{PHID....}} rule. Should mostly be useful in UI code that refers to Objects.

It doesn't add any mention links/transactions.

Test Plan

Comment with this, see email (plain + html) and comment box.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

avivey updated this revision to Diff 37325.Mar 16 2016, 11:15 PM

avivey retitled this revision from to Remarkup rule for rendering PHIDs as handles.

avivey updated this object.

avivey edited the test plan for this revision. (Show Details)

avivey added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMar 16 2016, 11:15 PM

Herald added a subscriber: Korvin. · View Herald Transcript

web (As renderTag):

html mail:

plain:

I'm not sure if rendering the handle as tag, or link, or hovercardLink is nicest; "tag" looks richest...

avivey updated this object.Mar 16 2016, 11:19 PM

avivey edited edge metadata.

Couple of inlines but this looks good to me overall.

src/applications/phid/remarkup/PhabricatorHandleRemarkupRule.php
11	I think we can just do `{PHID-...}`, not `{{PHID-...}}`. It isn't ambiguous with `{T123}`, etc. It would maybe even be OK to mark up plain `PHID-...` in the future, although I don't like changing the text when the reference might be ambiguous. But we could have these still say `PHID-...` and at least produce hovercards, which would be a little better in all cases and worse in none, I think.
11	Maybe make the PHID-capturing part a little stricter, like `[^\s}]` (i.e., prevent whitespace). I think this could capture a whole paragraph if you type like: {{PHID- is how you start a PHID reference and then you end it with }} ...which isn't the most faithful rendering of the original input text.
19	Because the pattern is liberal, we should have an `if (!$this->isFlatText($matches[0]))` check here which just returns `$matches[0]` for non-flat text. This prevents shenanigans like `{{PHID-x}}`, where you put rich text inside the rule. In some cases, this can lead to security issues in the generated markup.
19	I think we should also do a `phid_get_type()` here and just return `$matches[0]` if the pattern doesn't have a type or, ideally, if we don't recognize the type. That is, when the user types: {{PHID-x}} ...the best rendering is just their input text, not "Unknown Object (???)", since that could make a log or table or example which means to include this text as raw text unreadable and confusing.
65	HandleQuery should always produce a handle, even if the PHID is nonsensical or invalid, so this check might be impossible to miss. A better check might be `!$handle->isComplete()`, which indicates that the handle is inaccessible or not really referencing valid object.
85	I think you can `setPHID()` on `$link` to get a hovercard (which seems like a good/reasonable behavior to me).

This revision now requires changes to proceed.Mar 17 2016, 1:33 PM

Specifically, I'm imagining these as the eventual rules:

{PHID-TASK-nnnn} -> Taaa: Build Better Dogs
PHID-TASK-nnnn -> PHID-TASK-nnnn

In case (1), I think it's OK if {...} (single brackets) trigger the rule. This is consistent with other rules and I don't think it introduces ambiguity or will fire accidentally.

In case (2), we would preserve the input text (in case you were just talking about a PHID and didn't explicitly mean to trigger the rule, so users can still understand and copy/paste it) but augment it with a visual highlight and a hovercard.