Repair invalid configuration by setting values back to defaults

Summary:
When configuration is set incorrectly (e.g., of the wrong type), detect and repair it by setting it to the default value. A setup warning will be raised separately.

Notably, this removes the need to hard-code all the class types.

This runs separately from the "invalid config" check because we need to run it on every page, but do setup checks only once per restart (some of them are slow).

Also dirty setup when we edit configuration.

Test Plan: Set config incorrectly on purpose, saw Phabricator correct it on restart and on every subsequent page load until it was fixed.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2292

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

src/applications/config/check/PhabricatorSetupCheck.php

@@ -34,20 +34,41 @@ abstract class PhabricatorSetupCheck {
     $cache->setKey('phabricator.setup.issues', $count);
   }
 
+  final public static function getConfigNeedsRepair() {
+    $cache = PhabricatorCaches::getSetupCache();
+    return $cache->getKey('phabricator.setup.needs-repair');
+  }
+
+  final public static function setConfigNeedsRepair($needs_repair) {
+    $cache = PhabricatorCaches::getSetupCache();
+    $cache->setKey('phabricator.setup.needs-repair', $needs_repair);
+  }
+
+  final public static function deleteSetupCheckCache() {
+    $cache = PhabricatorCaches::getSetupCache();
+    $cache->deleteKeys(
+      array(
+        'phabricator.setup.needs-repair',
+        'phabricator.setup.issues',
+      ));
+  }
+
   final public static function willProcessRequest() {
     $issue_count = self::getOpenSetupIssueCount();
-    if ($issue_count !== null) {
+    if ($issue_count === null) {
-      // We've already run setup checks, didn't hit any fatals, and then set
+      $issues = self::runAllChecks();
-      // an issue count. This means we're good and don't need to do any extra
+      self::setOpenSetupIssueCount(count($issues));
-      // work.
-      return null;
     }
 
-    $issues = self::runAllChecks();
+    // Try to repair configuration unless we have a clean bill of health on it.
-
+    // We need to keep doing this on every page load until all the problems
-    self::setOpenSetupIssueCount(count($issues));
+    // are fixed, which is why it's separate from setup checks (which run
-
+    // once per restart).
-    return null;
+    $needs_repair = self::getConfigNeedsRepair();
+    if ($needs_repair !== false) {
+      $needs_repair = self::repairConfig();
+      self::setConfigNeedsRepair($needs_repair);
+    }
   }
 
   final public static function runAllChecks() {
@@ -76,4 +97,22 @@ abstract class PhabricatorSetupCheck {
     return $issues;
   }
 
+  final public static function repairConfig() {
+    $needs_repair = false;
+
+    $options = PhabricatorApplicationConfigOptions::loadAllOptions();
+    foreach ($options as $option) {
+      try {
+        $option->getGroup()->validateOption(
+          $option,
+          PhabricatorEnv::getEnvConfig($option->getKey()));
+      } catch (PhabricatorConfigValidationException $ex) {
+        PhabricatorEnv::repairConfig($option->getKey(), $option->getDefault());
+        $needs_repair = true;
+      }
+    }
+
+    return $needs_repair;
+  }
+
 }

src/applications/config/check/PhabricatorSetupCheckInvalidConfig.php

@@ -10,14 +10,15 @@ final class PhabricatorSetupCheckInvalidConfig extends PhabricatorSetupCheck {
         try {
           $group->validateOption(
             $option,
-            PhabricatorEnv::getEnvConfig($option->getKey()));
+            PhabricatorEnv::getUnrepairedEnvConfig($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",
+                "Configuration option '%s' has invalid value and ".
+                "was restored to the default: %s",
                 $option->getKey(),
                 $ex->getMessage()))
             ->addPhabricatorConfig($option->getKey());

src/applications/config/editor/PhabricatorConfigEditor.php

@@ -99,4 +99,9 @@ final class PhabricatorConfigEditor
     return parent::transactionHasEffect($object, $xaction);
   }
 
+  protected function didApplyTransactions(array $xactions) {
+    // Force all the setup checks to run on the next page load.
+    PhabricatorSetupCheck::deleteSetupCheckCache();
+  }
+
 }

src/applications/config/view/PhabricatorSetupIssueView.php

@@ -125,7 +125,7 @@ final class PhabricatorSetupIssueView extends AphrontView {
       $table[] = '<tr>';
       $table[] = '<th>'.phutil_escape_html($key).'</th>';
 
-      $value = PhabricatorEnv::getEnvConfig($key);
+      $value = PhabricatorEnv::getUnrepairedEnvConfig($key);
       if ($value === null) {
         $value = '<em>null</em>';
       } else if ($value === false) {

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -287,9 +287,16 @@ abstract class PhabricatorApplicationTransactionEditor
         $mailed);
     }
 
