Allow configuration to be explicitly validated, including validation of complex attributes

Summary:
  - Allows us to implement setup warnings for edits which don't go through the web UI, e.g. "you edited a config file and set value X to something goofy".
  - Allows us to implement more sophisticated validations, beyond basic type checks (e.g., "phabricator.base-uri" must be a URI).
  - Fixes T358 (or, close enough -- fixes it for all options which have been migrated as per T2255.

Test Plan: Set "darkconsole.enabled" to "xyz" in my config, observed setup warning. Added fake validation, observed web UI edit error.

Reviewers: codeblock, btrahan

Reviewed By: codeblock

CC: aran

Maniphest Tasks: T2255, T358

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

src/__phutil_library_map__.php

@@ -704,6 +704,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigStackSource' => 'infrastructure/env/PhabricatorConfigStackSource.php',
     'PhabricatorConfigTransaction' => 'applications/config/storage/PhabricatorConfigTransaction.php',
     'PhabricatorConfigTransactionQuery' => 'applications/config/query/PhabricatorConfigTransactionQuery.php',
+    'PhabricatorConfigValidationException' => 'applications/config/exception/PhabricatorConfigValidationException.php',
     'PhabricatorContentSource' => 'applications/metamta/contentsource/PhabricatorContentSource.php',
     'PhabricatorContentSourceView' => 'applications/metamta/contentsource/PhabricatorContentSourceView.php',
     'PhabricatorController' => 'applications/base/controller/PhabricatorController.php',
@@ -1155,6 +1156,7 @@ phutil_register_library_map(array(
     'PhabricatorSetupCheck' => 'applications/config/check/PhabricatorSetupCheck.php',
     'PhabricatorSetupCheckAPC' => 'applications/config/check/PhabricatorSetupCheckAPC.php',
     'PhabricatorSetupCheckExtraConfig' => 'applications/config/check/PhabricatorSetupCheckExtraConfig.php',
+    'PhabricatorSetupCheckInvalidConfig' => 'applications/config/check/PhabricatorSetupCheckInvalidConfig.php',
     'PhabricatorSetupCheckTimezone' => 'applications/config/check/PhabricatorSetupCheckTimezone.php',
     'PhabricatorSetupIssue' => 'applications/config/issue/PhabricatorSetupIssue.php',
     'PhabricatorSetupIssueView' => 'applications/config/view/PhabricatorSetupIssueView.php',
@@ -2046,6 +2048,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigStackSource' => 'PhabricatorConfigSource',
     'PhabricatorConfigTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorConfigValidationException' => 'Exception',
     'PhabricatorContentSourceView' => 'AphrontView',
     'PhabricatorController' => 'AphrontController',
     'PhabricatorCountdownController' => 'PhabricatorController',
@@ -2455,6 +2458,7 @@ phutil_register_library_map(array(
     'PhabricatorSettingsPanelSearchPreferences' => 'PhabricatorSettingsPanel',
     'PhabricatorSetupCheckAPC' => 'PhabricatorSetupCheck',
     'PhabricatorSetupCheckExtraConfig' => 'PhabricatorSetupCheck',
+    'PhabricatorSetupCheckInvalidConfig' => 'PhabricatorSetupCheck',
     'PhabricatorSetupCheckTimezone' => 'PhabricatorSetupCheck',
     'PhabricatorSetupIssueView' => 'AphrontView',
     'PhabricatorSlowvoteChoice' => 'PhabricatorSlowvoteDAO',

src/applications/config/check/PhabricatorSetupCheckInvalidConfig.php

@@ -0,0 +1,29 @@
+<?php
+
+final class PhabricatorSetupCheckInvalidConfig extends PhabricatorSetupCheck {
+
+  protected function executeChecks() {
+    $groups = PhabricatorApplicationConfigOptions::loadAll();
+    foreach ($groups as $group) {
+      $options = $group->getOptions();
+      foreach ($options as $option) {
+        try {
+          $group->validateOption(
+            $option,
+            PhabricatorEnv::getEnvConfig($option->getKey()));
+        } catch (PhabricatorConfigValidationException $ex) {
+          $this
+            ->newIssue('config.invalid.'.$option->getKey())
+            ->setName(pht("Config '%s' Invalid", $option->getKey()))
+            ->setMessage(
+              pht(
+                "Configuration option '%s' has invalid value: %s",
+                $option->getKey(),
+                $ex->getMessage()))
+            ->addPhabricatorConfig($option->getKey());
+        }
+      }
+    }
+  }
+
+}

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -78,10 +78,15 @@ final class PhabricatorConfigEditController
               PhabricatorContentSource::SOURCE_WEB,
               array(
                 'ip' => $request->getRemoteAddr(),
-              )))
-          ->applyTransactions($config_entry, array($xaction));
+              )));
 
+        try {
+          $editor->applyTransactions($config_entry, array($xaction));
           return id(new AphrontRedirectResponse())->setURI($done_uri);
+        } catch (PhabricatorConfigValidationException $ex) {
+          $e_value = pht('Invalid');
+          $errors[] = $ex->getMessage();
+        }
       }
     } else {
       $display_value = $this->getDisplayValue($option, $config_entry);

src/applications/config/editor/PhabricatorConfigEditor.php

@@ -42,6 +42,18 @@ final class PhabricatorConfigEditor
       case PhabricatorConfigTransaction::TYPE_EDIT:
         $v = $xaction->getNewValue();
 
+        // If this is a defined configuration option (vs a straggler from an
+        // old version of Phabricator or a configuration file misspelling)
+        // submit it to the validation gauntlet.
+        $key = $object->getConfigKey();
+        $all_options = PhabricatorApplicationConfigOptions::loadAllOptions();
+        $option = idx($all_options, $key);
+        if ($option) {
+          $option->getGroup()->validateOption(
+            $option,
+            $v['value']);
+        }
+
         $object->setIsDeleted($v['deleted']);
         $object->setValue($v['value']);
         break;

src/applications/config/exception/PhabricatorConfigValidationException.php

@@ -0,0 +1,5 @@
+<?php
+
+final class PhabricatorConfigValidationException extends Exception {
+
+}

src/applications/config/option/PhabricatorApplicationConfigOptions.php

@@ -6,6 +6,56 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject {
   abstract public function getDescription();
   abstract public function getOptions();
 
+  public function validateOption(PhabricatorConfigOption $option, $value) {
+    if ($value === $option->getDefault()) {
+      return;
+    }
+
+    if ($value === null) {
+      return;
+    }
+
+    switch ($option->getType()) {
+      case 'bool':
+        if ($value !== true &&
+            $value !== false) {
+          throw new PhabricatorConfigValidationException(
+            pht(
+              "Option '%s' is of type bool, but value is not true or false.",
+              $option->getKey()));
+        }
+        break;
+      case 'int':
+        if (!is_int($value)) {
+          throw new PhabricatorConfigValidationException(
+            pht(
+              "Option '%s' is of type int, but value is not an integer.",
+              $option->getKey()));
+        }
+        break;
+      case 'string':
+        if (!is_string($value)) {
+          throw new PhabricatorConfigValidationException(
+            pht(
+              "Option '%s' is of type string, but value is not a string.",
+              $option->getKey()));
+        }
+        break;
+      case 'wild':
+      default:
+        break;
+    }
+
+    $this->didValidateOption($option, $value);
+  }
+
+  protected function didValidateOption(
+    PhabricatorConfigOption $option,
+    $value) {
+    // Hook for subclasses to do complex validation.
+    return;
+  }
+
   public function getKey() {
     $class = get_class($this);
     $matches = null;