Use the unified markup cache for Maniphest

Summary:
  - See D2945.
  - Drop `cache` field from ManiphestTransaction.
  - Render task descriptions and transactions through PhabricatorMarkupEngine.
  - Also pull the list of macros more lazily.

Test Plan:
  - Verified transactions and transaction preview work correctly and interact with cache correctly.
  - Verified tasks descriptions and task preview work correctly.
  - Verified we don't hit the imagemacro table when we're rendering everything from cache anymore.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

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

resources/sql/patches/maniphestxcache.sql

@@ -0,0 +1,2 @@
+ALTER TABLE `{$NAMESPACE}_maniphest`.`maniphest_transaction`
+  DROP `cache`;

scripts/util/purge_cache.php

@@ -22,7 +22,6 @@ require_once $root.'/scripts/__init_script__.php';
 
 $purge_changesets   = false;
 $purge_differential = false;
-$purge_maniphest    = false;
 
 $args = array_slice($argv, 1);
 if (!$args) {
@@ -36,7 +35,6 @@ for ($ii = 0; $ii < $len; $ii++) {
     case '--all':
       $purge_changesets = true;
       $purge_differential = true;
-      $purge_maniphest = true;
       break;
     case '--changesets':
       $purge_changesets = true;
@@ -52,9 +50,6 @@ for ($ii = 0; $ii < $len; $ii++) {
     case '--differential':
       $purge_differential = true;
       break;
-    case '--maniphest':
-      $purge_maniphest = true;
-      break;
     case '--help':
       return help();
     default:
@@ -98,16 +93,6 @@ if ($purge_differential) {
   echo "Done.\n";
 }
 
-if ($purge_maniphest) {
-  echo "Purging Maniphest comment cache...\n";
-  $table = new ManiphestTransaction();
-  queryfx(
-    $table->establishConnection('w'),
-    'UPDATE %T SET cache = NULL',
-    $table->getTableName());
-  echo "Done.\n";
-}
-
 echo "Ok, caches purged.\n";
 
 function usage($message) {
@@ -122,7 +107,6 @@ function help() {
 **SUMMARY**
 
     **purge_cache.php**
-        [--maniphest]
         [--differential]
         [--changesets [changeset_id ...]]
     **purge_cache.php** --all
@@ -136,9 +120,8 @@ function help() {
     syntax highlighting, you may want to purge the changeset cache (with
     "--changesets") so your changes are reflected in older diffs.
 
-    If you change Remarkup rules, you may want to purge the Maniphest or
+    If you change Remarkup rules, you may want to purge the Differential
-    Differential comment caches ("--maniphest", "--differential") so older
+    comment caches ("--differential") so older comments pick up the new rules.
-    comments pick up the new rules.
 
     __--all__
         Purge all long-lived caches.
@@ -151,9 +134,6 @@ function help() {
     __--differential__
         Purge Differential comment formatting cache.
 
-    __--maniphest__: show this help
-        Purge Maniphest comment formatting cache.
-
     __--help__: show this help
 
 

src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php

@@ -27,11 +27,16 @@ final class ManiphestTaskDescriptionPreviewController
     $request = $this->getRequest();
     $description = $request->getStr('description');
 
-    $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine();
+    $task = new ManiphestTask();
+    $task->setDescription($description);
+
+    $output = PhabricatorMarkupEngine::renderOneObject(
+      $task,
+      ManiphestTask::MARKUP_FIELD_DESCRIPTION);
 
     $content =
       '<div class="phabricator-remarkup">'.
-        $engine->markupText($description).
+        $output.
       '</div>';
 
     return id(new AphrontAjaxResponse())

src/applications/maniphest/controller/ManiphestTaskDetailController.php

@@ -88,8 +88,6 @@ final class ManiphestTaskDetailController extends ManiphestController {
     $handles = id(new PhabricatorObjectHandleData($phids))
       ->loadHandles();
 
-    $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine();
-
     $dict = array();
     $dict['Status'] =
       '<strong>'.
@@ -305,9 +303,18 @@ final class ManiphestTaskDetailController extends ManiphestController {
     $headsup_panel->setActionList($action_list);
     $headsup_panel->setProperties($dict);
 
+    $engine = new PhabricatorMarkupEngine();
+    $engine->addObject($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION);
+    foreach ($transactions as $xaction) {
+      if ($xaction->hasComments()) {
+        $engine->addObject($xaction, ManiphestTransaction::MARKUP_FIELD_BODY);
+      }
+    }
+    $engine->process();
+
     $headsup_panel->appendChild(
       '<div class="phabricator-remarkup">'.
-        $engine->markupText($task->getDescription()).
+        $engine->getOutput($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION).
       '</div>');
 
     $transaction_types = ManiphestTransactionType::getTransactionTypeMap();

src/applications/maniphest/controller/ManiphestTransactionPreviewController.php

@@ -119,7 +119,9 @@ final class ManiphestTransactionPreviewController extends ManiphestController {
     $transactions   = array();
     $transactions[] = $transaction;
 
-    $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine();
+    $engine = new PhabricatorMarkupEngine();
+    $engine->addObject($transaction, ManiphestTransaction::MARKUP_FIELD_BODY);
+    $engine->process();
 
     $transaction_view = new ManiphestTransactionListView();
     $transaction_view->setTransactions($transactions);

src/applications/maniphest/storage/ManiphestTask.php

@@ -19,7 +19,10 @@
 /**
  * @group maniphest
  */
-final class ManiphestTask extends ManiphestDAO {
+final class ManiphestTask extends ManiphestDAO
+  implements PhabricatorMarkupInterface {
+
+  const MARKUP_FIELD_DESCRIPTION = 'markup:desc';
 
   protected $phid;
   protected $authorPHID;
@@ -214,4 +217,52 @@ final class ManiphestTask extends ManiphestDAO {
     }
   }
 
+
+/* -(  Markup Interface  )--------------------------------------------------- */
+
+
+  /**
+   * @task markup
+   */
+  public function getMarkupFieldKey($field) {
+    $hash = PhabricatorHash::digest($this->getMarkupText($field));
+    $id = $this->getID();
+    return "maniphest:T{$id}:{$field}:{$hash}";
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function getMarkupText($field) {
+    return $this->getDescription();
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newManiphestMarkupEngine();
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function didMarkupText(
+    $field,
+    $output,
+    PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function shouldUseMarkupCache($field) {
+    return (bool)$this->getID();
+  }
+
 }

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -17,9 +17,13 @@
  */
 
 /**
+ * @task markup Markup Interface
  * @group maniphest
  */
-final class ManiphestTransaction extends ManiphestDAO {
+final class ManiphestTransaction extends ManiphestDAO
+  implements PhabricatorMarkupInterface {
+
+  const MARKUP_FIELD_BODY = 'markup:body';
 
   protected $taskID;
   protected $authorPHID;
@@ -27,7 +31,6 @@ final class ManiphestTransaction extends ManiphestDAO {
   protected $oldValue;
   protected $newValue;
   protected $comments;
-  protected $cache;
   protected $metadata = array();
   protected $contentSource;
 
@@ -143,4 +146,55 @@ final class ManiphestTransaction extends ManiphestDAO {
     return PhabricatorContentSource::newFromSerialized($this->contentSource);
   }
 
+
+/* -(  Markup Interface  )--------------------------------------------------- */
+
+
+  /**
+   * @task markup
+   */
+  public function getMarkupFieldKey($field) {
+    if ($this->shouldUseMarkupCache($field)) {
+      $id = $this->getID();
+    } else {
+      $id = PhabricatorHash::digest($this->getMarkupText($field));
+    }
+    return "maniphest:x:{$field}:{$id}";
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function getMarkupText($field) {
+    return $this->getComments();
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function newMarkupEngine($field) {
+    return PhabricatorMarkupEngine::newManiphestMarkupEngine();
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function didMarkupText(
+    $field,
+    $output,
+    PhutilMarkupEngine $engine) {
+    return $output;
+  }
+
+
+  /**
+   * @task markup
+   */
+  public function shouldUseMarkupCache($field) {
+    return (bool)$this->getID();
+  }
+
 }

src/applications/maniphest/view/ManiphestTransactionDetailView.php

@@ -57,7 +57,7 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     return $this;
   }
 
-  public function setMarkupEngine(PhutilMarkupEngine $engine) {
+  public function setMarkupEngine(PhabricatorMarkupEngine $engine) {
     $this->markupEngine = $engine;
     return $this;
   }
@@ -199,22 +199,12 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     }
 
     if ($comment_transaction && $comment_transaction->hasComments()) {
-      $comments = $comment_transaction->getCache();
+      $comment_block = $this->markupEngine->getOutput(
-      if (!strlen($comments)) {
+        $comment_transaction,
-        $comments = $comment_transaction->getComments();
+        ManiphestTransaction::MARKUP_FIELD_BODY);
-        if (strlen($comments)) {
-          $comments = $this->markupEngine->markupText($comments);
-          $comment_transaction->setCache($comments);
-          if ($comment_transaction->getID() && !$this->preview) {
-            $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-            $comment_transaction->save();
-            unset($unguarded);
-          }
-        }
-      }
       $comment_block =
         '<div class="maniphest-transaction-comments phabricator-remarkup">'.
-          $comments.
+          $comment_block.
         '</div>';
     } else {
       $comment_block = null;

src/applications/maniphest/view/ManiphestTransactionListView.php

@@ -45,7 +45,7 @@ final class ManiphestTransactionListView extends ManiphestView {
     return $this;
   }
 
-  public function setMarkupEngine(PhutilMarkupEngine $engine) {
+  public function setMarkupEngine(PhabricatorMarkupEngine $engine) {
     $this->markupEngine = $engine;
     return $this;
   }

src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php

@@ -24,13 +24,6 @@ final class PhabricatorRemarkupRuleImageMacro
 
   private $images = array();
 
-  public function __construct() {
-    $rows = id(new PhabricatorFileImageMacro())->loadAll();
-    foreach ($rows as $row) {
-      $this->images[$row->getName()] = $row->getFilePHID();
-    }
-  }
-
   public function apply($text) {
     return preg_replace_callback(
       '@^([a-zA-Z0-9_\-]+)$@m',
@@ -39,6 +32,14 @@ final class PhabricatorRemarkupRuleImageMacro
   }
 
   public function markupImageMacro($matches) {
+    if ($this->images === null) {
+      $this->images = array();
+      $rows = id(new PhabricatorFileImageMacro())->loadAll();
+      foreach ($rows as $row) {
+        $this->images[$row->getName()] = $row->getFilePHID();
+      }
+    }
+
     if (array_key_exists($matches[1], $this->images)) {
       $phid = $this->images[$matches[1]];
 

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -911,6 +911,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'sql',
         'name'    => $this->getPatchPath('markupcache.sql'),
       ),
+      'maniphestxcache.sql' => array(
+        'type'    => 'sql',
+        'name'    => $this->getPatchPath('maniphestxcache.sql'),
+      ),
     );
   }