Allow PhabricatorEnv to be temporarily made mutable for unit tests

Summary: Introduces a scope-guarded way to override the env config, for unit tests which are sensitive to config values.

Test Plan: Ran unit tests.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D2237

src/__phutil_library_map__.php

@@ -842,6 +842,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryType' => 'applications/repository/constants/repositorytype',
     'PhabricatorS3FileStorageEngine' => 'applications/files/engine/s3',
     'PhabricatorSQLPatchList' => 'infrastructure/setup/sql',
+    'PhabricatorScopedEnv' => 'infrastructure/env',
     'PhabricatorSearchAbstractDocument' => 'applications/search/index/abstractdocument',
     'PhabricatorSearchAttachController' => 'applications/search/controller/attach',
     'PhabricatorSearchBaseController' => 'applications/search/controller/base',

src/infrastructure/env/PhabricatorEnv.php

@@ -17,58 +17,98 @@
  */
 
 /**
- * @task uri URI Validation
+ * Manages the execution environment configuration, exposing APIs to read
+ * configuration settings and other similar values that are derived directly
+ * from configuration settings.
+ *
+ *
+ * = Reading Configuration =
+ *
+ * The primary role of this class is to provide an API for reading
+ * Phabricator configuration, @{method:getEnvConfig}:
+ *
+ *   $value = PhabricatorEnv::getEnvConfig('some.key', $default);
+ *
+ * The class also handles some URI construction based on configuration, via
+ * the methods @{method:getURI}, @{method:getProductionURI},
+ * @{method:getCDNURI}, and @{method:getDoclink}.
+ *
+ * For configuration which allows you to choose a class to be responsible for
+ * some functionality (e.g., which mail adapter to use to deliver email),
+ * @{method:newObjectFromConfig} provides a simple interface that validates
+ * the configured value.
+ *
+ *
+ * = Unit Test Support =
+ *
+ * In unit tests, you can use @{method:beginScopedEnv} to create a temporary,
+ * mutable environment. The method returns a scope guard object which restores
+ * the environment when it is destroyed. For example:
+ *
+ *   public function testExample() {
+ *     $env = PhabricatorEnv::beginScopedEnv();
+ *     $env->overrideEnv('some.key', 'new-value-for-this-test');
+ *
+ *     // Some test which depends on the value of 'some.key'.
+ *
+ *   }
+ *
+ * Your changes will persist until the `$env` object leaves scope or is
+ * destroyed.
+ *
+ * You should //not// use this in normal code.
+ *
+ *
+ * @task read     Reading Configuration
+ * @task uri      URI Validation
+ * @task test     Unit Test Support
+ * @task internal Internals
  */
 final class PhabricatorEnv {
+
   private static $env;
+  private static $stack = array();
 
-  public static function setEnvConfig(array $config) {
-    self::$env = $config;
-  }
 
+/* -(  Reading Configuration  )---------------------------------------------- */
+
+
+  /**
+   * Get the current configuration setting for a given key.
+   *
+   * @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);
   }
 
-  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;
-  }
-
-  public static function getRequiredClasses() {
-    return array(
-      'metamta.mail-adapter' => 'PhabricatorMailImplementationAdapter',
-      'metamta.maniphest.reply-handler' => 'PhabricatorMailReplyHandler',
-      'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler',
-      'metamta.diffusion.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',
-    );
-  }
-
-  public static function envConfigExists($key) {
-    return array_key_exists($key, self::$env);
-  }
 
+  /**
+   * Get the fully-qualified URI for a path.
+   *
+   * @task read
+   */
   public static function getURI($path) {
     return rtrim(self::getEnvConfig('phabricator.base-uri'), '/').$path;
   }
 
+
+  /**
+   * Get the fully-qualified production URI for a path.
+   *
+   * @task read
+   */
   public static function getProductionURI($path) {
     // If we're passed a URI which already has a domain, simply return it
     // unmodified. In particular, files may have URIs which point to a CDN
@@ -85,6 +125,12 @@ final class PhabricatorEnv {
     return rtrim($production_domain, '/').$path;
   }
 
+
+  /**
+   * Get the fully-qualified production URI for a static resource path.
+   *
+   * @task read
+   */
   public static function getCDNURI($path) {
     $alt = self::getEnvConfig('security.alternate-file-domain');
     if (!$alt) {
@@ -95,15 +141,70 @@ final class PhabricatorEnv {
     return (string)$uri;
   }
 
-  public static function getAllConfigKeys() {
-    return self::$env;
-  }
 
+  /**
+   * Get the fully-qualified production URI for a documentation resource.
+   *
+   * @task read
+   */
   public static function getDoclink($resource) {
     return 'http://www.phabricator.com/docs/phabricator/'.$resource;
   }
 
 
+  /**
+   * Build a concrete object from a configuration key.
+   *
+   * @task read
+   */
+  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;
+  }
+
+
+/* -(  Unit Test Support  )-------------------------------------------------- */
+
+
+  /**
+   * @task test
+   */
+  public static function beginScopedEnv() {
+    return new PhabricatorScopedEnv(self::pushEnvironment());
+  }
+
+
+  /**
+   * @task test
+   */
+  private static function pushEnvironment() {
+    self::$stack[] = array();
+    return last_key(self::$stack);
+  }
+
+
+  /**
+   * @task test
+   */
+  public static function popEnvironment($key) {
+    $stack_key = last_key(self::$stack);
+
+    array_pop(self::$stack);
+
+    if ($stack_key !== $key) {
+      throw new Exception(
+        "Scoped environments were destroyed in a diffent order than they ".
+        "were initialized.");
+    }
+  }
+
+
 /* -(  URI Validation  )----------------------------------------------------- */
 
 
@@ -181,4 +282,63 @@ final class PhabricatorEnv {
     return true;
   }
 
+
+/* -(  Internals  )---------------------------------------------------------- */
+
+
+  /**
+   * @task internal
+   */
+  public static function setEnvConfig(array $config) {
+    self::$env = $config;
+  }
+
+
+  /**
+   * @task internal
+   */
+  public static function getRequiredClasses() {
+    return array(
+      'metamta.mail-adapter' => 'PhabricatorMailImplementationAdapter',
+      'metamta.maniphest.reply-handler' => 'PhabricatorMailReplyHandler',
+      'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler',
+      'metamta.diffusion.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',
+    );
+  }
+
+
+  /**
+   * @task internal
+   */
+  public static function envConfigExists($key) {
+    return array_key_exists($key, self::$env);
+  }
+
+
+  /**
+   * @task internal
+   */
+  public static function getAllConfigKeys() {
+    return self::$env;
+  }
+
+
+  /**
+   * @task internal
+   */
+  public static function overrideEnvConfig($stack_key, $key, $value) {
+    self::$stack[$stack_key][$key] = $value;
+  }
+
 }

src/infrastructure/env/PhabricatorScopedEnv.php

@@ -0,0 +1,72 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Scope guard to hold a temporary environment. See @{class:PhabricatorEnv} for
+ * instructions on use.
+ *
+ * @task internal Internals
+ * @task override Overriding Environment Configuration
+ */
+final class PhabricatorScopedEnv {
+
+  private $key;
+
+/* -(  Overriding Environment Configuration  )------------------------------- */
+
+  /**
+   * Override a configuration key in this scope, setting it to a new value.
+   *
+   * @param  string Key to override.
+   * @param  wild   New value.
+   * @return this
+   *
+   * @task override
+   */
+  public function overrideEnvConfig($key, $value) {
+    PhabricatorEnv::overrideEnvConfig(
+      $this->key,
+      $key,
+      $value);
+    return $this;
+  }
+
+
+/* -(  Internals  )---------------------------------------------------------- */
+
+
+  /**
+   *
+   * @task internal
+   */
+  public function __construct($stack_key) {
+    $this->key = $stack_key;
+  }
+
+
+  /**
+   * Release the scoped environment.
+   *
+   * @return void
+   * @task internal
+   */
+  public function __destruct() {
+    PhabricatorEnv::popEnvironment($this->key);
+  }
+
+}

src/infrastructure/env/__init__.php

@@ -11,3 +11,4 @@ phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('PhabricatorEnv.php');
+phutil_require_source('PhabricatorScopedEnv.php');

src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php

@@ -53,4 +53,56 @@ final class PhabricatorEnvTestCase extends PhabricatorTestCase {
         "Valid remote resource: {$uri}");
     }
   }
