From 5d8b75b4da9771fbfcc4cda264fffbb19b671733 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jul 2012 11:40:10 -0700 Subject: [PATCH] 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 | 2 + scripts/util/purge_cache.php | 24 +------- ...iphestTaskDescriptionPreviewController.php | 9 ++- .../ManiphestTaskDetailController.php | 13 ++++- .../ManiphestTransactionPreviewController.php | 4 +- .../maniphest/storage/ManiphestTask.php | 53 ++++++++++++++++- .../storage/ManiphestTransaction.php | 58 ++++++++++++++++++- .../view/ManiphestTransactionDetailView.php | 20 ++----- .../view/ManiphestTransactionListView.php | 2 +- .../PhabricatorRemarkupRuleImageMacro.php | 15 ++--- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 11 files changed, 150 insertions(+), 54 deletions(-) create mode 100644 resources/sql/patches/maniphestxcache.sql diff --git a/resources/sql/patches/maniphestxcache.sql b/resources/sql/patches/maniphestxcache.sql new file mode 100644 index 0000000000..131bb52bc1 --- /dev/null +++ b/resources/sql/patches/maniphestxcache.sql @@ -0,0 +1,2 @@ +ALTER TABLE `{$NAMESPACE}_maniphest`.`maniphest_transaction` + DROP `cache`; diff --git a/scripts/util/purge_cache.php b/scripts/util/purge_cache.php index 45c91040c1..0a273a72a2 100755 --- a/scripts/util/purge_cache.php +++ b/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 - Differential comment caches ("--maniphest", "--differential") so older - comments pick up the new rules. + If you change Remarkup rules, you may want to purge the Differential + comment caches ("--differential") so older 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 diff --git a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php b/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php index a01e15a349..17a89f6991 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php +++ b/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 = '
'. - $engine->markupText($description). + $output. '
'; return id(new AphrontAjaxResponse()) diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 6353a0f19f..311e537f13 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/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'] = ''. @@ -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( '
'. - $engine->markupText($task->getDescription()). + $engine->getOutput($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION). '
'); $transaction_types = ManiphestTransactionType::getTransactionTypeMap(); diff --git a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php index cb9e3fcfed..94e6c49b0d 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php +++ b/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); diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index cf0d88d77e..97a269d4f7 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/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(); + } + } diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index bbdd2c2756..c106e37904 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/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(); + } + } diff --git a/src/applications/maniphest/view/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/ManiphestTransactionDetailView.php index 8f8662e9e7..ccb4c7444f 100644 --- a/src/applications/maniphest/view/ManiphestTransactionDetailView.php +++ b/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(); - if (!strlen($comments)) { - $comments = $comment_transaction->getComments(); - 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 = $this->markupEngine->getOutput( + $comment_transaction, + ManiphestTransaction::MARKUP_FIELD_BODY); $comment_block = '
'. - $comments. + $comment_block. '
'; } else { $comment_block = null; diff --git a/src/applications/maniphest/view/ManiphestTransactionListView.php b/src/applications/maniphest/view/ManiphestTransactionListView.php index 3ecda00487..001eee94c5 100644 --- a/src/applications/maniphest/view/ManiphestTransactionListView.php +++ b/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; } diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php index 96248b38a1..714c539152 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php +++ b/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]]; diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index b67804920d..e4138845b8 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/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'), + ), ); }