From a3bff35b2b9bbd7f388a1e7e85a58a1e3b182322 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Jan 2013 14:09:17 -0800 Subject: [PATCH] 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 | 6 + .../PhabricatorSetupCheckExtraConfig.php | 31 +++ .../PhabricatorConfigEditController.php | 197 ++++++++++++++---- .../PhabricatorAccessLogConfigOptions.php | 74 +++++++ .../config/option/PhabricatorConfigOption.php | 35 +++- .../PhabricatorDeveloperConfigOptions.php | 92 ++++++++ 6 files changed, 393 insertions(+), 42 deletions(-) create mode 100644 src/applications/config/check/PhabricatorSetupCheckExtraConfig.php create mode 100644 src/applications/config/option/PhabricatorAccessLogConfigOptions.php create mode 100644 src/applications/config/option/PhabricatorDeveloperConfigOptions.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 83ff936e39..1180538b8d 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php new file mode 100644 index 0000000000..30037b2151 --- /dev/null +++ b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php @@ -0,0 +1,31 @@ +newIssue('config.unknown.'.$key) + ->setShortName(pht('Unknown Config')) + ->setName(pht('Unknown Configuration Option "%s"', $key)) + ->setSummary($summary) + ->setMessage($message) + ->addPhabricatorConfig($key); + } + } +} diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 3f13ab2a15..2e6afed464 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/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) { - $value = $config_entry->getValue(); - } else { + if (!$config_entry) { $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'); - if (strlen($new_value)) { - $json = json_decode($new_value, true); - if ($json === null && strtolower($new_value) != 'null') { - $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); - } + list($e_value, $value_errors, $display_value) = $this->readRequest( + $option, + $config_entry, + $request); - $config_entry->setValue($value); - $config_entry->setNamespace('default'); + $errors = array_merge($errors, $value_errors); 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()) - ->setLabel('JSON Value') - ->setError($e_value) - ->setValue($value) - ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) - ->setCustomClass('PhabricatorMonospaced') - ->setName('value')) + id(new AphrontFormMarkupControl()) + ->setLabel('Description') + ->setValue($description)) + ->appendChild($control) ->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; + } + } diff --git a/src/applications/config/option/PhabricatorAccessLogConfigOptions.php b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php new file mode 100644 index 0000000000..3e5cabd156 --- /dev/null +++ b/src/applications/config/option/PhabricatorAccessLogConfigOptions.php @@ -0,0 +1,74 @@ + 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), + ); + } + +} diff --git a/src/applications/config/option/PhabricatorConfigOption.php b/src/applications/config/option/PhabricatorConfigOption.php index cd120f078a..c5a5ad0d3a 100644 --- a/src/applications/config/option/PhabricatorConfigOption.php +++ b/src/applications/config/option/PhabricatorConfigOption.php @@ -1,6 +1,8 @@ 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; + } + } diff --git a/src/applications/config/option/PhabricatorDeveloperConfigOptions.php b/src/applications/config/option/PhabricatorDeveloperConfigOptions.php new file mode 100644 index 0000000000..4d0dadde04 --- /dev/null +++ b/src/applications/config/option/PhabricatorDeveloperConfigOptions.php @@ -0,0 +1,92 @@ +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.")), + ); + } + +}