Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14844625
D14601.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
19 KB
Referenced Files
None
Subscribers
None
D14601.diff
View Options
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -145,6 +145,7 @@
'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 @@
'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 @@
'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 @@
'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 @@
'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorPHID' => 'Phobject',
'PhabricatorPHIDConstants' => 'Phobject',
+ 'PhabricatorPHIDListEditField' => 'PhabricatorEditField',
'PhabricatorPHIDResolver' => 'Phobject',
'PhabricatorPHIDType' => 'Phobject',
'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase',
@@ -7437,7 +7441,7 @@
'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
--- /dev/null
+++ b/src/aphront/httpparametertype/AphrontListHTTPParameterType.php
@@ -0,0 +1,10 @@
+<?php
+
+abstract class AphrontListHTTPParameterType
+ extends AphrontHTTPParameterType {
+
+ protected function getParameterDefault() {
+ return array();
+ }
+
+}
diff --git a/src/aphront/httpparametertype/AphrontPHIDListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontPHIDListHTTPParameterType.php
--- a/src/aphront/httpparametertype/AphrontPHIDListHTTPParameterType.php
+++ b/src/aphront/httpparametertype/AphrontPHIDListHTTPParameterType.php
@@ -1,7 +1,7 @@
<?php
final class AphrontPHIDListHTTPParameterType
- extends AphrontHTTPParameterType {
+ extends AphrontListHTTPParameterType {
protected function getParameterValue(AphrontRequest $request, $key) {
$type = new AphrontStringListHTTPParameterType();
diff --git a/src/aphront/httpparametertype/AphrontProjectListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontProjectListHTTPParameterType.php
--- a/src/aphront/httpparametertype/AphrontProjectListHTTPParameterType.php
+++ b/src/aphront/httpparametertype/AphrontProjectListHTTPParameterType.php
@@ -1,7 +1,7 @@
<?php
final class AphrontProjectListHTTPParameterType
- extends AphrontHTTPParameterType {
+ extends AphrontListHTTPParameterType {
protected function getParameterValue(AphrontRequest $request, $key) {
$type = new AphrontStringListHTTPParameterType();
diff --git a/src/aphront/httpparametertype/AphrontStringListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontStringListHTTPParameterType.php
--- a/src/aphront/httpparametertype/AphrontStringListHTTPParameterType.php
+++ b/src/aphront/httpparametertype/AphrontStringListHTTPParameterType.php
@@ -1,7 +1,7 @@
<?php
final class AphrontStringListHTTPParameterType
- extends AphrontHTTPParameterType {
+ extends AphrontListHTTPParameterType {
protected function getParameterValue(AphrontRequest $request, $key) {
$list = $request->getArr($key, null);
diff --git a/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php b/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php
--- a/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php
+++ b/src/aphront/httpparametertype/AphrontUserListHTTPParameterType.php
@@ -1,7 +1,7 @@
<?php
final class AphrontUserListHTTPParameterType
- extends AphrontHTTPParameterType {
+ extends AphrontListHTTPParameterType {
protected function getParameterValue(AphrontRequest $request, $key) {
$type = new AphrontStringListHTTPParameterType();
diff --git a/src/applications/project/editor/PhabricatorProjectsEditEngineExtension.php b/src/applications/project/editor/PhabricatorProjectsEditEngineExtension.php
--- a/src/applications/project/editor/PhabricatorProjectsEditEngineExtension.php
+++ b/src/applications/project/editor/PhabricatorProjectsEditEngineExtension.php
@@ -48,6 +48,11 @@
->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
--- a/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php
+++ b/src/applications/subscriptions/editor/PhabricatorSubscriptionsEditEngineExtension.php
@@ -45,6 +45,11 @@
->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
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -1048,6 +1048,11 @@
$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
--- a/src/applications/transactions/editfield/PhabricatorEditField.php
+++ b/src/applications/transactions/editfield/PhabricatorEditField.php
@@ -7,6 +7,7 @@
private $label;
private $aliases = array();
private $value;
+ private $initialValue;
private $hasValue = false;
private $object;
private $transactionType;
@@ -262,6 +263,7 @@
public function setValue($value) {
$this->hasValue = true;
+ $this->initialValue = $value;
$this->value = $value;
return $this;
}
@@ -289,6 +291,10 @@
return $this;
}
+ public function getMetadata() {
+ return $this->metadata;
+ }
+
protected function getValueForTransaction() {
return $this->getValue();
}
@@ -348,6 +354,31 @@
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 @@
$value = $this->getDefaultValue();
}
$this->value = $value;
+
+ $initial_value = $this->getInitialValueFromSubmit($request, $key);
+ $this->initialValue = $initial_value;
+
return $this;
}
@@ -414,66 +449,30 @@
->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.
-
- $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.'),
- ),
- );
-
- 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,
- );
+ return $this->newEditType()
+ ->setEditType($type_key)
+ ->setTransactionType($transaction_type)
+ ->setDescription($this->getDescription())
+ ->setMetadata($this->getMetadata());
+ }
+
+ public function getEditTransactionTypes() {
+ $edit_type = $this->getEditTransactionType();
+
+ 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
--- /dev/null
+++ b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php
@@ -0,0 +1,114 @@
+<?php
+
+abstract class PhabricatorPHIDListEditField
+ extends PhabricatorEditField {
+
+ private $useEdgeTransactions;
+ private $transactionDescriptions = array();
+
+ public function setUseEdgeTransactions($use_edge_transactions) {
+ $this->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
--- a/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php
+++ b/src/applications/transactions/editfield/PhabricatorTokenizerEditField.php
@@ -1,9 +1,7 @@
<?php
abstract class PhabricatorTokenizerEditField
- extends PhabricatorEditField {
-
- private $originalValue;
+ extends PhabricatorPHIDListEditField {
abstract protected function newDatasource();
@@ -11,75 +9,16 @@
$control = id(new AphrontFormTokenizerControl())
->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');
}
}
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Mon, Feb 3, 5:13 PM (17 h, 8 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7087411
Default Alt Text
D14601.diff (19 KB)
Attached To
Mode
D14601: Improve code structure of PHID fields in EditEngine
Attached
Detach File
Event Timeline
Log In to Comment