Update Macro for EditEngine

Summary: Updates Macro to use EditEngine. Also removes "URL" field for adding a Macro, which I think it's worth pursuing. Test Plan: - Create a Macro - Forget to name it - Try a PDF - Use a Macro - Edit a macro (not working) Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17821
2024-11-10 08:52:39 +01:00 · 2017-05-03 11:11:14 -07:00 · 2017-05-03 11:11:14 -07:00 · c878bf5033
commit c878bf5033
parent 21ac196306
8 changed files with 181 additions and 305 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -2982,6 +2982,7 @@ phutil_register_library_map(array(
    'PhabricatorMacroDisableController' => 'applications/macro/controller/PhabricatorMacroDisableController.php',
    'PhabricatorMacroDisabledTransaction' => 'applications/macro/xaction/PhabricatorMacroDisabledTransaction.php',
    'PhabricatorMacroEditController' => 'applications/macro/controller/PhabricatorMacroEditController.php',
+    'PhabricatorMacroEditEngine' => 'applications/macro/editor/PhabricatorMacroEditEngine.php',
    'PhabricatorMacroEditor' => 'applications/macro/editor/PhabricatorMacroEditor.php',
    'PhabricatorMacroFileTransaction' => 'applications/macro/xaction/PhabricatorMacroFileTransaction.php',
    'PhabricatorMacroListController' => 'applications/macro/controller/PhabricatorMacroListController.php',
@ -8187,7 +8188,8 @@ phutil_register_library_map(array(
    'PhabricatorMacroDatasource' => 'PhabricatorTypeaheadDatasource',
    'PhabricatorMacroDisableController' => 'PhabricatorMacroController',
    'PhabricatorMacroDisabledTransaction' => 'PhabricatorMacroTransactionType',
-    'PhabricatorMacroEditController' => 'PhabricatorMacroController',
+    'PhabricatorMacroEditController' => 'PhameBlogController',
+    'PhabricatorMacroEditEngine' => 'PhabricatorEditEngine',
    'PhabricatorMacroEditor' => 'PhabricatorApplicationTransactionEditor',
    'PhabricatorMacroFileTransaction' => 'PhabricatorMacroTransactionType',
    'PhabricatorMacroListController' => 'PhabricatorMacroController',
--- a/src/applications/macro/application/PhabricatorMacroApplication.php
+++ b/src/applications/macro/application/PhabricatorMacroApplication.php
@ -33,7 +33,8 @@ final class PhabricatorMacroApplication extends PhabricatorApplication {
        'create/' => 'PhabricatorMacroEditController',
        'view/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroViewController',
        'comment/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroCommentController',
-        'edit/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroEditController',
+        $this->getEditRoutePattern('edit/')
+          => 'PhabricatorMacroEditController',
        'audio/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroAudioController',
        'disable/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroDisableController',
        'meme/' => 'PhabricatorMacroMemeController',
--- a/src/applications/macro/controller/PhabricatorMacroEditController.php
+++ b/src/applications/macro/controller/PhabricatorMacroEditController.php
@ -1,304 +1,10 @@
 <?php

-final class PhabricatorMacroEditController extends PhabricatorMacroController {
+final class PhabricatorMacroEditController extends PhameBlogController {

  public function handleRequest(AphrontRequest $request) {
-    $viewer = $request->getViewer();
-    $id = $request->getURIData('id');
-
-    $this->requireApplicationCapability(
-      PhabricatorMacroManageCapability::CAPABILITY);
-
-    if ($id) {
-      $macro = id(new PhabricatorMacroQuery())
-        ->setViewer($viewer)
-        ->withIDs(array($id))
-        ->needFiles(true)
-        ->executeOne();
-      if (!$macro) {
-        return new Aphront404Response();
-      }
-    } else {
-      $macro = new PhabricatorFileImageMacro();
-      $macro->setAuthorPHID($viewer->getPHID());
-    }
-
-    $errors = array();
-    $e_name = true;
-    $e_file = null;
-    $file = null;
-
-    if ($request->isFormPost()) {
-      $original = clone $macro;
-
-      $new_name = null;
-      if ($request->getBool('name_form') || !$macro->getID()) {
-        $new_name = $request->getStr('name');
-
-        $macro->setName($new_name);
-
-        if (!strlen($macro->getName())) {
-          $errors[] = pht('Macro name is required.');
-          $e_name = pht('Required');
-        } else if (!preg_match('/^[a-z0-9:_-]{3,}\z/', $macro->getName())) {
-          $errors[] = pht(
-            'Macro must be at least three characters long and contain only '.
-            'lowercase letters, digits, hyphens, colons and underscores.');
-          $e_name = pht('Invalid');
-        } else {
-          $e_name = null;
-        }
-      }
-
-      $uri = $request->getStr('url');
-
-      $engine = new PhabricatorDestructionEngine();
-
-      $file = null;
-      if ($request->getFileExists('file')) {
-        $file = PhabricatorFile::newFromPHPUpload(
-          $_FILES['file'],
-          array(
-            'name' => $request->getStr('name'),
-            'authorPHID' => $viewer->getPHID(),
-            'isExplicitUpload' => true,
-            'canCDN' => true,
-          ));
-      } else if ($uri) {
-        try {
-          // Rate limit outbound fetches to make this mechanism less useful for
-          // scanning networks and ports.
-          PhabricatorSystemActionEngine::willTakeAction(
-            array($viewer->getPHID()),
-            new PhabricatorFilesOutboundRequestAction(),
-            1);
-
-          $file = PhabricatorFile::newFromFileDownload(
-            $uri,
-            array(
-              'name' => $request->getStr('name'),
-              'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
-              'isExplicitUpload' => true,
-              'canCDN' => true,
-            ));
-
-          if (!$file->isViewableInBrowser()) {
-            $mime_type = $file->getMimeType();
-            $engine->destroyObject($file);
-            $file = null;
-            throw new Exception(
-              pht(
-                'The URI "%s" does not correspond to a valid image file, got '.
-                'a file with MIME type "%s". You must specify the URI of a '.
-                'valid image file.',
-                $uri,
-                $mime_type));
-          } else {
-            $file
-              ->setAuthorPHID($viewer->getPHID())
-              ->save();
-          }
-        } catch (HTTPFutureHTTPResponseStatus $status) {
-          $errors[] = pht(
-            'The URI "%s" could not be loaded, got %s error.',
-            $uri,
-            $status->getStatusCode());
-        } catch (Exception $ex) {
-          $errors[] = $ex->getMessage();
-        }
-      } else if ($request->getStr('phid')) {
-        $file = id(new PhabricatorFileQuery())
-          ->setViewer($viewer)
-          ->withPHIDs(array($request->getStr('phid')))
-          ->executeOne();
-      }
-
-      if ($file) {
-        if (!$file->isViewableInBrowser()) {
-          $errors[] = pht('You must upload an image.');
-          $e_file = pht('Invalid');
-        } else {
-          $macro->setFilePHID($file->getPHID());
-          $macro->attachFile($file);
-          $e_file = null;
-        }
-      }
-
-      if (!$macro->getID() && !$file) {
-        $errors[] = pht('You must upload an image to create a macro.');
-        $e_file = pht('Required');
-      }
-
-      if (!$errors) {
-        try {
-          $xactions = array();
-
-          if ($new_name !== null) {
-            $xactions[] = id(new PhabricatorMacroTransaction())
-              ->setTransactionType(
-                PhabricatorMacroNameTransaction::TRANSACTIONTYPE)
-              ->setNewValue($new_name);
-          }
-
-          if ($file) {
-            $xactions[] = id(new PhabricatorMacroTransaction())
-              ->setTransactionType(
-                PhabricatorMacroFileTransaction::TRANSACTIONTYPE)
-              ->setNewValue($file->getPHID());
-          }
-
-          $editor = id(new PhabricatorMacroEditor())
-            ->setActor($viewer)
-            ->setContinueOnNoEffect(true)
-            ->setContentSourceFromRequest($request);
-
-          $xactions = $editor->applyTransactions($original, $xactions);
-
-          $view_uri = $this->getApplicationURI('/view/'.$original->getID().'/');
-          return id(new AphrontRedirectResponse())->setURI($view_uri);
-        } catch (AphrontDuplicateKeyQueryException $ex) {
-          throw $ex;
-          $errors[] = pht('Macro name is not unique!');
-          $e_name = pht('Duplicate');
-        }
-      }
-    }
-
-    $current_file = null;
-    if ($macro->getFilePHID()) {
-      $current_file = $macro->getFile();
-    }
-
-    $form = new AphrontFormView();
-    $form->addHiddenInput('name_form', 1);
-    $form->setUser($request->getUser());
-
-    $form
-      ->setEncType('multipart/form-data')
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('Name'))
-          ->setName('name')
-          ->setValue($macro->getName())
-          ->setCaption(
-            pht('This word or phrase will be replaced with the image.'))
-          ->setError($e_name));
-
-    if (!$macro->getID()) {
-      if ($current_file) {
-        $current_file_view = id(new PhabricatorFileLinkView())
-          ->setViewer($viewer)
-          ->setFilePHID($current_file->getPHID())
-          ->setFileName($current_file->getName())
-          ->setFileViewable(true)
-          ->setFileViewURI($current_file->getBestURI())
-          ->render();
-        $form->addHiddenInput('phid', $current_file->getPHID());
-        $form->appendChild(
-          id(new AphrontFormMarkupControl())
-            ->setLabel(pht('Selected File'))
-            ->setValue($current_file_view));
-
-        $other_label = pht('Change File');
-      } else {
-        $other_label = pht('File');
-      }
-
-      $form->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('URL'))
-          ->setName('url')
-          ->setValue($request->getStr('url'))
-          ->setError($request->getFileExists('file') ? false : $e_file));
-
-      $form->appendChild(
-        id(new AphrontFormFileControl())
-          ->setLabel($other_label)
-          ->setName('file')
-          ->setError($request->getStr('url') ? false : $e_file));
-    }
-
-
-    $view_uri = $this->getApplicationURI('/view/'.$macro->getID().'/');
-
-    if ($macro->getID()) {
-      $cancel_uri = $view_uri;
-    } else {
-      $cancel_uri = $this->getApplicationURI();
-    }
-
-    $form
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->setValue(pht('Save Image Macro'))
-          ->addCancelButton($cancel_uri));
-
-    $crumbs = $this->buildApplicationCrumbs();
-
-    if ($macro->getID()) {
-      $title = pht('Edit Macro: %s', $macro->getName());
-      $crumb = pht('Edit Macro');
-      $header_icon = 'fa-pencil';
-
-      $crumbs->addTextCrumb(pht('Macro: %s', $macro->getName()), $view_uri);
-    } else {
-      $title = pht('Create Image Macro');
-      $crumb = pht('Create Macro');
-      $header_icon = 'fa-plus-square';
-    }
-
-    $crumbs->addTextCrumb($crumb, $request->getRequestURI());
-    $crumbs->setBorder(true);
-
-    $upload = null;
-    if ($macro->getID()) {
-      $upload_form = id(new AphrontFormView())
-        ->setEncType('multipart/form-data')
-        ->setUser($request->getUser());
-
-      $upload_form->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('URL'))
-          ->setName('url')
-          ->setValue($request->getStr('url')));
-
-      $upload_form
-        ->appendChild(
-          id(new AphrontFormFileControl())
-            ->setLabel(pht('File'))
-            ->setName('file'))
-        ->appendChild(
-          id(new AphrontFormSubmitControl())
-            ->setValue(pht('Upload File')));
-
-      $upload = id(new PHUIObjectBoxView())
-        ->setHeaderText(pht('Upload New File'))
-        ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-        ->setForm($upload_form);
-    }
-
-    $form_box = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Macro'))
-      ->setFormErrors($errors)
-      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-      ->setForm($form);
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title)
-      ->setHeaderIcon($header_icon);
-
-    $view = id(new PHUITwoColumnView())
-      ->setHeader($header)
-      ->setFooter(array(
-        $form_box,
-        $upload,
-      ));
-
-    return $this->newPage()
-      ->setTitle($title)
-      ->setCrumbs($crumbs)
-      ->appendChild($view);
-
+    return id(new PhabricatorMacroEditEngine())
+      ->setController($this)
+      ->buildResponse();
  }
