Add "Developer" and "Access Log" config option groups, some types

Summary:
  - Add a "developer" option group.
  - Add an "access log" option group.
  - Render the types "bool", "int" and "string" in a more tailored way.
  - Add a config check for dead config. Right now this serves as a "TODO" list of things that need to be migrated.

Test Plan: Looked at config options, setup issues. Edited bool, int, string options.

Reviewers: codeblock, btrahan

Reviewed By: codeblock

CC: aran

Maniphest Tasks: T2255

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

src/__phutil_library_map__.php

@@ -570,6 +570,7 @@ phutil_register_library_map(array(
     'PackageModifyMail' => 'applications/owners/mail/PackageModifyMail.php',
     'Phabricator404Controller' => 'applications/base/controller/Phabricator404Controller.php',
     'PhabricatorAccessLog' => 'infrastructure/PhabricatorAccessLog.php',
+    'PhabricatorAccessLogConfigOptions' => 'applications/config/option/PhabricatorAccessLogConfigOptions.php',
     'PhabricatorActionListExample' => 'applications/uiexample/examples/PhabricatorActionListExample.php',
     'PhabricatorActionListView' => 'view/layout/PhabricatorActionListView.php',
     'PhabricatorActionView' => 'view/layout/PhabricatorActionView.php',
@@ -727,6 +728,7 @@ phutil_register_library_map(array(
     'PhabricatorDaemonTimelineEventController' => 'applications/daemon/controller/PhabricatorDaemonTimelineEventController.php',
     'PhabricatorDefaultFileStorageEngineSelector' => 'applications/files/engineselector/PhabricatorDefaultFileStorageEngineSelector.php',
     'PhabricatorDefaultSearchEngineSelector' => 'applications/search/selector/PhabricatorDefaultSearchEngineSelector.php',
+    'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php',
     'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php',
     'PhabricatorDifferentialConfigOptions' => 'applications/differential/config/PhabricatorDifferentialConfigOptions.php',
     'PhabricatorDirectoryController' => 'applications/directory/controller/PhabricatorDirectoryController.php',
@@ -1143,6 +1145,7 @@ phutil_register_library_map(array(
     'PhabricatorSetup' => 'infrastructure/PhabricatorSetup.php',
     'PhabricatorSetupCheck' => 'applications/config/check/PhabricatorSetupCheck.php',
     'PhabricatorSetupCheckAPC' => 'applications/config/check/PhabricatorSetupCheckAPC.php',
+    'PhabricatorSetupCheckExtraConfig' => 'applications/config/check/PhabricatorSetupCheckExtraConfig.php',
     'PhabricatorSetupCheckTimezone' => 'applications/config/check/PhabricatorSetupCheckTimezone.php',
     'PhabricatorSetupIssue' => 'applications/config/issue/PhabricatorSetupIssue.php',
     'PhabricatorSetupIssueView' => 'applications/config/view/PhabricatorSetupIssueView.php',
@@ -1884,6 +1887,7 @@ phutil_register_library_map(array(
     'PackageDeleteMail' => 'PackageMail',
     'PackageModifyMail' => 'PackageMail',
     'Phabricator404Controller' => 'PhabricatorController',
+    'PhabricatorAccessLogConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorActionListExample' => 'PhabricatorUIExample',
     'PhabricatorActionListView' => 'AphrontView',
     'PhabricatorActionView' => 'AphrontView',
@@ -2050,6 +2054,7 @@ phutil_register_library_map(array(
     'PhabricatorDaemonTimelineEventController' => 'PhabricatorDaemonController',
     'PhabricatorDefaultFileStorageEngineSelector' => 'PhabricatorFileStorageEngineSelector',
     'PhabricatorDefaultSearchEngineSelector' => 'PhabricatorSearchEngineSelector',
+    'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDifferentialConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorDirectoryController' => 'PhabricatorController',
     'PhabricatorDirectoryMainController' => 'PhabricatorDirectoryController',
@@ -2427,6 +2432,7 @@ phutil_register_library_map(array(
     'PhabricatorSettingsPanelSSHKeys' => 'PhabricatorSettingsPanel',
     'PhabricatorSettingsPanelSearchPreferences' => 'PhabricatorSettingsPanel',
     'PhabricatorSetupCheckAPC' => 'PhabricatorSetupCheck',
+    'PhabricatorSetupCheckExtraConfig' => 'PhabricatorSetupCheck',
     'PhabricatorSetupCheckTimezone' => 'PhabricatorSetupCheck',
     'PhabricatorSetupIssueView' => 'AphrontView',
     'PhabricatorSlowvoteChoice' => 'PhabricatorSlowvoteDAO',

src/applications/config/check/PhabricatorSetupCheckExtraConfig.php

@@ -0,0 +1,31 @@
+<?php
+
+final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
+
+  protected function executeChecks() {
+    $all_keys = PhabricatorEnv::getAllConfigKeys();
+    $all_keys = array_keys($all_keys);
+    sort($all_keys);
+
+    $defined_keys = PhabricatorApplicationConfigOptions::loadAllOptions();
+
+    foreach ($all_keys as $key) {
+      if (isset($defined_keys[$key])) {
+        continue;
+      }
+      $summary = pht("This option is not recognized. It may be misspelled.");
+      $message = pht(
+        "This configuration option 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.");
+
+      $this
+        ->newIssue('config.unknown.'.$key)
+        ->setShortName(pht('Unknown Config'))
+        ->setName(pht('Unknown Configuration Option "%s"', $key))
+        ->setSummary($summary)
+        ->setMessage($message)
+        ->addPhabricatorConfig($key);
+    }
+  }
+}

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -30,56 +30,37 @@ final class PhabricatorConfigEditController
       $done_uri = $group_uri;
     }
 
-    // TODO: This isn't quite correct -- we should read from the entire
-    // configuration stack, ignoring database configuration. For now, though,
-    // it's a reasonable approximation.
-    $default_value = $option->getDefault();
-
-    $default = $this->prettyPrintJSON($default_value);
-
     // Check if the config key is already stored in the database.
     // Grab the value if it is.
-    $value = null;
     $config_entry = id(new PhabricatorConfigEntry())
       ->loadOneWhere(
         'configKey = %s AND namespace = %s',
         $this->key,
         'default');
-    if ($config_entry) {
+    if (!$config_entry) {
-      $value = $config_entry->getValue();
-    } else {
       $config_entry = id(new PhabricatorConfigEntry())
-        ->setConfigKey($this->key);
+        ->setConfigKey($this->key)
+        ->setNamespace('default')
+        ->setIsDeleted(true);
     }
 
     $e_value = null;
     $errors = array();
     if ($request->isFormPost()) {
 
-      $new_value = $request->getStr('value');
+      list($e_value, $value_errors, $display_value) = $this->readRequest(
-      if (strlen($new_value)) {
+        $option,
-        $json = json_decode($new_value, true);
+        $config_entry,
-        if ($json === null && strtolower($new_value) != 'null') {
+        $request);
-          $e_value = 'Invalid';
-          $errors[] = 'The given value must be valid JSON. This means, among '.
-            'other things, that you must wrap strings in double-quotes.';
-          $value = $new_value;
-        } else {
-          $value = $json;
-        }
-      } else {
-        // TODO: When we do Transactions, make this just set isDeleted = 1
-        $config_entry->delete();
-        return id(new AphrontRedirectResponse())->setURI($done_uri);
-      }
 
-      $config_entry->setValue($value);
+      $errors = array_merge($errors, $value_errors);
-      $config_entry->setNamespace('default');
 
       if (!$errors) {
         $config_entry->save();
         return id(new AphrontRedirectResponse())->setURI($done_uri);
       }
+    } else {
+      $display_value = $this->getDisplayValue($option, $config_entry);
     }
 
     $form = new AphrontFormView();
@@ -88,27 +69,44 @@ final class PhabricatorConfigEditController
     $error_view = null;
     if ($errors) {
       $error_view = id(new AphrontErrorView())
-        ->setTitle('You broke everything!')
+        ->setTitle(pht('You broke everything!'))
         ->setErrors($errors);
-    } else {
-      $value = $this->prettyPrintJSON($value);
     }
 
+    $control = $this->renderControl(
+      $option,
+      $display_value,
+      $e_value);
+
+    $engine = new PhabricatorMarkupEngine();
+    $engine->addObject($option, 'description');
+    $engine->process();
+    $description = phutil_render_tag(
+      'div',
+      array(
+        'class' => 'phabricator-remarkup',
+      ),
+      $engine->getOutput($option, 'description'));
+
     $form
       ->setUser($user)
       ->addHiddenInput('issue', $request->getStr('issue'))
       ->appendChild(
-        id(new AphrontFormTextAreaControl())
+        id(new AphrontFormMarkupControl())
-          ->setLabel('JSON Value')
+          ->setLabel('Description')
-          ->setError($e_value)
+          ->setValue($description))
-          ->setValue($value)
+      ->appendChild($control)
-          ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL)
-          ->setCustomClass('PhabricatorMonospaced')
-        ->setName('value'))
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->addCancelButton($done_uri)
-          ->setValue(pht('Save Config Entry')))
+          ->setValue(pht('Save Config Entry')));
+
+
+    // TODO: This isn't quite correct -- we should read from the entire
+    // configuration stack, ignoring database configuration. For now, though,
+    // it's a reasonable approximation.
+    $default = $this->prettyPrintJSON($option->getDefault());
+    $form
       ->appendChild(
         phutil_render_tag(
           'p',
@@ -156,4 +154,121 @@ final class PhabricatorConfigEditController
       ));
   }
 
+  private function readRequest(
+    PhabricatorConfigOption $option,
+    PhabricatorConfigEntry $entry,
+    AphrontRequest $request) {
+
+    $e_value = null;
+    $errors = array();
+
+    $value = $request->getStr('value');
+    if (!strlen($value)) {
+      $value = null;
+      $entry->setValue($value);
+      $entry->setIsDeleted(true);
+      return array($e_value, $errors, $value);
+    } else {
+      $entry->setIsDeleted(false);
+    }
+
+    $type = $option->getType();
+    switch ($type) {
+      case 'int':
+        if (preg_match('/^-?[0-9]+$/', trim($value))) {
+          $entry->setValue((int)$value);
+        } else {
+          $e_value = pht('Invalid');
+          $errors[] = pht('Value must be an integer.');
+        }
+        break;
+      case 'string':
+        break;
+      case 'bool':
+        switch ($value) {
+          case 'true':
+            $entry->setValue(true);
+            break;
+          case 'false':
+            $entry->setValue(false);
+            break;
+          default:
+            $e_value = pht('Invalid');
+            $errors[] = pht('Value must be boolean, "true" or "false".');
+            break;
+        }
+        break;
+      default:
+        $json = json_decode($value, true);
+        if ($json === null && strtolower($value) != 'null') {
+          $e_value = pht('Invalid');
+          $errors[] = pht(
+            'The given value must be valid JSON. This means, among '.
+            'other things, that you must wrap strings in double-quotes.');
+          $entry->setValue($json);
+        }
+        break;
+    }
+
+    return array($e_value, $errors, $value);
+  }
+
+  private function getDisplayValue(
+    PhabricatorConfigOption $option,
+    PhabricatorConfigEntry $entry) {
+
+    if ($entry->getIsDeleted()) {
+      return null;
+    }
+
+    $type = $option->getType();
+    $value = $entry->getValue();
+    switch ($type) {
+      case 'int':
+      case 'string':
+        return $value;
+      case 'bool':
+        return $value ? 'true' : 'false';
+      default:
+        return $this->prettyPrintJSON($value);
+    }
+  }
+
+  private function renderControl(
+    PhabricatorConfigOption $option,
+    $display_value,
+    $e_value) {
+
+    $type = $option->getType();
+    switch ($type) {
+      case 'int':
+      case 'string':
+        $control = id(new AphrontFormTextControl());
+        break;
+      case 'bool':
+        $control = id(new AphrontFormSelectControl())
+          ->setOptions(
+            array(
+              ''      => '(Use Default)',
+              'true'  => idx($option->getOptions(), 0),
+              'false' => idx($option->getOptions(), 1),
+            ));
+        break;
+      default:
+        $control = id(new AphrontFormTextAreaControl())
+          ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL)
+          ->setCustomClass('PhabricatorMonospaced')
+          ->setCaption(pht('Enter value in JSON.'));
+        break;
+    }
+
+    $control
+      ->setLabel('Value')
+      ->setError($e_value)
+      ->setValue($display_value)
+      ->setName('value');
+
+    return $control;
+  }
+
 }

src/applications/config/option/PhabricatorAccessLogConfigOptions.php

@@ -0,0 +1,74 @@
+<?php
+
+final class PhabricatorAccessLogConfigOptions
+  extends PhabricatorApplicationConfigOptions {
+
+  public function getName() {
+    return pht("Access Log");
+  }
+
+  public function getDescription() {
+    return pht("Configure the access log, which logs all requests.");
+  }
+
+  public function getOptions() {
+    $map = array(
+      'c' => pht("The HTTP response code."),
+      'C' => pht("The controller which handled the request."),
+      'D' => pht("The request date."),
+      'e' => pht("Epoch timestamp."),
+      'h' => pht("The webserver's host name."),
+      'p' => pht("The PID of the server process."),
+      'R' => pht("The HTTP referrer."),
+      'r' => pht("The remote IP."),
+      'T' => pht("The request duration, in microseconds."),
+      'U' => pht("The request path."),
+      'u' => pht("The logged-in user, if one is logged in."),
+      'M' => pht("The HTTP method."),
+      'm' => pht("For conduit, the Conduit method which was invoked."),
+    );
+
+    $fdesc = pht("Format for the access log. Available variables are:");
+    $fdesc .= "\n\n";
+    foreach ($map as $key => $desc) {
+      $fdesc .= "  - %".$key." ".$desc."\n";
+    }
+    $fdesc .= "\n";
+    $fdesc .= pht(
+      "If a variable isn't available (for example, %%m appears in the file ".
+      "format but the request is not a Conduit request), it will be rendered ".
+      "as '-'");
+    $fdesc .= "\n\n";
+    $fdesc .= pht(
+      "Note that the default format is subject to change in the future, so ".
+      "if you rely on the log's format, specify it explicitly.");
+
+    return array(
+      $this->newOption('log.access.path', 'string', null)
+        ->setSummary(pht("Access log location."))
+        ->setDescription(
+          pht(
+            "To enable the Phabricator access log, specify a path. The ".
+            "access log can provide more detailed information about ".
+            "Phabricator access than normal HTTP access logs (for instance, ".
+            "it can show logged-in users, controllers, and other application ".
+            "data).\n\n".
+            "If not set, no log will be written."))
+        ->addExample(
+          null,
+          pht('Disable access log'))
+        ->addExample(
+          '/var/log/phabricator/access.log',
+          pht('Write access log here')),
+      $this->newOption(
+        'log.access.format',
+        // NOTE: This is 'wild' intead of 'string' so "\t" and such can be
+        // specified.
+        'wild',
+        "[%D]\t%p\t%h\t%r\t%u\t%C\t%m\t%U\t%R\t%c\t%T")
+        ->setSummary(pht("Access log format."))
+        ->setDescription($fdesc),
+    );
+  }
+
+}

src/applications/config/option/PhabricatorConfigOption.php

@@ -1,6 +1,8 @@
 <?php
 
-final class PhabricatorConfigOption extends Phobject {
+final class PhabricatorConfigOption
+  extends Phobject
+  implements PhabricatorMarkupInterface{
 
   private $key;
   private $default;
@@ -9,7 +11,16 @@ final class PhabricatorConfigOption extends Phobject {
   private $type;
   private $options;
   private $group;
+  private $examples;
 
+  public function addExample($value, $description) {
+    $this->examples[] = array($value, $description);
+    return $this;
+  }
+
+  public function getExamples() {
+    return $this->examples;
+  }
 
   public function setGroup(PhabricatorApplicationConfigOptions $group) {
     $this->group = $group;
@@ -74,4 +85,26 @@ final class PhabricatorConfigOption extends Phobject {
     return $this->type;
   }
 
+/* -(  PhabricatorMarkupInterface  )----------------------------------------- */
+
+  public function getMarkupFieldKey($field) {
+    return $this->getKey().':'.$field;
+  }
+
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newMarkupEngine(array());
+  }
+
+  public function getMarkupText($field) {
+    return $this->getDescription();
+  }
+
+  public function didMarkupText($field, $output, PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+  public function shouldUseMarkupCache($field) {
+    return false;
+  }
+
 }

src/applications/config/option/PhabricatorDeveloperConfigOptions.php

@@ -0,0 +1,92 @@
+<?php
+
+final class PhabricatorDeveloperConfigOptions
+  extends PhabricatorApplicationConfigOptions {
+
+  public function getName() {
+    return pht("Developer / Debugging");
+  }
+
+  public function getDescription() {
+    return pht("Options for Phabricator developers, including debugging.");
+  }
+
+  public function getOptions() {
+    return array(
+      $this->newOption('darkconsole.enabled', 'bool', false)
+        ->setOptions(
+          array(
+            pht("Disable DarkConsole"),
+            pht("Enable DarkConsole"),
+          ))
+        ->setSummary(pht("Enable Phabricator's debugging console."))
+        ->setDescription(
+          pht(
+            "DarkConsole is a development and profiling tool built into ".
+            "Phabricator's web interface. You should leave it disabled unless ".
+            "you are developing or debugging Phabricator.\n\n".
+            "Set this option to enable DarkConsole, which will put a link ".
+            "in the page footer to actually activate it. Once activated, ".
+            "it will appear at the top of every page and can be toggled ".
+            "by pressing the '`' key.\n\n".
+            "DarkConsole exposes potentially sensitive data (like queries, ".
+            "stack traces, and configuration) so you generally should not ".
+            "turn it on in production.")),
+      $this->newOption('darkconsole.always-on', 'bool', false)
+        ->setOptions(
+          array(
+            pht("Require DarkConsole Activation"),
+            pht("Always Activate DarkConsole"),
+          ))
+        ->setSummary(pht("Activate DarkConsole on every page."))
+        ->setDescription(
+          pht(
+            "This option allows you to enable DarkConsole on every page, ".
+            "even for logged-out users. This is only really useful if you ".
+            "need to debug something on a logged-out page. You should not ".
+            "enable this option in production.\n\n".
+            "You must enable DarkConsole by setting {{darkconsole.enabled}} ".
+            "before this option will have any effect.")),
+      $this->newOption('debug.stop-on-redirect', 'bool', false)
+        ->setOptions(
+          array(
+            pht("Use Normal HTTP Redirects"),
+            pht("Stop Before HTTP Redirect"),
+          ))
+        ->setSummary(
+          pht(
+            "Confirm before redirecting so DarkConsole can be examined."))
+        ->setDescription(
+          pht(
+            "Normally, Phabricator issues HTTP redirects after a successful ".
+            "POST. This can make it difficult to debug things which happen ".
+            "while processing the POST, because service and profiling ".
+            "information are lost. By setting this configuration option, ".
+            "Phabricator will show a page instead of automatically ".
+            "redirecting, allowing you to examine service and profiling ".
+            "information. It also makes the UX awful, so you should only ".
+            "enable it when debugging.")),
+      $this->newOption('debug.profile-rate', 'int', 0)
+        ->addExample(0,     pht('No profiling'))
+        ->addExample(1,     pht('Profile every request (slow)'))
+        ->addExample(1000,  pht('Profile 0.1%% of all requests'))
+        ->setSummary(pht("Automatically profile some percentage of pages."))
+        ->setDescription(
+          pht(
+            "Normally, Phabricator profiles pages only when explicitly ".
+            "requested via DarkConsole. However, it may be useful to profile ".
+            "some pages automatically.\n\n".
+            "Set this option to a positive integer N to profile 1 / N pages ".
+            "automatically. For example, setting it to 1 will profile every ".
+            "page, while setting it to 1000 will profile 1 page per 1000 ".
+            "requests (i.e., 0.1%% of requests).\n\n".
+            "Since profiling is slow and generates a lot of data, you should ".
+            "set this to 0 in production (to disable it) or to a large number ".
+            "(to collect a few samples, if you're interested in having some ".
+            "data to look at eventually). In development, it may be useful to ".
+            "set it to 1 in order to debug performance problems.\n\n".
+            "NOTE: You must install XHProf for profiling to work.")),
+    );
+  }
+
+}