From b7c584137f4081d580229a45529862ee70198dd4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 6 Jun 2013 14:52:40 -0700 Subject: [PATCH] Begin generalizing custom fields Summary: Ref T1703. We have currently have two custom field implementations (Maniphest, Differential) and are about to add a third (User, see D6122). I'd like to generalize custom fields before doing a third implementation, so we don't back ourselves into the ApplicationTransactions corner we have with Maniphest/Differential/Audit. For the most part, the existing custom fields work well and can be directly generalized. There are three specific things I want to improve, though: - Integration with ApplicationSearch: Custom fields aren't indexable. ApplicationSearch is now online and seems stable and good. D5278 provides a template for a backend which can integrate with ApplicationSearch, and ApplicationSearch solves many of the other UI problems implied by exposing custom fields into search (principally, giant pages full of query fields). Generally, I want to provide stronger builtin integration between custom fields and ApplicationSearch. - Integration with ApplicationTransactions: Likewise, custom fields should support more native integrations with ApplicationTransactions, which are also online and seem stable and well designed. - Selection and sorting: Selecting and sorting custom fields is a huge mess right now. I want to move this into config now that we have the UI to support it, and move away from requiring users to subclass a ton of stuff just to add a field. For ApplicationSearch, I've adopted and generalized D5278. For ApplicationTransactions, I haven't made any specific affordances yet. For selection and sorting, I've partially implemented config-based selection and sorting. It will work like this: - We add a new configuration value, like `differential.fields`. In the UI, this is a draggable list of supported fields. Fields can be reordered, and most fields can be disabled. - We load every avialable field to populate this list. New fields will appear at the bottom. - There are two downsides to this approach: - If we add fields in the upstream at a later date, they will appear at the end of the list if an install has customized list order or disabled fields, even if we insert them elsewhere in the upstream. - If we reorder fields in the upstream, the reordering will not be reflected in install which have customized the order/availability. - I think these are both acceptable costs. We only incur them if an admin edits this config, which implies they'll know how to fix it if they want to. - We can fix both of these problems with a straightforward configuration migration if we want to bother. - There are numerous upsides to this approach: - We can delete a bunch of code and replace it with simple configuration. - In general, we don't need the "selector" classes anymore. - Users can enable available-but-disabled fields with one click. - Users can add fields by putting their implementations in `src/extensions/` with zero subclassing or libphutil stuff. - Generally, it's super easy for users to understand. This doesn't actually do anything yet and will probably see some adjustments before anything starts running it. Test Plan: Static checks only, this code isn't reachable yet. Reviewers: chad, seporaitis Reviewed By: chad CC: aran Maniphest Tasks: T1703 Differential Revision: https://secure.phabricator.com/D6147 --- src/__phutil_library_map__.php | 15 + ...orCustomFieldDataNotAvailableException.php | 16 + ...FieldImplementationIncompleteException.php | 17 ++ ...bricatorCustomFieldValidationException.php | 6 + .../field/PhabricatorCustomField.php | 280 ++++++++++++++++++ .../PhabricatorCustomFieldIndexStorage.php | 18 ++ ...bricatorCustomFieldNumericIndexStorage.php | 15 + .../storage/PhabricatorCustomFieldStorage.php | 16 + ...abricatorCustomFieldStringIndexStorage.php | 15 + 9 files changed, 398 insertions(+) create mode 100644 src/infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php create mode 100644 src/infrastructure/customfield/exception/PhabricatorCustomFieldImplementationIncompleteException.php create mode 100644 src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php create mode 100644 src/infrastructure/customfield/field/PhabricatorCustomField.php create mode 100644 src/infrastructure/customfield/storage/PhabricatorCustomFieldIndexStorage.php create mode 100644 src/infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php create mode 100644 src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php create mode 100644 src/infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dbf888da4d..e0f0209ec5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -917,6 +917,14 @@ phutil_register_library_map(array( 'PhabricatorCrumbView' => 'view/layout/PhabricatorCrumbView.php', 'PhabricatorCrumbsView' => 'view/layout/PhabricatorCrumbsView.php', 'PhabricatorCursorPagedPolicyAwareQuery' => 'infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php', + 'PhabricatorCustomField' => 'infrastructure/customfield/field/PhabricatorCustomField.php', + 'PhabricatorCustomFieldDataNotAvailableException' => 'infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php', + 'PhabricatorCustomFieldImplementationIncompleteException' => 'infrastructure/customfield/exception/PhabricatorCustomFieldImplementationIncompleteException.php', + 'PhabricatorCustomFieldIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldIndexStorage.php', + 'PhabricatorCustomFieldNumericIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php', + 'PhabricatorCustomFieldStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php', + 'PhabricatorCustomFieldStringIndexStorage' => 'infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php', + 'PhabricatorCustomFieldValidationException' => 'infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php', 'PhabricatorDaemon' => 'infrastructure/daemon/PhabricatorDaemon.php', 'PhabricatorDaemonCombinedLogController' => 'applications/daemon/controller/PhabricatorDaemonCombinedLogController.php', 'PhabricatorDaemonConsoleController' => 'applications/daemon/controller/PhabricatorDaemonConsoleController.php', @@ -2753,6 +2761,13 @@ phutil_register_library_map(array( 'PhabricatorCrumbView' => 'AphrontView', 'PhabricatorCrumbsView' => 'AphrontView', 'PhabricatorCursorPagedPolicyAwareQuery' => 'PhabricatorPolicyAwareQuery', + 'PhabricatorCustomFieldDataNotAvailableException' => 'Exception', + 'PhabricatorCustomFieldImplementationIncompleteException' => 'Exception', + 'PhabricatorCustomFieldIndexStorage' => 'PhabricatorLiskDAO', + 'PhabricatorCustomFieldNumericIndexStorage' => 'PhabricatorCustomFieldIndexStorage', + 'PhabricatorCustomFieldStorage' => 'PhabricatorLiskDAO', + 'PhabricatorCustomFieldStringIndexStorage' => 'PhabricatorCustomFieldIndexStorage', + 'PhabricatorCustomFieldValidationException' => 'Exception', 'PhabricatorDaemon' => 'PhutilDaemon', 'PhabricatorDaemonCombinedLogController' => 'PhabricatorDaemonController', 'PhabricatorDaemonConsoleController' => 'PhabricatorDaemonController', diff --git a/src/infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php b/src/infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php new file mode 100644 index 0000000000..26141dc506 --- /dev/null +++ b/src/infrastructure/customfield/exception/PhabricatorCustomFieldDataNotAvailableException.php @@ -0,0 +1,16 @@ +getFieldKey(); + $name = $field->getFieldName(); + $class = get_class($field); + + parent::__construct( + "Custom field '{$name}' (with key '{$key}', of class '{$class}') is ". + "attempting to access data which is not available in this context."); + } + +} diff --git a/src/infrastructure/customfield/exception/PhabricatorCustomFieldImplementationIncompleteException.php b/src/infrastructure/customfield/exception/PhabricatorCustomFieldImplementationIncompleteException.php new file mode 100644 index 0000000000..2d87c7e767 --- /dev/null +++ b/src/infrastructure/customfield/exception/PhabricatorCustomFieldImplementationIncompleteException.php @@ -0,0 +1,17 @@ +getFieldKey(); + $name = $field->getFieldName(); + $class = get_class($field); + + parent::__construct( + "Custom field '{$name}' (with key '{$key}', of class '{$class}') is ". + "incompletely implemented: it claims to support a feature, but does not ". + "implement all of the required methods for that feature."); + } + +} diff --git a/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php b/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php new file mode 100644 index 0000000000..0cfc42251e --- /dev/null +++ b/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php @@ -0,0 +1,6 @@ +getFieldKey()); + } + + public function getFieldName() { + return $this->getFieldKey(); + } + + public function createFields() { + return array($this); + } + + public function isFieldEnabled() { + return true; + } + + public function canDisableField() { + return true; + } + + public static function buildFieldList($base_class, array $spec) { + $this_class = __CLASS__; + if (!($base_class instanceof $this_class)) { + throw new Exception( + "Base class ('{$base_class}') must extend '{$this_class}'."); + } + + $field_objects = id(new PhutilSymbolLoader()) + ->setAncestorClass($base_class) + ->loadObjects(); + + $fields = array(); + $from_map = array(); + foreach ($field_objects as $field_object) { + $current_class = get_class($field_object); + foreach ($field_object->createFields() as $field) { + $key = $field->getFieldKey(); + if (isset($fields[$key])) { + $original_class = $from_map[$key]; + throw new Exception( + "Both '{$original_class}' and '{$current_class}' define a custom ". + "field with field key '{$key}'. Field keys must be unique."); + } + $from_map[$key] = $current_class; + $fields[$key] = $field; + } + } + + foreach ($fields as $key => $field) { + if (!$field->isEnabled()) { + unset($fields[$key]); + } + } + + $fields = array_select_keys($fields, array_keys($spec)); + + foreach ($spec as $key => $config) { + if (empty($fields[$key])) { + continue; + } + if (!empty($config['disabled'])) { + if ($fields[$key]->canDisableField()) { + unset($fields[$key]); + } + } + } + + return $fields; + } + + +/* -( Contextual Data )---------------------------------------------------- */ + + + /** + * @task context + */ + final public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + + /** + * @task context + */ + final public function getViewer() { + return $this->viewer; + } + + + /** + * @task context + */ + final protected function requireViewer() { + if (!$this->viewer) { + throw new PhabricatorCustomFieldDataNotAvailableException($this); + } + return $this->viewer; + } + + +/* -( Storage )------------------------------------------------------------ */ + + + /** + * Return a unique string used to key storage of this field's value, like + * "mycompany.fieldname" or similar. You can return null (the default) to + * indicate that this field does not use any storage. + * + * Fields which can be edited by the user will most commonly use storage, + * while some other types of fields (for instance, those which just display + * information in some stylized way) may not. Many builtin fields do not use + * storage because their data is available on the object itself. + * + * If you implement this, you must also implement @{method:getValueForStorage} + * and @{method:setValueFromStorage}. + * + * In most cases, a reasonable implementation is to simply reuse the field + * key: + * + * return $this->getFieldKey(); + * + * @return string|null Unique key which identifies this field in auxiliary + * field storage. Alternatively, return null (default) + * to indicate that this field does not use storage. + * @task storage + */ + public function getStorageKey() { + return null; + } + + + /** + * Return a serialized representation of the field value, appropriate for + * storing in auxiliary field storage. You must implement this method if + * you implement @{method:getStorageKey}. + * + * If the field value is a scalar, it can be returned unmodiifed. If not, + * it should be serialized (for example, using JSON). + * + * @return string Serialized field value. + * @task storage + */ + public function getValueForStorage() { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + + /** + * Set the field's value given a serialized storage value. This is called + * when the field is loaded; if no data is available, the value will be + * null. You must implement this method if you implement + * @{method:getStorageKey}. + * + * Usually, the value can be loaded directly. If it isn't a scalar, you'll + * need to undo whatever serialization you applied in + * @{method:getValueForStorage}. + * + * @param string|null Serialized field representation (from + * @{method:getValueForStorage}) or null if no value has + * ever been stored. + * @return this + * @task storage + */ + public function setValueFromStorage($value) { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + +/* -( ApplicationSearch )-------------------------------------------------- */ + + + /** + * Appearing in ApplicationSearch allows a field to be indexed and searched + * for. + * + * @return bool True to appear in ApplicationSearch. + * @task appsearch + */ + public function shouldAppearInApplicationSearch() { + return false; + } + + + /** + * Return one or more indexes which this field can meaningfully query against + * to implement ApplicationSearch. + * + * Normally, you should build these using @{method:newStringIndex} and + * @{method:newNumericIndex}. For example, if a field holds a numeric value + * it might return a single numeric index: + * + * return array($this->newNumericIndex($this->getValue())); + * + * If a field holds a more complex value (like a list of users), it might + * return several string indexes: + * + * $indexes = array(); + * foreach ($this->getValue() as $phid) { + * $indexes[] = $this->newStringIndex($phid); + * } + * return $indexes; + * + * @return list List of indexes. + * @task appsearch + */ + public function buildFieldIndexes() { + return array(); + } + + + /** + * Build a new empty storage object for storing string indexes. Normally, + * this should be a concrete subclass of + * @{class:PhabricatorCustomFieldStringIndexStorage}. + * + * @return PhabricatorCustomFieldStringIndexStorage Storage object. + * @task appsearch + */ + protected function newStringIndexStorage() { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + + /** + * Build a new empty storage object for storing string indexes. Normally, + * this should be a concrete subclass of + * @{class:PhabricatorCustomFieldStringIndexStorage}. + * + * @return PhabricatorCustomFieldStringIndexStorage Storage object. + * @task appsearch + */ + protected function newNumericIndexStorage() { + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + + /** + * Build and populate storage for a string index. + * + * @param string String to index. + * @return PhabricatorCustomFieldStringIndexStorage Populated storage. + * @task appsearch + */ + protected function newStringIndex($value) { + $key = $this->getFieldIndexKey(); + return $this->newStringIndexStorage() + ->setIndexKey($key) + ->setIndexValue($value); + } + + + /** + * Build and populate storage for a numeric index. + * + * @param string Numeric value to index. + * @return PhabricatorCustomFieldNumericIndexStorage Populated storage. + * @task appsearch + */ + protected function newNumericIndex($value) { + $key = $this->getFieldIndexKey(); + return $this->newNumericIndexStorage() + ->setIndexKey($key) + ->setIndexValue($value); + } + +} diff --git a/src/infrastructure/customfield/storage/PhabricatorCustomFieldIndexStorage.php b/src/infrastructure/customfield/storage/PhabricatorCustomFieldIndexStorage.php new file mode 100644 index 0000000000..7a01fd4df7 --- /dev/null +++ b/src/infrastructure/customfield/storage/PhabricatorCustomFieldIndexStorage.php @@ -0,0 +1,18 @@ + false, + ) + parent::getConfiguration(); + } + + abstract public function formatForInsert(AphrontDatabaseConnection $conn); + +} diff --git a/src/infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php b/src/infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php new file mode 100644 index 0000000000..f7d93456f8 --- /dev/null +++ b/src/infrastructure/customfield/storage/PhabricatorCustomFieldNumericIndexStorage.php @@ -0,0 +1,15 @@ +getObjectPHID(), + $this->getIndexKey(), + $this->getIndexValue()); + } + +} diff --git a/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php new file mode 100644 index 0000000000..31e3c25165 --- /dev/null +++ b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStorage.php @@ -0,0 +1,16 @@ + false, + ) + parent::getConfiguration(); + } + +} diff --git a/src/infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php new file mode 100644 index 0000000000..73ccc7597a --- /dev/null +++ b/src/infrastructure/customfield/storage/PhabricatorCustomFieldStringIndexStorage.php @@ -0,0 +1,15 @@ +getObjectPHID(), + $this->getIndexKey(), + $this->getIndexValue()); + } + +}