-
 }
--- a/src/applications/macro/editor/PhabricatorMacroEditEngine.php
+++ b/src/applications/macro/editor/PhabricatorMacroEditEngine.php
@ -0,0 +1,88 @@
+<?php
+
+final class PhabricatorMacroEditEngine
+  extends PhabricatorEditEngine {
+
+  const ENGINECONST = 'macro.image';
+
+  public function getEngineName() {
+    return pht('Macro Imagea');
+  }
+
+  public function getSummaryHeader() {
+    return pht('Configure Macro Image Forms');
+  }
+
+  public function getSummaryText() {
+    return pht('Configure creation and editing of Macro images.');
+  }
+
+  public function getEngineApplicationClass() {
+    return 'PhabricatorMacroApplication';
+  }
+
+  protected function newEditableObject() {
+    $viewer = $this->getViewer();
+    return PhabricatorFileImageMacro::initializeNewFileImageMacro($viewer);
+  }
+
+  protected function newObjectQuery() {
+    return new PhabricatorMacroQuery();
+  }
+
+  protected function getObjectCreateTitleText($object) {
+    return pht('Create New Macro');
+  }
+
+  protected function getObjectEditTitleText($object) {
+    return pht('Edit %s', $object->getName());
+  }
+
+  protected function getObjectEditShortText($object) {
+    return $object->getName();
+  }
+
+  protected function getObjectCreateShortText() {
+    return pht('Create Macro');
+  }
+
+  protected function getObjectName() {
+    return pht('Macro');
+  }
+
+  protected function getObjectViewURI($object) {
+    return $object->getViewURI();
+  }
+
+  protected function getEditorURI() {
+    return $this->getApplication()->getApplicationURI('edit/');
+  }
+
+  protected function getCreateNewObjectPolicy() {
+    return $this->getApplication()->getPolicy(
+      PhabricatorMacroManageCapability::CAPABILITY);
+  }
+
+  protected function buildCustomEditFields($object) {
+
+    return array(
+      id(new PhabricatorTextEditField())
+        ->setKey('name')
+        ->setLabel(pht('Name'))
+        ->setDescription(pht('Macro name.'))
+        ->setConduitDescription(pht('Rename the macro.'))
+        ->setConduitTypeDescription(pht('New macro name.'))
+        ->setTransactionType(PhabricatorMacroNameTransaction::TRANSACTIONTYPE)
+        ->setValue($object->getName()),
+      id(new PhabricatorFileEditField())
+        ->setKey('filePHID')
+        ->setLabel(pht('Image File'))
+        ->setDescription(pht('Image file to import.'))
+        ->setTransactionType(PhabricatorMacroFileTransaction::TRANSACTIONTYPE)
+        ->setConduitDescription(pht('File PHID to import.'))
+        ->setConduitTypeDescription(pht('File PHID.')),
+    );
+
+  }
+
+}
--- a/src/applications/macro/editor/PhabricatorMacroEditor.php
+++ b/src/applications/macro/editor/PhabricatorMacroEditor.php
@ -11,11 +11,12 @@ final class PhabricatorMacroEditor
    return pht('Macros');
  }

