Replace Differential Edit controller with EditEngine-driven EditPro controller

Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.

This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.

Test Plan:
  - Created a revision via web diffs.
  - Updated a revision via web diffs.
  - Edited a revision via web.
  - Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
  - Created a revision via CLI.
  - Updated a revision via CLI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114

Differential Revision: https://secure.phabricator.com/D17054

src/__phutil_library_map__.php

@@ -517,7 +517,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php',
-    'DifferentialRevisionEditController' => 'applications/differential/controller/DifferentialRevisionEditController.php',
     'DifferentialRevisionEditEngine' => 'applications/differential/editor/DifferentialRevisionEditEngine.php',
     'DifferentialRevisionEditProController' => 'applications/differential/controller/DifferentialRevisionEditProController.php',
     'DifferentialRevisionFulltextEngine' => 'applications/differential/search/DifferentialRevisionFulltextEngine.php',
@@ -5173,7 +5172,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionControlSystem' => 'Phobject',
     'DifferentialRevisionDependedOnByRevisionEdgeType' => 'PhabricatorEdgeType',
     'DifferentialRevisionDependsOnRevisionEdgeType' => 'PhabricatorEdgeType',
-    'DifferentialRevisionEditController' => 'DifferentialController',
     'DifferentialRevisionEditEngine' => 'PhabricatorEditEngine',
     'DifferentialRevisionEditProController' => 'DifferentialController',
     'DifferentialRevisionFulltextEngine' => 'PhabricatorFulltextEngine',

src/applications/differential/application/PhabricatorDifferentialApplication.php

