Make UX for misspelled or delted config much less bad

Summary:
Fixes T3436. Currently, when installs have configuration options which we don't know about, we raise a fairly confusing/ambiguous message about the options being unknown. Instead:

  - Keep a list of previously valid (but now deleted) config, with explanatory reasons for what happened to it. Present this information, along with altenate wording ("Obsolete Config" instead of "Unknown Config") where applicable.
  - Show a list of all the places the config is defined.
  - Provide an active link to delete it from the web UI.
  - Provide a command to delete it from the CLI.
  - Allow `bin/config delete` to delete configuration options which no longer have a definition.

Test Plan:
  - Set an auth key in database, local and file config.
  - Walked through the setup issue, cleaning it up.
  - Set an invalid key and made sure I still got a reasonable error (this now has better cleanup instructions).

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3436

Differential Revision: https://secure.phabricator.com/D6317

src/applications/config/check/PhabricatorSetupCheckExtraConfig.php

@@ -3,6 +3,8 @@
 final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
 
   protected function executeChecks() {
+    $ancient_config = self::getAncientConfig();
+
     $all_keys = PhabricatorEnv::getAllConfigKeys();
     $all_keys = array_keys($all_keys);
     sort($all_keys);
@@ -13,20 +15,132 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
       if (isset($defined_keys[$key])) {
         continue;
       }
-      $summary = pht("This option is not recognized. It may be misspelled.");
-      $message = pht(
-        "The configuration option '%s' is not recognized. It may be ".
-        "misspelled, or it might have existed in an older version of ".
-        "Phabricator. It has no effect, and should be corrected or deleted.",
-        $key);
 
-      $this
+      if (isset($ancient_config[$key])) {
-        ->newIssue('config.unknown.'.$key)
+        $summary = pht(
-        ->setShortName(pht('Unknown Config'))
+          'This option has been removed. You may delete it at your '.
-        ->setName(pht('Unknown Configuration Option "%s"', $key))
+          'convenience.');
-        ->setSummary($summary)
+        $message = pht(
-        ->setMessage($message)
+          "The configuration option '%s' has been removed. You may delete ".
-        ->addPhabricatorConfig($key);
+          "it at your convenience.".
+          "\n\n%s",
+          $key,
+          $ancient_config[$key]);
+        $short = pht('Obsolete Config');
+        $name = pht('Obsolete Configuration Option "%s"', $key);
+      } else {
+        $summary = pht("This option is not recognized. It may be misspelled.");
+        $message = pht(
+          "The configuration option '%s' is not recognized. It may be ".
+          "misspelled, or it might have existed in an older version of ".
+          "Phabricator. It has no effect, and should be corrected or deleted.",
+          $key);
+        $short = pht('Unknown Config');
+        $name = pht('Unknown Configuration Option "%s"', $key);
+      }
+
+      $issue = $this->newIssue('config.unknown.'.$key)
+        ->setShortName($short)
+        ->setName($name)
+        ->setSummary($summary);
+
+      $stack = PhabricatorEnv::getConfigSourceStack();
+      $stack = $stack->getStack();
+
+      $found = array();
+      $found_local = false;
+      $found_database = false;
+
+      foreach ($stack as $source_key => $source) {
+        $value = $source->getKeys(array($key));
+        if ($value) {
+          $found[] = $source->getName();
+          if ($source instanceof PhabricatorConfigDatabaseSource) {
+            $found_database = true;
+          }
+          if ($source instanceof PhabricatorConfigLocalSource) {
+            $found_local = true;
+          }
+        }
+      }
+
+      $message = $message."\n\n".pht(
+        "This configuration value is defined in in these %d ".
+        "configuration source(s): %s.",
+        count($found),
+        implode(', ', $found));
+      $issue->setMessage($message);
+
+      if ($found_local) {
+        $command = csprintf('phabricator/ $ ./bin/config delete %s', $key);
+        $issue->addCommand($command);
+      }
+
+      if ($found_database) {
+        $issue->addPhabricatorConfig($key);
+      }
     }
   }
