From 52ec6c02ee540115daa5eed828d1671a7f838194 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Aug 2011 11:29:56 -0700 Subject: [PATCH] Move Differential's simple fields to the extensible field schema Summary: Differential has a bunch of display-only fields, implement them all as field specifications instead of hard-coded fields. Also add some more documentation and fix redundant string constants in blame rev/revert plan fields. Test Plan: Viewed, edited, and hit conduit for revisions. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: aran, jungejason, epriestley Differential Revision: 807 --- src/__phutil_library_map__.php | 11 ++ ...uitAPI_differential_getrevision_Method.php | 2 +- .../DifferentialRevisionViewController.php | 24 +-- ...lFieldSpecificationIncompleteException.php | 3 +- ...erentialFieldDataNotAvailableException.php | 30 ++++ .../field/exception/notavailable/__init__.php | 10 ++ .../DifferentialDefaultFieldSelector.php | 8 +- .../field/selector/default/__init__.php | 5 + ...fferentialApplyPatchFieldSpecification.php | 35 +++++ .../specification/applypatch/__init__.php | 12 ++ .../base/DifferentialFieldSpecification.php | 146 ++++++++++++++++-- .../field/specification/base/__init__.php | 1 + ...rentialBlameRevisionFieldSpecification.php | 4 +- ...ferentialExportPatchFieldSpecification.php | 35 +++++ .../specification/exportpatch/__init__.php | 12 ++ .../DifferentialHostFieldSpecification.php | 39 +++++ .../field/specification/host/__init__.php | 14 ++ .../DifferentialLinesFieldSpecification.php | 35 +++++ .../field/specification/lines/__init__.php | 14 ++ .../DifferentialPathFieldSpecification.php | 46 ++++++ .../field/specification/path/__init__.php | 14 ++ ...fferentialRevertPlanFieldSpecification.php | 4 +- .../DifferentialAuxiliaryField.php | 1 + 23 files changed, 465 insertions(+), 40 deletions(-) create mode 100644 src/applications/differential/field/exception/notavailable/DifferentialFieldDataNotAvailableException.php create mode 100644 src/applications/differential/field/exception/notavailable/__init__.php create mode 100644 src/applications/differential/field/specification/applypatch/DifferentialApplyPatchFieldSpecification.php create mode 100644 src/applications/differential/field/specification/applypatch/__init__.php create mode 100644 src/applications/differential/field/specification/exportpatch/DifferentialExportPatchFieldSpecification.php create mode 100644 src/applications/differential/field/specification/exportpatch/__init__.php create mode 100644 src/applications/differential/field/specification/host/DifferentialHostFieldSpecification.php create mode 100644 src/applications/differential/field/specification/host/__init__.php create mode 100644 src/applications/differential/field/specification/lines/DifferentialLinesFieldSpecification.php create mode 100644 src/applications/differential/field/specification/lines/__init__.php create mode 100644 src/applications/differential/field/specification/path/DifferentialPathFieldSpecification.php create mode 100644 src/applications/differential/field/specification/path/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1a655fd589..688bbe2e3d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -133,6 +133,7 @@ phutil_register_library_map(array( 'DatabaseConfigurationProvider' => 'applications/base/storage/configuration', 'DifferentialAction' => 'applications/differential/constants/action', 'DifferentialAddCommentView' => 'applications/differential/view/addcomment', + 'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch', 'DifferentialAuxiliaryField' => 'applications/differential/storage/auxiliaryfield', 'DifferentialBlameRevisionFieldSpecification' => 'applications/differential/field/specification/blamerev', 'DifferentialCCWelcomeMail' => 'applications/differential/mail/ccwelcome', @@ -162,18 +163,23 @@ phutil_register_library_map(array( 'DifferentialDiffTableOfContentsView' => 'applications/differential/view/difftableofcontents', 'DifferentialDiffViewController' => 'applications/differential/controller/diffview', 'DifferentialExceptionMail' => 'applications/differential/mail/exception', + 'DifferentialExportPatchFieldSpecification' => 'applications/differential/field/specification/exportpatch', + 'DifferentialFieldDataNotAvailableException' => 'applications/differential/field/exception/notavailable', 'DifferentialFieldSelector' => 'applications/differential/field/selector/base', 'DifferentialFieldSpecification' => 'applications/differential/field/specification/base', 'DifferentialFieldSpecificationIncompleteException' => 'applications/differential/field/exception/incomplete', 'DifferentialFieldValidationException' => 'applications/differential/field/exception/validation', + 'DifferentialHostFieldSpecification' => 'applications/differential/field/specification/host', 'DifferentialHunk' => 'applications/differential/storage/hunk', 'DifferentialInlineComment' => 'applications/differential/storage/inlinecomment', 'DifferentialInlineCommentEditController' => 'applications/differential/controller/inlinecommentedit', 'DifferentialInlineCommentPreviewController' => 'applications/differential/controller/inlinecommentpreview', 'DifferentialInlineCommentView' => 'applications/differential/view/inlinecomment', + 'DifferentialLinesFieldSpecification' => 'applications/differential/field/specification/lines', 'DifferentialLintStatus' => 'applications/differential/constants/lintstatus', 'DifferentialMail' => 'applications/differential/mail/base', 'DifferentialNewDiffMail' => 'applications/differential/mail/newdiff', + 'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/path', 'DifferentialPrimaryPaneView' => 'applications/differential/view/primarypane', 'DifferentialReplyHandler' => 'applications/differential/replyhandler', 'DifferentialRevertPlanFieldSpecification' => 'applications/differential/field/specification/revertplan', @@ -775,6 +781,7 @@ phutil_register_library_map(array( 'DarkConsoleServicesPlugin' => 'DarkConsolePlugin', 'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin', 'DifferentialAddCommentView' => 'AphrontView', + 'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialAuxiliaryField' => 'DifferentialDAO', 'DifferentialBlameRevisionFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialCCWelcomeMail' => 'DifferentialReviewRequestMail', @@ -796,12 +803,16 @@ phutil_register_library_map(array( 'DifferentialDiffTableOfContentsView' => 'AphrontView', 'DifferentialDiffViewController' => 'DifferentialController', 'DifferentialExceptionMail' => 'DifferentialMail', + 'DifferentialExportPatchFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialHostFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialHunk' => 'DifferentialDAO', 'DifferentialInlineComment' => 'DifferentialDAO', 'DifferentialInlineCommentEditController' => 'DifferentialController', 'DifferentialInlineCommentPreviewController' => 'DifferentialController', 'DifferentialInlineCommentView' => 'AphrontView', + 'DifferentialLinesFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail', + 'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialPrimaryPaneView' => 'AphrontView', 'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler', 'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification', diff --git a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php index 35d2f09778..de5b3bef06 100644 --- a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php +++ b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php @@ -116,7 +116,7 @@ class ConduitAPI_differential_getrevision_Method extends ConduitAPIMethod { $revision, $aux_fields); - return mpull($aux_fields, 'getValueForConduit', 'getStorageKey'); + return mpull($aux_fields, 'getValueForConduit', 'getKeyForConduit'); } } diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index eb34bfda4a..823c46b53a 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -43,8 +43,6 @@ class DifferentialRevisionViewController extends DifferentialController { "This revision has no diffs. Something has gone quite wrong."); } - $aux_fields = $this->loadAuxiliaryFields($revision); - $diff_vs = $request->getInt('vs'); $target = end($diffs); @@ -60,6 +58,11 @@ class DifferentialRevisionViewController extends DifferentialController { $diff_vs = null; } + $aux_fields = $this->loadAuxiliaryFields($revision); + foreach ($aux_fields as $aux_field) { + $aux_field->setDiff($target); + } + list($changesets, $vs_map, $rendering_references) = $this->loadChangesetsAndVsMap($diffs, $diff_vs, $target); @@ -336,17 +339,6 @@ class DifferentialRevisionViewController extends DifferentialController { $handles, $revision->getCCPHIDs())); - $host = $diff->getSourceMachine(); - if ($host) { - $properties['Host'] = phutil_escape_html($host); - } - - $path = $diff->getSourcePath(); - if ($path) { - $branch = $diff->getBranch() ? ' ('.$diff->getBranch().')' : ''; - $properties['Path'] = phutil_escape_html("{$path} {$branch}"); - } - $lstar = DifferentialRevisionUpdateHistoryView::renderDiffLintStar($diff); $lmsg = DifferentialRevisionUpdateHistoryView::getDiffLintMessage($diff); $ldata = idx($diff_properties, 'arc:lint'); @@ -474,18 +466,12 @@ class DifferentialRevisionViewController extends DifferentialController { $properties['Commits'] = implode('
', $links); } - $properties['Lines'] = number_format($diff->getLineCount()); $arcanist_phid = $diff->getArcanistProjectPHID(); if ($arcanist_phid) { $properties['Arcanist Project'] = phutil_escape_html( $handles[$arcanist_phid]->getName()); } - $properties['Apply Patch'] = - 'arc patch D'.$revision->getID().''; - $properties['Export Patch'] = - 'arc export --revision '.$revision->getID().''; - return $properties; } diff --git a/src/applications/differential/field/exception/incomplete/DifferentialFieldSpecificationIncompleteException.php b/src/applications/differential/field/exception/incomplete/DifferentialFieldSpecificationIncompleteException.php index fb96c4c93e..2d49f77d5f 100644 --- a/src/applications/differential/field/exception/incomplete/DifferentialFieldSpecificationIncompleteException.php +++ b/src/applications/differential/field/exception/incomplete/DifferentialFieldSpecificationIncompleteException.php @@ -16,7 +16,8 @@ * limitations under the License. */ -class DifferentialFieldSpecificationIncompleteException extends Exception { +final class DifferentialFieldSpecificationIncompleteException + extends Exception { public function __construct(DifferentialFieldSpecification $spec) { $key = $spec->getStorageKey(); diff --git a/src/applications/differential/field/exception/notavailable/DifferentialFieldDataNotAvailableException.php b/src/applications/differential/field/exception/notavailable/DifferentialFieldDataNotAvailableException.php new file mode 100644 index 0000000000..c5cba09267 --- /dev/null +++ b/src/applications/differential/field/exception/notavailable/DifferentialFieldDataNotAvailableException.php @@ -0,0 +1,30 @@ +getStorageKey(); + $class = get_class($spec); + + parent::__construct( + "Differential field specification for '{$key}' (of class '{$class}') is ". + "attempting to access data which is not available in this context."); + } + +} diff --git a/src/applications/differential/field/exception/notavailable/__init__.php b/src/applications/differential/field/exception/notavailable/__init__.php new file mode 100644 index 0000000000..43006619be --- /dev/null +++ b/src/applications/differential/field/exception/notavailable/__init__.php @@ -0,0 +1,10 @@ +getRevision(); + return 'arc patch D'.$revision->getID().''; + } + +} diff --git a/src/applications/differential/field/specification/applypatch/__init__.php b/src/applications/differential/field/specification/applypatch/__init__.php new file mode 100644 index 0000000000..bab891a69d --- /dev/null +++ b/src/applications/differential/field/specification/applypatch/__init__.php @@ -0,0 +1,12 @@ +value = $request->getStr('my-custom-field'); + * + * If you have some particularly complicated field, you may need to read + * more data; this is why you have access to the entire request. + * + * You must implement this if you implement @{method:shouldAppearOnEdit}. + * + * You should not perform field validation here; instead, you should implement + * @{method:validateField}. + * + * @param AphrontRequest HTTP request representing a user submitting a form + * with this field in it. + * @return this * @task edit */ public function setValueFromRequest(AphrontRequest $request) { @@ -90,6 +134,21 @@ abstract class DifferentialFieldSpecification { /** + * Build a renderable object (generally, some @{class:AphrontFormControl}) + * which can be appended to a @{class:AphrontFormView} and represents the + * interface the user sees on the "Edit Revision" screen when interacting + * with this field. + * + * For example: + * + * return id(new AphrontFormTextControl()) + * ->setLabel('Custom Field') + * ->setName('my-custom-key') + * ->setValue($this->value); + * + * You must implement this if you implement @{method:shouldAppearOnEdit}. + * + * @return AphrontView|string Something renderable. * @task edit */ public function renderEditControl() { @@ -98,10 +157,19 @@ abstract class DifferentialFieldSpecification { /** + * This method will be called after @{method:setValueFromRequest} but before + * the field is saved. It gives you an opportunity to inspect the field value + * and throw a @{class:DifferentialFieldValidationException} if there is a + * problem with the value the user has provided (for example, the value the + * user entered is not correctly formatted). + * + * By default, fields are not validated. + * + * @return void * @task edit */ public function validateField() { - throw new DifferentialFieldSpecificationIncompleteException($this); + return; } @@ -149,4 +217,54 @@ abstract class DifferentialFieldSpecification { throw new DifferentialFieldSpecificationIncompleteException($this); } + /** + * @task conduit + */ + public function getKeyForConduit() { + $key = $this->getStorageKey(); + if ($key === null) { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + return $key; + } + + +/* -( Contextual Data )---------------------------------------------------- */ + + /** + * @task context + */ + final public function setRevision(DifferentialRevision $revision) { + $this->revision = $revision; + return $this; + } + + /** + * @task context + */ + final public function setDiff(DifferentialDiff $diff) { + $this->diff = $diff; + return $this; + } + + /** + * @task context + */ + final protected function getRevision() { + if (empty($this->revision)) { + throw new DifferentialFieldDataNotAvailableException($this); + } + return $this->revision; + } + + /** + * @task context + */ + final protected function getDiff() { + if (empty($this->diff)) { + throw new DifferentialFieldDataNotAvailableException($this); + } + return $this->diff; + } + } diff --git a/src/applications/differential/field/specification/base/__init__.php b/src/applications/differential/field/specification/base/__init__.php index a0f7013fe7..6dcca1a4da 100644 --- a/src/applications/differential/field/specification/base/__init__.php +++ b/src/applications/differential/field/specification/base/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/differential/field/exception/incomplete'); +phutil_require_module('phabricator', 'applications/differential/field/exception/notavailable'); phutil_require_source('DifferentialFieldSpecification.php'); diff --git a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php index a47b0ae429..6c877c2024 100644 --- a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php +++ b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php @@ -39,7 +39,7 @@ final class DifferentialBlameRevisionFieldSpecification } public function setValueFromRequest(AphrontRequest $request) { - $this->value = $request->getStr('aux:phabricator:blame-revision'); + $this->value = $request->getStr($this->getStorageKey()); return $this; } @@ -47,7 +47,7 @@ final class DifferentialBlameRevisionFieldSpecification return id(new AphrontFormTextControl()) ->setLabel('Blame Revision') ->setCaption('Revision which broke the stuff which this change fixes.') - ->setName('aux:phabricator:blame-revision') + ->setName($this->getStorageKey()) ->setValue($this->value); } diff --git a/src/applications/differential/field/specification/exportpatch/DifferentialExportPatchFieldSpecification.php b/src/applications/differential/field/specification/exportpatch/DifferentialExportPatchFieldSpecification.php new file mode 100644 index 0000000000..7499a5ce1a --- /dev/null +++ b/src/applications/differential/field/specification/exportpatch/DifferentialExportPatchFieldSpecification.php @@ -0,0 +1,35 @@ +getRevision(); + return 'arc export --revision '.$revision->getID().''; + } + +} diff --git a/src/applications/differential/field/specification/exportpatch/__init__.php b/src/applications/differential/field/specification/exportpatch/__init__.php new file mode 100644 index 0000000000..41c1df6212 --- /dev/null +++ b/src/applications/differential/field/specification/exportpatch/__init__.php @@ -0,0 +1,12 @@ +getDiff(); + $host = $diff->getSourceMachine(); + if (!$host) { + return null; + } + return phutil_escape_html($host); + } + +} diff --git a/src/applications/differential/field/specification/host/__init__.php b/src/applications/differential/field/specification/host/__init__.php new file mode 100644 index 0000000000..e1b563b162 --- /dev/null +++ b/src/applications/differential/field/specification/host/__init__.php @@ -0,0 +1,14 @@ +getDiff(); + return phutil_escape_html(number_format($diff->getLineCount())); + } + +} diff --git a/src/applications/differential/field/specification/lines/__init__.php b/src/applications/differential/field/specification/lines/__init__.php new file mode 100644 index 0000000000..4c1bc36017 --- /dev/null +++ b/src/applications/differential/field/specification/lines/__init__.php @@ -0,0 +1,14 @@ +getDiff(); + + $path = $diff->getSourcePath(); + if (!$path) { + return null; + } + + $branch = $diff->getBranch(); + if ($branch) { + $branch = ' ('.$branch.')'; + } + + return phutil_escape_html($path.$branch); + } + +} diff --git a/src/applications/differential/field/specification/path/__init__.php b/src/applications/differential/field/specification/path/__init__.php new file mode 100644 index 0000000000..f1c24de348 --- /dev/null +++ b/src/applications/differential/field/specification/path/__init__.php @@ -0,0 +1,14 @@ +value = $request->getStr('aux:phabricator:revert-plan'); + $this->value = $request->getStr($this->getStorageKey()); return $this; } public function renderEditControl() { return id(new AphrontFormTextAreaControl()) ->setLabel('Revert Plan') - ->setName('aux:phabricator:revert-plan') + ->setName($this->getStorageKey()) ->setCaption('Special steps required to safely revert this change.') ->setValue($this->value); } diff --git a/src/applications/differential/storage/auxiliaryfield/DifferentialAuxiliaryField.php b/src/applications/differential/storage/auxiliaryfield/DifferentialAuxiliaryField.php index 643f1aa29b..5bd7d51a77 100644 --- a/src/applications/differential/storage/auxiliaryfield/DifferentialAuxiliaryField.php +++ b/src/applications/differential/storage/auxiliaryfield/DifferentialAuxiliaryField.php @@ -47,6 +47,7 @@ final class DifferentialAuxiliaryField extends DifferentialDAO { } foreach ($aux_fields as $aux_field) { + $aux_field->setRevision($revision); $key = $aux_field->getStorageKey(); if ($key) { $aux_field->setValueFromStorage(idx($field_data, $key));