From 73bc34b26d13f7b8700e5efc8245de6e8776b79b Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 7 Nov 2012 13:31:52 -0800 Subject: [PATCH 1/5] Add support for differential field specifications to be indexed in search Summary: ...and do so for a few fields -- summary, test plan, and revert plan. Test Plan: added NATASHA and BULLWINKLE to summary and test plan of existing diff. Diff showed up in search! Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T654 Differential Revision: https://secure.phabricator.com/D3915 --- .../DifferentialFieldSpecification.php | 25 +++++++++++++++++++ ...fferentialRevertPlanFieldSpecification.php | 12 +++++++++ .../DifferentialSummaryFieldSpecification.php | 12 +++++++++ ...DifferentialTestPlanFieldSpecification.php | 14 +++++++++++ .../constants/PhabricatorSearchField.php | 1 - .../PhabricatorSearchDifferentialIndexer.php | 23 ++++++++++++----- 6 files changed, 80 insertions(+), 7 deletions(-) diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php index d5310530f2..eb64558a94 100644 --- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php @@ -383,6 +383,31 @@ abstract class DifferentialFieldSpecification { return $key; } +/* -( Extending the Search Interface )------------------------------------ */ + + /** + * @task search + */ + public function shouldAddToSearchIndex() { + return false; + } + + /** + * @task search + */ + public function getValueForSearchIndex() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + + /** + * NOTE: Keys *must be* 4 characters for + * @{class:PhabricatorSearchEngineMySQL}. + * + * @task search + */ + public function getKeyForSearchIndex() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } /* -( Extending Commit Messages )------------------------------------------ */ diff --git a/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php b/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php index 4e84e3f492..f23b39c875 100644 --- a/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php @@ -95,4 +95,16 @@ final class DifferentialRevertPlanFieldSpecification return $value; } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->value; + } + + public function getKeyForSearchIndex() { + return 'rpln'; + } + } diff --git a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php index e47966cc02..20a11bfef7 100644 --- a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php @@ -70,4 +70,16 @@ final class DifferentialSummaryFieldSpecification return $this->summary; } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->summary; + } + + public function getKeyForSearchIndex() { + return PhabricatorSearchField::FIELD_BODY; + } + } diff --git a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php index 30238271e2..e73a715f54 100644 --- a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php @@ -103,8 +103,22 @@ final class DifferentialTestPlanFieldSpecification return "TEST PLAN\n".preg_replace('/^/m', ' ', $this->plan); } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->plan; + } + + public function getKeyForSearchIndex() { + return 'tpln'; + } + private function isRequired() { return PhabricatorEnv::getEnvConfig('differential.require-test-plan-field'); } + + } diff --git a/src/applications/search/constants/PhabricatorSearchField.php b/src/applications/search/constants/PhabricatorSearchField.php index 3e40debefe..24d28b6769 100644 --- a/src/applications/search/constants/PhabricatorSearchField.php +++ b/src/applications/search/constants/PhabricatorSearchField.php @@ -7,7 +7,6 @@ final class PhabricatorSearchField { const FIELD_TITLE = 'titl'; const FIELD_BODY = 'body'; - const FIELD_TEST_PLAN = 'tpln'; const FIELD_COMMENT = 'cmnt'; } diff --git a/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php b/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php index a12b29067a..90c0c86d1e 100644 --- a/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php +++ b/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php @@ -14,12 +14,23 @@ final class PhabricatorSearchDifferentialIndexer $doc->setDocumentCreated($rev->getDateCreated()); $doc->setDocumentModified($rev->getDateModified()); - $doc->addField( - PhabricatorSearchField::FIELD_BODY, - $rev->getSummary()); - $doc->addField( - PhabricatorSearchField::FIELD_TEST_PLAN, - $rev->getTestPlan()); + $aux_fields = DifferentialFieldSelector::newSelector() + ->getFieldSpecifications(); + foreach ($aux_fields as $key => $aux_field) { + if (!$aux_field->shouldAddToSearchIndex()) { + unset($aux_fields[$key]); + } + } + + $aux_fields = DifferentialAuxiliaryField::loadFromStorage( + $rev, + $aux_fields); + foreach ($aux_fields as $aux_field) { + $doc->addField( + $aux_field->getKeyForSearchIndex(), + $aux_field->getValueForSearchIndex() + ); + } $doc->addRelationship( PhabricatorSearchRelationship::RELATIONSHIP_AUTHOR, From 7332599e032610bb66c43b974ad5750b3975c0ed Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 7 Nov 2012 13:33:07 -0800 Subject: [PATCH 2/5] Provide an IDS_COUNTER mechanism for ID assignment Summary: See D3912 for discussion. InnoDB may reuse autoincrement IDs after restart; provide a way to avoid it. Test Plan: Unit tests. Scheduled and executed tasks through `drydock lease --type host` and `phd debug taskmaster`. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D3914 --- resources/sql/patches/liskcounters.php | 42 +++++++++++ resources/sql/patches/liskcounters.sql | 9 +++ .../storage/PhabricatorWorkerActiveTask.php | 1 + src/infrastructure/storage/lisk/LiskDAO.php | 72 +++++++++++++++++-- .../lisk/__tests__/LiskFixtureTestCase.php | 35 +++++++++ .../patch/PhabricatorBuiltinPatchList.php | 8 +++ 6 files changed, 161 insertions(+), 6 deletions(-) create mode 100644 resources/sql/patches/liskcounters.php create mode 100644 resources/sql/patches/liskcounters.sql diff --git a/resources/sql/patches/liskcounters.php b/resources/sql/patches/liskcounters.php new file mode 100644 index 0000000000..1cfac2c99c --- /dev/null +++ b/resources/sql/patches/liskcounters.php @@ -0,0 +1,42 @@ +establishConnection('w'); + +$active_auto = head(queryfx_one( + $conn_w, + 'SELECT auto_increment FROM information_schema.tables + WHERE table_name = %s + AND table_schema = DATABASE()', + $active_table->getTableName())); + +$active_max = head(queryfx_one( + $conn_w, + 'SELECT MAX(id) FROM %T', + $active_table->getTableName())); + +$archive_max = head(queryfx_one( + $conn_w, + 'SELECT MAX(id) FROM %T', + $archive_table->getTableName())); + +$initial_counter = max((int)$active_auto, (int)$active_max, (int)$archive_max); + +queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (counterName, counterValue) + VALUES (%s, %d)', + LiskDAO::COUNTER_TABLE_NAME, + $active_table->getTableName(), + $initial_counter + 1); + +// Drop AUTO_INCREMENT from the ID column. +queryfx( + $conn_w, + 'ALTER TABLE %T CHANGE id id INT UNSIGNED NOT NULL', + $active_table->getTableName()); diff --git a/resources/sql/patches/liskcounters.sql b/resources/sql/patches/liskcounters.sql new file mode 100644 index 0000000000..f617a9699f --- /dev/null +++ b/resources/sql/patches/liskcounters.sql @@ -0,0 +1,9 @@ +CREATE TABLE `{$NAMESPACE}_harbormaster`.`lisk_counter` ( + counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + counterValue BIGINT UNSIGNED NOT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE `{$NAMESPACE}_worker`.`lisk_counter` ( + counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + counterValue BIGINT UNSIGNED NOT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index 0364091cb7..37c750f9e8 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -11,6 +11,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { public function getConfiguration() { return array( + self::CONFIG_IDS => self::IDS_COUNTER, self::CONFIG_TIMESTAMPS => false, ) + parent::getConfiguration(); } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 954191ef25..b78a90ebda 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -177,9 +177,12 @@ abstract class LiskDAO { const SERIALIZATION_PHP = 'php'; const IDS_AUTOINCREMENT = 'ids-auto'; + const IDS_COUNTER = 'ids-counter'; const IDS_PHID = 'ids-phid'; const IDS_MANUAL = 'ids-manual'; + const COUNTER_TABLE_NAME = 'lisk_counter'; + private $__dirtyFields = array(); private $__missingFields = array(); private static $processIsolationLevel = 0; @@ -327,8 +330,23 @@ abstract class LiskDAO { * Lisk objects need to have a unique identifying ID. The three mechanisms * available for generating this ID are IDS_AUTOINCREMENT (default, assumes * the ID column is an autoincrement primary key), IDS_PHID (to generate a - * unique PHID for each object) or IDS_MANUAL (you are taking full - * responsibility for ID management). + * unique PHID for each object), IDS_MANUAL (you are taking full + * responsibility for ID management), or IDS_COUNTER (see below). + * + * InnoDB does not persist the value of `auto_increment` across restarts, + * and instead initializes it to `MAX(id) + 1` during startup. This means it + * may reissue the same autoincrement ID more than once, if the row is deleted + * and then the database is restarted. To avoid this, you can set an object to + * use a counter table with IDS_COUNTER. This will generally behave like + * IDS_AUTOINCREMENT, except that the counter value will persist across + * restarts and inserts will be slightly slower. If a database stores any + * DAOs which use this mechanism, you must create a table there with this + * schema: + * + * CREATE TABLE lisk_counter ( + * counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + * counterValue BIGINT UNSIGNED NOT NULL + * ) ENGINE=InnoDB DEFAULT CHARSET=utf8; * * CONFIG_TIMESTAMPS * Lisk can automatically handle keeping track of a `dateCreated' and @@ -365,7 +383,6 @@ abstract class LiskDAO { * directly access or assign protected members of your class (use the getters * and setters). * - * * @return dictionary Map of configuration options to values. * * @task config @@ -1181,7 +1198,6 @@ abstract class LiskDAO { return $this; } - /** * Internal implementation of INSERT and REPLACE. * @@ -1193,6 +1209,8 @@ abstract class LiskDAO { $this->willSaveObject(); $data = $this->getPropertyValues(); + $conn = $this->establishConnection('w'); + $id_mechanism = $this->getConfigOption(self::CONFIG_IDS); switch ($id_mechanism) { case self::IDS_AUTOINCREMENT: @@ -1204,6 +1222,17 @@ abstract class LiskDAO { unset($data[$id_key]); } break; + case self::IDS_COUNTER: + // If we are using counter IDs, assign a new ID if we don't already have + // one. + $id_key = $this->getIDKeyForUse(); + if (empty($data[$id_key])) { + $counter_name = $this->getTableName(); + $id = self::loadNextCounterID($conn, $counter_name); + $this->setID($id); + $data[$id_key] = $id; + } + break; case self::IDS_PHID: if (empty($data[$this->getIDKeyForUse()])) { $phid = $this->generatePHID(); @@ -1219,8 +1248,6 @@ abstract class LiskDAO { $this->willWriteData($data); - $conn = $this->establishConnection('w'); - $columns = array_keys($data); foreach ($data as $key => $value) { @@ -1761,4 +1788,37 @@ abstract class LiskDAO { $this->$name = $value; } + /** + * Increments a named counter and returns the next value. + * + * @param AphrontDatabaseConnection Database where the counter resides. + * @param string Counter name to create or increment. + * @return int Next counter value. + * + * @task util + */ + public static function loadNextCounterID( + AphrontDatabaseConnection $conn_w, + $counter_name) { + + // NOTE: If an insert does not touch an autoincrement row or call + // LAST_INSERT_ID(), MySQL normally does not change the value of + // LAST_INSERT_ID(). This can cause a counter's value to leak to a + // new counter if the second counter is created after the first one is + // updated. To avoid this, we insert LAST_INSERT_ID(1), to ensure the + // LAST_INSERT_ID() is always updated and always set correctly after the + // query completes. + + queryfx( + $conn_w, + 'INSERT INTO %T (counterName, counterValue) VALUES + (%s, LAST_INSERT_ID(1)) + ON DUPLICATE KEY UPDATE + counterValue = LAST_INSERT_ID(counterValue + 1)', + self::COUNTER_TABLE_NAME, + $counter_name); + + return $conn_w->getInsertID(); + } + } diff --git a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php index 0f3b1db99d..25e0208ebb 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php @@ -95,5 +95,40 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { $this->assertEqual(true, (bool)$load->load((string)$id)); } + public function testCounters() { + $obj = new HarbormasterObject(); + $conn_w = $obj->establishConnection('w'); + + // Test that the counter bascially behaves as expected. + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'a')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'a')); + $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_w, 'a')); + + // This first insert is primarily a test that the previous LAST_INSERT_ID() + // value does not bleed into the creation of a new counter. + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'b')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'b')); + + // These inserts alternate database connections. Since unit tests are + // transactional by default, we need to break out of them or we'll deadlock + // since the transactions don't normally close until we exit the test. + LiskDAO::endIsolateAllLiskEffectsToTransactions(); + try { + + $conn_1 = $obj->establishConnection('w', $force_new = true); + $conn_2 = $obj->establishConnection('w', $force_new = true); + + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_1, 'z')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_2, 'z')); + $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_1, 'z')); + $this->assertEqual(4, LiskDAO::loadNextCounterID($conn_2, 'z')); + $this->assertEqual(5, LiskDAO::loadNextCounterID($conn_1, 'z')); + + LiskDAO::beginIsolateAllLiskEffectsToTransactions(); + } catch (Exception $ex) { + LiskDAO::beginIsolateAllLiskEffectsToTransactions(); + throw $ex; + } + } } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 625572b3be..9a048650c1 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1012,6 +1012,14 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('drydockresourcetype.sql'), ), + 'liskcounters.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('liskcounters.sql'), + ), + 'liskcounters.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('liskcounters.php'), + ), ); } From 9966af50dd5d670f151db6423000fee3836e87c6 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 7 Nov 2012 14:31:43 -0800 Subject: [PATCH 3/5] Delete PhabricatorRemarkupRuleProxyImage Summary: don't need it now that uploading files is so easy. Plus it made for some buggy jonx if / when there were bad image links coupled with caching. In theory this is a lot less pretty though if folks linked to a bunch of files served elsewhere using images. Test Plan: http://does-not-exist.com/imaginary.jpg rendered as a link! Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2000 Differential Revision: https://secure.phabricator.com/D3908 --- conf/default.conf.php | 11 ---- resources/sql/patches/dropfileproxyimage.sql | 1 + src/__phutil_library_map__.php | 6 --- .../PhabricatorFileProxyController.php | 54 ------------------- .../storage/PhabricatorFileProxyImage.php | 18 ------- src/docs/userguide/remarkup.diviner | 10 ++-- .../markup/PhabricatorMarkupEngine.php | 8 +-- .../PhabricatorRemarkupRuleProxyImage.php | 45 ---------------- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 9 files changed, 10 insertions(+), 147 deletions(-) create mode 100644 resources/sql/patches/dropfileproxyimage.sql delete mode 100644 src/applications/files/controller/PhabricatorFileProxyController.php delete mode 100644 src/applications/files/storage/PhabricatorFileProxyImage.php delete mode 100644 src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 612a853411..0cb3f9db72 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -856,17 +856,6 @@ return array( 'image/vnd.microsoft.icon' => true, ), - // Phabricator can proxy images from other servers so you can paste the URI - // to a funny picture of a cat into the comment box and have it show up as an - // image. However, this means the webserver Phabricator is running on will - // make HTTP requests to arbitrary URIs. If the server has access to internal - // resources, this could be a security risk. You should only enable it if you - // are installed entirely a VPN and VPN access is required to access - // Phabricator, or if the webserver has no special access to anything. If - // unsure, it is safer to leave this disabled. - 'files.enable-proxy' => false, - - // -- Storage --------------------------------------------------------------- // // Phabricator allows users to upload files, and can keep them in various diff --git a/resources/sql/patches/dropfileproxyimage.sql b/resources/sql/patches/dropfileproxyimage.sql new file mode 100644 index 0000000000..982c0ebf8d --- /dev/null +++ b/resources/sql/patches/dropfileproxyimage.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_file.file_proxyimage; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 62965e0e03..606f232572 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -744,8 +744,6 @@ phutil_register_library_map(array( 'PhabricatorFileLinkListView' => 'view/layout/PhabricatorFileLinkListView.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', 'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php', - 'PhabricatorFileProxyController' => 'applications/files/controller/PhabricatorFileProxyController.php', - 'PhabricatorFileProxyImage' => 'applications/files/storage/PhabricatorFileProxyImage.php', 'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php', 'PhabricatorFileShortcutController' => 'applications/files/controller/PhabricatorFileShortcutController.php', 'PhabricatorFileSideNavView' => 'applications/files/view/PhabricatorFileSideNavView.php', @@ -987,7 +985,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleObjectName' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php', 'PhabricatorRemarkupRulePaste' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePaste.php', 'PhabricatorRemarkupRulePhriction' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePhriction.php', - 'PhabricatorRemarkupRuleProxyImage' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php', 'PhabricatorRemarkupRuleYoutube' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php', 'PhabricatorRepository' => 'applications/repository/storage/PhabricatorRepository.php', 'PhabricatorRepositoryArcanistProject' => 'applications/repository/storage/PhabricatorRepositoryArcanistProject.php', @@ -1955,8 +1952,6 @@ phutil_register_library_map(array( 'PhabricatorFileLinkListView' => 'AphrontView', 'PhabricatorFileLinkView' => 'AphrontView', 'PhabricatorFileListController' => 'PhabricatorFileController', - 'PhabricatorFileProxyController' => 'PhabricatorFileController', - 'PhabricatorFileProxyImage' => 'PhabricatorFileDAO', 'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorFileShortcutController' => 'PhabricatorFileController', 'PhabricatorFileSideNavView' => 'AphrontView', @@ -2172,7 +2167,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleObjectName' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRulePaste' => 'PhabricatorRemarkupRuleObjectName', 'PhabricatorRemarkupRulePhriction' => 'PhutilRemarkupRule', - 'PhabricatorRemarkupRuleProxyImage' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleYoutube' => 'PhutilRemarkupRule', 'PhabricatorRepository' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryArcanistProject' => 'PhabricatorRepositoryDAO', diff --git a/src/applications/files/controller/PhabricatorFileProxyController.php b/src/applications/files/controller/PhabricatorFileProxyController.php deleted file mode 100644 index 51b1e98654..0000000000 --- a/src/applications/files/controller/PhabricatorFileProxyController.php +++ /dev/null @@ -1,54 +0,0 @@ -getRequest(); - $uri = $request->getStr('uri'); - - $proxy = id(new PhabricatorFileProxyImage())->loadOneWhere( - 'uri = %s', - $uri); - - if (!$proxy) { - // This write is fine to skip CSRF checks for, we're just building a - // cache of some remote image. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileDownload( - $uri, - nonempty(basename($uri), 'proxied-file')); - if ($file) { - $proxy = new PhabricatorFileProxyImage(); - $proxy->setURI($uri); - $proxy->setFilePHID($file->getPHID()); - $proxy->save(); - } - - unset($unguarded); - } - - if ($proxy) { - $file = id(new PhabricatorFile())->loadOneWhere('phid = %s', - $proxy->getFilePHID()); - if ($file) { - $view_uri = $file->getBestURI(); - } else { - $bad_phid = $proxy->getFilePHID(); - throw new Exception( - "Unable to load file with phid {$bad_phid}." - ); - } - return id(new AphrontRedirectResponse())->setURI($view_uri); - } - - return new Aphront400Response(); - } -} diff --git a/src/applications/files/storage/PhabricatorFileProxyImage.php b/src/applications/files/storage/PhabricatorFileProxyImage.php deleted file mode 100644 index 180bde5fd1..0000000000 --- a/src/applications/files/storage/PhabricatorFileProxyImage.php +++ /dev/null @@ -1,18 +0,0 @@ - false, - ) + parent::getConfiguration(); - } - - static public function getProxyImageURI($uri) { - return '/file/proxy/?uri='.phutil_escape_uri($uri); - } -} - diff --git a/src/docs/userguide/remarkup.diviner b/src/docs/userguide/remarkup.diviner index 7840a67814..1b680e46af 100644 --- a/src/docs/userguide/remarkup.diviner +++ b/src/docs/userguide/remarkup.diviner @@ -307,16 +307,14 @@ Valid options are: = Embedding Media = -If you set configuration flags, you can embed media directly in text: +If you set a configuration flag, you can embed media directly in text: - - **files.enable-proxy**: allows you to paste in image URLs and have them - render inline. - **remarkup.enable-embedded-youtube**: allows you to paste in YouTube videos and have them render inline. -These options are disabled by default because they have security and/or -silliness implications, read their descriptions in ##default.conf.php## before -enabling them. +This option is disabled by default because it has security and/or +silliness implications. Read the description in ##default.conf.php## before +enabling it. = Image Macros = diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index f8bf913409..3e0273e1aa 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -41,7 +41,7 @@ final class PhabricatorMarkupEngine { private $objects = array(); private $viewer; - private $version = 0; + private $version = 1; /* -( Markup Pipeline )---------------------------------------------------- */ @@ -286,7 +286,6 @@ final class PhabricatorMarkupEngine { return self::newMarkupEngine( array( 'macros' => false, - 'fileproxy' => false, 'youtube' => false, )); @@ -345,7 +344,6 @@ final class PhabricatorMarkupEngine { private static function getMarkupEngineDefaultConfiguration() { return array( 'pygments' => PhabricatorEnv::getEnvConfig('pygments.enabled'), - 'fileproxy' => PhabricatorEnv::getEnvConfig('files.enable-proxy'), 'youtube' => PhabricatorEnv::getEnvConfig( 'remarkup.enable-embedded-youtube'), 'custom-inline' => array(), @@ -394,10 +392,6 @@ final class PhabricatorMarkupEngine { $rules[] = new PhutilRemarkupRuleDocumentLink(); - if ($options['fileproxy']) { - $rules[] = new PhabricatorRemarkupRuleProxyImage(); - } - if ($options['youtube']) { $rules[] = new PhabricatorRemarkupRuleYoutube(); } diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php deleted file mode 100644 index 0083a3bae7..0000000000 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php +++ /dev/null @@ -1,45 +0,0 @@ -]@', - array($this, 'markupProxyImage'), - $text); - - $text = preg_replace_callback( - '@(?<=^|\s)(\w{3,}://\S+'.$filetypes.')(?=\s|$)@', - array($this, 'markupProxyImage'), - $text); - - return $text; - } - - public function markupProxyImage($matches) { - - $uri = PhabricatorFileProxyImage::getProxyImageURI($matches[1]); - - return $this->getEngine()->storeText( - phutil_render_tag( - 'a', - array( - 'href' => $uri, - 'target' => '_blank', - ), - phutil_render_tag( - 'img', - array( - 'src' => $uri, - 'class' => 'remarkup-proxy-image', - )))); - } - -} diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 9a048650c1..90308fee61 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1020,6 +1020,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'php', 'name' => $this->getPatchPath('liskcounters.php'), ), + 'dropfileproxyimage.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('dropfileproxyimage.sql'), + ), ); } From c812d7d686541644a1fb339a0554d8bf203217ed Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Nov 2012 08:22:08 -0800 Subject: [PATCH 4/5] Add a simple button UIExample Summary: For visualizing D3919. Test Plan: {F22921} Reviewers: chad, btrahan Reviewed By: chad CC: aran Differential Revision: https://secure.phabricator.com/D3922 --- src/__phutil_library_map__.php | 2 + .../examples/PhabricatorButtonsExample.php | 45 +++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 src/applications/uiexample/examples/PhabricatorButtonsExample.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 606f232572..4bf8b5aa82 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -608,6 +608,7 @@ phutil_register_library_map(array( 'PhabricatorBarePageView' => 'view/page/PhabricatorBarePageView.php', 'PhabricatorBaseEnglishTranslation' => 'infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php', 'PhabricatorBuiltinPatchList' => 'infrastructure/storage/patch/PhabricatorBuiltinPatchList.php', + 'PhabricatorButtonsExample' => 'applications/uiexample/examples/PhabricatorButtonsExample.php', 'PhabricatorCacheDAO' => 'applications/cache/storage/PhabricatorCacheDAO.php', 'PhabricatorCalendarBrowseController' => 'applications/calendar/controller/PhabricatorCalendarBrowseController.php', 'PhabricatorCalendarController' => 'applications/calendar/controller/PhabricatorCalendarController.php', @@ -1821,6 +1822,7 @@ phutil_register_library_map(array( 'PhabricatorBarePageView' => 'AphrontPageView', 'PhabricatorBaseEnglishTranslation' => 'PhabricatorTranslation', 'PhabricatorBuiltinPatchList' => 'PhabricatorSQLPatchList', + 'PhabricatorButtonsExample' => 'PhabricatorUIExample', 'PhabricatorCacheDAO' => 'PhabricatorLiskDAO', 'PhabricatorCalendarBrowseController' => 'PhabricatorCalendarController', 'PhabricatorCalendarController' => 'PhabricatorController', diff --git a/src/applications/uiexample/examples/PhabricatorButtonsExample.php b/src/applications/uiexample/examples/PhabricatorButtonsExample.php new file mode 100644 index 0000000000..03228412db --- /dev/null +++ b/src/applications/uiexample/examples/PhabricatorButtonsExample.php @@ -0,0 +1,45 @@ +<button> to render buttons.'; + } + + public function renderExample() { + $request = $this->getRequest(); + $user = $request->getUser(); + + $colors = array('', 'green', 'grey', 'disabled'); + $sizes = array('', 'small'); + $tags = array('a', 'button'); + + $view = array(); + foreach ($tags as $tag) { + foreach ($colors as $color) { + foreach ($sizes as $size) { + $class = implode(' ', array($color, $size)); + + if ($tag == 'a') { + $class .= ' button'; + } + + $view[] = phutil_render_tag( + $tag, + array( + 'class' => $class, + ), + phutil_escape_html(ucwords($size.' '.$color.' '.$tag))); + + $view[] = '

