Page MenuHomePhabricator

Add an "eval" rule to Remarkup
ClosedPublic

Authored by epriestley on Jul 27 2021, 9:07 PM.

Details

Summary

Ref T13658. This adds a simple expression evaluator to Remarkup and supports platform name expressions. The syntax is:

${{{strings.platform.server.name}}}

Note that this won't work inside code blocks (or literal blocks, or other block-level literal elements) right now, although it could be made to selectively (the ".path" expressions might be useful in documentation codeblocks).

Test Plan

Screen Shot 2021-07-27 at 2.03.04 PM.png (881×675 px, 65 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This looks pretty cool and will be useful for maintaining existing text that goes through remarkup. Really useful doc on PhutilRemarkupBlockStorage btw.

With this we'll be able to update Diviner books to also use ${{{strings.platform.server.name}}} within their content. I think there are some books with "Phabricator" in the title though, which I don't believe remarkup is used for rendering. In those cases would it make sense to expose evaluateExpression() to be called directly when rendering the book titles or should document titles have some minimal PhutilRemarkupEngine which only specifies this rendering rule and apply it? As a side-note the only custom change my installation runs is a ~2-liner that auto-links Differential Revision titles to an external bug tracker, similar to the bugtraq.url setting, and this is giving me some ideas for what a general solution might look like.

Note that this won't work inside code blocks (or literal blocks, or other block-level literal elements) right now, although it could be made to selectively (the ".path" expressions might be useful in documentation codeblocks).

Would this be done by fiddling with the priority somehow, or using the engine's state stack like how PhutilRemarkupHeaderBlockRule uses it when generating the ToC, or some other means?

This revision is now accepted and ready to land.Jul 28 2021, 3:11 AM

You can either escape ${{{strings.x.y}}} as \${{{strings.x.y}}} or suggest a different syntax for the "eval" rule --- I'm not married to ${{{...}}}.

I think this format makes sense to me. Nothing else really comes to mind. The squiggles are keeping consistency with other remarkup rules like figlet and cowsay. I think I've seen $ used in other places (and languages) with some relation to string substitution. I think python convention uses _, so another possibility could be _{{{...}}}.

Showcasing some possibilities

${{{strings.platform.server.name}}}

_{{{strings.platform.server.name}}}

$[[[strings.platform.server.name]]]

_[[[strings.platform.server.name]]]

$<<<strings.platform.server.name>>>

_<<<strings.platform.server.name>>>

$eval(strings.platform.server.name)

Hmm function call style feels interesting though that might be bias

src/infrastructure/markup/markuprule/PhutilRemarkupEvalRule.php
63

Were you thinking of other non-string behavior being useful in the future, so .e.g ${{{config.phabricator.base-uri}}} could substitute the value of the config or something (assuming policy checks are added)?

I suspect escaping things in PHP will be pretty rare and that the "collides with PHP strings" downside will be very small.

I think _() is the pht() of Gnu "gettext". Modern programers may mostly be more familiar with Python than with gettext, of course.

I am going to tweak this slightly and make ${{{some-invalid-thing}}} render literally outside of previews (similar to how most other modern rules work) so that this discussion of these rules doesn't immediately turn into gibberish once the rule deploys.

src/infrastructure/markup/markuprule/PhutilRemarkupEvalRule.php
63

Were you thinking of other non-string behavior being useful in the future, so .e.g ${{{config.phabricator.base-uri}}} could substitute the value of the config or something (assuming policy checks are added)?

Yeah, I'm not sure how much traction this will really get but I imagine making the things-that-can-be-evaluated modular so a source could expose Config or, perhaps, nonsense like:

Remaining Open Tasks: ${{{query.key("abcdef").count}}}

src/infrastructure/markup/markuprule/PhutilRemarkupEvalRule.php
93–95

Oh, so would this just return $expression in the case that the lookup failed, or include the escape characters as well, return '${{{'.$expression.'}}}';

Or should we try to return the original text passed in above without reconstructing it

I think _() is the pht() of Gnu "gettext". Modern programers may mostly be more familiar with Python than with gettext, of course.

Oh yea that's right. A long time ago I worked on a python web application that used this was probably confusing it.

When intent is ambiguous (the user might or might not be trying to invoke a Remarkup rule), I try to make the output of an "invalid" input exactly the same as the input, so (for example) copy/pasting text into Phabricator doesn't mangle it into a big blob of nonsense just because you happened to have some magic words in there.

For example, if you write @gmail.com, it renders as "@gmail.com", not something like "<Invalid User gmail.com>", even though this syntax is valid and you could have a user named gmail.com.

When rendering for a preview, rules can be more aggressive about communicating errors without causing the "Phabricator mangled my text" problem. So we can use "error information on preview, literal input when rendered" to still give users information about mistakes without mangling anything if they're just copy/pasting or discussing something without actually intending to invoke the rule. Most rules don't really do this today (I think only one or two, mostly using CSS?) but I'd like to do more of it since it feels like a good balancing of concerns to me when rules do it.

There's still a lot of grey area ("when is intent ambiguous?") but since we've been writing a lot of "${{{example.thing}}}" in this discussion without intending to invoke the evaluator, I think this probably meets the bar even though meta-discussions about this rule aren't likely to be common.

  • For now, just return the literal input if we fail to evaluate an expression.

This could show a fancier error message later on if this is used in a non-niche capacity.

cspeckmim added inline comments.
src/infrastructure/markup/PhabricatorMarkupEngine.php
45

Does this cause previous render caches to invalidate?

src/infrastructure/markup/PhabricatorMarkupEngine.php
45

Yep.

This revision was automatically updated to reflect the committed changes.