-  public function getTransactionTypes() {
-    $types = parent::getTransactionTypes();
-    $types[] = PhabricatorTransactions::TYPE_COMMENT;
+  public function getCreateObjectTitle($author, $object) {
+    return pht('%s created this macro.', $author);
+  }

-    return $types;
+  public function getCreateObjectTitleForFeed($author, $object) {
+    return pht('%s created %s.', $author, $object);
  }

  protected function applyCustomExternalTransaction(
--- a/src/applications/macro/storage/PhabricatorFileImageMacro.php
+++ b/src/applications/macro/storage/PhabricatorFileImageMacro.php
@ -41,6 +41,12 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO
    return $this->assertAttached($this->audio);
  }

+  public static function initializeNewFileImageMacro(PhabricatorUser $actor) {
+    $macro = id(new self())
+      ->setAuthorPHID($actor->getPHID());
+    return $macro;
+  }
+
  protected function getConfiguration() {
    return array(
      self::CONFIG_AUX_PHID  => true,
@ -80,6 +86,10 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO
    return parent::save();
  }

+  public function getViewURI() {
+    return '/macro/view/'.$this->getID().'/';
+  }
+

 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */

@ -128,11 +138,19 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO
  public function getCapabilities() {
    return array(
      PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
    );
  }

  public function getPolicy($capability) {
-    return PhabricatorPolicies::getMostOpenPolicy();
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_VIEW:
+        return PhabricatorPolicies::getMostOpenPolicy();
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        $app = PhabricatorApplication::getByClass(
+          'PhabricatorMacroApplication');
+        return $app->getPolicy(PhabricatorMacroManageCapability::CAPABILITY);
+    }
  }

  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
--- a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php
+++ b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php
@ -26,6 +26,41 @@ final class PhabricatorMacroFileTransaction
      $this->renderObject());
  }

