From abf96dbd591c80853aaa7d1429e91af9e33678c3 Mon Sep 17 00:00:00 2001 From: Nicholas Harper Date: Fri, 30 Sep 2011 16:56:29 -0700 Subject: [PATCH] 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 | 112 ++++++++++++++++++------------ src/storage/lisk/dao/__init__.php | 1 + 2 files changed, 68 insertions(+), 45 deletions(-) diff --git a/src/storage/lisk/dao/LiskDAO.php b/src/storage/lisk/dao/LiskDAO.php index e45613ed25..b81b23f6cf 100644 --- a/src/storage/lisk/dao/LiskDAO.php +++ b/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; } diff --git a/src/storage/lisk/dao/__init__.php b/src/storage/lisk/dao/__init__.php index ba4ced2eed..854007959b 100644 --- a/src/storage/lisk/dao/__init__.php +++ b/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');