'; + } + } + } + + return '
'.implode('', $view).'
'; + } +} From 2e993f756118eca8d8c2a4335b539084ad8bac8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Nov 2012 09:05:38 -0800 Subject: [PATCH 5/5] Fix an issue where projects queried in "Any Projects" in Maniphest did not have their handles loaded Summary: See https://github.com/facebook/phabricator/issues/230. If you searched for a project with the "Any Projects" field, we didn't explicitly include it in the list of handles to fetch. Usually this works fine because something else fetches the handle, but if you, e.g., search for a project that has no tasks, you get a fatal. Test Plan: Reproduced fatal described in report by performing a custom query for "Any Projects" using a project with no tasks; applied patch; query worked correctly. Verified `$xproject_phids` and `$project_phids` are already queried. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D3923 --- .../maniphest/controller/ManiphestTaskListController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/maniphest/controller/ManiphestTaskListController.php b/src/applications/maniphest/controller/ManiphestTaskListController.php index 9120dc7dad..56ca9b76f0 100644 --- a/src/applications/maniphest/controller/ManiphestTaskListController.php +++ b/src/applications/maniphest/controller/ManiphestTaskListController.php @@ -543,6 +543,7 @@ final class ManiphestTaskListController extends ManiphestController { $owner_phids, $author_phids, $project_group_phids, + $any_project_phids, array_mergev(mpull($data, 'getProjectPHIDs'))); $handles = id(new PhabricatorObjectHandleData($handle_phids)) ->loadHandles();