diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php index 37fd89142c..82cdda49fa 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php @@ -89,20 +89,6 @@ abstract class ManiphestAuxiliaryFieldSpecification } - /** - * When the user creates a task, the UI prompts them to "Create another - * similar task". This copies some fields (e.g., Owner and CCs) but not other - * fields (e.g., description). If this custom field should also be copied, - * return true from this method. - * - * @return bool True to copy the default value from the template task when - * creating a new similar task. - */ - public function shouldCopyWhenCreatingSimilarTask() { - return false; - } - - /** * Render a verb to appear in email titles when a transaction involving this * field occurs. Specifically, Maniphest emails are formatted like this: @@ -247,23 +233,56 @@ abstract class ManiphestAuxiliaryFieldSpecification return $this->getLabel(); } + public function readValueFromRequest(AphrontRequest $request) { + return $this->setValueFromRequest($request); + } -/* -( Legacy Migration Support )------------------------------------------- */ - - - // TODO: Migrate to common storage and remove this. - public static function loadLegacyDataFromStorage( + public static function writeLegacyAuxiliaryUpdates( ManiphestTask $task, - PhabricatorCustomFieldList $list) { + array $map) { - $task->loadAndAttachAuxiliaryAttributes(); + $table = new ManiphestCustomFieldStorage(); + $conn_w = $table->establishConnection('w'); + $update = array(); + $remove = array(); - foreach ($list->getFields() as $field) { - if ($task->getID()) { - $key = $field->getAuxiliaryKey(); - $field->setValueFromStorage($task->getAuxiliaryAttribute($key)); + foreach ($map as $key => $value) { + $index = PhabricatorHash::digestForIndex($key); + if ($value === null) { + $remove[$index] = true; + } else { + $update[$index] = $value; } } + + if ($remove) { + queryfx( + $conn_w, + 'DELETE FROM %T WHERE objectPHID = %s AND fieldIndex IN (%Ls)', + $table->getTableName(), + $task->getPHID(), + array_keys($remove)); + } + + if ($update) { + $sql = array(); + foreach ($update as $index => $val) { + $sql[] = qsprintf( + $conn_w, + '(%s, %s, %s)', + $task->getPHID(), + $index, + $val); + } + queryfx( + $conn_w, + 'INSERT INTO %T (objectPHID, fieldIndex, fieldValue) + VALUES %Q ON DUPLICATE KEY + UPDATE fieldValue = VALUES(fieldValue)', + $table->getTableName(), + implode(', ', $sql)); + } + } } diff --git a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php index ed719f165f..648506e83b 100644 --- a/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php +++ b/src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php @@ -157,19 +157,31 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod { $transactions[] = $transaction; } + $field_list = PhabricatorCustomField::getObjectFields( + $task, + PhabricatorCustomField::ROLE_EDIT); + $auxiliary = $request->getValue('auxiliary'); if ($auxiliary) { - $task->loadAndAttachAuxiliaryAttributes(); - foreach ($auxiliary as $aux_key => $aux_value) { + foreach ($field_list->getFields() as $key => $field) { + if (!array_key_exists($key, $auxiliary)) { + continue; + } $transaction = clone $template; $transaction->setTransactionType( ManiphestTransactionType::TYPE_AUXILIARY); - $transaction->setMetadataValue('aux:key', $aux_key); - $transaction->setNewValue($aux_value); + $transaction->setMetadataValue('aux:key', $key); + $transaction->setOldValue( + $field->getOldValueForApplicationTransactions()); + $transaction->setNewValue($auxiliary[$key]); $transactions[] = $transaction; } } + if (!$transactions) { + return; + } + $event = new PhabricatorEvent( PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK, array( @@ -186,6 +198,7 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod { $editor = new ManiphestTransactionEditor(); $editor->setActor($request->getUser()); + $editor->setAuxiliaryFields($field_list->getFields()); $editor->applyTransactions($task, $transactions); $event = new PhabricatorEvent( @@ -217,7 +230,15 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod { $result = array(); foreach ($tasks as $task) { // TODO: Batch this get as CustomField gets cleaned up. - $auxiliary = $task->loadLegacyAuxiliaryFieldMap(); + $field_list = PhabricatorCustomField::getObjectFields( + $task, + PhabricatorCustomField::ROLE_EDIT); + $field_list->readFieldsFromStorage($task); + + $auxiliary = mpull( + $field_list->getFields(), + 'getValueForStorage', + 'getFieldKey'); $task_deps = $all_deps->getDestinationPHIDs( array($task->getPHID()), diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 388720770c..bf10fcf63b 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -93,9 +93,7 @@ final class ManiphestTaskEditController extends ManiphestController { $field->setViewer($user); } - ManiphestAuxiliaryFieldSpecification::loadLegacyDataFromStorage( - $task, - $field_list); + $field_list->readFieldsFromStorage($task); $aux_fields = $field_list->getFields(); @@ -135,12 +133,17 @@ final class ManiphestTaskEditController extends ManiphestController { $errors[] = pht('Title is required.'); } + $old_values = array(); foreach ($aux_fields as $aux_arr_key => $aux_field) { - $aux_field->setValueFromRequest($request); - $aux_key = $aux_field->getAuxiliaryKey(); - $aux_old_value = $task->getAuxiliaryAttribute($aux_key); + // TODO: This should be buildFieldTransactionsFromRequest() once we + // switch to ApplicationTransactions properly. - if ((int)$aux_old_value === $aux_field->getValueForStorage()) { + $aux_old_value = $aux_field->getOldValueForApplicationTransactions(); + $aux_field->readValueFromRequest($request); + $aux_new_value = $aux_field->getNewValueForApplicationTransactions(); + + // TODO: What's going on here? + if ((int)$aux_old_value === $aux_new_value) { unset($aux_fields[$aux_arr_key]); continue; } @@ -156,6 +159,8 @@ final class ManiphestTaskEditController extends ManiphestController { $errors[] = $e->getMessage(); $aux_field->setError(pht('Invalid')); } + + $old_values[$aux_field->getFieldKey()] = $aux_old_value; } if ($errors) { @@ -220,9 +225,11 @@ final class ManiphestTaskEditController extends ManiphestController { $transaction = clone $template; $transaction->setTransactionType( ManiphestTransactionType::TYPE_AUXILIARY); - $aux_key = $aux_field->getAuxiliaryKey(); + $aux_key = $aux_field->getFieldKey(); $transaction->setMetadataValue('aux:key', $aux_key); - $transaction->setNewValue($aux_field->getValueForStorage()); + $transaction->setOldValue(idx($old_values, $aux_key)); + $transaction->setNewValue( + $aux_field->getNewValueForApplicationTransactions()); $transactions[] = $transaction; } } @@ -302,16 +309,27 @@ final class ManiphestTaskEditController extends ManiphestController { $task->setOwnerPHID($template_task->getOwnerPHID()); $task->setPriority($template_task->getPriority()); - if ($aux_fields) { - $template_task->loadAndAttachAuxiliaryAttributes(); - foreach ($aux_fields as $aux_field) { - if (!$aux_field->shouldCopyWhenCreatingSimilarTask()) { - continue; - } + $template_fields = PhabricatorCustomField::getObjectFields( + $template_task, + PhabricatorCustomField::ROLE_EDIT); - $aux_key = $aux_field->getAuxiliaryKey(); - $value = $template_task->getAuxiliaryAttribute($aux_key); - $aux_field->setValueFromStorage($value); + $fields = $template_fields->getFields(); + foreach ($fields as $key => $field) { + if (!$field->shouldCopyWhenCreatingSimilarTask()) { + unset($fields[$key]); + } + if (empty($aux_fields[$key])) { + unset($fields[$key]); + } + } + + if ($fields) { + id(new PhabricatorCustomFieldList($fields)) + ->readFieldsFromStorage($template_task); + + foreach ($fields as $key => $field) { + $aux_fields[$key]->setValueFromStorage( + $field->getValueForStorage()); } } } diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index bde7aaf22d..b5bf62f196 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -9,7 +9,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor { private $auxiliaryFields = array(); public function setAuxiliaryFields(array $fields) { - assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification'); + assert_instances_of($fields, 'ManiphestCustomField'); $this->auxiliaryFields = $fields; return $this; } @@ -28,6 +28,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor { $email_to[] = $task->getOwnerPHID(); $pri_changed = $this->isCreate($transactions); + $aux_writes = array(); foreach ($transactions as $key => $transaction) { $type = $transaction->getTransactionType(); @@ -75,7 +76,8 @@ final class ManiphestTransactionEditor extends PhabricatorEditor { throw new Exception( "Expected 'aux:key' metadata on TYPE_AUXILIARY transaction."); } - $old = $task->getAuxiliaryAttribute($aux_key); + // This has already been populated. + $old = $transaction->getOldValue(); break; default: throw new Exception('Unknown action type.'); @@ -160,7 +162,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor { break; case ManiphestTransactionType::TYPE_AUXILIARY: $aux_key = $transaction->getMetadataValue('aux:key'); - $task->setAuxiliaryAttribute($aux_key, $new); + $aux_writes[$aux_key] = $new; break; case ManiphestTransactionType::TYPE_EDGE: // Edge edits are accomplished through PhabricatorEdgeEditor, which @@ -184,6 +186,13 @@ final class ManiphestTransactionEditor extends PhabricatorEditor { } $task->save(); + + if ($aux_writes) { + ManiphestAuxiliaryFieldSpecification::writeLegacyAuxiliaryUpdates( + $task, + $aux_writes); + } + foreach ($transactions as $transaction) { $transaction->setTaskID($task->getID()); $transaction->save(); diff --git a/src/applications/maniphest/field/ManiphestCustomField.php b/src/applications/maniphest/field/ManiphestCustomField.php index 26c18cbb8c..6fd679f0a5 100644 --- a/src/applications/maniphest/field/ManiphestCustomField.php +++ b/src/applications/maniphest/field/ManiphestCustomField.php @@ -15,4 +15,18 @@ abstract class ManiphestCustomField return new ManiphestCustomFieldNumericIndex(); } + /** + * When the user creates a task, the UI prompts them to "Create another + * similar task". This copies some fields (e.g., Owner and CCs) but not other + * fields (e.g., description). If this custom field should also be copied, + * return true from this method. + * + * @return bool True to copy the default value from the template task when + * creating a new similar task. + */ + public function shouldCopyWhenCreatingSimilarTask() { + return false; + } + + } diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 661f41aeac..312fe5cc52 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -35,8 +35,6 @@ final class ManiphestTask extends ManiphestDAO protected $ownerOrdering; - private $auxiliaryAttributes = self::ATTACHABLE; - private $auxiliaryDirty = array(); private $groupByProjectPHID = self::ATTACHABLE; private $customFields = self::ATTACHABLE; @@ -97,19 +95,6 @@ final class ManiphestTask extends ManiphestDAO return $this; } - public function getAuxiliaryAttribute($key, $default = null) { - $this->assertAttached($this->auxiliaryAttributes); - return idx($this->auxiliaryAttributes, $key, $default); - } - - public function setAuxiliaryAttribute($key, $val) { - $this->assertAttached($this->auxiliaryAttributes); - - $this->auxiliaryAttributes[$key] = $val; - $this->auxiliaryDirty[$key] = true; - return $this; - } - public function setTitle($title) { $this->title = $title; if (!$this->getID()) { @@ -127,41 +112,6 @@ final class ManiphestTask extends ManiphestDAO return $this->assertAttached($this->groupByProjectPHID); } - public function attachAuxiliaryAttributes(array $attrs) { - if ($this->auxiliaryDirty) { - throw new Exception( - "This object has dirty attributes, you can not attach new attributes ". - "without writing or discarding the dirty attributes."); - } - $this->auxiliaryAttributes = $attrs; - return $this; - } - - public function loadLegacyAuxiliaryFieldMap() { - $field_list = PhabricatorCustomField::getObjectFields( - $this, - PhabricatorCustomField::ROLE_EDIT); - $field_list->readFieldsFromStorage($this); - - $map = array(); - foreach ($field_list->getFields() as $field) { - $map[$field->getFieldKey()] = $field->getValueForStorage(); - } - - return $map; - } - - public function loadAndAttachAuxiliaryAttributes() { - if (!$this->getPHID()) { - $this->auxiliaryAttributes = array(); - return $this; - } - - $this->auxiliaryAttributes = $this->loadLegacyAuxiliaryFieldMap(); - - return $this; - } - public function save() { if (!$this->mailKey) { $this->mailKey = Filesystem::readRandomCharacters(20); @@ -183,59 +133,9 @@ final class ManiphestTask extends ManiphestDAO $this->subscribersNeedUpdate = false; } - if ($this->auxiliaryDirty) { - $this->writeAuxiliaryUpdates(); - $this->auxiliaryDirty = array(); - } - return $result; } - private function writeAuxiliaryUpdates() { - $table = new ManiphestCustomFieldStorage(); - $conn_w = $table->establishConnection('w'); - $update = array(); - $remove = array(); - - foreach ($this->auxiliaryDirty as $key => $dirty) { - $value = $this->getAuxiliaryAttribute($key); - - $index = PhabricatorHash::digestForIndex($key); - if ($value === null) { - $remove[$index] = true; - } else { - $update[$index] = $value; - } - } - - if ($remove) { - queryfx( - $conn_w, - 'DELETE FROM %T WHERE objectPHID = %s AND fieldIndex IN (%Ls)', - $table->getTableName(), - $this->getPHID(), - array_keys($remove)); - } - - if ($update) { - $sql = array(); - foreach ($update as $index => $val) { - $sql[] = qsprintf( - $conn_w, - '(%s, %s, %s)', - $this->getPHID(), - $index, - $val); - } - queryfx( - $conn_w, - 'INSERT INTO %T (objectPHID, fieldIndex, fieldValue) - VALUES %Q ON DUPLICATE KEY - UPDATE fieldValue = VALUES(fieldValue)', - $table->getTableName(), - implode(', ', $sql)); - } - } /* -( Markup Interface )--------------------------------------------------- */ diff --git a/src/applications/maniphest/view/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/ManiphestTransactionDetailView.php index 3b3c81b482..f08bcb1f95 100644 --- a/src/applications/maniphest/view/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/ManiphestTransactionDetailView.php @@ -19,7 +19,7 @@ final class ManiphestTransactionDetailView extends ManiphestView { private $auxiliaryFields; public function setAuxiliaryFields(array $fields) { - assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification'); + assert_instances_of($fields, 'ManiphestCustomField'); $this->auxiliaryFields = mpull($fields, null, 'getAuxiliaryKey'); return $this; } diff --git a/src/applications/maniphest/view/ManiphestTransactionListView.php b/src/applications/maniphest/view/ManiphestTransactionListView.php index 9e8ccc8f08..00830eea95 100644 --- a/src/applications/maniphest/view/ManiphestTransactionListView.php +++ b/src/applications/maniphest/view/ManiphestTransactionListView.php @@ -34,7 +34,7 @@ final class ManiphestTransactionListView extends ManiphestView { } public function setAuxiliaryFields(array $fields) { - assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification'); + assert_instances_of($fields, 'ManiphestCustomField'); $this->auxiliaryFields = $fields; return $this; }