+
+  /**
+   * Return a map of deleted config options. Keys are option keys; values are
+   * explanations of what happened to the option.
+   */
+  public static function getAncientConfig() {
+    $reason_auth = pht(
+      'This option has been migrated to the "Auth" application. Your old '.
+      'configuration is still in effect, but now stored in "Auth" instead of '.
+      'configuration. Going forward, you can manage authentication from '.
+      'the web UI.');
+
+    $auth_config = array(
+      'controller.oauth-registration',
+      'auth.password-auth-enabled',
+      'facebook.auth-enabled',
+      'facebook.registration-enabled',
+      'facebook.auth-permanent',
+      'facebook.application-id',
+      'facebook.application-secret',
+      'facebook.require-https-auth',
+      'github.auth-enabled',
+      'github.registration-enabled',
+      'github.auth-permanent',
+      'github.application-id',
+      'github.application-secret',
+      'google.auth-enabled',
+      'google.registration-enabled',
+      'google.auth-permanent',
+      'google.application-id',
+      'google.application-secret',
+      'ldap.auth-enabled',
+      'ldap.hostname',
+      'ldap.port',
+      'ldap.base_dn',
+      'ldap.search_attribute',
+      'ldap.search-first',
+      'ldap.username-attribute',
+      'ldap.real_name_attributes',
+      'ldap.activedirectory_domain',
+      'ldap.version',
+      'ldap.referrals',
+      'ldap.anonymous-user-name',
+      'ldap.anonymous-user-password',
+      'ldap.start-tls',
+      'disqus.auth-enabled',
+      'disqus.registration-enabled',
+      'disqus.auth-permanent',
+      'disqus.application-id',
+      'disqus.application-secret',
+      'phabricator.oauth-uri',
+      'phabricator.auth-enabled',
+      'phabricator.registration-enabled',
+      'phabricator.auth-permanent',
+      'phabricator.application-id',
+      'phabricator.application-secret',
+    );
+
+    $ancient_config = array_fill_keys($auth_config, $reason_auth);
+
+    return $ancient_config;
+  }
 }

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -16,6 +16,18 @@ final class PhabricatorConfigEditController
 
     $options = PhabricatorApplicationConfigOptions::loadAllOptions();
     if (empty($options[$this->key])) {
+      $ancient = PhabricatorSetupCheckExtraConfig::getAncientConfig();
+      if (isset($ancient[$this->key])) {
+        $desc = pht(
+          "This configuration has been removed. You can safely delete ".
+          "it.\n\n%s",
+          $ancient[$this->key]);
+      } else {
+        $desc = pht(
+          "This configuration option is unknown. It may be misspelled, ".
+          "or have existed in a previous version of Phabricator.");
+      }
+
       // This may be a dead config entry, which existed in the past but no
       // longer exists. Allow it to be edited so it can be reviewed and
       // deleted.
@@ -23,10 +35,7 @@ final class PhabricatorConfigEditController
         ->setKey($this->key)
         ->setType('wild')
         ->setDefault(null)
-        ->setDescription(
+        ->setDescription($desc);
-          pht(
-            "This configuration option is unknown. It may be misspelled, ".
-            "or have existed in a previous version of Phabricator."));
       $group = null;
       $group_uri = $this->getApplicationURI();
     } else {
@@ -465,20 +474,6 @@ final class PhabricatorConfigEditController
     $stack = PhabricatorEnv::getConfigSourceStack();
     $stack = $stack->getStack();
 
-    /*
-
-      TODO: Once DatabaseSource lands, do this:
-
-      foreach ($stack as $key => $source) {
-        unset($stack[$key]);
-        if ($source instanceof PhabricatorConfigDatabaseSource) {
-          break;
-        }
-      }
-
-    */
-
-
     $table = array();
     $table[] = hsprintf(
       '<tr class="column-labels"><th>%s</th><th>%s</th></tr>',

src/applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php

@@ -33,13 +33,6 @@ final class PhabricatorConfigManagementDeleteWorkflow
         "Too many arguments: expected one key.");
     }
 
-    $options = PhabricatorApplicationConfigOptions::loadAllOptions();
-    if (empty($options[$key])) {
-      throw new PhutilArgumentUsageException(
-        "No such configuration key '{$key}'! Use `config list` to list all ".
-        "keys.");
-    }
-
     $config = new PhabricatorConfigLocalSource();
     $values = $config->getKeys(array($key));
     if (!$values) {

src/applications/config/view/PhabricatorSetupIssueView.php

@@ -177,7 +177,7 @@ final class PhabricatorSetupIssueView extends AphrontView {
     } else {
       $update = array();
       foreach ($configs as $config) {
-        if (!idx($options, $config) || $options[$config]->getLocked()) {
+        if (idx($options, $config) && $options[$config]->getLocked()) {
           continue;
         }
         $link = phutil_tag(