From 19b2c3d3d00fc4bc235adfd2b84d485e33fd123b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 25 Dec 2012 06:44:29 -0800 Subject: [PATCH] Formalize configuration sources and source stacks Summary: Currently, we have a configuration stack for unit tests, but they're built in to `PhabricatorEnv`. Pull them out and formalize them, so we can add more configuration sources (e.g., database). Test Plan: Ran unit tests, web requests, scripts. This code had fairly good existing test coverage. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Maniphest Tasks: T2223, T2221 Differential Revision: https://secure.phabricator.com/D4284 --- src/__phutil_library_map__.php | 15 +++- .../env/PhabricatorConfigDictionarySource.php | 36 +++++++++ .../env/PhabricatorConfigFileSource.php | 23 ++++++ .../env/PhabricatorConfigProxySource.php | 43 +++++++++++ .../env/PhabricatorConfigSource.php | 20 +++++ .../env/PhabricatorConfigStackSource.php | 76 ++++++++++++++++++ .../{ => env}/PhabricatorEnv.php | 77 ++++++++----------- .../{ => env}/PhabricatorScopedEnv.php | 5 +- .../__tests__/PhabricatorEnvTestCase.php | 70 +++++++++++++++++ 9 files changed, 315 insertions(+), 50 deletions(-) create mode 100644 src/infrastructure/env/PhabricatorConfigDictionarySource.php create mode 100644 src/infrastructure/env/PhabricatorConfigFileSource.php create mode 100644 src/infrastructure/env/PhabricatorConfigProxySource.php create mode 100644 src/infrastructure/env/PhabricatorConfigSource.php create mode 100644 src/infrastructure/env/PhabricatorConfigStackSource.php rename src/infrastructure/{ => env}/PhabricatorEnv.php (89%) rename src/infrastructure/{ => env}/PhabricatorScopedEnv.php (91%) rename src/infrastructure/{ => env}/__tests__/PhabricatorEnvTestCase.php (63%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8277b13d05..0f92af9106 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -679,6 +679,11 @@ phutil_register_library_map(array( 'PhabricatorConduitLogController' => 'applications/conduit/controller/PhabricatorConduitLogController.php', 'PhabricatorConduitMethodCallLog' => 'applications/conduit/storage/PhabricatorConduitMethodCallLog.php', 'PhabricatorConduitTokenController' => 'applications/conduit/controller/PhabricatorConduitTokenController.php', + 'PhabricatorConfigDictionarySource' => 'infrastructure/env/PhabricatorConfigDictionarySource.php', + 'PhabricatorConfigFileSource' => 'infrastructure/env/PhabricatorConfigFileSource.php', + 'PhabricatorConfigProxySource' => 'infrastructure/env/PhabricatorConfigProxySource.php', + 'PhabricatorConfigSource' => 'infrastructure/env/PhabricatorConfigSource.php', + 'PhabricatorConfigStackSource' => 'infrastructure/env/PhabricatorConfigStackSource.php', 'PhabricatorContentSource' => 'applications/metamta/contentsource/PhabricatorContentSource.php', 'PhabricatorContentSourceView' => 'applications/metamta/contentsource/PhabricatorContentSourceView.php', 'PhabricatorController' => 'applications/base/controller/PhabricatorController.php', @@ -726,8 +731,8 @@ phutil_register_library_map(array( 'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php', 'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php', 'PhabricatorEnglishTranslation' => 'infrastructure/internationalization/PhabricatorEnglishTranslation.php', - 'PhabricatorEnv' => 'infrastructure/PhabricatorEnv.php', - 'PhabricatorEnvTestCase' => 'infrastructure/__tests__/PhabricatorEnvTestCase.php', + 'PhabricatorEnv' => 'infrastructure/env/PhabricatorEnv.php', + 'PhabricatorEnvTestCase' => 'infrastructure/env/__tests__/PhabricatorEnvTestCase.php', 'PhabricatorErrorExample' => 'applications/uiexample/examples/PhabricatorErrorExample.php', 'PhabricatorEvent' => 'infrastructure/events/PhabricatorEvent.php', 'PhabricatorEventEngine' => 'infrastructure/events/PhabricatorEventEngine.php', @@ -1083,7 +1088,7 @@ phutil_register_library_map(array( 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', 'PhabricatorSQLPatchList' => 'infrastructure/storage/patch/PhabricatorSQLPatchList.php', 'PhabricatorSSHWorkflow' => 'infrastructure/ssh/PhabricatorSSHWorkflow.php', - 'PhabricatorScopedEnv' => 'infrastructure/PhabricatorScopedEnv.php', + 'PhabricatorScopedEnv' => 'infrastructure/env/PhabricatorScopedEnv.php', 'PhabricatorSearchAbstractDocument' => 'applications/search/index/PhabricatorSearchAbstractDocument.php', 'PhabricatorSearchAttachController' => 'applications/search/controller/PhabricatorSearchAttachController.php', 'PhabricatorSearchBaseController' => 'applications/search/controller/PhabricatorSearchBaseController.php', @@ -1981,6 +1986,10 @@ phutil_register_library_map(array( 'PhabricatorConduitLogController' => 'PhabricatorConduitController', 'PhabricatorConduitMethodCallLog' => 'PhabricatorConduitDAO', 'PhabricatorConduitTokenController' => 'PhabricatorConduitController', + 'PhabricatorConfigDictionarySource' => 'PhabricatorConfigSource', + 'PhabricatorConfigFileSource' => 'PhabricatorConfigProxySource', + 'PhabricatorConfigProxySource' => 'PhabricatorConfigSource', + 'PhabricatorConfigStackSource' => 'PhabricatorConfigSource', 'PhabricatorContentSourceView' => 'AphrontView', 'PhabricatorController' => 'AphrontController', 'PhabricatorCountdownController' => 'PhabricatorController', diff --git a/src/infrastructure/env/PhabricatorConfigDictionarySource.php b/src/infrastructure/env/PhabricatorConfigDictionarySource.php new file mode 100644 index 0000000000..530651bec9 --- /dev/null +++ b/src/infrastructure/env/PhabricatorConfigDictionarySource.php @@ -0,0 +1,36 @@ +dictionary = $dictionary; + } + + public function getAllKeys() { + return $this->dictionary; + } + + public function getKeys(array $keys) { + return array_select_keys($this->dictionary, $keys); + } + + public function canWrite() { + return true; + } + + public function setKeys(array $keys) { + $this->dictionary = $keys + $this->dictionary; + return $this; + } + + public function deleteKeys(array $keys) { + foreach ($keys as $key) { + unset($this->dictionary[$key]); + } + return $keys; + } + +} diff --git a/src/infrastructure/env/PhabricatorConfigFileSource.php b/src/infrastructure/env/PhabricatorConfigFileSource.php new file mode 100644 index 0000000000..fa02040c3e --- /dev/null +++ b/src/infrastructure/env/PhabricatorConfigFileSource.php @@ -0,0 +1,23 @@ +setSource(new PhabricatorConfigDictionarySource($dictionary)); + } + +} diff --git a/src/infrastructure/env/PhabricatorConfigProxySource.php b/src/infrastructure/env/PhabricatorConfigProxySource.php new file mode 100644 index 0000000000..15055d6cb9 --- /dev/null +++ b/src/infrastructure/env/PhabricatorConfigProxySource.php @@ -0,0 +1,43 @@ +source) { + throw new Exception("No configuration source set!"); + } + return $this->source; + } + + final protected function setSource(PhabricatorConfigSource $source) { + $this->source = $source; + return $this; + } + + public function getAllKeys() { + return $this->getSource()->getAllKeys(); + } + + public function getKeys(array $keys) { + return $this->getSource()->getKeys($keys); + } + + public function canWrite() { + return $this->getSource->canWrite(); + } + + public function setKeys(array $keys) { + return $this->getSource->setKeys(); + } + + public function deleteKeys(array $keys) { + return $this->getSource->deleteKeys(); + } + +} diff --git a/src/infrastructure/env/PhabricatorConfigSource.php b/src/infrastructure/env/PhabricatorConfigSource.php new file mode 100644 index 0000000000..ce6d4acd53 --- /dev/null +++ b/src/infrastructure/env/PhabricatorConfigSource.php @@ -0,0 +1,20 @@ +stack, $source); + return $this; + } + + public function popSource() { + if (empty($this->stack)) { + throw new Exception("Popping an empty config stack!"); + } + return array_shift($this->stack); + } + + public function getKeys(array $keys) { + $result = array(); + foreach ($this->stack as $source) { + $result = $result + $source->getKeys($keys); + } + return $result; + } + + public function getAllKeys() { + $result = array(); + foreach ($this->stack as $source) { + $result = $result + $source->getAllKeys(); + } + return $result; + } + + public function canWrite() { + foreach ($this->stack as $source) { + if ($source->canWrite()) { + return true; + } + } + return false; + } + + public function setKeys(array $keys) { + foreach ($this->stack as $source) { + if ($source->canWrite()) { + $source->setKeys($keys); + return; + } + } + + // We can't write; this will throw an appropriate exception. + parent::setKeys($keys); + } + + public function deleteKeys(array $keys) { + foreach ($this->stack as $source) { + if ($source->canWrite()) { + $source->deleteKeys($keys); + return; + } + } + + // We can't write; this will throw an appropriate exception. + parent::deleteKeys($keys); + } + +} diff --git a/src/infrastructure/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php similarity index 89% rename from src/infrastructure/PhabricatorEnv.php rename to src/infrastructure/env/PhabricatorEnv.php index 0bddb995e3..784736cf2e 100644 --- a/src/infrastructure/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -50,8 +50,7 @@ */ final class PhabricatorEnv { - private static $env; - private static $stack = array(); + private static $sourceStack; /** * @phutil-external-symbol class PhabricatorStartup @@ -93,18 +92,12 @@ final class PhabricatorEnv { AphrontWriteGuard::allowDangerousUnguardedWrites(true); } - /** - * @phutil-external-symbol function phabricator_read_config_file - */ + private static function initializeCommonEnvironment() { $env = self::getSelectedEnvironmentName(); - $root = dirname(phutil_get_library_root('phabricator')); - require_once $root.'/conf/__init_conf__.php'; - $conf = phabricator_read_config_file($env); - $conf['phabricator.env'] = $env; - - PhabricatorEnv::setEnvConfig($conf); + self::$sourceStack = new PhabricatorConfigStackSource(); + self::$sourceStack->pushSource(new PhabricatorConfigFileSource($env)); PhutilErrorHandler::initialize(); @@ -159,18 +152,8 @@ final class PhabricatorEnv { * @task read */ public static function getEnvConfig($key, $default = null) { - - // If we have environment overrides via beginScopedEnv(), check them for - // the key first. - if (self::$stack) { - foreach (array_reverse(self::$stack) as $override) { - if (array_key_exists($key, $override)) { - return $override[$key]; - } - } - } - - return idx(self::$env, $key, $default); + $result = self::$sourceStack->getKeys(array($key)); + return idx($result, $key, $default); } @@ -256,27 +239,26 @@ final class PhabricatorEnv { * @task test */ public static function beginScopedEnv() { - return new PhabricatorScopedEnv(self::pushEnvironment()); + return new PhabricatorScopedEnv(self::pushTestEnvironment()); } /** * @task test */ - private static function pushEnvironment() { - self::$stack[] = array(); - return last_key(self::$stack); + private static function pushTestEnvironment() { + $source = new PhabricatorConfigDictionarySource(array()); + self::$sourceStack->pushSource($source); + return spl_object_hash($source); } /** * @task test */ - public static function popEnvironment($key) { - $stack_key = last_key(self::$stack); - - array_pop(self::$stack); - + public static function popTestEnvironment($key) { + $source = self::$sourceStack->popSource(); + $stack_key = spl_object_hash($source); if ($stack_key !== $key) { throw new Exception( "Scoped environments were destroyed in a diffent order than they ". @@ -366,14 +348,6 @@ final class PhabricatorEnv { /* -( Internals )---------------------------------------------------------- */ - /** - * @task internal - */ - public static function setEnvConfig(array $config) { - self::$env = $config; - } - - /** * @task internal */ @@ -405,7 +379,7 @@ final class PhabricatorEnv { * @task internal */ public static function envConfigExists($key) { - return array_key_exists($key, self::$env); + return array_key_exists($key, self::$sourceStack->getKeys(array($key))); } @@ -413,15 +387,30 @@ final class PhabricatorEnv { * @task internal */ public static function getAllConfigKeys() { - return self::$env; + return self::$sourceStack->getAllKeys(); } /** * @task internal */ - public static function overrideEnvConfig($stack_key, $key, $value) { - self::$stack[$stack_key][$key] = $value; + public static function overrideTestEnvConfig($stack_key, $key, $value) { + $tmp = array(); + + // If we don't have the right key, we'll throw when popping the last + // source off the stack. + do { + $source = self::$sourceStack->popSource(); + array_unshift($tmp, $source); + if (spl_object_hash($source) == $stack_key) { + $source->setKeys(array($key => $value)); + break; + } + } while (true); + + foreach ($tmp as $source) { + self::$sourceStack->pushSource($source); + } } } diff --git a/src/infrastructure/PhabricatorScopedEnv.php b/src/infrastructure/env/PhabricatorScopedEnv.php similarity index 91% rename from src/infrastructure/PhabricatorScopedEnv.php rename to src/infrastructure/env/PhabricatorScopedEnv.php index b2a42e2e1c..86bbe6e6a0 100644 --- a/src/infrastructure/PhabricatorScopedEnv.php +++ b/src/infrastructure/env/PhabricatorScopedEnv.php @@ -24,7 +24,7 @@ final class PhabricatorScopedEnv { * @task override */ public function overrideEnvConfig($key, $value) { - PhabricatorEnv::overrideEnvConfig( + PhabricatorEnv::overrideTestEnvConfig( $this->key, $key, $value); @@ -36,7 +36,6 @@ final class PhabricatorScopedEnv { /** - * * @task internal */ public function __construct($stack_key) { @@ -52,7 +51,7 @@ final class PhabricatorScopedEnv { */ public function __destruct() { if (!$this->isPopped) { - PhabricatorEnv::popEnvironment($this->key); + PhabricatorEnv::popTestEnvironment($this->key); $this->isPopped = true; } } diff --git a/src/infrastructure/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php similarity index 63% rename from src/infrastructure/__tests__/PhabricatorEnvTestCase.php rename to src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php index 34242592a8..4adbe11459 100644 --- a/src/infrastructure/__tests__/PhabricatorEnvTestCase.php +++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php @@ -38,8 +38,78 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase { } } + public function testDictionarySource() { + $source = new PhabricatorConfigDictionarySource(array('x' => 1)); + + $this->assertEqual( + array( + 'x' => 1, + ), + $source->getKeys(array('x', 'z'))); + + $source->setKeys(array('z' => 2)); + + $this->assertEqual( + array( + 'x' => 1, + 'z' => 2, + ), + $source->getKeys(array('x', 'z'))); + + $source->setKeys(array('x' => 3)); + + $this->assertEqual( + array( + 'x' => 3, + 'z' => 2, + ), + $source->getKeys(array('x', 'z'))); + + $source->deleteKeys(array('x')); + + $this->assertEqual( + array( + 'z' => 2, + ), + $source->getKeys(array('x', 'z'))); + } + + public function testStackSource() { + $s1 = new PhabricatorConfigDictionarySource(array('x' => 1)); + $s2 = new PhabricatorConfigDictionarySource(array('x' => 2)); + + $stack = new PhabricatorConfigStackSource(); + + $this->assertEqual(array(), $stack->getKeys(array('x'))); + + $stack->pushSource($s1); + $this->assertEqual(array('x' => 1), $stack->getKeys(array('x'))); + + $stack->pushSource($s2); + $this->assertEqual(array('x' => 2), $stack->getKeys(array('x'))); + + $stack->setKeys(array('x' => 3)); + $this->assertEqual(array('x' => 3), $stack->getKeys(array('x'))); + + $stack->popSource(); + $this->assertEqual(array('x' => 1), $stack->getKeys(array('x'))); + + $stack->popSource(); + $this->assertEqual(array(), $stack->getKeys(array('x'))); + + $caught = null; + try { + $stack->popSource(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual(true, ($caught instanceof Exception)); + } + public function testOverrides() { $outer = PhabricatorEnv::beginScopedEnv(); + $outer->overrideEnvConfig('test.value', 1); $this->assertEqual(1, PhabricatorEnv::getEnvConfig('test.value'));