From 07b60b2016252d5cc6aede7289b1c6324e00b5d6 Mon Sep 17 00:00:00 2001
From: vrana <jakubv@fb.com>
Date: Thu, 15 Mar 2012 12:15:23 -0700
Subject: [PATCH] Require valid class for certain config settings

Summary:
It is now possible to set config setting requiring class of certain implementation to something completely else.
The consequence is that your Phabricator may stop working after update because you didn't implement some new method.

This diff validates the class upon usage.
It throws exception which is better than fatal thrown currently after calling undefined method.

Better solution would be to validate classes when setting the config but it would be too expensive - respective class definitions would have to be loaded and checked by reflection.

I was also thinking about some check script but nobody would run it after changing config.

The same behavior should be implemented for these settings:

- metamta.mail-adapter
- metamta.maniphest.reply-handler
- metamta.differential.reply-handler
- metamta.diffusion.reply-handler
- storage.engine-selector
- search.engine-selector
- differential.field-selector
- maniphest.custom-task-extensions-class
- aphront.default-application-configuration-class
- controller.oauth-registration

Test Plan:
Send comment, verify that it pass.
Change `metamta.differential.reply-handler` to incompatible class, verify that sending comment shows nice red exception.
Set `metamta.differential.reply-handler` to empty string, verify that it throws.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1919
---
 .../differential/mail/base/DifferentialMail.php  | 16 ++++------------
 src/infrastructure/env/PhabricatorEnv.php        | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php
index 6e506d6486..67b1b963a7 100644
--- a/src/applications/differential/mail/base/DifferentialMail.php
+++ b/src/applications/differential/mail/base/DifferentialMail.php
@@ -215,27 +215,19 @@ EOTEXT;
   }
 
   public function getReplyHandler() {
-    if ($this->replyHandler) {
-      return $this->replyHandler;
+    if (!$this->replyHandler) {
+      $this->replyHandler =
+        self::newReplyHandlerForRevision($this->getRevision());
     }
 
-    $handler_class = PhabricatorEnv::getEnvConfig(
-      'metamta.differential.reply-handler');
-
-    $reply_handler = self::newReplyHandlerForRevision($this->getRevision());
-
-    $this->replyHandler = $reply_handler;
-
     return $this->replyHandler;
   }
 
   public static function newReplyHandlerForRevision(
     DifferentialRevision $revision) {
 
-    $handler_class = PhabricatorEnv::getEnvConfig(
+    $reply_handler = PhabricatorEnv::newObjectFromConfig(
       'metamta.differential.reply-handler');
-
-    $reply_handler = newv($handler_class, array());
     $reply_handler->setMailReceiver($revision);
 
     return $reply_handler;
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index eb8f22faa0..2e3b6151c8 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -22,6 +22,10 @@
 final class PhabricatorEnv {
   private static $env;
 
+  private static $requiredClasses = array(
+    'metamta.differential.reply-handler' => 'PhabricatorMailReplyHandler',
+  );
+
   public static function setEnvConfig(array $config) {
     self::$env = $config;
   }
@@ -30,6 +34,17 @@ final class PhabricatorEnv {
     return idx(self::$env, $key, $default);
   }
 
+  public static function newObjectFromConfig($key, $args = array()) {
+    $class = self::getEnvConfig($key);
+    $object = newv($class, $args);
+    $instanceof = idx(self::$requiredClasses, $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 envConfigExists($key) {
     return array_key_exists($key, self::$env);
   }