+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+    $viewer = $this->getActor();
+
+    foreach ($xactions as $xaction) {
+      $file_phid = $xaction->getNewValue();
+
+      if ($this->isEmptyTextTransaction($file_phid, $xactions)) {
+        $errors[] = $this->newRequiredError(
+          pht('Image macros must have a file.'));
+      }
+
+      $file = id(new PhabricatorFileQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($file_phid))
+        ->executeOne();
+
+      if (!$file) {
+        $errors[] = $this->newInvalidError(
+          pht('"%s" is not a valid file PHID.',
+          $file_phid));
+      } else {
+        if (!$file->isViewableInBrowser()) {
+          $mime_type = $file->getMimeType();
+          $errors[] = $this->newInvalidError(
+            pht('File mime type of "%s" is not a valid viewable image.',
+            $mime_type));
+        }
+      }
+
+    }
+
+    return $errors;
+  }
+
  public function getIcon() {
    return 'fa-file-image-o';
  }
--- a/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php
+++ b/src/applications/macro/xaction/PhabricatorMacroNameTransaction.php
@ -32,6 +32,7 @@ final class PhabricatorMacroNameTransaction

  public function validateTransactions($object, array $xactions) {
    $errors = array();
+    $viewer = $this->getActor();

    if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
      $errors[] = $this->newRequiredError(
@ -40,13 +41,37 @@ final class PhabricatorMacroNameTransaction

    $max_length = $object->getColumnMaximumByteLength('name');
    foreach ($xactions as $xaction) {
+      $old_value = $this->generateOldValue($object);
      $new_value = $xaction->getNewValue();
+
      $new_length = strlen($new_value);
      if ($new_length > $max_length) {
        $errors[] = $this->newInvalidError(
          pht('The name can be no longer than %s characters.',
          new PhutilNumber($max_length)));
      }
+
+      if (!preg_match('/^[a-z0-9:_-]{3,}\z/', $new_value)) {
+      $errors[] = $this->newInvalidError(
+        pht('Macro name "%s" be at least three characters long and contain '.
+            'only lowercase letters, digits, hyphens, colons and '.
+            'underscores.',
+            $new_value));
+      }
+
+      // Check name is unique when updating / creating
+      if ($old_value != $new_value) {
+        $macro = id(new PhabricatorMacroQuery())
+          ->setViewer($viewer)
+          ->withNames(array($new_value))
+          ->executeOne();
+
+        if ($macro) {
+        $errors[] = $this->newInvalidError(
+          pht('Macro "%s" already exists.', $new_value));
+        }
+      }
+
    }

    return $errors;