+
+  public function testOverrides() {
+    $outer = PhabricatorEnv::beginScopedEnv();
+      $outer->overrideEnvConfig('test.value', 1);
+      $this->assertEqual(1, PhabricatorEnv::getEnvConfig('test.value'));
+
+      $inner = PhabricatorEnv::beginScopedEnv();
+        $inner->overrideEnvConfig('test.value', 2);
+        $this->assertEqual(2, PhabricatorEnv::getEnvConfig('test.value'));
+      unset($inner);
+
+      $this->assertEqual(1, PhabricatorEnv::getEnvConfig('test.value'));
+    unset($outer);
+  }
+
+  public function testOverrideOrder() {
+    $outer = PhabricatorEnv::beginScopedEnv();
+    $middle = PhabricatorEnv::beginScopedEnv();
+    $inner = PhabricatorEnv::beginScopedEnv();
+
+    $caught = null;
+    try {
+      unset($middle);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertEqual(
+      true,
+      $caught instanceof Exception,
+      "Destroying a scoped environment which is not on the top of the stack ".
+      "should throw.");
+
+    $caught = null;
+    try {
+      unset($inner);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertEqual(
+      true,
+      $caught instanceof Exception,
+      "Destroying a scoped environment which is not on the top of the stack ".
+      "should throw.");
+
+    // Although we popped the other two out-of-order, we still expect to end
+    // up in the right state after handling the exceptions, so this should
+    // execute without issues.
+    unset($outer);
+  }
+
 }

src/infrastructure/testing/testcase/PhabricatorTestCase.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,9 +28,10 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
    * not rely on external resources like databases, and should not produce
    * side effects.
    */
-  const PHABRICATOR_TESTCONFIG_ISOLATE_LISK = 'isolate-lisk';
+  const PHABRICATOR_TESTCONFIG_ISOLATE_LISK     = 'isolate-lisk';
 
   private $configuration;
+  private $env;
 
   protected function getPhabricatorTestCaseConfiguration() {
     return array();
@@ -38,7 +39,7 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
 
   private function getComputedConfiguration() {
     return $this->getPhabricatorTestCaseConfiguration() + array(
-      self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK => true,
+      self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK     => true,
     );
   }
 
@@ -51,6 +52,8 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
     if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
       LiskDAO::beginIsolateAllLiskEffectsToCurrentProcess();
     }
+
+    $this->env = PhabricatorEnv::beginScopedEnv();
   }
 
   protected function didRunTests() {
@@ -59,6 +62,14 @@ abstract class PhabricatorTestCase extends ArcanistPhutilTestCase {
     if ($config[self::PHABRICATOR_TESTCONFIG_ISOLATE_LISK]) {
       LiskDAO::endIsolateAllLiskEffectsToCurrentProcess();
     }
+
+    try {
+      unset($this->env);
+    } catch (Exception $ex) {
+      throw new Exception(
+        "Some test called PhabricatorEnv::beginScopedEnv(), but is still ".
+        "holding a reference to the scoped environment!");
+    }
   }
 
 }

src/infrastructure/testing/testcase/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
 
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'storage/lisk/dao');
 
 phutil_require_module('phutil', 'moduleutils');