diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9664594652..3c2106b883 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1063,7 +1063,6 @@ phutil_register_library_map(array( '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', @@ -3182,7 +3181,6 @@ phutil_register_library_map(array( 'PhabricatorCustomFieldNumericIndexStorage' => 'PhabricatorCustomFieldIndexStorage', 'PhabricatorCustomFieldStorage' => 'PhabricatorLiskDAO', 'PhabricatorCustomFieldStringIndexStorage' => 'PhabricatorCustomFieldIndexStorage', - 'PhabricatorCustomFieldValidationException' => 'Exception', 'PhabricatorDaemon' => 'PhutilDaemon', 'PhabricatorDaemonCombinedLogController' => 'PhabricatorDaemonController', 'PhabricatorDaemonConsoleController' => 'PhabricatorDaemonController', diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php index 335691f1c3..6b0cbc8671 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php @@ -273,40 +273,60 @@ class ManiphestAuxiliaryFieldDefaultSpecification return $this->setValue($value); } - public function validate() { - switch ($this->getFieldType()) { - case self::TYPE_INT: - if ($this->getValue() && !is_numeric($this->getValue())) { - throw new PhabricatorCustomFieldValidationException( - pht( - '%s must be an integer value.', - $this->getLabel())); - } - break; - case self::TYPE_BOOL: - return true; - case self::TYPE_STRING: - return true; - case self::TYPE_SELECT: - return true; - case self::TYPE_DATE: - if ((int)$this->getValue() <= 0 && $this->isRequired()) { - throw new PhabricatorCustomFieldValidationException( - pht( - '%s must be a valid date.', - $this->getLabel())); - } - break; - case self::TYPE_USER: - case self::TYPE_USERS: - if (!is_array($this->getValue())) { - throw new PhabricatorCustomFieldValidationException( - pht( - '%s is not a valid list of user PHIDs.', - $this->getLabel())); - } - break; + public function validateApplicationTransactions( + PhabricatorApplicationTransactionEditor $editor, + $type, + array $xactions) { + + $errors = parent::validateApplicationTransactions( + $editor, + $type, + $xactions); + + $has_value = false; + $value = null; + foreach ($xactions as $xaction) { + $has_value = true; + $value = $xaction->getNewValue(); + switch ($this->getFieldType()) { + case self::TYPE_INT: + if ($value && !is_numeric($value)) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht('%s must be an integer value.', $this->getLabel()), + $xaction); + $this->setError(pht('Invalid')); + } + break; + case self::TYPE_DATE: + if ((int)$value <= 0 && $this->isRequired()) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht('%s must be a valid date.', $this->getLabel()), + $xaction); + $this->setError(pht('Invalid')); + } + break; + } } + + if ($this->isRequired()) { + if (!$has_value) { + $value = $this->getOldValueForApplicationTransactions(); + } + if (!$value) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('%s is required.', $this->getLabel()), + null); + $this->setError(pht('Required')); + } + } + + return $errors; } public function setDefaultValue($value) { diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php index eb7c35b5d1..0e2593123f 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php @@ -63,10 +63,6 @@ abstract class ManiphestAuxiliaryFieldSpecification return $this->value; } - public function validate() { - return true; - } - public function isRequired() { return false; } @@ -173,7 +169,11 @@ abstract class ManiphestAuxiliaryFieldSpecification } public function renderPropertyViewValue() { - return $this->renderForDetailView(); + $value = $this->renderForDetailView(); + if (!strlen($value)) { + return null; + } + return $value; } public function renderPropertyViewLabel() { diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 85b918fbd5..fe33126384 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -142,22 +142,24 @@ final class ManiphestTaskEditController extends ManiphestController { $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; - } - - if ($aux_field->isRequired() && !$aux_field->getValue()) { - $errors[] = pht('%s is required.', $aux_field->getLabel()); - $aux_field->setError(pht('Required')); - } + // TODO: We're faking a call to the ApplicaitonTransaction validation + // logic here. We need valid objects to pass, but they aren't used + // in a meaningful way. For now, build User objects. Once the Maniphest + // objects exist, this will switch over automatically. This is a big + // hack but shouldn't be long for this world. + $placeholder_editor = new PhabricatorUserProfileEditor(); + + $field_errors = $aux_field->validateApplicationTransactions( + $placeholder_editor, + PhabricatorTransactions::TYPE_CUSTOMFIELD, + array( + id(new PhabricatorUserTransaction()) + ->setOldValue($aux_old_value) + ->setNewValue($aux_new_value), + )); - try { - $aux_field->validate(); - } catch (Exception $e) { - $errors[] = $e->getMessage(); - $aux_field->setError(pht('Invalid')); + foreach ($field_errors as $error) { + $errors[] = $error->getMessage(); } $old_values[$aux_field->getFieldKey()] = $aux_old_value; @@ -490,12 +492,6 @@ final class ManiphestTaskEditController extends ManiphestController { ->setDatasource('/typeahead/common/projects/')); foreach ($aux_fields as $aux_field) { - if ($aux_field->isRequired() && - !$aux_field->getError() && - !$aux_field->getValue()) { - $aux_field->setError(true); - } - $aux_control = $aux_field->renderEditControl(); $form->appendChild($aux_control); } diff --git a/src/applications/maniphest/field/ManiphestCustomField.php b/src/applications/maniphest/field/ManiphestCustomField.php index beb98ed298..56ffa47cdf 100644 --- a/src/applications/maniphest/field/ManiphestCustomField.php +++ b/src/applications/maniphest/field/ManiphestCustomField.php @@ -36,14 +36,6 @@ abstract class ManiphestCustomField public function setHandles(array $handles) { } - public function isRequired() { - return false; - } - - public function validate() { - return true; - } - /** * Render a verb to appear in email titles when a transaction involving this * field occurs. Specifically, Maniphest emails are formatted like this: diff --git a/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php b/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php deleted file mode 100644 index 0cfc42251e..0000000000 --- a/src/infrastructure/customfield/exception/PhabricatorCustomFieldValidationException.php +++ /dev/null @@ -1,6 +0,0 @@ -getRequired()) { - $value = null; + $value = $this->getOldValueForApplicationTransactions(); $transaction = null; foreach ($xactions as $xaction) { $value = $xaction->getNewValue(); diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php index 198080dc22..93cd789a92 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php @@ -63,7 +63,7 @@ final class PhabricatorStandardCustomFieldDate ->setName($this->getFieldKey()) ->setUser($this->getViewer()) ->setCaption($this->getCaption()) - ->setAllowNull(true); + ->setAllowNull(!$this->getRequired()); $control->setValue($this->getFieldValue());