Change structure of Lisk for custom setters and getters

Summary:
This diff changes the way Lisk should be used for custom setters and getters,
changing it from having subclasses of Lisk implement their custom setter or
getter to having them override the readField and writeField methods (which get
called by the getters and setters). This diff also has a configurable option
to throw an exception if a subclass of Lisk implements a custom setter or
getter.

Test Plan:
Without the config set to throw, tested in sandbox by browsing differential
and playing with the differential typeahead. With the config set to throw,
tried to load a phabricator page and saw in the error log an exception thrown
by Lisk because of custom getters in PhabricatorUser.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, jungejason, epriestley

Differential Revision: 974

src/storage/lisk/dao/LiskDAO.php

@@ -96,8 +96,11 @@
  *     ->save();
  *
  * Note that **Lisk automatically builds getters and setters for all of your
- * object's properties** via __call(). You can override these by defining
- * versions yourself.
+ * object's properties** via __call(). If you want to add custom behavior to
+ * your getters or setters, you can do so by overriding the readField and
+ * writeField methods. Do not implement your own setters and getters, as they
+ * will result in an inconsistent object state (and new values won't get
+ * written to the database).
  *
  * Calling save() will persist the object to the database. After calling
  * save(), you can call getID() to retrieve the object's ID.
@@ -162,6 +165,7 @@ abstract class LiskDAO {
 
   private $__connections            = array();
   private static $processIsolationLevel = 0;
+  private static $__checkedClasses = array();
 
   /**
    *  Build an empty object.
@@ -173,6 +177,29 @@ abstract class LiskDAO {
     if ($id_key) {
       $this->$id_key = null;
     }
+
+    $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}.");
+            }
+          }
+        }
+      }
+    }
   }
 
   abstract protected function establishLiveConnection($mode);
@@ -526,10 +553,11 @@ abstract class LiskDAO {
 
 
   /**
-   * Retrieve a list of all object properties. Note that some may be
-   * "transient", which means they should not be persisted to the database.
-   * Transient properties can be identified by calling
-   * getTransientProperties().
+   * Retrieve a list of all object properties. This list only includes
+   * properties that are declared as protected, and it is expected that
+   * all properties returned by this function should be persisted to the
+   * database.
+   * Properties that should not be persisted must be declared as private.
    *
    * @return dict  Dictionary of normalized (lowercase) to canonical (original
    *               case) property names.
@@ -645,23 +673,6 @@ abstract class LiskDAO {
   }
 
 
-  /**
-   * Convert this object into a property dictionary containing only properties
-   * which will be persisted to the database.
-   *
-   * @return dict  Dictionary of persistent object properties.
-   *
-   * @task   info
-   */
-  protected function getPersistentPropertyValues() {
-    $map = $this->getPropertyValues();
-    foreach ($this->getTransientProperties() as $p) {
-      unset($map[$p]);
-    }
-    return $map;
-  }
-
-
 /* -(  Writing Objects  )---------------------------------------------------- */
 
 
@@ -721,7 +732,7 @@ abstract class LiskDAO {
     $use_locks = $this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS);
 
     $this->willSaveObject();
-    $data = $this->getPersistentPropertyValues();
+    $data = $this->getPropertyValues();
     $this->willWriteData($data);
 
     $map = array();
@@ -803,7 +814,7 @@ abstract class LiskDAO {
    */
   protected function insertRecordIntoDatabase($mode) {
     $this->willSaveObject();
-    $data = $this->getPersistentPropertyValues();
+    $data = $this->getPropertyValues();
 
     $id_mechanism = $this->getConfigOption(self::CONFIG_IDS);
     switch ($id_mechanism) {
@@ -963,21 +974,6 @@ abstract class LiskDAO {
   }
 
 
-  /**
-   * If your object has properties which you don't want to be persisted to the
-   * database, you can override this method and specify them.
-   *
-   * @return list    List of properties which should NOT be persisted.
-   *                 Property names should be in normalized (lowercase) form.
-   *                 By default, all properties are persistent.
-   *
-   * @task   hook
-   */
-  protected function getTransientProperties() {
-    return array();
-  }
-
-
   /**
    * Hook to apply serialization or validation to data before it is written to
    * the database. See also willReadData().
@@ -1056,6 +1052,35 @@ abstract class LiskDAO {
    */
   protected function didDelete() {}
 
+  /**
+   * Reads the value from a field. Override this method for custom behavior
+   * of getField() instead of overriding getField directly.
+   *
+   * @param  string  Canonical field name
+   * @return mixed   Value of the field
+   *
+   * @task hook
+   */
+  protected function readField($field) {
+    if (isset($this->$field)) {
+      return $this->$field;
+    }
+    return null;
+  }
+
+  /**
+   * Writes a value to a field. Override this method for custom behavior of
+   * setField($value) instead of overriding setField directly.
+   *
+   * @param  string  Canonical field name
+   * @param  mixed   Value to write
+   *
+   * @task hook
+   */
+  protected function writeField($field, $value) {
+    $this->$field = $value;
+  }
+
 
 /* -(  Isolation  )---------------------------------------------------------- */
 
@@ -1148,10 +1173,7 @@ abstract class LiskDAO {
       if (count($args) !== 0) {
         throw new Exception("Getter call should have zero args: {$method}");
       }
-      if (isset($this->$property)) {
-        return $this->$property;
-      }
-      return null;
+      return $this->readField($property);
     }
 
     if (!strncmp($method, 'set', 3)) {
@@ -1166,7 +1188,7 @@ abstract class LiskDAO {
       if ($property == 'ID') {
         $property = $this->getIDKeyForUse();
       }
-      $this->$property = $args[0];
+      $this->writeField($property, $args[0]);
       return $this;
     }
 

src/storage/lisk/dao/__init__.php

@@ -6,6 +6,7 @@
 
 
 
+phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'storage/connection/isolated');
 phutil_require_module('phabricator', 'storage/exception/count');
 phutil_require_module('phabricator', 'storage/exception/objectmissing');