@@ -65,8 +65,6 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication {
         ),
         'changeset/' => 'DifferentialChangesetViewController',
         'revision/' => array(
-          'edit/(?:(?P<id>[1-9]\d*)/)?'
-            => 'DifferentialRevisionEditController',
           $this->getEditRoutePattern('editpro/')
             => 'DifferentialRevisionEditProController',
           $this->getEditRoutePattern('attach/(?P<diffID>[^/]+)/to/')

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -23,6 +23,20 @@ final class DifferentialDiffViewController extends DifferentialController {
         ->setURI('/D'.$diff->getRevisionID().'?id='.$diff->getID());
     }
 
+    if ($request->isFormPost()) {
+      $diff_id = $diff->getID();
+      $revision_id = $request->getInt('revisionID');
+      if ($revision_id) {
+        $attach_uri = "/revision/attach/{$diff_id}/to/{$revision_id}/";
+      } else {
+        $attach_uri = "/revision/attach/{$diff_id}/to/";
+      }
+      $attach_uri = $this->getApplicationURI($attach_uri);
+
+      return id(new AphrontRedirectResponse())
+        ->setURI($attach_uri);
+    }
+
     $diff_phid = $diff->getPHID();
     $buildables = id(new HarbormasterBuildableQuery())
       ->setViewer($viewer)
@@ -78,13 +92,7 @@ final class DifferentialDiffViewController extends DifferentialController {
       $select);
 
     $form = id(new AphrontFormView())
-      ->setUser($request->getUser())
+      ->setViewer($viewer)
-      ->setAction('/differential/revision/edit/')
-      ->addHiddenInput('diffID', $diff->getID())
-      ->addHiddenInput('viaDiffView', 1)
-      ->addHiddenInput(
-        id(new DifferentialRepositoryField())->getFieldKey(),
-        $diff->getRepositoryPHID())
       ->appendRemarkupInstructions(
         pht(
           'Review the diff for correctness. When you are satisfied, either '.
@@ -98,7 +106,7 @@ final class DifferentialDiffViewController extends DifferentialController {
         ->setValue(pht('Continue')));
 
     $props = id(new DifferentialDiffProperty())->loadAllWhere(
-    'diffID = %d',
+      'diffID = %d',
       $diff->getID());
     $props = mpull($props, 'getData', 'getName');
 

src/applications/differential/controller/DifferentialRevisionEditController.php

@@ -1,214 +0,0 @@
-<?php
-
-final class DifferentialRevisionEditController
-  extends DifferentialController {
-
-  public function handleRequest(AphrontRequest $request) {
-    $viewer = $this->getViewer();
-    $id = $request->getURIData('id');
-
-    if (!$id) {
-      $id = $request->getInt('revisionID');
-    }
-
-    if ($id) {
-      $revision = id(new DifferentialRevisionQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($id))
-        ->needRelationships(true)
-        ->needReviewerStatus(true)
-        ->needActiveDiffs(true)
-        ->requireCapabilities(
-          array(
-            PhabricatorPolicyCapability::CAN_VIEW,
-            PhabricatorPolicyCapability::CAN_EDIT,
-          ))
-        ->executeOne();
-      if (!$revision) {
-        return new Aphront404Response();
-      }
-    } else {
-      $revision = DifferentialRevision::initializeNewRevision($viewer);
-      $revision->attachReviewerStatus(array());
-    }
-
-    $diff_id = $request->getInt('diffID');
-    if ($diff_id) {
-      $diff = id(new DifferentialDiffQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($diff_id))
-        ->executeOne();
-      if (!$diff) {
-        return new Aphront404Response();
-      }
-      if ($diff->getRevisionID()) {
-        // TODO: Redirect?
-        throw new Exception(
-          pht('This diff is already attached to a revision!'));
-      }
-    } else {
-      $diff = null;
-    }
-
-    if (!$diff) {
-      if (!$revision->getID()) {
-        throw new Exception(
-          pht('You can not create a new revision without a diff!'));
-      }
-    } else {
-      // TODO: It would be nice to show the diff being attached in the UI.
-    }
-
-    $field_list = PhabricatorCustomField::getObjectFields(
-      $revision,
-      PhabricatorCustomField::ROLE_EDIT);
-    $field_list
-      ->setViewer($viewer)
-      ->readFieldsFromStorage($revision);
-
-    if ($request->getStr('viaDiffView') && $diff) {
-      $repo_key = id(new DifferentialRepositoryField())->getFieldKey();
-      $repository_field = idx(
-        $field_list->getFields(),
-        $repo_key);
-      if ($repository_field) {
-        $repository_field->setValue($request->getStr($repo_key));
-      }
-      $view_policy_key = id(new DifferentialViewPolicyField())->getFieldKey();
-      $view_policy_field = idx(
-        $field_list->getFields(),
-        $view_policy_key);
-      if ($view_policy_field) {
-        $view_policy_field->setValue($diff->getViewPolicy());
-      }
-    }
-
-    $validation_exception = null;
-    if ($request->isFormPost() && !$request->getStr('viaDiffView')) {
-
-      $editor = id(new DifferentialTransactionEditor())
-        ->setActor($viewer)
-        ->setContentSourceFromRequest($request)
-        ->setContinueOnNoEffect(true);
-
-      $xactions = $field_list->buildFieldTransactionsFromRequest(
-        new DifferentialTransaction(),
-        $request);
-
-      if ($diff) {
-        $repository_phid = null;
-        $repository_tokenizer = $request->getArr(
-          id(new DifferentialRepositoryField())->getFieldKey());
-        if ($repository_tokenizer) {
-          $repository_phid = reset($repository_tokenizer);
-        }
-
-        $xactions[] = id(new DifferentialTransaction())
-          ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-          ->setNewValue($diff->getPHID());
-
-        $editor->setRepositoryPHIDOverride($repository_phid);
-      }
-
-      $comments = $request->getStr('comments');
-      if (strlen($comments)) {
-        $xactions[] = id(new DifferentialTransaction())
-          ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-          ->attachComment(
-            id(new DifferentialTransactionComment())
-              ->setContent($comments));
-      }
-
-      try {
-        $editor->applyTransactions($revision, $xactions);
-        $revision_uri = '/D'.$revision->getID();
-        return id(new AphrontRedirectResponse())->setURI($revision_uri);
-      } catch (PhabricatorApplicationTransactionValidationException $ex) {
-        $validation_exception = $ex;
-      }
-    }
-
-
-    $form = new AphrontFormView();
-    $form->setUser($request->getUser());
-    if ($diff) {
-      $form->addHiddenInput('diffID', $diff->getID());
-    }
-
-    if ($revision->getID()) {
-      $form->setAction('/differential/revision/edit/'.$revision->getID().'/');
-    } else {
-      $form->setAction('/differential/revision/edit/');
-    }
-
-    if ($diff && $revision->getID()) {
-      $form
-        ->appendChild(
-          id(new AphrontFormTextAreaControl())
-            ->setLabel(pht('Comments'))
-            ->setName('comments')
-            ->setCaption(pht("Explain what's new in this diff."))
-            ->setValue($request->getStr('comments')))
-        ->appendChild(
-          id(new AphrontFormSubmitControl())
-            ->setValue(pht('Save')))
-        ->appendChild(
-          id(new AphrontFormDividerControl()));
-    }
-
-    $field_list->appendFieldsToForm($form);
-
-    $submit = id(new AphrontFormSubmitControl())
-      ->setValue('Save');
-    if ($diff) {
-      $submit->addCancelButton('/differential/diff/'.$diff->getID().'/');
-    } else {
-      $submit->addCancelButton('/D'.$revision->getID());
-    }
-
-    $form->appendChild($submit);
-
-    $crumbs = $this->buildApplicationCrumbs();
-    if ($revision->getID()) {
-      if ($diff) {
-        $header_icon = 'fa-upload';
-        $title = pht('Update Revision');
-        $crumbs->addTextCrumb(
-          'D'.$revision->getID(),
-          '/differential/diff/'.$diff->getID().'/');
-      } else {
-        $header_icon = 'fa-pencil';
-        $title = pht('Edit Revision: %s', $revision->getTitle());
-        $crumbs->addTextCrumb(
-          'D'.$revision->getID(),
-          '/D'.$revision->getID());
-      }
-    } else {
-      $header_icon = 'fa-plus-square';
-      $title = pht('Create New Differential Revision');
-    }
-
-    $form_box = id(new PHUIObjectBoxView())
-      ->setHeaderText('Revision')
-      ->setValidationException($validation_exception)
-      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-      ->setForm($form);
-
-    $crumbs->addTextCrumb($title);
-    $crumbs->setBorder(true);
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title)
-      ->setHeaderIcon($header_icon);
-
-    $view = id(new PHUITwoColumnView())
-      ->setHeader($header)
-      ->setFooter($form_box);
-
-    return $this->newPage()
-      ->setTitle($title)
-      ->setCrumbs($crumbs)
-      ->appendChild($view);
-  }
-
-}

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -570,7 +570,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $curtain->addAction(
       id(new PhabricatorActionView())
         ->setIcon('fa-pencil')
-        ->setHref("/differential/revision/edit/{$revision_id}/")
+        ->setHref("/differential/revision/editpro/{$revision_id}/")
         ->setName(pht('Edit Revision'))
         ->setDisabled(!$can_edit)
         ->setWorkflow(!$can_edit));

src/applications/differential/customfield/DifferentialCoreCustomField.php

@@ -118,10 +118,6 @@ abstract class DifferentialCoreCustomField
     return true;
   }
 
-  public function shouldAppearInEditView() {
-    return true;
-  }
-
   public function readValueFromObject(PhabricatorCustomFieldInterface $object) {
     if ($this->isCoreFieldRequired()) {
       $this->setFieldError(true);

src/applications/differential/customfield/DifferentialManiphestTasksField.php

@@ -15,10 +15,6 @@ final class DifferentialManiphestTasksField
     return false;
   }
 
-  public function shouldAppearInEditView() {
-    return false;
-  }
-
   public function getFieldName() {
     return pht('Maniphest Tasks');
   }

src/applications/differential/customfield/DifferentialProjectsField.php

@@ -19,10 +19,6 @@ final class DifferentialProjectsField
     return false;
   }
 
-  public function shouldAppearInEditView() {
-    return true;
-  }
-
   public function shouldAppearInApplicationTransactions() {
     return true;
   }

src/applications/differential/customfield/DifferentialRequiredSignaturesField.php

@@ -19,10 +19,6 @@ final class DifferentialRequiredSignaturesField
     return true;
   }
 