+    $this->didApplyTransactions($xactions);
+
     return $xactions;
   }
 
+  protected function didApplyTransactions(array $xactions) {
+    // Hook for subclasses.
+    return;
+  }
+
   private function loadHandles(array $xactions) {
     $phids = array();
     foreach ($xactions as $key => $xaction) {

src/infrastructure/PhabricatorSetup.php

@@ -716,29 +716,6 @@ final class PhabricatorSetup {
       self::write("[OKAY] Mail configuration OKAY\n");
     }
 
-    self::writeHeader('CONFIG CLASSES');
-    foreach (PhabricatorEnv::getRequiredClasses() as $key => $instanceof) {
-      $config = PhabricatorEnv::getEnvConfig($key);
-      if (!$config) {
-        self::writeNote("'$key' is not set.");
-      } else {
-        try {
-          $r = new ReflectionClass($config);
-          if (!$r->isSubclassOf($instanceof)) {
-            throw new Exception(
-              "Config setting '$key' must be an instance of '$instanceof'.");
-          } else if (!$r->isInstantiable()) {
-            throw new Exception("Config setting '$key' must be instantiable.");
-          }
-        } catch (Exception $ex) {
-          self::writeFailure();
-          self::write("Setup failure! ".$ex->getMessage());
-          return;
-        }
-      }
-    }
-    self::write("[OKAY] Config classes OKAY\n");
-
     self::writeHeader('SUCCESS!');
     self::write(
       "Congratulations! Your setup seems mostly correct, or at least fairly ".

src/infrastructure/env/PhabricatorEnv.php

@@ -51,6 +51,7 @@
 final class PhabricatorEnv {
 
   private static $sourceStack;
+  private static $repairSource;
 
   /**
    * @phutil-external-symbol class PhabricatorStartup
@@ -155,6 +156,28 @@ final class PhabricatorEnv {
         ->setName(pht("Database")));
   }
 
+  public static function repairConfig($key, $value) {
+    if (!self::$repairSource) {
+      self::$repairSource = id(new PhabricatorConfigDictionarySource(array()))
+        ->setName(pht("Repaired Config"));
+      self::$sourceStack->pushSource(self::$repairSource);
+    }
+    self::$repairSource->setKeys(array($key => $value));
+  }
+
+  public static function getUnrepairedEnvConfig($key, $default = null) {
+    foreach (self::$sourceStack->getStack() as $source) {
+      if ($source === self::$repairSource) {
+        continue;
+      }
+      $result = $source->getKeys(array($key));
+      if ($result) {
+        return $result[$key];
+      }
+    }
+    return $default;
+  }
+
   public static function getSelectedEnvironmentName() {
     $env_var = 'PHABRICATOR_ENV';
 
@@ -259,13 +282,7 @@ final class PhabricatorEnv {
    */
   public static function newObjectFromConfig($key, $args = array()) {
     $class = self::getEnvConfig($key);
-    $object = newv($class, $args);
+    return newv($class, $args);
-    $instanceof = idx(self::getRequiredClasses(), $key);
-    if (!($object instanceof $instanceof)) {
-      throw new Exception("Config setting '$key' must be an instance of ".
-        "'$instanceof', is '".get_class($object)."'.");
-    }
-    return $object;
   }
 
 
@@ -386,33 +403,6 @@ final class PhabricatorEnv {
 /* -(  Internals  )---------------------------------------------------------- */
 
 
-  /**
-   * @task internal
-   */
-  public static function getRequiredClasses() {
-    return array(
-      'translation.provider' => 'PhabricatorTranslation',
-      'metamta.mail-adapter' => 'PhabricatorMailImplementationAdapter',
-      'metamta.maniphest.reply-handler' => 'PhabricatorMailReplyHandler',
-      'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler',
-      'metamta.diffusion.reply-handler' => 'PhabricatorMailReplyHandler',
-      'metamta.package.reply-handler' => 'PhabricatorMailReplyHandler',
-      'storage.engine-selector' => 'PhabricatorFileStorageEngineSelector',
-      'search.engine-selector' => 'PhabricatorSearchEngineSelector',
-      'differential.field-selector' => 'DifferentialFieldSelector',
-      'maniphest.custom-task-extensions-class' => 'ManiphestTaskExtensions',
-      'aphront.default-application-configuration-class' =>
-        'AphrontApplicationConfiguration',
-      'controller.oauth-registration' =>
-        'PhabricatorOAuthRegistrationController',
-      'mysql.implementation' => 'AphrontMySQLDatabaseConnectionBase',
-      'differential.attach-task-class' => 'DifferentialTasksAttacher',
-      'mysql.configuration-provider' => 'DatabaseConfigurationProvider',
-      'syntax-highlighter.engine' => 'PhutilSyntaxHighlighterEngine',
-    );
-  }
-
-
   /**
    * @task internal
    */