From 3e6fa4365811fdf635e2d658a36b19a05fd2a7b3 Mon Sep 17 00:00:00 2001 From: Nick Pellegrino Date: Sat, 19 Jan 2013 12:11:11 -0800 Subject: [PATCH] getConfigEnv fails fast when key is not found and no default value is given. Summary: T2345 getConfig throws an Exception when the key does not exist. Also removes dead code that throws an Exception. Test Plan: Reloaded the Phabricator home page. In the process, found 2 Exceptions thrown due to nonexistent keys. After addressing these problems, the home page loads without Exceptions. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D4541 --- .../config/option/PhabricatorConfigOption.php | 6 ++--- .../option/PhabricatorCoreConfigOptions.php | 3 +++ .../DifferentialRevisionViewController.php | 6 ++--- .../editor/DifferentialCommentEditor.php | 8 +++--- .../mail/DifferentialCommentMail.php | 4 +-- .../parser/DifferentialChangesetParser.php | 3 +-- .../differential/storage/DifferentialDiff.php | 3 +-- .../feed/PhabricatorFeedStoryPublisher.php | 2 +- .../constants/ManiphestTaskPriority.php | 4 +-- .../storage/PhabricatorMetaMTAMail.php | 5 +--- .../PhabricatorSearchAbstractDocument.php | 3 +-- src/infrastructure/env/PhabricatorEnv.php | 10 ++++++-- src/infrastructure/storage/lisk/LiskDAO.php | 25 ------------------- 13 files changed, 29 insertions(+), 53 deletions(-) diff --git a/src/applications/config/option/PhabricatorConfigOption.php b/src/applications/config/option/PhabricatorConfigOption.php index daed159362..df3a780ab4 100644 --- a/src/applications/config/option/PhabricatorConfigOption.php +++ b/src/applications/config/option/PhabricatorConfigOption.php @@ -41,7 +41,7 @@ final class PhabricatorConfigOption } return idx( - PhabricatorEnv::getEnvConfig('config.mask', array()), + PhabricatorEnv::getEnvConfig('config.mask'), $this->getKey(), false); } @@ -57,7 +57,7 @@ final class PhabricatorConfigOption } return idx( - PhabricatorEnv::getEnvConfig('config.hide', array()), + PhabricatorEnv::getEnvConfig('config.hide'), $this->getKey(), false); } @@ -77,7 +77,7 @@ final class PhabricatorConfigOption } return idx( - PhabricatorEnv::getEnvConfig('config.lock', array()), + PhabricatorEnv::getEnvConfig('config.lock'), $this->getKey(), false); } diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php index 68c8d48233..c10de994ed 100644 --- a/src/applications/config/option/PhabricatorCoreConfigOptions.php +++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php @@ -127,6 +127,9 @@ final class PhabricatorCoreConfigOptions $this->newOption('phabricator.setup', 'bool', false) ->setLocked(true) ->setDescription(pht('Internal / deprecated.')), + $this->newOption('test.value', 'wild', null) + ->setLocked(true) + ->setDescription(pht('Unit test value.')), ); } diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index ca829e26a9..9617844b10 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -562,11 +562,11 @@ final class DifferentialRevisionViewController extends DifferentialController { $status = $revision->getStatus(); $allow_self_accept = PhabricatorEnv::getEnvConfig( - 'differential.allow-self-accept', false); + 'differential.allow-self-accept'); $always_allow_close = PhabricatorEnv::getEnvConfig( - 'differential.always-allow-close', false); + 'differential.always-allow-close'); $allow_reopen = PhabricatorEnv::getEnvConfig( - 'differential.allow-reopen', false); + 'differential.allow-reopen'); if ($viewer_is_owner) { switch ($status) { diff --git a/src/applications/differential/editor/DifferentialCommentEditor.php b/src/applications/differential/editor/DifferentialCommentEditor.php index b1731434ee..386a0d9e22 100644 --- a/src/applications/differential/editor/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/DifferentialCommentEditor.php @@ -99,11 +99,11 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $actor_phid = $actor->getPHID(); $actor_is_author = ($actor_phid == $revision->getAuthorPHID()); $allow_self_accept = PhabricatorEnv::getEnvConfig( - 'differential.allow-self-accept', false); + 'differential.allow-self-accept'); $always_allow_close = PhabricatorEnv::getEnvConfig( - 'differential.always-allow-close', false); + 'differential.always-allow-close'); $allow_reopen = PhabricatorEnv::getEnvConfig( - 'differential.allow-reopen', false); + 'differential.allow-reopen'); $revision_status = $revision->getStatus(); $revision->loadRelationships(); @@ -652,7 +652,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $removed_reviewers = $this->getRemovedReviewers(); $reviewer_phids = $revision->getReviewers(); $allow_self_accept = PhabricatorEnv::getEnvConfig( - 'differential.allow-self-accept', false); + 'differential.allow-self-accept'); $reviewer_phids_map = array_fill_keys($reviewer_phids, true); foreach ($added_reviewers as $k => $user_phid) { diff --git a/src/applications/differential/mail/DifferentialCommentMail.php b/src/applications/differential/mail/DifferentialCommentMail.php index ab93109676..addd8f9a41 100644 --- a/src/applications/differential/mail/DifferentialCommentMail.php +++ b/src/applications/differential/mail/DifferentialCommentMail.php @@ -154,7 +154,7 @@ final class DifferentialCommentMail extends DifferentialMail { $hunk_parser = new DifferentialHunkParser(); if (PhabricatorEnv::getEnvConfig( - 'metamta.differential.unified-comment-context', false)) { + 'metamta.differential.unified-comment-context')) { foreach ($changesets as $changeset) { $changeset->attachHunks($changeset->loadHunks()); } @@ -176,7 +176,7 @@ final class DifferentialCommentMail extends DifferentialMail { $inline_content = $inline->getContent(); if (!PhabricatorEnv::getEnvConfig( - 'metamta.differential.unified-comment-context', false)) { + 'metamta.differential.unified-comment-context')) { $body[] = $this->formatText("{$file}:{$range} {$inline_content}"); } else { $body[] = "================"; diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index cd970e9cd3..cedec46990 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -370,8 +370,7 @@ final class DifferentialChangesetParser { if (!$generated_guess) { $generated_path_regexps = PhabricatorEnv::getEnvConfig( - 'differential.generated-paths', - array()); + 'differential.generated-paths'); foreach ($generated_path_regexps as $regexp) { if (preg_match($regexp, $this->changeset->getFilename())) { $generated_guess = true; diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 5bad0cca8f..da3c7b8b4f 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -258,8 +258,7 @@ final class DifferentialDiff extends DifferentialDAO { ->loadOneWhere('phid = %s', $this->getAuthorPHID()); $use_emails = - PhabricatorEnv::getEnvConfig('differential.expose-emails-prudently', - false); + PhabricatorEnv::getEnvConfig('differential.expose-emails-prudently'); switch ($this->getSourceControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: diff --git a/src/applications/feed/PhabricatorFeedStoryPublisher.php b/src/applications/feed/PhabricatorFeedStoryPublisher.php index b4addafcca..6fe916b0d4 100644 --- a/src/applications/feed/PhabricatorFeedStoryPublisher.php +++ b/src/applications/feed/PhabricatorFeedStoryPublisher.php @@ -104,7 +104,7 @@ final class PhabricatorFeedStoryPublisher { $this->sendNotification($chrono_key); } - $uris = PhabricatorEnv::getEnvConfig('feed.http-hooks', array()); + $uris = PhabricatorEnv::getEnvConfig('feed.http-hooks'); foreach ($uris as $uri) { $task = PhabricatorWorker::scheduleTask( 'FeedPublisherWorker', diff --git a/src/applications/maniphest/constants/ManiphestTaskPriority.php b/src/applications/maniphest/constants/ManiphestTaskPriority.php index 1de52360c0..846805cffc 100644 --- a/src/applications/maniphest/constants/ManiphestTaskPriority.php +++ b/src/applications/maniphest/constants/ManiphestTaskPriority.php @@ -84,9 +84,7 @@ final class ManiphestTaskPriority extends ManiphestConstants { */ public static function getDefaultPriority() { return PhabricatorEnv::getEnvConfig( - 'maniphest.default-priority', - self::PRIORITY_TRIAGE - ); + 'maniphest.default-priority'); } /** diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index c31290e592..86f02f427e 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -738,10 +738,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { * specified by the `metamta.user-address-format` configuration value. */ private function getUserName($user) { - $format = PhabricatorEnv::getEnvConfig( - 'metamta.user-address-format', - 'full' - ); + $format = PhabricatorEnv::getEnvConfig('metamta.user-address-format'); switch ($format) { case 'short': diff --git a/src/applications/search/index/PhabricatorSearchAbstractDocument.php b/src/applications/search/index/PhabricatorSearchAbstractDocument.php index 85da62f1ae..de034bd468 100644 --- a/src/applications/search/index/PhabricatorSearchAbstractDocument.php +++ b/src/applications/search/index/PhabricatorSearchAbstractDocument.php @@ -14,7 +14,6 @@ final class PhabricatorSearchAbstractDocument { private $relationships = array(); public static function getSupportedTypes() { - $more = PhabricatorEnv::getEnvConfig('search.more-document-types', array()); return array( PhabricatorPHIDConstants::PHID_TYPE_DREV => 'Differential Revisions', PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'Repository Commits', @@ -22,7 +21,7 @@ final class PhabricatorSearchAbstractDocument { PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'Phriction Documents', PhabricatorPHIDConstants::PHID_TYPE_USER => 'Phabricator Users', PhabricatorPHIDConstants::PHID_TYPE_QUES => 'Ponder Questions', - ) + $more; + ); } public function setPHID($phid) { diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index 627b70bcbc..3351675413 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -227,11 +227,17 @@ final class PhabricatorEnv { /** * Get the current configuration setting for a given key. * + * If the key is not found, then throw an Exception. + * * @task read */ - public static function getEnvConfig($key, $default = null) { + public static function getEnvConfig($key) { $result = self::$sourceStack->getKeys(array($key)); - return idx($result, $key, $default); + if (array_key_exists($key, $result)) { + return $result[$key]; + } else { + throw new Exception("No config value specified for key '{$key}'."); + } } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index b78a90ebda..095b39a523 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -187,7 +187,6 @@ abstract class LiskDAO { private $__missingFields = array(); private static $processIsolationLevel = 0; private static $transactionIsolationLevel = 0; - private static $__checkedClasses = array(); private $__ephemeral = false; @@ -213,30 +212,6 @@ abstract class LiskDAO { if ($this->getConfigOption(self::CONFIG_PARTIAL_OBJECTS)) { $this->resetDirtyFields(); - - $this_class = get_class($this); - if (empty(self::$__checkedClasses[$this_class])) { - self::$__checkedClasses = true; - - if (PhabricatorEnv::getEnvConfig('lisk.check_property_methods')) { - $class = new ReflectionClass(get_class($this)); - $methods = $class->getMethods(); - $properties = $this->getProperties(); - foreach ($methods as $method) { - $name = strtolower($method->getName()); - if (!(strncmp($name, 'get', 3) && strncmp($name, 'set', 3))) { - $name = substr($name, 3); - $declaring_class_name = $method->getDeclaringClass()->getName(); - if (isset($properties[$name]) && - $declaring_class_name !== 'LiskDAO') { - throw new Exception( - "Cannot implement method {$method->getName()} in ". - "{$declaring_class_name}."); - } - } - } - } - } } }