-  public function shouldAppearInEditView() {
-    return false;
-  }
-
   protected function readValueFromRevision(DifferentialRevision $revision) {
     return self::loadForRevision($revision);
   }

src/applications/differential/customfield/DifferentialReviewedByField.php

@@ -23,10 +23,6 @@ final class DifferentialReviewedByField
     return false;
   }
 
-  public function shouldAppearInEditView() {
-    return false;
-  }
-
   public function canDisableField() {
     return true;
   }

src/applications/differential/editor/DifferentialRevisionEditEngine.php

@@ -95,23 +95,26 @@ final class DifferentialRevisionEditEngine
       $diff_phid = null;
     }
 
-    $is_update = ($diff && $object->getID());
+    $is_create = $this->getIsCreate();
+    $is_update = ($diff && !$is_create);
 
     $fields = array();
 
-    $fields[] = id(new PhabricatorHandlesEditField())
+    if ($diff || $is_create) {
-      ->setKey('update')
+      $fields[] = id(new PhabricatorHandlesEditField())
-      ->setLabel(pht('Update Diff'))
+        ->setKey('update')
-      ->setDescription(pht('New diff to create or update the revision with.'))
+        ->setLabel(pht('Update Diff'))
-      ->setConduitDescription(pht('Create or update a revision with a diff.'))
+        ->setDescription(pht('New diff to create or update the revision with.'))
-      ->setConduitTypeDescription(pht('PHID of the diff.'))
+        ->setConduitDescription(pht('Create or update a revision with a diff.'))
-      ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+        ->setConduitTypeDescription(pht('PHID of the diff.'))
-      ->setHandleParameterType(new AphrontPHIDListHTTPParameterType())
+        ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
-      ->setSingleValue($diff_phid)
+        ->setHandleParameterType(new AphrontPHIDListHTTPParameterType())
-      ->setIsReorderable(false)
+        ->setSingleValue($diff_phid)
-      ->setIsDefaultable(false)
+        ->setIsReorderable(false)
-      ->setIsInvisible(true)
+        ->setIsDefaultable(false)
-      ->setIsLockable(false);
+        ->setIsInvisible(true)
+        ->setIsLockable(false);
+    }
 
     if ($is_update) {
       $fields[] = id(new PhabricatorInstructionsEditField())
@@ -194,7 +197,7 @@ final class DifferentialRevisionEditEngine
   private function isCustomFieldEnabled(DifferentialRevision $revision, $key) {
     $field_list = PhabricatorCustomField::getObjectFields(
       $revision,
-      PhabricatorCustomField::ROLE_EDIT);
+      PhabricatorCustomField::ROLE_VIEW);
 
     $fields = $field_list->getFields();
     return isset($fields[$key]);

src/applications/differential/storage/DifferentialRevision.php

@@ -70,6 +70,7 @@ final class DifferentialRevision extends DifferentialDAO
       ->setAuthorPHID($actor->getPHID())
       ->attachRelationships(array())
       ->attachRepository(null)
+      ->attachReviewerStatus(array())
       ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
   }
 

