diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 791f26dc23..7f5b6db0d4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -145,6 +145,7 @@ phutil_register_library_map(array( 'AphrontJavelinView' => 'view/AphrontJavelinView.php', 'AphrontKeyboardShortcutsAvailableView' => 'view/widget/AphrontKeyboardShortcutsAvailableView.php', 'AphrontListFilterView' => 'view/layout/AphrontListFilterView.php', + 'AphrontListHTTPParameterType' => 'aphront/httpparametertype/AphrontListHTTPParameterType.php', 'AphrontMalformedRequestException' => 'aphront/exception/AphrontMalformedRequestException.php', 'AphrontMoreView' => 'view/layout/AphrontMoreView.php', 'AphrontMultiColumnView' => 'view/layout/AphrontMultiColumnView.php', @@ -2580,6 +2581,7 @@ phutil_register_library_map(array( 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', 'PhabricatorPHIDInterface' => 'applications/phid/interface/PhabricatorPHIDInterface.php', + 'PhabricatorPHIDListEditField' => 'applications/transactions/editfield/PhabricatorPHIDListEditField.php', 'PhabricatorPHIDResolver' => 'applications/phid/resolver/PhabricatorPHIDResolver.php', 'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php', 'PhabricatorPHIDTypeTestCase' => 'applications/phid/type/__tests__/PhabricatorPHIDTypeTestCase.php', @@ -3943,18 +3945,19 @@ phutil_register_library_map(array( 'AphrontJavelinView' => 'AphrontView', 'AphrontKeyboardShortcutsAvailableView' => 'AphrontView', 'AphrontListFilterView' => 'AphrontView', + 'AphrontListHTTPParameterType' => 'AphrontHTTPParameterType', 'AphrontMalformedRequestException' => 'AphrontException', 'AphrontMoreView' => 'AphrontView', 'AphrontMultiColumnView' => 'AphrontView', 'AphrontMySQLDatabaseConnectionTestCase' => 'PhabricatorTestCase', 'AphrontNullView' => 'AphrontView', 'AphrontPHIDHTTPParameterType' => 'AphrontHTTPParameterType', - 'AphrontPHIDListHTTPParameterType' => 'AphrontHTTPParameterType', + 'AphrontPHIDListHTTPParameterType' => 'AphrontListHTTPParameterType', 'AphrontPHPHTTPSink' => 'AphrontHTTPSink', 'AphrontPageView' => 'AphrontView', 'AphrontPlainTextResponse' => 'AphrontResponse', 'AphrontProgressBarView' => 'AphrontBarView', - 'AphrontProjectListHTTPParameterType' => 'AphrontHTTPParameterType', + 'AphrontProjectListHTTPParameterType' => 'AphrontListHTTPParameterType', 'AphrontProxyResponse' => array( 'AphrontResponse', 'AphrontResponseProducerInterface', @@ -3974,13 +3977,13 @@ phutil_register_library_map(array( 'AphrontStackTraceView' => 'AphrontView', 'AphrontStandaloneHTMLResponse' => 'AphrontHTMLResponse', 'AphrontStringHTTPParameterType' => 'AphrontHTTPParameterType', - 'AphrontStringListHTTPParameterType' => 'AphrontHTTPParameterType', + 'AphrontStringListHTTPParameterType' => 'AphrontListHTTPParameterType', 'AphrontTableView' => 'AphrontView', 'AphrontTagView' => 'AphrontView', 'AphrontTokenizerTemplateView' => 'AphrontView', 'AphrontTypeaheadTemplateView' => 'AphrontView', 'AphrontUnhandledExceptionResponse' => 'AphrontStandaloneHTMLResponse', - 'AphrontUserListHTTPParameterType' => 'AphrontHTTPParameterType', + 'AphrontUserListHTTPParameterType' => 'AphrontListHTTPParameterType', 'AphrontView' => array( 'Phobject', 'PhutilSafeHTMLProducerInterface', @@ -6752,6 +6755,7 @@ phutil_register_library_map(array( 'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorPHID' => 'Phobject', 'PhabricatorPHIDConstants' => 'Phobject', + 'PhabricatorPHIDListEditField' => 'PhabricatorEditField', 'PhabricatorPHIDResolver' => 'Phobject', 'PhabricatorPHIDType' => 'Phobject', 'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase', @@ -7437,7 +7441,7 @@ phutil_register_library_map(array( 'PhabricatorTokenReceiverQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorTokenTokenPHIDType' => 'PhabricatorPHIDType', 'PhabricatorTokenUIEventListener' => 'PhabricatorEventListener', - 'PhabricatorTokenizerEditField' => 'PhabricatorEditField', + 'PhabricatorTokenizerEditField' => 'PhabricatorPHIDListEditField', 'PhabricatorTokensApplication' => 'PhabricatorApplication', 'PhabricatorTokensSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorTooltipUIExample' => 'PhabricatorUIExample', diff --git a/src/aphront/httpparametertype/AphrontListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontListHTTPParameterType.php new file mode 100644 index 0000000000..af972d0f29 --- /dev/null +++ b/src/aphront/httpparametertype/AphrontListHTTPParameterType.php @@ -0,0 +1,10 @@ +getArr($key, null); diff --git a/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php index 3254542115..9599ea49e1 100644 --- a/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php +++ b/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php @@ -1,7 +1,7 @@ setEditTypeKey('projects') ->setDescription(pht('Add or remove associated projects.')) ->setAliases(array('project', 'projects')) + ->setUseEdgeTransactions(true) + ->setEdgeTransactionDescriptions( + pht('Add projects.'), + pht('Remove projects.'), + pht('Set associated projects, overwriting current value.')) ->setTransactionType($edge_type) ->setMetadataValue('edge:type', $project_edge_type) ->setValue($project_phids); diff --git a/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php b/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php index 6532ed2521..ba9c5f1b76 100644 --- a/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php +++ b/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php @@ -45,6 +45,11 @@ final class PhabricatorSubscriptionsEditEngineExtension ->setEditTypeKey('subscribers') ->setDescription(pht('Manage subscribers.')) ->setAliases(array('subscriber', 'subscribers')) + ->setUseEdgeTransactions(true) + ->setEdgeTransactionDescriptions( + pht('Add subscribers.'), + pht('Remove subscribers.'), + pht('Set subscribers, overwriting current value.')) ->setTransactionType($subscribers_type) ->setValue($sub_phids); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 1445ea59b2..8b339c0032 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1048,6 +1048,11 @@ abstract class PhabricatorEditEngine $types = array(); foreach ($fields as $field) { $field_types = $field->getEditTransactionTypes(); + + if ($field_types === null) { + continue; + } + foreach ($field_types as $field_type) { $field_type->setField($field); $types[$field_type->getEditType()] = $field_type; diff --git a/src/applications/transactions/editfield/PhabricatorEditField.php b/src/applications/transactions/editfield/PhabricatorEditField.php index 1378ab4a39..031fcc270b 100644 --- a/src/applications/transactions/editfield/PhabricatorEditField.php +++ b/src/applications/transactions/editfield/PhabricatorEditField.php @@ -7,6 +7,7 @@ abstract class PhabricatorEditField extends Phobject { private $label; private $aliases = array(); private $value; + private $initialValue; private $hasValue = false; private $object; private $transactionType; @@ -262,6 +263,7 @@ abstract class PhabricatorEditField extends Phobject { public function setValue($value) { $this->hasValue = true; + $this->initialValue = $value; $this->value = $value; return $this; } @@ -289,6 +291,10 @@ abstract class PhabricatorEditField extends Phobject { return $this; } + public function getMetadata() { + return $this->metadata; + } + protected function getValueForTransaction() { return $this->getValue(); } @@ -348,6 +354,31 @@ abstract class PhabricatorEditField extends Phobject { return $this->getValueFromSubmit($request, $key); } + + /** + * Read and return the value the object had when the user first loaded the + * form. + * + * This is the initial value from the user's point of view when they started + * the edit process, and used primarily to prevent race conditions for fields + * like "Projects" and "Subscribers" that use tokenizers and support edge + * transactions. + * + * Most fields do not need to store these values or deal with initial value + * handling. + * + * @param AphrontRequest Request to read from. + * @param string Key to read. + * @return wild Value read from request. + */ + protected function getInitialValueFromSubmit(AphrontRequest $request, $key) { + return null; + } + + public function getInitialValue() { + return $this->initialValue; + } + public function readValueFromSubmit(AphrontRequest $request) { $key = $this->getKey(); if ($this->getValueExistsInSubmit($request, $key)) { @@ -356,6 +387,10 @@ abstract class PhabricatorEditField extends Phobject { $value = $this->getDefaultValue(); } $this->value = $value; + + $initial_value = $this->getInitialValueFromSubmit($request, $key); + $this->initialValue = $initial_value; + return $this; } @@ -414,66 +449,30 @@ abstract class PhabricatorEditField extends Phobject { ->setValueType($this->getHTTPParameterType()->getTypeName()); } - public function getEditTransactionTypes() { + protected function getEditTransactionType() { $transaction_type = $this->getTransactionType(); + if ($transaction_type === null) { - return array(); + return null; } $type_key = $this->getEditTypeKey(); - // TODO: This is a pretty big pile of hard-coded hacks for now. + return $this->newEditType() + ->setEditType($type_key) + ->setTransactionType($transaction_type) + ->setDescription($this->getDescription()) + ->setMetadata($this->getMetadata()); + } - $edge_types = array( - PhabricatorTransactions::TYPE_EDGE => array( - '+' => pht('Add projects.'), - '-' => pht('Remove projects.'), - '=' => pht('Set associated projects, overwriting current value.'), - ), - PhabricatorTransactions::TYPE_SUBSCRIBERS => array( - '+' => pht('Add subscribers.'), - '-' => pht('Remove subscribers.'), - '=' => pht('Set subscribers, overwriting current value.'), - ), - ); + public function getEditTransactionTypes() { + $edit_type = $this->getEditTransactionType(); - if (isset($edge_types[$transaction_type])) { - $base = id(new PhabricatorEdgeEditType()) - ->setTransactionType($transaction_type) - ->setMetadata($this->metadata); - - $strings = $edge_types[$transaction_type]; - - $add = id(clone $base) - ->setEditType($type_key.'.add') - ->setEdgeOperation('+') - ->setDescription($strings['+']) - ->setValueDescription(pht('List of PHIDs to add.')); - $rem = id(clone $base) - ->setEditType($type_key.'.remove') - ->setEdgeOperation('-') - ->setDescription($strings['-']) - ->setValueDescription(pht('List of PHIDs to remove.')); - $set = id(clone $base) - ->setEditType($type_key.'.set') - ->setEdgeOperation('=') - ->setDescription($strings['=']) - ->setValueDescription(pht('List of PHIDs to set.')); - - return array( - $add, - $rem, - $set, - ); + if ($edit_type === null) { + return null; } - return array( - $this->newEditType() - ->setEditType($type_key) - ->setTransactionType($transaction_type) - ->setDescription($this->getDescription()) - ->setMetadata($this->metadata), - ); + return array($edit_type); } } diff --git a/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php new file mode 100644 index 0000000000..4bf22317a8 --- /dev/null +++ b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php @@ -0,0 +1,114 @@ +useEdgeTransactions = $use_edge_transactions; + return $this; + } + + public function getUseEdgeTransactions() { + return $this->useEdgeTransactions; + } + + public function setEdgeTransactionDescriptions($add, $rem, $set) { + $this->transactionDescriptions = array( + '+' => $add, + '-' => $rem, + '=' => $set, + ); + return $this; + } + + protected function newHTTPParameterType() { + return new AphrontPHIDListHTTPParameterType(); + } + + protected function getValueForTransaction() { + $new = parent::getValueForTransaction(); + + if (!$this->getUseEdgeTransactions()) { + return $new; + } + + $old = $this->getInitialValue(); + if ($old === null) { + return array( + '=' => array_fuse($new), + ); + } + + // If we're building an edge transaction and the request has data about the + // original value the user saw when they loaded the form, interpret the + // edit as a mixture of "+" and "-" operations instead of a single "=" + // operation. This limits our exposure to race conditions by making most + // concurrent edits merge correctly. + + $add = array_diff($new, $old); + $rem = array_diff($old, $new); + + $value = array(); + + if ($add) { + $value['+'] = array_fuse($add); + } + if ($rem) { + $value['-'] = array_fuse($rem); + } + + return $value; + } + + protected function newEditType() { + if ($this->getUseEdgeTransactions()) { + return new PhabricatorEdgeEditType(); + } + + return parent::newEditType(); + } + + public function getEditTransactionTypes() { + if (!$this->getUseEdgeTransactions()) { + return parent::getEditTransactionTypes(); + } + + $transaction_type = $this->getTransactionType(); + if ($transaction_type === null) { + return array(); + } + + $type_key = $this->getEditTypeKey(); + $strings = $this->transactionDescriptions; + + $base = $this->getEditTransactionType(); + + $add = id(clone $base) + ->setEditType($type_key.'.add') + ->setEdgeOperation('+') + ->setDescription(idx($strings, '+')) + ->setValueDescription(pht('List of PHIDs to add.')); + + $rem = id(clone $base) + ->setEditType($type_key.'.remove') + ->setEdgeOperation('-') + ->setDescription(idx($strings, '-')) + ->setValueDescription(pht('List of PHIDs to remove.')); + + $set = id(clone $base) + ->setEditType($type_key.'.set') + ->setEdgeOperation('=') + ->setDescription(idx($strings, '=')) + ->setValueDescription(pht('List of PHIDs to set.')); + + return array( + $add, + $rem, + $set, + ); + } + +} diff --git a/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php b/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php index cced1236a1..d7d72920f4 100644 --- a/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php +++ b/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php @@ -1,9 +1,7 @@ setDatasource($this->newDatasource()); - if ($this->originalValue !== null) { - $control->setOriginalValue($this->originalValue); + $initial_value = $this->getInitialValue(); + if ($initial_value !== null) { + $control->setOriginalValue($initial_value); } return $control; } - public function setValue($value) { - $this->originalValue = $value; - return parent::setValue($value); - } - - protected function getValueFromSubmit(AphrontRequest $request, $key) { - // TODO: Maybe move this unusual read somewhere else so subclassing this - // correctly is easier? - $this->originalValue = $request->getArr($key.'.original'); - - return parent::getValueFromSubmit($request, $key); - } - - protected function getValueForTransaction() { - $new = parent::getValueForTransaction(); - - $edge_types = array( - PhabricatorTransactions::TYPE_EDGE => true, - PhabricatorTransactions::TYPE_SUBSCRIBERS => true, - ); - - if (isset($edge_types[$this->getTransactionType()])) { - if ($this->originalValue !== null) { - // If we're building an edge transaction and the request has data - // about the original value the user saw when they loaded the form, - // interpret the edit as a mixture of "+" and "-" operations instead - // of a single "=" operation. This limits our exposure to race - // conditions by making most concurrent edits merge correctly. - - $new = parent::getValueForTransaction(); - $old = $this->originalValue; - - $add = array_diff($new, $old); - $rem = array_diff($old, $new); - - $value = array(); - - if ($add) { - $value['+'] = array_fuse($add); - } - if ($rem) { - $value['-'] = array_fuse($rem); - } - - return $value; - } else { - - if (!is_array($new)) { - throw new Exception(print_r($new, true)); - } - - return array( - '=' => array_fuse($new), - ); - } - } - - return $new; - } - - protected function newHTTPParameterType() { - return new AphrontPHIDListHTTPParameterType(); + protected function getInitialValueFromSubmit(AphrontRequest $request, $key) { + return $request->getArr($key.'.original'); } }