From b0d815d1574c4275759ae6ad55f54096a9a0f5bc Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Jan 2013 16:25:38 -0800 Subject: [PATCH] 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 --- .../config/check/PhabricatorSetupCheck.php | 59 +++++++++++++++---- .../PhabricatorSetupCheckInvalidConfig.php | 5 +- .../config/editor/PhabricatorConfigEditor.php | 5 ++ .../config/view/PhabricatorSetupIssueView.php | 2 +- ...habricatorApplicationTransactionEditor.php | 7 +++ src/infrastructure/PhabricatorSetup.php | 23 -------- src/infrastructure/env/PhabricatorEnv.php | 58 ++++++++---------- 7 files changed, 89 insertions(+), 70 deletions(-) diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 187236c174..4648965e4e 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/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) { - // We've already run setup checks, didn't hit any fatals, and then set - // an issue count. This means we're good and don't need to do any extra - // work. - return null; + if ($issue_count === null) { + $issues = self::runAllChecks(); + self::setOpenSetupIssueCount(count($issues)); } - $issues = self::runAllChecks(); - - self::setOpenSetupIssueCount(count($issues)); - - return null; + // 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 + // are fixed, which is why it's separate from setup checks (which run + // once per restart). + $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; + } + } diff --git a/src/applications/config/check/PhabricatorSetupCheckInvalidConfig.php b/src/applications/config/check/PhabricatorSetupCheckInvalidConfig.php index 39b7d77cf3..d17c830ee0 100644 --- a/src/applications/config/check/PhabricatorSetupCheckInvalidConfig.php +++ b/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()); diff --git a/src/applications/config/editor/PhabricatorConfigEditor.php b/src/applications/config/editor/PhabricatorConfigEditor.php index 673eff2307..a2ca330fdf 100644 --- a/src/applications/config/editor/PhabricatorConfigEditor.php +++ b/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(); + } + } diff --git a/src/applications/config/view/PhabricatorSetupIssueView.php b/src/applications/config/view/PhabricatorSetupIssueView.php index 2d8004ea00..3d33a7cd6a 100644 --- a/src/applications/config/view/PhabricatorSetupIssueView.php +++ b/src/applications/config/view/PhabricatorSetupIssueView.php @@ -125,7 +125,7 @@ final class PhabricatorSetupIssueView extends AphrontView { $table[] = ''; $table[] = ''.phutil_escape_html($key).''; - $value = PhabricatorEnv::getEnvConfig($key); + $value = PhabricatorEnv::getUnrepairedEnvConfig($key); if ($value === null) { $value = 'null'; } else if ($value === false) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index c9510f636d..ec717468d3 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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) { diff --git a/src/infrastructure/PhabricatorSetup.php b/src/infrastructure/PhabricatorSetup.php index 96d2f725fd..e7e6a30261 100644 --- a/src/infrastructure/PhabricatorSetup.php +++ b/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 ". diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index 970114d9d7..afa90883c6 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/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); - $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; + return newv($class, $args); } @@ -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 */