From a246c85c6b51faabafcd71a7da7a4c3d89158340 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 25 Mar 2014 16:08:40 -0700 Subject: [PATCH] Use ApplicationTransactions and CustomField to implement build steps Summary: Ref T1049. Fixes T4602. Moves all the funky field stuff to CustomField. Uses ApplicationTransactions to apply and record edits. This makes "artifact" fields a little less nice (but still perfectly usable). With D8599, I think they're reasonable overall. We can improve this in the future. All other field types are better (e.g., fixes weird bugs with "bool", fixes lots of weird behavior around required fields), and this gives us access to many new field types. Test Plan: Made a bunch of step edits. Here's an example: {F133694} Note that: - "Required" fields work correctly. - the transaction record is shown at the bottom of the page. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4602, T1049 Differential Revision: https://secure.phabricator.com/D8600 --- .../20140321.harbor.1.bxaction.sql | 21 +++ src/__phutil_library_map__.php | 15 +++ .../HarbormasterStepEditController.php | 125 +++++------------- .../HarbormasterBuildStepCoreCustomField.php | 41 ++++++ .../HarbormasterBuildStepCustomField.php | 6 + .../editor/HarbormasterBuildStepEditor.php | 6 + .../HarbormasterBuildStepTransactionQuery.php | 10 ++ .../step/BuildStepImplementation.php | 50 ++----- .../step/CommandBuildStepImplementation.php | 36 ++--- ...sterHTTPRequestBuildStepImplementation.php | 44 ++---- .../step/LeaseHostBuildStepImplementation.php | 31 ++--- ...PublishFragmentBuildStepImplementation.php | 36 ++--- .../step/SleepBuildStepImplementation.php | 23 +--- .../UploadArtifactBuildStepImplementation.php | 51 ++----- .../configuration/HarbormasterBuildStep.php | 26 +++- .../HarbormasterBuildStepTransaction.php | 14 ++ .../worker/HarbormasterTargetWorker.php | 11 +- .../field/ManiphestConfiguredCustomField.php | 2 +- .../PhabricatorUserConfiguredCustomField.php | 2 +- ...habricatorProjectConfiguredCustomField.php | 2 +- .../PhabricatorProjectDescriptionField.php | 2 +- ...PhabricatorCustomFieldConfigOptionType.php | 7 +- .../field/PhabricatorCustomField.php | 18 ++- .../PhabricatorStandardCustomField.php | 11 ++ 24 files changed, 273 insertions(+), 317 deletions(-) create mode 100644 resources/sql/autopatches/20140321.harbor.1.bxaction.sql create mode 100644 src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php create mode 100644 src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php create mode 100644 src/applications/harbormaster/editor/HarbormasterBuildStepEditor.php create mode 100644 src/applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php create mode 100644 src/applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php diff --git a/resources/sql/autopatches/20140321.harbor.1.bxaction.sql b/resources/sql/autopatches/20140321.harbor.1.bxaction.sql new file mode 100644 index 0000000000..1d65d2914a --- /dev/null +++ b/resources/sql/autopatches/20140321.harbor.1.bxaction.sql @@ -0,0 +1,21 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_buildsteptransaction ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + phid VARCHAR(64) NOT NULL COLLATE utf8_bin, + authorPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + viewPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + editPolicy VARCHAR(64) NOT NULL COLLATE utf8_bin, + commentPHID VARCHAR(64) COLLATE utf8_bin, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) NOT NULL COLLATE utf8_bin, + oldValue LONGTEXT NOT NULL COLLATE utf8_bin, + newValue LONGTEXT NOT NULL COLLATE utf8_bin, + contentSource LONGTEXT NOT NULL COLLATE utf8_bin, + metadata LONGTEXT NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + + UNIQUE KEY `key_phid` (phid), + KEY `key_object` (objectPHID) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f0aaa79da7..2871718c8e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -712,7 +712,12 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanTransactionQuery.php', 'HarbormasterBuildQuery' => 'applications/harbormaster/query/HarbormasterBuildQuery.php', 'HarbormasterBuildStep' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStep.php', + 'HarbormasterBuildStepCoreCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php', + 'HarbormasterBuildStepCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php', + 'HarbormasterBuildStepEditor' => 'applications/harbormaster/editor/HarbormasterBuildStepEditor.php', 'HarbormasterBuildStepQuery' => 'applications/harbormaster/query/HarbormasterBuildStepQuery.php', + 'HarbormasterBuildStepTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php', + 'HarbormasterBuildStepTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildStepTransactionQuery.php', 'HarbormasterBuildTarget' => 'applications/harbormaster/storage/build/HarbormasterBuildTarget.php', 'HarbormasterBuildTargetQuery' => 'applications/harbormaster/query/HarbormasterBuildTargetQuery.php', 'HarbormasterBuildViewController' => 'applications/harbormaster/controller/HarbormasterBuildViewController.php', @@ -3310,8 +3315,18 @@ phutil_register_library_map(array( array( 0 => 'HarbormasterDAO', 1 => 'PhabricatorPolicyInterface', + 2 => 'PhabricatorCustomFieldInterface', ), + 'HarbormasterBuildStepCoreCustomField' => + array( + 0 => 'HarbormasterBuildStepCustomField', + 1 => 'PhabricatorStandardCustomFieldInterface', + ), + 'HarbormasterBuildStepCustomField' => 'PhabricatorCustomField', + 'HarbormasterBuildStepEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildStepTransaction' => 'PhabricatorApplicationTransaction', + 'HarbormasterBuildStepTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'HarbormasterBuildTarget' => array( 0 => 'HarbormasterDAO', diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index 255d008e59..973e590e54 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -27,85 +27,39 @@ final class HarbormasterStepEditController $plan = $step->getBuildPlan(); $implementation = $step->getStepImplementation(); - $implementation->validateSettingDefinitions(); - $settings = $implementation->getSettings(); + + $field_list = PhabricatorCustomField::getObjectFields( + $step, + PhabricatorCustomField::ROLE_EDIT); + $field_list + ->setViewer($viewer) + ->readFieldsFromStorage($step); $errors = array(); + $validation_exception = null; if ($request->isFormPost()) { - foreach ($implementation->getSettingDefinitions() as $name => $opt) { - $readable_name = $this->getReadableName($name, $opt); - $value = $this->getValueFromRequest($request, $name, $opt['type']); + $xactions = $field_list->buildFieldTransactionsFromRequest( + new HarbormasterBuildStepTransaction(), + $request); - // TODO: This won't catch any validation issues unless the field - // is missing completely. How should we check if the user is - // required to enter an integer? - if ($value === null) { - $errors[] = $readable_name.' is not valid.'; - } else { - $step->setDetail($name, $value); - } - } + $editor = id(new HarbormasterBuildStepEditor()) + ->setActor($viewer) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request); - if (!$errors) { - $step->save(); + try { + $editor->applyTransactions($step, $xactions); return id(new AphrontRedirectResponse()) ->setURI($this->getApplicationURI('plan/'.$plan->getID().'/')); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; } } $form = id(new AphrontFormView()) ->setUser($viewer); - // We need to render out all of the fields for the settings that - // the implementation has. - foreach ($implementation->getSettingDefinitions() as $name => $opt) { - if ($request->isFormPost()) { - $value = $this->getValueFromRequest($request, $name, $opt['type']); - } else { - $value = $settings[$name]; - } - - switch ($opt['type']) { - case BuildStepImplementation::SETTING_TYPE_STRING: - case BuildStepImplementation::SETTING_TYPE_INTEGER: - $control = id(new AphrontFormTextControl()) - ->setLabel($this->getReadableName($name, $opt)) - ->setName($name) - ->setValue($value); - break; - case BuildStepImplementation::SETTING_TYPE_BOOLEAN: - $control = id(new AphrontFormCheckboxControl()) - ->setLabel($this->getReadableName($name, $opt)) - ->setName($name) - ->setValue($value); - break; - case BuildStepImplementation::SETTING_TYPE_ARTIFACT: - $filter = $opt['artifact_type']; - $available_artifacts = - BuildStepImplementation::loadAvailableArtifacts( - $plan, - $step, - $filter); - $options = array(); - foreach ($available_artifacts as $key => $type) { - $options[$key] = $key; - } - $control = id(new AphrontFormSelectControl()) - ->setLabel($this->getReadableName($name, $opt)) - ->setName($name) - ->setValue($value) - ->setOptions($options); - break; - default: - throw new Exception("Unable to render field with unknown type."); - } - - if (isset($opt['description'])) { - $control->setCaption($opt['description']); - } - - $form->appendChild($control); - } + $field_list->appendFieldsToForm($form); $form->appendChild( id(new AphrontFormSubmitControl()) @@ -115,7 +69,7 @@ final class HarbormasterStepEditController $box = id(new PHUIObjectBoxView()) ->setHeaderText('Edit Step: '.$implementation->getName()) - ->setValidationException(null) + ->setValidationException($validation_exception) ->setForm($form); $crumbs = $this->buildApplicationCrumbs(); @@ -127,11 +81,23 @@ final class HarbormasterStepEditController $variables = $this->renderBuildVariablesTable(); + $xactions = id(new HarbormasterBuildStepTransactionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($step->getPHID())) + ->execute(); + + $xaction_view = id(new PhabricatorApplicationTransactionView()) + ->setUser($viewer) + ->setObjectPHID($step->getPHID()) + ->setTransactions($xactions) + ->setShouldTerminate(true); + return $this->buildApplicationPage( array( $crumbs, $box, $variables, + $xaction_view, ), array( 'title' => $implementation->getName(), @@ -139,31 +105,6 @@ final class HarbormasterStepEditController )); } - public function getReadableName($name, $opt) { - $readable_name = $name; - if (isset($opt['name'])) { - $readable_name = $opt['name']; - } - return $readable_name; - } - - public function getValueFromRequest(AphrontRequest $request, $name, $type) { - switch ($type) { - case BuildStepImplementation::SETTING_TYPE_STRING: - case BuildStepImplementation::SETTING_TYPE_ARTIFACT: - return $request->getStr($name); - break; - case BuildStepImplementation::SETTING_TYPE_INTEGER: - return $request->getInt($name); - break; - case BuildStepImplementation::SETTING_TYPE_BOOLEAN: - return $request->getBool($name); - break; - default: - throw new Exception("Unsupported setting type '".$type."'."); - } - } - private function renderBuildVariablesTable() { $viewer = $this->getRequest()->getUser(); diff --git a/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php new file mode 100644 index 0000000000..84b67974d7 --- /dev/null +++ b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php @@ -0,0 +1,41 @@ +getStepImplementation()->getFieldSpecifications(); + return PhabricatorStandardCustomField::buildStandardFields($this, $specs); + } + + public function shouldUseStorage() { + return false; + } + + public function readValueFromObject(PhabricatorCustomFieldInterface $object) { + $key = $this->getProxy()->getRawStandardFieldKey(); + $this->setValueFromStorage($object->getDetail($key)); + } + + public function applyApplicationTransactionInternalEffects( + PhabricatorApplicationTransaction $xaction) { + $object = $this->getObject(); + $key = $this->getProxy()->getRawStandardFieldKey(); + + $this->setValueFromApplicationTransactions($xaction->getNewValue()); + $value = $this->getValueForStorage(); + + $object->setDetail($key, $value); + } + + public function applyApplicationTransactionExternalEffects( + PhabricatorApplicationTransaction $xaction) { + return; + } + +} diff --git a/src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php b/src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php new file mode 100644 index 0000000000..b4ba34342e --- /dev/null +++ b/src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php @@ -0,0 +1,6 @@ +setAncestorClass("BuildStepImplementation") + ->setAncestorClass('BuildStepImplementation') ->setConcreteOnly(true) ->selectAndLoadSymbols(); return ipull($symbols, 'name'); @@ -33,7 +26,7 @@ abstract class BuildStepImplementation { * The description of the implementation, based on the current settings. */ public function getDescription() { - return ''; + return $this->getGenericDescription(); } /** @@ -54,44 +47,13 @@ abstract class BuildStepImplementation { return idx($this->settings, $key, $default); } - /** - * Validate the current settings of this build step. - */ - public function validateSettings() { - return true; - } - /** * Loads the settings for this build step implementation from a build * step or target. */ public final function loadSettings($build_object) { - $this->settings = array(); - $this->validateSettingDefinitions(); - foreach ($this->getSettingDefinitions() as $name => $opt) { - $this->settings[$name] = $build_object->getDetail($name); - } - return $this->settings; - } - - /** - * Validates that the setting definitions for this implementation are valid. - */ - public final function validateSettingDefinitions() { - foreach ($this->getSettingDefinitions() as $name => $opt) { - if (!isset($opt['type'])) { - throw new Exception( - 'Setting definition \''.$name. - '\' is missing type requirement.'); - } - } - } - - /** - * Return an array of settings for this step implementation. - */ - public function getSettingDefinitions() { - return array(); + $this->settings = $build_object->getDetails(); + return $this; } /** @@ -197,4 +159,8 @@ abstract class BuildStepImplementation { return call_user_func($function, $pattern, $argv); } + public function getFieldSpecifications() { + return array(); + } + } diff --git a/src/applications/harbormaster/step/CommandBuildStepImplementation.php b/src/applications/harbormaster/step/CommandBuildStepImplementation.php index d48fb5994c..7dcfed68d5 100644 --- a/src/applications/harbormaster/step/CommandBuildStepImplementation.php +++ b/src/applications/harbormaster/step/CommandBuildStepImplementation.php @@ -74,22 +74,6 @@ final class CommandBuildStepImplementation } } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['command'] === null || !is_string($settings['command'])) { - return false; - } - if ($settings['hostartifact'] === null || - !is_string($settings['hostartifact'])) { - return false; - } - - // TODO: Check if the host artifact is provided by previous build steps. - - return true; - } - public function getArtifactInputs() { return array( array( @@ -100,19 +84,19 @@ final class CommandBuildStepImplementation ); } - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'command' => array( - 'name' => 'Command', - 'description' => 'The command to execute on the remote machine.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), + 'name' => pht('Command'), + 'type' => 'text', + 'required' => true, + ), 'hostartifact' => array( - 'name' => 'Host Artifact', - 'description' => - 'The host artifact that determines what machine the command '. - 'will run on.', - 'type' => BuildStepImplementation::SETTING_TYPE_ARTIFACT, - 'artifact_type' => HarbormasterBuildArtifact::TYPE_HOST)); + 'name' => pht('Host'), + 'type' => 'text', + 'required' => true, + ), + ); } } diff --git a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php index c331b34c23..a6ac25a024 100644 --- a/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php @@ -34,10 +34,7 @@ final class HarbormasterHTTPRequestBuildStepImplementation $log_body = $build->createLog($build_target, $uri, 'http-body'); $start = $log_body->start(); - $method = 'POST'; - if ($settings['method'] !== '') { - $method = $settings['method']; - } + $method = nonempty(idx($settings, 'method'), 'POST'); list($status, $body, $headers) = id(new HTTPSFuture($uri)) ->setMethod($method) @@ -52,42 +49,17 @@ final class HarbormasterHTTPRequestBuildStepImplementation } } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['uri'] === null || !is_string($settings['uri'])) { - return false; - } - - $methods = array( - 'GET' => true, - 'POST' => true, - 'DELETE' => true, - 'PUT' => true, - ); - - $method = idx($settings, 'method'); - if (strlen($method)) { - if (empty($methods[$method])) { - return false; - } - } - - return true; - } - - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'uri' => array( - 'name' => 'URI', - 'description' => pht('The URI to request.'), - 'type' => BuildStepImplementation::SETTING_TYPE_STRING, + 'name' => pht('URI'), + 'type' => 'text', + 'required' => true, ), 'method' => array( - 'name' => 'Method', - 'description' => - pht('Request type. Should be GET, POST, PUT, or DELETE.'), - 'type' => BuildStepImplementation::SETTING_TYPE_STRING, + 'name' => pht('HTTP Method'), + 'type' => 'select', + 'options' => array_fuse(array('POST', 'GET', 'PUT', 'DELETE')), ), ); } diff --git a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php index 07bd2fef73..8ed9f15c53 100644 --- a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php +++ b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php @@ -51,19 +51,6 @@ final class LeaseHostBuildStepImplementation $artifact->save(); } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['name'] === null || !is_string($settings['name'])) { - return false; - } - if ($settings['platform'] === null || !is_string($settings['platform'])) { - return false; - } - - return true; - } - public function getArtifactOutputs() { return array( array( @@ -74,17 +61,19 @@ final class LeaseHostBuildStepImplementation ); } - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'name' => array( - 'name' => 'Artifact Name', - 'description' => - 'The name of the artifact to reference in future build steps.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), + 'name' => pht('Artifact Name'), + 'type' => 'text', + 'required' => true, + ), 'platform' => array( - 'name' => 'Platform', - 'description' => 'The platform of the host.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING)); + 'name' => pht('Platform'), + 'type' => 'text', + 'required' => true, + ), + ); } } diff --git a/src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php b/src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php index f084be66bd..f8340aa3d5 100644 --- a/src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php +++ b/src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php @@ -57,22 +57,6 @@ final class PublishFragmentBuildStepImplementation } } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['path'] === null || !is_string($settings['path'])) { - return false; - } - if ($settings['artifact'] === null || - !is_string($settings['artifact'])) { - return false; - } - - // TODO: Check if the file artifact is provided by previous build steps. - - return true; - } - public function getArtifactInputs() { return array( array( @@ -83,19 +67,19 @@ final class PublishFragmentBuildStepImplementation ); } - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'path' => array( - 'name' => 'Path', - 'description' => - 'The path of the fragment that will be published.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), + 'name' => pht('Path'), + 'type' => 'text', + 'required' => true, + ), 'artifact' => array( - 'name' => 'File Artifact', - 'description' => - 'The file artifact that will be published to Phragment.', - 'type' => BuildStepImplementation::SETTING_TYPE_ARTIFACT, - 'artifact_type' => HarbormasterBuildArtifact::TYPE_FILE)); + 'name' => pht('File Artifact'), + 'type' => 'text', + 'required' => true, + ), + ); } } diff --git a/src/applications/harbormaster/step/SleepBuildStepImplementation.php b/src/applications/harbormaster/step/SleepBuildStepImplementation.php index 120732226d..cc957b1e64 100644 --- a/src/applications/harbormaster/step/SleepBuildStepImplementation.php +++ b/src/applications/harbormaster/step/SleepBuildStepImplementation.php @@ -25,24 +25,15 @@ final class SleepBuildStepImplementation extends BuildStepImplementation { sleep($settings['seconds']); } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['seconds'] === null) { - return false; - } - if (!is_int($settings['seconds'])) { - return false; - } - return true; - } - - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'seconds' => array( - 'name' => 'Seconds', - 'description' => 'The number of seconds to sleep for.', - 'type' => BuildStepImplementation::SETTING_TYPE_INTEGER)); + 'name' => pht('Seconds'), + 'type' => 'int', + 'required' => true, + 'caption' => pht('The number of seconds to sleep for.'), + ), + ); } } diff --git a/src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php b/src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php index 00833e3add..570e42c482 100644 --- a/src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php +++ b/src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php @@ -51,25 +51,6 @@ final class UploadArtifactBuildStepImplementation $artifact->save(); } - public function validateSettings() { - $settings = $this->getSettings(); - - if ($settings['path'] === null || !is_string($settings['path'])) { - return false; - } - if ($settings['name'] === null || !is_string($settings['name'])) { - return false; - } - if ($settings['hostartifact'] === null || - !is_string($settings['hostartifact'])) { - return false; - } - - // TODO: Check if the host artifact is provided by previous build steps. - - return true; - } - public function getArtifactInputs() { return array( array( @@ -90,28 +71,24 @@ final class UploadArtifactBuildStepImplementation ); } - public function getSettingDefinitions() { + public function getFieldSpecifications() { return array( 'path' => array( - 'name' => 'Path', - 'description' => - 'The path of the file that should be retrieved. Note that on '. - 'Windows machines running FreeSSHD, this path will be relative '. - 'to the SFTP root path (configured under the SFTP tab). You can '. - 'not specify an absolute path for Windows machines.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), + 'name' => pht('Path'), + 'type' => 'text', + 'required' => true, + ), 'name' => array( - 'name' => 'Local Name', - 'description' => - 'The name for the file when it is stored in Phabricator.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), + 'name' => pht('Local Name'), + 'type' => 'text', + 'required' => true, + ), 'hostartifact' => array( - 'name' => 'Host Artifact', - 'description' => - 'The host artifact that determines what machine the command '. - 'will run on.', - 'type' => BuildStepImplementation::SETTING_TYPE_ARTIFACT, - 'artifact_type' => HarbormasterBuildArtifact::TYPE_HOST)); + 'name' => pht('Host Artifact'), + 'type' => 'text', + 'required' => true, + ), + ); } } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 7863518306..d1bf744e7b 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -1,7 +1,9 @@ assertAttached($this->customFields); + } + + public function attachCustomFields(PhabricatorCustomFieldAttachment $fields) { + $this->customFields = $fields; + return $this; + } + + } diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php new file mode 100644 index 0000000000..85513e3901 --- /dev/null +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStepTransaction.php @@ -0,0 +1,14 @@ +getImplementation(); - if (!$implementation->validateSettings()) { - $target->setTargetStatus(HarbormasterBuildTarget::STATUS_FAILED); - $target->save(); - } else { - $implementation->execute($build, $target); - $target->setTargetStatus(HarbormasterBuildTarget::STATUS_PASSED); - $target->save(); - } + $implementation->execute($build, $target); + $target->setTargetStatus(HarbormasterBuildTarget::STATUS_PASSED); + $target->save(); } catch (Exception $ex) { phlog($ex); diff --git a/src/applications/maniphest/field/ManiphestConfiguredCustomField.php b/src/applications/maniphest/field/ManiphestConfiguredCustomField.php index bbf409ddbd..9e6b4f038b 100644 --- a/src/applications/maniphest/field/ManiphestConfiguredCustomField.php +++ b/src/applications/maniphest/field/ManiphestConfiguredCustomField.php @@ -8,7 +8,7 @@ final class ManiphestConfiguredCustomField return 'maniphest'; } - public function createFields() { + public function createFields($object) { $config = PhabricatorEnv::getEnvConfig( 'maniphest.custom-field-definitions', array()); diff --git a/src/applications/people/customfield/PhabricatorUserConfiguredCustomField.php b/src/applications/people/customfield/PhabricatorUserConfiguredCustomField.php index f324ca313a..9c58b970bb 100644 --- a/src/applications/people/customfield/PhabricatorUserConfiguredCustomField.php +++ b/src/applications/people/customfield/PhabricatorUserConfiguredCustomField.php @@ -8,7 +8,7 @@ final class PhabricatorUserConfiguredCustomField return 'user'; } - public function createFields() { + public function createFields($object) { return PhabricatorStandardCustomField::buildStandardFields( $this, PhabricatorEnv::getEnvConfig('user.custom-field-definitions', array())); diff --git a/src/applications/project/customfield/PhabricatorProjectConfiguredCustomField.php b/src/applications/project/customfield/PhabricatorProjectConfiguredCustomField.php index 8e2cb3dce1..6ee623147d 100644 --- a/src/applications/project/customfield/PhabricatorProjectConfiguredCustomField.php +++ b/src/applications/project/customfield/PhabricatorProjectConfiguredCustomField.php @@ -8,7 +8,7 @@ final class PhabricatorProjectConfiguredCustomField return 'project'; } - public function createFields() { + public function createFields($object) { return PhabricatorStandardCustomField::buildStandardFields( $this, PhabricatorEnv::getEnvConfig( diff --git a/src/applications/project/customfield/PhabricatorProjectDescriptionField.php b/src/applications/project/customfield/PhabricatorProjectDescriptionField.php index f16687388a..aaeeaeceb5 100644 --- a/src/applications/project/customfield/PhabricatorProjectDescriptionField.php +++ b/src/applications/project/customfield/PhabricatorProjectDescriptionField.php @@ -3,7 +3,7 @@ final class PhabricatorProjectDescriptionField extends PhabricatorProjectStandardCustomField { - public function createFields() { + public function createFields($object) { return PhabricatorStandardCustomField::buildStandardFields( $this, array( diff --git a/src/infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php b/src/infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php index d1a0f5f458..ef6417ac5f 100644 --- a/src/infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php +++ b/src/infrastructure/customfield/config/PhabricatorCustomFieldConfigOptionType.php @@ -43,9 +43,14 @@ final class PhabricatorCustomFieldConfigOptionType foreach ($faux_spec as $key => $spec) { unset($faux_spec[$key]['disabled']); } + + // TODO: We might need to build a real object here eventually. + $faux_object = null; + $fields = PhabricatorCustomField::buildFieldList( $field_base_class, - $faux_spec); + $faux_spec, + $faux_object); $list_id = celerity_generate_unique_node_id(); $input_id = celerity_generate_unique_node_id(); diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 11a5ccc41f..f9bf92ac4d 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -60,7 +60,10 @@ abstract class PhabricatorCustomField { "object of class '{$obj_class}'."); } - $fields = PhabricatorCustomField::buildFieldList($base_class, $spec); + $fields = PhabricatorCustomField::buildFieldList( + $base_class, + $spec, + $object); foreach ($fields as $key => $field) { if (!$field->shouldEnableForRole($role)) { @@ -97,7 +100,7 @@ abstract class PhabricatorCustomField { /** * @task apps */ - public static function buildFieldList($base_class, array $spec) { + public static function buildFieldList($base_class, array $spec, $object) { $field_objects = id(new PhutilSymbolLoader()) ->setAncestorClass($base_class) ->loadObjects(); @@ -106,7 +109,7 @@ abstract class PhabricatorCustomField { $from_map = array(); foreach ($field_objects as $field_object) { $current_class = get_class($field_object); - foreach ($field_object->createFields() as $field) { + foreach ($field_object->createFields($object) as $field) { $key = $field->getFieldKey(); if (isset($fields[$key])) { $original_class = $from_map[$key]; @@ -200,10 +203,11 @@ abstract class PhabricatorCustomField { * For general implementations, the general field implementation can return * multiple field instances here. * + * @param object The object to create fields for. * @return list List of fields. * @task core */ - public function createFields() { + public function createFields($object) { return array($this); } @@ -239,9 +243,9 @@ abstract class PhabricatorCustomField { * @task core */ public function shouldEnableForRole($role) { - if ($this->proxy) { - return $this->proxy->shouldEnableForRole($role); - } + + // NOTE: All of these calls proxy individually, so we don't need to + // proxy this call as a whole. switch ($role) { case self::ROLE_APPLICATIONTRANSACTIONS: diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index afa2db84a2..14a9ef6d54 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -3,6 +3,7 @@ abstract class PhabricatorStandardCustomField extends PhabricatorCustomField { + private $rawKey; private $fieldKey; private $fieldName; private $fieldValue; @@ -40,6 +41,7 @@ abstract class PhabricatorStandardCustomField $template = clone $template; $standard = id(clone $types[$type]) + ->setRawStandardFieldKey($key) ->setFieldKey($full_key) ->setFieldConfig($value) ->setApplicationField($template); @@ -142,6 +144,15 @@ abstract class PhabricatorStandardCustomField return $this->required; } + public function setRawStandardFieldKey($raw_key) { + $this->rawKey = $raw_key; + return $this; + } + + public function getRawStandardFieldKey() { + return $this->rawKey; + } + /* -( PhabricatorCustomField )--------------------------------------------- */