From b62ecb7c1159da660763dcb422da94881cfc4e66 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Jun 2013 11:01:19 -0700 Subject: [PATCH] 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 --- .../PhabricatorSetupCheckExtraConfig.php | 140 ++++++++++++++++-- .../PhabricatorConfigEditController.php | 31 ++-- ...bricatorConfigManagementDeleteWorkflow.php | 7 - .../config/view/PhabricatorSetupIssueView.php | 2 +- 4 files changed, 141 insertions(+), 39 deletions(-) diff --git a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php index 22038e7cf6..44455cb203 100644 --- a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php +++ b/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 - ->newIssue('config.unknown.'.$key) - ->setShortName(pht('Unknown Config')) - ->setName(pht('Unknown Configuration Option "%s"', $key)) - ->setSummary($summary) - ->setMessage($message) - ->addPhabricatorConfig($key); + if (isset($ancient_config[$key])) { + $summary = pht( + 'This option has been removed. You may delete it at your '. + 'convenience.'); + $message = pht( + "The configuration option '%s' has been removed. You may delete ". + "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; + } } diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 1dcfc3796f..fcc73511fd 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/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( - pht( - "This configuration option is unknown. It may be misspelled, ". - "or have existed in a previous version of Phabricator.")); + ->setDescription($desc); $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( '%s%s', diff --git a/src/applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php b/src/applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php index d765b5ffe9..cf1f05be74 100644 --- a/src/applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php +++ b/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) { diff --git a/src/applications/config/view/PhabricatorSetupIssueView.php b/src/applications/config/view/PhabricatorSetupIssueView.php index 5736c66c43..ffa21ecd40 100644 --- a/src/applications/config/view/PhabricatorSetupIssueView.php +++ b/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(