mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-10 08:52:39 +01:00
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
This commit is contained in:
parent
90b45b3a0a
commit
b7c584137f
9 changed files with 398 additions and 0 deletions
|
@ -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',
|
||||
|
|
|
@ -0,0 +1,16 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorCustomFieldDataNotAvailableException
|
||||
extends Exception {
|
||||
|
||||
public function __construct(PhabricatorCustomField $field) {
|
||||
$key = $field->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.");
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,17 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorCustomFieldImplementationIncompleteException
|
||||
extends Exception {
|
||||
|
||||
public function __construct(PhabricatorCustomField $field) {
|
||||
$key = $field->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.");
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,6 @@
|
|||
<?php
|
||||
|
||||
final class PhabricatorCustomFieldValidationException
|
||||
extends Exception {
|
||||
|
||||
}
|
280
src/infrastructure/customfield/field/PhabricatorCustomField.php
Normal file
280
src/infrastructure/customfield/field/PhabricatorCustomField.php
Normal file
|
@ -0,0 +1,280 @@
|
|||
<?php
|
||||
|
||||
/**
|
||||
* @task context Contextual Data
|
||||
* @task storage Field Storage
|
||||
*/
|
||||
abstract class PhabricatorCustomField {
|
||||
|
||||
private $viewer;
|
||||
|
||||
abstract public function getFieldKey();
|
||||
|
||||
public function getFieldIndex() {
|
||||
return PhabricatorHash::digestForIndex($this->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<PhabricatorCustomFieldIndexStorage> 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);
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,18 @@
|
|||
<?php
|
||||
|
||||
abstract class PhabricatorCustomFieldIndexStorage
|
||||
extends PhabricatorLiskDAO {
|
||||
|
||||
protected $objectPHID;
|
||||
protected $indexKey;
|
||||
protected $indexValue;
|
||||
|
||||
public function getConfiguration() {
|
||||
return array(
|
||||
self::CONFIG_TIMESTAMPS => false,
|
||||
) + parent::getConfiguration();
|
||||
}
|
||||
|
||||
abstract public function formatForInsert(AphrontDatabaseConnection $conn);
|
||||
|
||||
}
|
|
@ -0,0 +1,15 @@
|
|||
<?php
|
||||
|
||||
abstract class PhabricatorCustomFieldNumericIndexStorage
|
||||
extends PhabricatorCustomFieldIndexStorage {
|
||||
|
||||
public function formatForInsert(AphrontDatabaseConnection $conn) {
|
||||
return qsprintf(
|
||||
$conn,
|
||||
'(%s, %s, %d)',
|
||||
$this->getObjectPHID(),
|
||||
$this->getIndexKey(),
|
||||
$this->getIndexValue());
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,16 @@
|
|||
<?php
|
||||
|
||||
abstract class PhabricatorCustomFieldStorage
|
||||
extends PhabricatorLiskDAO {
|
||||
|
||||
protected $objectPHID;
|
||||
protected $fieldIndex;
|
||||
protected $fieldValue;
|
||||
|
||||
public function getConfiguration() {
|
||||
return array(
|
||||
self::CONFIG_TIMESTAMPS => false,
|
||||
) + parent::getConfiguration();
|
||||
}
|
||||
|
||||
}
|
|
@ -0,0 +1,15 @@
|
|||
<?php
|
||||
|
||||
abstract class PhabricatorCustomFieldStringIndexStorage
|
||||
extends PhabricatorCustomFieldIndexStorage {
|
||||
|
||||
public function formatForInsert(AphrontDatabaseConnection $conn) {
|
||||
return qsprintf(
|
||||
$conn,
|
||||
'(%s, %s, %s)',
|
||||
$this->getObjectPHID(),
|
||||
$this->getIndexKey(),
|
||||
$this->getIndexValue());
|
||||
}
|
||||
|
||||
}
|
Loading…
Reference in a new issue