Page MenuHomePhabricator
Create Task
Maniphest T12784

Allow color escapes in PhutilConsoleTable rendering
Closed, InvalidPublic
Actions

Description

When attempting to help users disambiguate between things presented in a table, we may want to provide valance to the user by setting the background colors:

$table = id(new PhutilConsoleTable())
       ->setShowHeader(false)
       ->setBorders(true)
       ->setPadding(1)
       ->addColumn('command', array('title' => 'Command'))
       ->addColumn('usage',   array('title' => 'Usage'));
$table->addRow(array(
  'command' => phutil_console_format(
    "<bg:red>** %s **</bg>", 'arc thingy' ),
  'usage'   => 'Does the dangerous thingy.',
));
$table->addRow(array(
  'command' => phutil_console_format(
    "<bg:green>** %s **</bg>", 'arc dongle' ),
  'usage'   => 'Does the safe dongle.',
));
$table->draw();

Unfortunately, PhutilConsoleTable, being a PhutilConsoleView, forcibly escapes all ANSI escape codes before displaying them:

+==============+============================+
| <ESC>[41m<ESC>[1m arc thingy <ESC>[m<ESC>[49m | Does the dangerous thingy. |
| <ESC>[42m<ESC>[1m arc dongle <ESC>[m<ESC>[49m | Does the safe dongle.      |
+==============+============================+

Interestingly, the widths are calculated correctly.

As with any escaping or similar transformation, turning it off at one layer requires that the layer built atop it take full responsibility for escaping, itself -- or T8243 will be made worse. While the below potential solution is sufficient to implement the feature request, it does so at the cost of introducing a loaded gun, and possibly breaking the encapsulation (and purpose) of PhutilConsoleView.

P2057 Masterwork From Distant Lands
1diff --git a/src/console/view/PhutilConsoleTable.php b/src/console/view/PhutilConsoleTable.php
2index 7c29f99..d19141d 100644
3--- a/src/console/view/PhutilConsoleTable.php
4+++ b/src/console/view/PhutilConsoleTable.php
5@@ -30,6 +30,7 @@ final class PhutilConsoleTable extends PhutilConsoleView {
6 private $borders = false;
7 private $padding = 1;
8 private $showHeader = true;
9+ private $escape = true;
10
11 const ALIGN_LEFT = 'left';
12 const ALIGN_CENTER = 'center';
13@@ -54,6 +55,11 @@ final class PhutilConsoleTable extends PhutilConsoleView {
14 return $this;
15 }
16
17+ public function setEscapeContents($escape) {
18+ $this->escape = $escape;
19+ return $this;
20+ }
21+
22
23 /* -( Data )--------------------------------------------------------------- */
24
25@@ -88,6 +94,16 @@ final class PhutilConsoleTable extends PhutilConsoleView {
26
27 /* -( Drawing )------------------------------------------------------------ */
28
29+ public function drawConsoleString() {
30+ if ($this->escape) {
31+ return parent::drawConsoleString();
32+ }
33+
34+ $view = $this->drawView();
35+ $parts = $this->reduceView($view);
36+ return implode('', $parts);
37+ }
38+
39 protected function drawView() {
40 return $this->drawLines(
41 array_merge(
42diff --git a/src/console/view/PhutilConsoleView.php b/src/console/view/PhutilConsoleView.php
43index 85f5b8f..96087ca 100644
44--- a/src/console/view/PhutilConsoleView.php
45+++ b/src/console/view/PhutilConsoleView.php
46@@ -41,7 +41,7 @@ abstract class PhutilConsoleView extends Phobject {
47 * @return string Console-printable string.
48 * @task draw
49 */
50- final public function drawConsoleString() {
51+ public function drawConsoleString() {
52 $view = $this->drawView();
53 $parts = $this->reduceView($view);
54
55@@ -61,7 +61,7 @@ abstract class PhutilConsoleView extends Phobject {
56 * @return list<wild> List of unnested drawables.
57 * @task draw
58 */
59- private function reduceView($view) {
60+ protected function reduceView($view) {
61 if ($view instanceof PhutilConsoleView) {
62 $view = $view->drawView();
63 return $this->reduceView($view);

Related Objects

Event Timeline

alexmv created this task.Jun 1 2017, 3:35 AM
epriestley added a subscriber: epriestley.Jun 1 2017, 12:47 PM

Use tsprintf() ("terminal string print formatted") instead of phutil_console_format():

Screen Shot 2017-06-01 at 5.40.16 AM.png (79×423 px, 13 KB)

This API is possibly still not final. tsprintf() + ConsoleView are efforts to mimic the way we do composition and escaping in phabricator/ for web, which works well, but they feel like they aren't quite there yet.

One major issue we're left with is that we need a lot of tsprintf(pht()), which feel clunky. But if we extract the raw tsprintf() output for internationalization, we open ourselves up to the possibility that bad/malicious translations can cause security issues, which is probably a very bad road to walk down. See T3526 for a vaguely similar issue elsewhere.

It's also currently much more of a pain to decompose a lot of console elements than it is to decompose PHUI elements.

So I'm not 100% sure where this API is headed, but I believe tsprintf() can work safely in all cases today, even if it often feels like it's clunkier than it should be.

I hope to get more of a handle on this soon (in the next arc iteration) since we'll be inviting more third-party development, but if the pathway looks like "bring string typing to internationalization" a rework may not be in the cards during this iteration.

alexmv closed this task as Invalid.Jun 2 2017, 2:14 AM

Aha! That works wonderfully. I think this is INVALID, then.

Phabricator · Built by Phacility