From 006cb659cbf1a0153548c6fc287535b678addb48 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 12 Aug 2019 11:19:08 -0700
Subject: [PATCH] Make the success message from "bin/config" more clear

Summary:
Ref T13373. When you "bin/config set x ..." a value, the success message ("Set x ...") is somewhat ambiguous and can be interpreted as "First, you need to set x..." rather than "Success, wrote x...".

Make the messaging more explicit. Also make this string more translatable.

Test Plan: Ran `bin/config set ...` with various combinations of flags, saw more clear messaging.

Maniphest Tasks: T13373

Differential Revision: https://secure.phabricator.com/D20711
---
 ...PhabricatorConfigManagementSetWorkflow.php | 44 ++++++++++++-------
 .../env/PhabricatorConfigLocalSource.php      |  5 +++
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php
index 22b760872e..9eb83bd61e 100644
--- a/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php
+++ b/src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php
@@ -30,11 +30,10 @@ final class PhabricatorConfigManagementSetWorkflow
   }
 
   public function execute(PhutilArgumentParser $args) {
-    $console = PhutilConsole::getConsole();
     $argv = $args->getArg('args');
-    if (count($argv) == 0) {
+    if (!$argv) {
       throw new PhutilArgumentUsageException(
-        pht('Specify a configuration key and a value to set it to.'));
+        pht('Specify the configuration key you want to set.'));
     }
 
     $is_stdin = $args->getArg('stdin');
@@ -45,7 +44,8 @@ final class PhabricatorConfigManagementSetWorkflow
       if (count($argv) > 1) {
         throw new PhutilArgumentUsageException(
           pht(
-            'Too many arguments: expected only a key when using "--stdin".'));
+            'Too many arguments: expected only a configuration key when '.
+            'using "--stdin".'));
       }
 
       fprintf(STDERR, tsprintf("%s\n", pht('Reading value from stdin...')));
@@ -54,7 +54,8 @@ final class PhabricatorConfigManagementSetWorkflow
       if (count($argv) == 1) {
         throw new PhutilArgumentUsageException(
           pht(
-            "Specify a value to set the key '%s' to.",
+            'Specify a value to set the configuration key "%s" to, or '.
+            'use "--stdin" to read a value from stdin.',
             $key));
       }
 
@@ -67,14 +68,13 @@ final class PhabricatorConfigManagementSetWorkflow
       $value = $argv[1];
     }
 
-
     $options = PhabricatorApplicationConfigOptions::loadAllOptions();
     if (empty($options[$key])) {
       throw new PhutilArgumentUsageException(
         pht(
-          "No such configuration key '%s'! Use `%s` to list all keys.",
-          $key,
-          'config list'));
+          'Configuration key "%s" is unknown. Use "bin/config list" to list '.
+          'all known keys.',
+          $key));
     }
 
     $option = $options[$key];
@@ -99,7 +99,7 @@ final class PhabricatorConfigManagementSetWorkflow
             switch ($type) {
               default:
                 $message = pht(
-                  'Config key "%s" is of type "%s". Specify it in JSON.',
+                  'Configuration key "%s" is of type "%s". Specify it in JSON.',
                   $key,
                   $type);
                 break;
@@ -128,7 +128,6 @@ final class PhabricatorConfigManagementSetWorkflow
     }
 
     if ($use_database) {
-      $config_type = 'database';
       $config_entry = PhabricatorConfigEntry::loadConfigEntry($key);
       $config_entry->setValue($value);
 
@@ -136,15 +135,28 @@ final class PhabricatorConfigManagementSetWorkflow
       $config_entry->setIsDeleted(0);
 
       $config_entry->save();
+
+      $write_message = pht(
+        'Wrote configuration key "%s" to database storage.',
+        $key);
     } else {
-      $config_type = 'local';
-      id(new PhabricatorConfigLocalSource())
+      $config_source = id(new PhabricatorConfigLocalSource())
         ->setKeys(array($key => $value));
+
+      $local_path = $config_source->getReadablePath();
+
+      $write_message = pht(
+        'Wrote configuration key "%s" to local storage (in file "%s").',
+        $key,
+        $local_path);
     }
 
-    $console->writeOut(
-      "%s\n",
-      pht("Set '%s' in %s configuration.", $key, $config_type));
+    echo tsprintf(
+      "<bg:green>** %s **</bg> %s\n",
+      pht('DONE'),
+      $write_message);
+
+    return 0;
   }
 
 }
diff --git a/src/infrastructure/env/PhabricatorConfigLocalSource.php b/src/infrastructure/env/PhabricatorConfigLocalSource.php
index 16dc43a9bc..fc1c83f812 100644
--- a/src/infrastructure/env/PhabricatorConfigLocalSource.php
+++ b/src/infrastructure/env/PhabricatorConfigLocalSource.php
@@ -65,4 +65,9 @@ final class PhabricatorConfigLocalSource extends PhabricatorConfigProxySource {
     return $path;
   }
 
+  public function getReadablePath() {
+    $path = $this->getConfigPath();
+    return Filesystem::readablePath($path);
+  }
+
 }