src/infrastructure/customfield/editor/PhabricatorCustomFieldEditField.php

@@ -47,14 +47,13 @@ final class PhabricatorCustomFieldEditField
   }
 
   protected function newEditType() {
-    $conduit_type = $this->newConduitParameterType();
-    if (!$conduit_type) {
-      return null;
-    }
-
     $type = id(new PhabricatorCustomFieldEditType())
-      ->setCustomField($this->getCustomField())
+      ->setCustomField($this->getCustomField());
-      ->setConduitParameterType($conduit_type);
+
+    $conduit_type = $this->newConduitParameterType();
+    if ($conduit_type) {
+      $type->setConduitParameterType($conduit_type);
+    }
 
     return $type;
   }

src/infrastructure/customfield/engineextension/PhabricatorCustomFieldEditEngineExtension.php

@@ -27,12 +27,6 @@ final class PhabricatorCustomFieldEditEngineExtension
     PhabricatorEditEngine $engine,
     PhabricatorApplicationTransactionInterface $object) {
 
-    // TODO: Remove this hack once Differential modernizes more fully. Today,
-    // its custom fields are too custom to interact cleanly with EditEngine.
-    if ($object instanceof DifferentialRevision) {
-      return array();
-    }
-
     $viewer = $this->getViewer();
 
     $field_list = PhabricatorCustomField::getObjectFields(