From a2a19e29aaceb6eee8ae1214cdd18b143dea7065 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sat, 17 Sep 2016 21:23:54 +0000 Subject: [PATCH 01/23] Remove TYPE_FILES from Conpherence Summary: I believe these are left over from widgets, when we added a "Files" widget that kept track of everything added to the window. Test Plan: Added files to a Conpherece, Set an image when editing. Anything else? Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D16567 --- .../__tests__/ConpherenceRoomTestCase.php | 19 ----- ...ConpherenceQueryThreadConduitAPIMethod.php | 4 +- ...onpherenceUpdateThreadConduitAPIMethod.php | 3 +- .../ConpherenceUpdateController.php | 1 - .../conpherence/editor/ConpherenceEditor.php | 70 ------------------- .../query/ConpherenceThreadQuery.php | 22 ------ .../conpherence/storage/ConpherenceThread.php | 10 --- .../storage/ConpherenceTransaction.php | 27 ------- .../view/ConpherenceTransactionView.php | 3 - 9 files changed, 2 insertions(+), 157 deletions(-) diff --git a/src/applications/conpherence/__tests__/ConpherenceRoomTestCase.php b/src/applications/conpherence/__tests__/ConpherenceRoomTestCase.php index a4eeb3ed86..f34dd25110 100644 --- a/src/applications/conpherence/__tests__/ConpherenceRoomTestCase.php +++ b/src/applications/conpherence/__tests__/ConpherenceRoomTestCase.php @@ -112,25 +112,6 @@ final class ConpherenceRoomTestCase extends ConpherenceTestCase { } } - public function testAddMessageWithFileAttachments() { - $creator = $this->generateNewTestUser(); - $friend_1 = $this->generateNewTestUser(); - - $participant_map = array( - $creator->getPHID() => $creator, - $friend_1->getPHID() => $friend_1, - ); - - $conpherence = $this->createRoom( - $creator, - array_keys($participant_map)); - - foreach ($participant_map as $phid => $user) { - $xactions = $this->addMessageWithFile($user, $conpherence); - $this->assertEqual(2, count($xactions)); - } - } - private function createRoom( PhabricatorUser $creator, array $participant_phids) { diff --git a/src/applications/conpherence/conduit/ConpherenceQueryThreadConduitAPIMethod.php b/src/applications/conpherence/conduit/ConpherenceQueryThreadConduitAPIMethod.php index cd5fa1bff7..c66a90f585 100644 --- a/src/applications/conpherence/conduit/ConpherenceQueryThreadConduitAPIMethod.php +++ b/src/applications/conpherence/conduit/ConpherenceQueryThreadConduitAPIMethod.php @@ -37,8 +37,7 @@ final class ConpherenceQueryThreadConduitAPIMethod $query = id(new ConpherenceThreadQuery()) ->setViewer($user) - ->needParticipantCache(true) - ->needFilePHIDs(true); + ->needParticipantCache(true); if ($ids) { $conpherences = $query @@ -73,7 +72,6 @@ final class ConpherenceQueryThreadConduitAPIMethod 'conpherenceTitle' => $conpherence->getTitle(), 'messageCount' => $conpherence->getMessageCount(), 'recentParticipantPHIDs' => $conpherence->getRecentParticipantPHIDs(), - 'filePHIDs' => $conpherence->getFilePHIDs(), 'conpherenceURI' => $this->getConpherenceURI($conpherence), ); } diff --git a/src/applications/conpherence/conduit/ConpherenceUpdateThreadConduitAPIMethod.php b/src/applications/conpherence/conduit/ConpherenceUpdateThreadConduitAPIMethod.php index ebcf7f9a04..01b86f9a42 100644 --- a/src/applications/conpherence/conduit/ConpherenceUpdateThreadConduitAPIMethod.php +++ b/src/applications/conpherence/conduit/ConpherenceUpdateThreadConduitAPIMethod.php @@ -44,8 +44,7 @@ final class ConpherenceUpdateThreadConduitAPIMethod $id = $request->getValue('id'); $phid = $request->getValue('phid'); $query = id(new ConpherenceThreadQuery()) - ->setViewer($user) - ->needFilePHIDs(true); + ->setViewer($user); if ($id) { $query->withIDs(array($id)); } else if ($phid) { diff --git a/src/applications/conpherence/controller/ConpherenceUpdateController.php b/src/applications/conpherence/controller/ConpherenceUpdateController.php index f8602a15cf..5a1ad4e8fa 100644 --- a/src/applications/conpherence/controller/ConpherenceUpdateController.php +++ b/src/applications/conpherence/controller/ConpherenceUpdateController.php @@ -36,7 +36,6 @@ final class ConpherenceUpdateController $conpherence = id(new ConpherenceThreadQuery()) ->setViewer($user) ->withIDs(array($conpherence_id)) - ->needFilePHIDs(true) ->needOrigPics(true) ->needCropPics(true) ->needParticipants($need_participants) diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index b7b37a3e6d..8507e9817f 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/src/applications/conpherence/editor/ConpherenceEditor.php @@ -22,7 +22,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { $topic) { $conpherence = ConpherenceThread::initializeNewRoom($creator); - $files = array(); $errors = array(); if (empty($participant_phids)) { $errors[] = self::ERROR_EMPTY_PARTICIPANTS; @@ -35,26 +34,11 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { $errors[] = self::ERROR_EMPTY_MESSAGE; } - $file_phids = PhabricatorMarkupEngine::extractFilePHIDsFromEmbeddedFiles( - $creator, - array($message)); - if ($file_phids) { - $files = id(new PhabricatorFileQuery()) - ->setViewer($creator) - ->withPHIDs($file_phids) - ->execute(); - } - if (!$errors) { $xactions = array(); $xactions[] = id(new ConpherenceTransaction()) ->setTransactionType(ConpherenceTransaction::TYPE_PARTICIPANTS) ->setNewValue(array('+' => $participant_phids)); - if ($files) { - $xactions[] = id(new ConpherenceTransaction()) - ->setTransactionType(ConpherenceTransaction::TYPE_FILES) - ->setNewValue(array('+' => mpull($files, 'getPHID'))); - } if ($title) { $xactions[] = id(new ConpherenceTransaction()) ->setTransactionType(ConpherenceTransaction::TYPE_TITLE) @@ -88,27 +72,7 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { ConpherenceThread $conpherence, $text) { - $files = array(); - $file_phids = PhabricatorMarkupEngine::extractFilePHIDsFromEmbeddedFiles( - $viewer, - array($text)); - // Since these are extracted from text, we might be re-including the - // same file -- e.g. a mock under discussion. Filter files we - // already have. - $existing_file_phids = $conpherence->getFilePHIDs(); - $file_phids = array_diff($file_phids, $existing_file_phids); - if ($file_phids) { - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($file_phids) - ->execute(); - } $xactions = array(); - if ($files) { - $xactions[] = id(new ConpherenceTransaction()) - ->setTransactionType(ConpherenceTransaction::TYPE_FILES) - ->setNewValue(array('+' => mpull($files, 'getPHID'))); - } $xactions[] = id(new ConpherenceTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) ->attachComment( @@ -126,7 +90,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { $types[] = ConpherenceTransaction::TYPE_TITLE; $types[] = ConpherenceTransaction::TYPE_TOPIC; $types[] = ConpherenceTransaction::TYPE_PARTICIPANTS; - $types[] = ConpherenceTransaction::TYPE_FILES; $types[] = ConpherenceTransaction::TYPE_PICTURE; $types[] = ConpherenceTransaction::TYPE_PICTURE_CROP; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; @@ -154,8 +117,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { return array(); } return $object->getParticipantPHIDs(); - case ConpherenceTransaction::TYPE_FILES: - return $object->getFilePHIDs(); } } @@ -172,7 +133,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { $file = $xaction->getNewValue(); return $file->getPHID(); case ConpherenceTransaction::TYPE_PARTICIPANTS: - case ConpherenceTransaction::TYPE_FILES: return $this->getPHIDTransactionNewValue($xaction); } } @@ -335,27 +295,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case ConpherenceTransaction::TYPE_FILES: - $editor = new PhabricatorEdgeEditor(); - $edge_type = PhabricatorObjectHasFileEdgeType::EDGECONST; - $old = array_fill_keys($xaction->getOldValue(), true); - $new = array_fill_keys($xaction->getNewValue(), true); - $add_edges = array_keys(array_diff_key($new, $old)); - $remove_edges = array_keys(array_diff_key($old, $new)); - foreach ($add_edges as $file_phid) { - $editor->addEdge( - $object->getPHID(), - $edge_type, - $file_phid); - } - foreach ($remove_edges as $file_phid) { - $editor->removeEdge( - $object->getPHID(), - $edge_type, - $file_phid); - } - $editor->save(); - break; case ConpherenceTransaction::TYPE_PARTICIPANTS: if ($this->getIsNewObject()) { continue; @@ -488,14 +427,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { PhabricatorPolicyCapability::CAN_EDIT); } break; - // This is similar to PhabricatorTransactions::TYPE_COMMENT so - // use CAN_VIEW - case ConpherenceTransaction::TYPE_FILES: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_VIEW); - break; case ConpherenceTransaction::TYPE_TITLE: case ConpherenceTransaction::TYPE_TOPIC: PhabricatorPolicyFilter::requireCapability( @@ -514,7 +445,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor { switch ($type) { case ConpherenceTransaction::TYPE_TITLE: return $v; - case ConpherenceTransaction::TYPE_FILES: case ConpherenceTransaction::TYPE_PARTICIPANTS: return $this->mergePHIDOrEdgeTransactions($u, $v); } diff --git a/src/applications/conpherence/query/ConpherenceThreadQuery.php b/src/applications/conpherence/query/ConpherenceThreadQuery.php index dc1e761c43..c3d5322c15 100644 --- a/src/applications/conpherence/query/ConpherenceThreadQuery.php +++ b/src/applications/conpherence/query/ConpherenceThreadQuery.php @@ -13,17 +13,11 @@ final class ConpherenceThreadQuery private $needOrigPics; private $needTransactions; private $needParticipantCache; - private $needFilePHIDs; private $afterTransactionID; private $beforeTransactionID; private $transactionLimit; private $fulltext; - public function needFilePHIDs($need_file_phids) { - $this->needFilePHIDs = $need_file_phids; - return $this; - } - public function needParticipantCache($participant_cache) { $this->needParticipantCache = $participant_cache; return $this; @@ -116,9 +110,6 @@ final class ConpherenceThreadQuery if ($this->needTransactions) { $this->loadTransactionsAndHandles($conpherences); } - if ($this->needFilePHIDs) { - $this->loadFilePHIDs($conpherences); - } if ($this->needOrigPics || $this->needCropPics) { $this->initImages($conpherences); } @@ -275,19 +266,6 @@ final class ConpherenceThreadQuery return $this; } - private function loadFilePHIDs(array $conpherences) { - $edge_type = PhabricatorObjectHasFileEdgeType::EDGECONST; - $file_edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array_keys($conpherences)) - ->withEdgeTypes(array($edge_type)) - ->execute(); - foreach ($file_edges as $conpherence_phid => $data) { - $conpherence = $conpherences[$conpherence_phid]; - $conpherence->attachFilePHIDs(array_keys($data[$edge_type])); - } - return $this; - } - private function loadOrigPics(array $conpherences) { return $this->loadPics( $conpherences, diff --git a/src/applications/conpherence/storage/ConpherenceThread.php b/src/applications/conpherence/storage/ConpherenceThread.php index ec8449f87f..80efc92f5a 100644 --- a/src/applications/conpherence/storage/ConpherenceThread.php +++ b/src/applications/conpherence/storage/ConpherenceThread.php @@ -20,7 +20,6 @@ final class ConpherenceThread extends ConpherenceDAO private $participants = self::ATTACHABLE; private $transactions = self::ATTACHABLE; private $handles = self::ATTACHABLE; - private $filePHIDs = self::ATTACHABLE; private $images = self::ATTACHABLE; public static function initializeNewRoom(PhabricatorUser $sender) { @@ -31,7 +30,6 @@ final class ConpherenceThread extends ConpherenceDAO ->setTitle('') ->setTopic('') ->attachParticipants(array()) - ->attachFilePHIDs(array()) ->attachImages(array()) ->setViewPolicy($default_policy) ->setEditPolicy($default_policy) @@ -158,14 +156,6 @@ final class ConpherenceThread extends ConpherenceDAO $amount); } - public function attachFilePHIDs(array $file_phids) { - $this->filePHIDs = $file_phids; - return $this; - } - public function getFilePHIDs() { - return $this->assertAttached($this->filePHIDs); - } - public function loadImageURI($size) { $file = $this->getImage($size); diff --git a/src/applications/conpherence/storage/ConpherenceTransaction.php b/src/applications/conpherence/storage/ConpherenceTransaction.php index 1090be43c8..29d23dfbc3 100644 --- a/src/applications/conpherence/storage/ConpherenceTransaction.php +++ b/src/applications/conpherence/storage/ConpherenceTransaction.php @@ -2,7 +2,6 @@ final class ConpherenceTransaction extends PhabricatorApplicationTransaction { - const TYPE_FILES = 'files'; const TYPE_TITLE = 'title'; const TYPE_TOPIC = 'topic'; const TYPE_PARTICIPANTS = 'participants'; @@ -44,8 +43,6 @@ final class ConpherenceTransaction extends PhabricatorApplicationTransaction { case self::TYPE_PICTURE: case self::TYPE_DATE_MARKER: return false; - case self::TYPE_FILES: - return true; case self::TYPE_PICTURE_CROP: return true; } @@ -68,29 +65,6 @@ final class ConpherenceTransaction extends PhabricatorApplicationTransaction { case self::TYPE_PICTURE: return $this->getRoomTitle(); break; - case self::TYPE_FILES: - $add = array_diff($new, $old); - $rem = array_diff($old, $new); - - if ($add && $rem) { - $title = pht( - '%s edited files(s), added %d and removed %d.', - $this->renderHandleLink($author_phid), - count($add), - count($rem)); - } else if ($add) { - $title = pht( - '%s added %s files(s).', - $this->renderHandleLink($author_phid), - phutil_count($add)); - } else { - $title = pht( - '%s removed %s file(s).', - $this->renderHandleLink($author_phid), - phutil_count($rem)); - } - return $title; - break; case self::TYPE_PARTICIPANTS: $add = array_diff($new, $old); $rem = array_diff($old, $new); @@ -252,7 +226,6 @@ final class ConpherenceTransaction extends PhabricatorApplicationTransaction { switch ($this->getTransactionType()) { case self::TYPE_TITLE: case self::TYPE_PICTURE: - case self::TYPE_FILES: case self::TYPE_DATE_MARKER: break; case self::TYPE_PARTICIPANTS: diff --git a/src/applications/conpherence/view/ConpherenceTransactionView.php b/src/applications/conpherence/view/ConpherenceTransactionView.php index 467ddf1411..e99013fda7 100644 --- a/src/applications/conpherence/view/ConpherenceTransactionView.php +++ b/src/applications/conpherence/view/ConpherenceTransactionView.php @@ -227,9 +227,6 @@ final class ConpherenceTransactionView extends AphrontView { $content = null; $handles = $this->getHandles(); switch ($transaction->getTransactionType()) { - case ConpherenceTransaction::TYPE_FILES: - $content = $transaction->getTitle(); - break; case ConpherenceTransaction::TYPE_TITLE: case ConpherenceTransaction::TYPE_TOPIC: case ConpherenceTransaction::TYPE_PICTURE: From 1afd8cbe0e0c7e1e66128658cf193f97e2fc7edc Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sun, 18 Sep 2016 18:16:58 -0400 Subject: [PATCH 02/23] Remove old kdb CSS Summary: Fixes T11654 Test Plan: Hit ? on home / differential Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T11654 Differential Revision: https://secure.phabricator.com/D16568 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/application/base/standard-page-view.css | 8 -------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index ca33f2d429..d4fd4641a9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '80a3fcb3', 'conpherence.pkg.js' => '89b4837e', - 'core.pkg.css' => '476e9330', + 'core.pkg.css' => 'eb1298d4', 'core.pkg.js' => '1d376fa9', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '3fb7f532', @@ -38,7 +38,7 @@ return array( 'rsrc/css/application/base/notification-menu.css' => 'b3ab500d', 'rsrc/css/application/base/phabricator-application-launch-view.css' => '95351601', 'rsrc/css/application/base/phui-theme.css' => '027ba77e', - 'rsrc/css/application/base/standard-page-view.css' => '2b592894', + 'rsrc/css/application/base/standard-page-view.css' => '3026770e', 'rsrc/css/application/chatlog/chatlog.css' => 'd295b020', 'rsrc/css/application/conduit/conduit-api.css' => '7bc725c4', 'rsrc/css/application/config/config-options.css' => '0ede4c9b', @@ -869,7 +869,7 @@ return array( 'phabricator-shaped-request' => '7cbe244b', 'phabricator-slowvote-css' => 'a94b7230', 'phabricator-source-code-view-css' => 'cbeef983', - 'phabricator-standard-page-view' => '2b592894', + 'phabricator-standard-page-view' => '3026770e', 'phabricator-textareautils' => '320810c8', 'phabricator-title' => 'df5e11d2', 'phabricator-tooltip' => '6323f942', diff --git a/webroot/rsrc/css/application/base/standard-page-view.css b/webroot/rsrc/css/application/base/standard-page-view.css index e5d50dc825..43215df445 100644 --- a/webroot/rsrc/css/application/base/standard-page-view.css +++ b/webroot/rsrc/css/application/base/standard-page-view.css @@ -56,14 +56,6 @@ body.white-background { color: {$greytext}; } -.keyboard-shortcut-help kbd { - background: #222222; - padding: 6px; - color: #ffffff; - font-weight: bold; - border: 1px solid #555555; -} - .keyboard-focus-focus-reticle { background: #ffffd3; position: absolute; From dda06c6bdc7499bd3a500ba8a01f1e105593613c Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Thu, 15 Sep 2016 20:03:18 -0400 Subject: [PATCH 03/23] Added a 'name' field to the results for harbormaster.build.search endpoint Summary: Fixes T11642. Added a 'name' field to the results from harbormaster.build.search. Test Plan: Went to `/conduit/method/harbormaster.build.search/` and ran a search that would yield results (because otherwise there will be nothing there). Noted that there was, in fact, a name in the results. Reviewers: yelirekim, #blessed_reviewers, epriestley Reviewed By: yelirekim, #blessed_reviewers, epriestley Subscribers: epriestley, yelirekim Maniphest Tasks: T11642 Differential Revision: https://secure.phabricator.com/D16569 --- .../harbormaster/storage/build/HarbormasterBuild.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 9255b6ec4a..958eaa1f2b 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -420,6 +420,10 @@ final class HarbormasterBuild extends HarbormasterDAO ->setKey('initiatorPHID') ->setType('phid') ->setDescription(pht('The person (or thing) that started this build.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('name') + ->setType('string') + ->setDescription(pht('The name of this build.')), ); } @@ -433,6 +437,7 @@ final class HarbormasterBuild extends HarbormasterDAO 'name' => HarbormasterBuildStatus::getBuildStatusName($status), ), 'initiatorPHID' => nonempty($this->getInitiatorPHID(), null), + 'name' => $this->getName(), ); } From 2e4b5b45a20b1ce382712825a57036a063bcf6b2 Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Thu, 15 Sep 2016 20:40:46 -0400 Subject: [PATCH 04/23] Update DarkConsole for handleRequest Summary: Ref T8628 Test Plan: Updated DarkConsoleDataController and observed that the darkconsole still works as expected Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley, yelirekim Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D16570 --- .../controller/DarkConsoleDataController.php | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/applications/console/controller/DarkConsoleDataController.php b/src/applications/console/controller/DarkConsoleDataController.php index 6582189f48..cae183fbae 100644 --- a/src/applications/console/controller/DarkConsoleDataController.php +++ b/src/applications/console/controller/DarkConsoleDataController.php @@ -2,8 +2,6 @@ final class DarkConsoleDataController extends PhabricatorController { - private $key; - public function shouldRequireLogin() { return !PhabricatorEnv::getEnvConfig('darkconsole.always-on'); } @@ -16,19 +14,15 @@ final class DarkConsoleDataController extends PhabricatorController { return true; } - public function willProcessRequest(array $data) { - $this->key = $data['key']; - } - - public function processRequest() { - $request = $this->getRequest(); - $user = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $request->getViewer(); + $key = $request->getURIData('key'); $cache = new PhabricatorKeyValueDatabaseCache(); $cache = new PhutilKeyValueCacheProfiler($cache); $cache->setProfiler(PhutilServiceProfiler::getInstance()); - $result = $cache->getKey('darkconsole:'.$this->key); + $result = $cache->getKey('darkconsole:'.$key); if (!$result) { return new Aphront400Response(); } @@ -43,7 +37,7 @@ final class DarkConsoleDataController extends PhabricatorController { return new Aphront400Response(); } - if ($result['user'] != $user->getPHID()) { + if ($result['user'] != $viewer->getPHID()) { return new Aphront400Response(); } From 799ecdc27886119cef36c9e8110a8d1a33db6f1b Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Thu, 15 Sep 2016 20:45:36 -0400 Subject: [PATCH 05/23] Update RedirectController for handleRequest Summary: Ref T8628. Test Plan: Performed an action that uses the redirect controller (trying to visit a repo page while not logged in). Logged in and was redirected as expected Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, yelirekim Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D16571 --- .../controller/PhabricatorRedirectController.php | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/applications/base/controller/PhabricatorRedirectController.php b/src/applications/base/controller/PhabricatorRedirectController.php index d12c728e1c..491382e192 100644 --- a/src/applications/base/controller/PhabricatorRedirectController.php +++ b/src/applications/base/controller/PhabricatorRedirectController.php @@ -2,9 +2,6 @@ final class PhabricatorRedirectController extends PhabricatorController { - private $uri; - private $allowExternal; - public function shouldRequireLogin() { return false; } @@ -13,15 +10,12 @@ final class PhabricatorRedirectController extends PhabricatorController { return false; } - public function willProcessRequest(array $data) { - $this->uri = $data['uri']; - $this->allowExternal = idx($data, 'external', false); - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { + $uri = $request->getURIData('uri'); + $external = $request->getURIData('external', false); return id(new AphrontRedirectResponse()) - ->setURI($this->uri) - ->setIsExternal($this->allowExternal); + ->setURI($uri) + ->setIsExternal($external); } } From adf9d5ffdd61671e4211e4adc96b5e0ba005d1f7 Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Thu, 15 Sep 2016 21:21:58 -0400 Subject: [PATCH 06/23] Removed willProcessRequest from DifferentialRevisionLandController Summary: Ref T8628. Test Plan: Landed a revision through the web UI Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley, yelirekim Maniphest Tasks: T8628 Differential Revision: https://secure.phabricator.com/D16572 --- .../DifferentialRevisionLandController.php | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionLandController.php b/src/applications/differential/controller/DifferentialRevisionLandController.php index f2d2497a42..0e222370df 100644 --- a/src/applications/differential/controller/DifferentialRevisionLandController.php +++ b/src/applications/differential/controller/DifferentialRevisionLandController.php @@ -2,19 +2,12 @@ final class DifferentialRevisionLandController extends DifferentialController { - private $revisionID; - private $strategyClass; private $pushStrategy; - public function willProcessRequest(array $data) { - $this->revisionID = $data['id']; - $this->strategyClass = $data['strategy']; - } - public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - - $revision_id = $this->revisionID; + $revision_id = $request->getURIData('id'); + $strategy_class = $request->getURIData('strategy'); $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) @@ -24,15 +17,15 @@ final class DifferentialRevisionLandController extends DifferentialController { return new Aphront404Response(); } - if (is_subclass_of($this->strategyClass, 'DifferentialLandingStrategy')) { - $this->pushStrategy = newv($this->strategyClass, array()); + if (is_subclass_of($strategy_class, 'DifferentialLandingStrategy')) { + $this->pushStrategy = newv($strategy_class, array()); } else { throw new Exception( pht( "Strategy type must be a valid class name and must subclass ". "%s. '%s' is not a subclass of %s", 'DifferentialLandingStrategy', - $this->strategyClass, + $strategy_class, 'DifferentialLandingStrategy')); } From e41a64607e680e0e8d2d3163c2389467717ff4a7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Sep 2016 16:20:33 -0700 Subject: [PATCH 07/23] Retain repository update cooldowns across daemon restarts Summary: Ref T11665. Fixes T7865. When we restart the daemons, the repository pull daemon currently resets the cooldowns on all of its pulls. This can generate a burst of initial load when restarting a lot of instance daemons (as in the Phacility cluster), described in T7865. This smooths things out so that recent pulls are considered, and any repositories which were waiting keep waiting. Somewhat counterintuitively, hosted repositories write `TYPE_FETCH` status messages, so this should work equally well for hosted and observed repositories. This also paves the way for better backoff behavior on repository errors, described in T11665. The error backoff now uses the same logic that the standard backoff does. The next change will make backoff computation consider recent errors. (This is technically too large for repositories which have encountered one error and have a low commit rate, but I'll fix that in the following change; this is just a checkpoint on the way there.) Test Plan: Ran `bin/phd debug pull`, saw the daemon compute reasonable windows based on previous pull activity. Reviewers: chad Maniphest Tasks: T7865, T11665 Differential Revision: https://secure.phabricator.com/D16574 --- .../PhabricatorRepositoryPullLocalDaemon.php | 101 +++++++++++++----- .../storage/PhabricatorRepository.php | 2 +- 2 files changed, 74 insertions(+), 29 deletions(-) diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index bbd73d2d90..9cbb8db8e9 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -102,43 +102,58 @@ final class PhabricatorRepositoryPullLocalDaemon $retry_after, array_keys($pullable)); - // Figure out which repositories we need to queue for an update. foreach ($pullable as $id => $repository) { - $monogram = $repository->getMonogram(); + $now = PhabricatorTime::getNow(); + $display_name = $repository->getDisplayName(); if (isset($futures[$id])) { - $this->log(pht('Repository "%s" is currently updating.', $monogram)); + $this->log( + pht( + 'Repository "%s" is currently updating.', + $display_name)); continue; } if (isset($queue[$id])) { - $this->log(pht('Repository "%s" is already queued.', $monogram)); + $this->log( + pht( + 'Repository "%s" is already queued.', + $display_name)); continue; } - $after = idx($retry_after, $id, 0); + $after = idx($retry_after, $id); + if (!$after) { + $smart_wait = $repository->loadUpdateInterval($min_sleep); + $last_update = $this->loadLastUpdate($repository); + + $after = $last_update + $smart_wait; + $retry_after[$id] = $after; + + $this->log( + pht( + 'Scheduling repository "%s" with an update window of %s '. + 'second(s). Last update was %s second(s) ago.', + $display_name, + new PhutilNumber($smart_wait), + new PhutilNumber($now - $last_update))); + } + if ($after > time()) { $this->log( pht( 'Repository "%s" is not due for an update for %s second(s).', - $monogram, - new PhutilNumber($after - time()))); + $display_name, + new PhutilNumber($after - $now))); continue; } - if (!$after) { - $this->log( - pht( - 'Scheduling repository "%s" for an initial update.', - $monogram)); - } else { - $this->log( - pht( - 'Scheduling repository "%s" for an update (%s seconds overdue).', - $monogram, - new PhutilNumber(time() - $after))); - } + $this->log( + pht( + 'Scheduling repository "%s" for an update (%s seconds overdue).', + $display_name, + new PhutilNumber($now - $after))); $queue[$id] = $after; } @@ -157,8 +172,11 @@ final class PhabricatorRepositoryPullLocalDaemon continue; } - $monogram = $repository->getMonogram(); - $this->log(pht('Starting update for repository "%s".', $monogram)); + $display_name = $repository->getDisplayName(); + $this->log( + pht( + 'Starting update for repository "%s".', + $display_name)); unset($queue[$id]); $futures[$id] = $this->buildUpdateFuture( @@ -296,6 +314,32 @@ final class PhabricatorRepositoryPullLocalDaemon } + /** + * @task pull + */ + private function loadLastUpdate(PhabricatorRepository $repository) { + $table = new PhabricatorRepositoryStatusMessage(); + $conn = $table->establishConnection('r'); + + $epoch = queryfx_one( + $conn, + 'SELECT MAX(epoch) last_update FROM %T + WHERE repositoryID = %d + AND statusType IN (%Ls)', + $table->getTableName(), + $repository->getID(), + array( + PhabricatorRepositoryStatusMessage::TYPE_INIT, + PhabricatorRepositoryStatusMessage::TYPE_FETCH, + )); + + if ($epoch) { + return (int)$epoch['last_update']; + } + + return PhabricatorTime::getNow(); + } + /** * @task pull */ @@ -385,9 +429,9 @@ final class PhabricatorRepositoryPullLocalDaemon ExecFuture $future, $min_sleep) { - $monogram = $repository->getMonogram(); + $display_name = $repository->getDisplayName(); - $this->log(pht('Resolving update for "%s".', $monogram)); + $this->log(pht('Resolving update for "%s".', $display_name)); try { list($stdout, $stderr) = $future->resolvex(); @@ -395,17 +439,18 @@ final class PhabricatorRepositoryPullLocalDaemon $proxy = new PhutilProxyException( pht( 'Error while updating the "%s" repository.', - $repository->getMonogram()), + $display_name), $ex); phlog($proxy); - return time() + $min_sleep; + $smart_wait = $repository->loadUpdateInterval($min_sleep); + return PhabricatorTime::getNow() + $smart_wait; } if (strlen($stderr)) { $stderr_msg = pht( 'Unexpected output while updating repository "%s": %s', - $monogram, + $display_name, $stderr); phlog($stderr_msg); } @@ -416,10 +461,10 @@ final class PhabricatorRepositoryPullLocalDaemon pht( 'Based on activity in repository "%s", considering a wait of %s '. 'seconds before update.', - $repository->getMonogram(), + $display_name, new PhutilNumber($smart_wait))); - return time() + $smart_wait; + return PhabricatorTime::getNow() + $smart_wait; } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 3e59c7c53d..3ab3912f2c 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1783,7 +1783,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $smart_wait = $minimum; } - return $smart_wait; + return (int)$smart_wait; } From d3280c406d59a936bc7ca7a505fa9a05078704b6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Sep 2016 17:01:59 -0700 Subject: [PATCH 08/23] When repositories hit pull errors, stop updating them as frequently Summary: Ref T11665. Currently, when a repository hits an error, we retry it after 15s. This is correct if the error was temporary/transient/config-related (e.g., bad network or administrator setting up credentials) but not so great if the error is long-lasting (completely bad authentication, invalid URI, etc), as it can pile up to a meaningful amount of unnecessary load over time. Instead, record how many times in a row we've hit an error and adjust backoff behavior: first error is 15s, then 30s, 45s, etc. Additionally, when computing the backoff for an empty repository, use the repository creation time as though it was the most recent commit. This is a good proxy which gives us reasonable backoff behavior. This required removing the `CODE_WORKING` messages, since they would have reset the error count. We could restore them (as a different type of message), but I think they aren't particularly useful since cloning usually doesn't take too long and there's more status information avilable now than there was when this stuff was written. Test Plan: - Ran `bin/phd debug pull`. - Saw sensible, increasing backoffs selected for repositories with errors. - Saw sensible backoffs selected for empty repositories. Reviewers: chad Maniphest Tasks: T11665 Differential Revision: https://secure.phabricator.com/D16575 --- .../20160919.repo.messagecount.sql | 2 + ...ffusionRepositoryStatusManagementPanel.php | 9 +- .../PhabricatorRepositoryPullEngine.php | 2 - .../storage/PhabricatorRepository.php | 94 ++++++++++++++----- .../PhabricatorRepositoryStatusMessage.php | 3 +- 5 files changed, 73 insertions(+), 37 deletions(-) create mode 100644 resources/sql/autopatches/20160919.repo.messagecount.sql diff --git a/resources/sql/autopatches/20160919.repo.messagecount.sql b/resources/sql/autopatches/20160919.repo.messagecount.sql new file mode 100644 index 0000000000..a28bbb1e7e --- /dev/null +++ b/resources/sql/autopatches/20160919.repo.messagecount.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_statusmessage + ADD messageCount INT UNSIGNED NOT NULL; diff --git a/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php index 573162b927..0fb220c385 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php @@ -359,20 +359,13 @@ final class DiffusionRepositoryStatusManagementPanel return $view; } break; - case PhabricatorRepositoryStatusMessage::CODE_WORKING: + default: $view->addItem( id(new PHUIStatusItemView()) ->setIcon(PHUIStatusItemView::ICON_CLOCK, 'green') ->setTarget(pht('Initializing Working Copy')) ->setNote(pht('Daemons are initializing the working copy.'))); return $view; - default: - $view->addItem( - id(new PHUIStatusItemView()) - ->setIcon(PHUIStatusItemView::ICON_WARNING, 'red') - ->setTarget(pht('Unknown Init Status')) - ->setNote($message->getStatusCode())); - return $view; } } else { $view->addItem( diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 08fe8ea224..d7a403acf8 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -172,8 +172,6 @@ final class PhabricatorRepositoryPullEngine } private function logPull($message) { - $code_working = PhabricatorRepositoryStatusMessage::CODE_WORKING; - $this->updateRepositoryInitStatus($code_working, $message); $this->log('%s', $message); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 3ab3912f2c..ee8e0ad54d 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1649,21 +1649,33 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $this->getID(), $status_type); } else { + // If the existing message has the same code (e.g., we just hit an + // error and also previously hit an error) we increment the message + // count by 1. This allows us to determine how many times in a row + // we've run into an error. + queryfx( $conn_w, 'INSERT INTO %T - (repositoryID, statusType, statusCode, parameters, epoch) - VALUES (%d, %s, %s, %s, %d) + (repositoryID, statusType, statusCode, parameters, epoch, + messageCount) + VALUES (%d, %s, %s, %s, %d, %d) ON DUPLICATE KEY UPDATE statusCode = VALUES(statusCode), parameters = VALUES(parameters), - epoch = VALUES(epoch)', + epoch = VALUES(epoch), + messageCount = + IF( + statusCode = VALUES(statusCode), + messageCount + 1, + VALUES(messageCount))', $table_name, $this->getID(), $status_type, $status_code, json_encode($parameters), - time()); + time(), + 1); } return $this; @@ -1738,6 +1750,33 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO * @return int Repository update interval, in seconds. */ public function loadUpdateInterval($minimum = 15) { + // First, check if we've hit errors recently. If we have, wait one period + // for each consecutive error. Normally, this corresponds to a backoff of + // 15s, 30s, 45s, etc. + + $message_table = new PhabricatorRepositoryStatusMessage(); + $conn = $message_table->establishConnection('r'); + $error_count = queryfx_one( + $conn, + 'SELECT MAX(messageCount) error_count FROM %T + WHERE repositoryID = %d + AND statusType IN (%Ls) + AND statusCode IN (%Ls)', + $message_table->getTableName(), + $this->getID(), + array( + PhabricatorRepositoryStatusMessage::TYPE_INIT, + PhabricatorRepositoryStatusMessage::TYPE_FETCH, + ), + array( + PhabricatorRepositoryStatusMessage::CODE_ERROR, + )); + + $error_count = (int)$error_count['error_count']; + if ($error_count > 0) { + return (int)($minimum * $error_count); + } + // If a repository is still importing, always pull it as frequently as // possible. This prevents us from hanging for a long time at 99.9% when // importing an inactive repository. @@ -1758,31 +1797,34 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $window_start); if ($last_commit) { $time_since_commit = ($window_start - $last_commit['epoch']); - - $last_few_days = phutil_units('3 days in seconds'); - - if ($time_since_commit <= $last_few_days) { - // For repositories with activity in the recent past, we wait one - // extra second for every 10 minutes since the last commit. This - // shorter backoff is intended to handle weekends and other short - // breaks from development. - $smart_wait = ($time_since_commit / 600); - } else { - // For repositories without recent activity, we wait one extra second - // for every 4 minutes since the last commit. This longer backoff - // handles rarely used repositories, up to the maximum. - $smart_wait = ($time_since_commit / 240); - } - - // We'll never wait more than 6 hours to pull a repository. - $longest_wait = phutil_units('6 hours in seconds'); - $smart_wait = min($smart_wait, $longest_wait); - - $smart_wait = max($minimum, $smart_wait); } else { - $smart_wait = $minimum; + // If the repository has no commits, treat the creation date as + // though it were the date of the last commit. This makes empty + // repositories update quickly at first but slow down over time + // if they don't see any activity. + $time_since_commit = ($window_start - $this->getDateCreated()); } + $last_few_days = phutil_units('3 days in seconds'); + + if ($time_since_commit <= $last_few_days) { + // For repositories with activity in the recent past, we wait one + // extra second for every 10 minutes since the last commit. This + // shorter backoff is intended to handle weekends and other short + // breaks from development. + $smart_wait = ($time_since_commit / 600); + } else { + // For repositories without recent activity, we wait one extra second + // for every 4 minutes since the last commit. This longer backoff + // handles rarely used repositories, up to the maximum. + $smart_wait = ($time_since_commit / 240); + } + + // We'll never wait more than 6 hours to pull a repository. + $longest_wait = phutil_units('6 hours in seconds'); + $smart_wait = min($smart_wait, $longest_wait); + $smart_wait = max($minimum, $smart_wait); + return (int)$smart_wait; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryStatusMessage.php b/src/applications/repository/storage/PhabricatorRepositoryStatusMessage.php index 79d75cc50c..a9a1b1ae8c 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryStatusMessage.php +++ b/src/applications/repository/storage/PhabricatorRepositoryStatusMessage.php @@ -8,7 +8,6 @@ final class PhabricatorRepositoryStatusMessage const TYPE_NEEDS_UPDATE = 'needs-update'; const CODE_ERROR = 'error'; - const CODE_WORKING = 'working'; const CODE_OKAY = 'okay'; protected $repositoryID; @@ -16,6 +15,7 @@ final class PhabricatorRepositoryStatusMessage protected $statusCode; protected $parameters = array(); protected $epoch; + protected $messageCount; protected function getConfiguration() { return array( @@ -26,6 +26,7 @@ final class PhabricatorRepositoryStatusMessage self::CONFIG_COLUMN_SCHEMA => array( 'statusType' => 'text32', 'statusCode' => 'text32', + 'messageCount' => 'uint32', ), self::CONFIG_KEY_SCHEMA => array( 'repositoryID' => array( From 51f8ec4487be02e290b1182a7cd5b4069fe7d49f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Sep 2016 19:43:24 -0700 Subject: [PATCH 09/23] Add a default value for messageCount so writes from old tiers survive the update query Auditors: chad --- resources/sql/autopatches/20160919.repo.messagedefault.sql | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 resources/sql/autopatches/20160919.repo.messagedefault.sql diff --git a/resources/sql/autopatches/20160919.repo.messagedefault.sql b/resources/sql/autopatches/20160919.repo.messagedefault.sql new file mode 100644 index 0000000000..0c8b84d44a --- /dev/null +++ b/resources/sql/autopatches/20160919.repo.messagedefault.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_statusmessage + CHANGE messageCount messageCount INT UNSIGNED NOT NULL DEFAULT 0; From 03d323e9fdc2482b0f5cce149809666347aa2e87 Mon Sep 17 00:00:00 2001 From: Daniel Stone Date: Tue, 20 Sep 2016 10:47:30 +0000 Subject: [PATCH 10/23] Fix config-migration text for dashboard options Summary: The commit which added checks for the old homepage options (now in Dashboard) in rP9d9a47e9cf, added them to the auth section, where they would present: This option has been migrated to the "Auth" application. Your old configuration is still in effect, but now stored in "Auth" instead of configuration. Going forward, you can manage authentication from the web UI. Remove them from the moved-to-Auth list, and coalesce the multiple definitions of the help text into one. Test Plan: - set maniphest.priorities.unbreak-now to something - observe the setup issue reported - hope it tells you the right thing Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley, chad Differential Revision: https://secure.phabricator.com/D16576 --- .../PhabricatorExtraConfigSetupCheck.php | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index e32d7a1476..f28a6ab648 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -143,9 +143,6 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'phabricator.auth-permanent', 'phabricator.application-id', 'phabricator.application-secret', - 'maniphest.priorities.unbreak-now', - 'maniphest.priorities.needs-triage', - 'welcome.html', ); $ancient_config = array_fill_keys($auth_config, $reason_auth); @@ -197,6 +194,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'The "Re: Prefix" and "Vary Subjects" settings are now configured '. 'in global settings.'); + $dashboard_reason = pht( + 'This option has been removed, you can use Dashboards to provide '. + 'homepage customization. See T11533 for more details.'); + $ancient_config += array( 'phid.external-loaders' => pht( @@ -336,17 +337,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'This option has been replaced with `ui.logo`, which provides more '. 'flexible configuration options.'), - 'welcome.html' => pht( - 'This option has been removed, you can use Dashboards to provide '. - 'homepage customization. See T11533 for more details.'), - - 'maniphest.priorities.unbreak-now' => pht( - 'This option has been removed, you can use Dashboards to provide '. - 'homepage customization. See T11533 for more details.'), - - 'maniphest.priorities.needs-triage' => pht( - 'This option has been removed, you can use Dashboards to provide '. - 'homepage customization. See T11533 for more details.'), + 'welcome.html' => $dashboard_reason, + 'maniphest.priorities.unbreak-now' => $dashboard_reason, + 'maniphest.priorities.needs-triage' => $dashboard_reason, ); From 9329e6a12d9f9fdc0cc04b2667d1412f627ddedf Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Sep 2016 05:57:52 -0700 Subject: [PATCH 11/23] Stop doing an excessive amount of work in `diffusion.rawdiffquery` Ref T11665. Without `-n 1`, this logs the ENTIRE history of the repository. We actually get the right result, but this is egregiously slow. Add `-n 1` to return only one result. It appears that I wrote this wrong way back in 2011, in D953. This query is rarely used (until recently) which is likely why it has escaped notice for so long. Test Plan: Used Conduit console to execute `diffusion.rawdiffquery`. Got the same results but spent 8ms instead of 200ms executing this command, in a very small repository. --- .../diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php index 18e5a9ec9a..41d91c00ca 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php @@ -23,7 +23,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { // Check if this is the root commit by seeing if it has parents, since // `git diff X^ X` does not work if "X" is the initial commit. list($parents) = $repository->execxLocalCommand( - 'log --format=%s %s --', + 'log -n 1 --format=%s %s --', '%P', $commit); From 0817eb14a94becd8478033742e339aef9ed6520d Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Mon, 19 Sep 2016 13:45:12 -0400 Subject: [PATCH 12/23] Update Phurl to use EditEngine Summary: Fixes T10673. Set up Phurl to use Edit Engine. There's no way this is all I needed to do to get it working, so I'll be making another pass at it and testing more thoroughly... Test Plan: Ran through the Phurl URL creation/edit/deletion process. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley, yelirekim Maniphest Tasks: T10673 Differential Revision: https://secure.phabricator.com/D16573 --- src/__phutil_library_map__.php | 2 + .../PhabricatorPasteApplication.php | 1 - .../PhabricatorPhurlApplication.php | 4 +- .../controller/PhabricatorPhurlController.php | 13 +- .../PhabricatorPhurlURLEditController.php | 261 +----------------- .../editor/PhabricatorPhurlURLEditEngine.php | 104 +++++++ 6 files changed, 113 insertions(+), 272 deletions(-) create mode 100644 src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dafd4928e7..646efbdd80 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3184,6 +3184,7 @@ phutil_register_library_map(array( 'PhabricatorPhurlURLCommentController' => 'applications/phurl/controller/PhabricatorPhurlURLCommentController.php', 'PhabricatorPhurlURLCreateCapability' => 'applications/phurl/capability/PhabricatorPhurlURLCreateCapability.php', 'PhabricatorPhurlURLEditController' => 'applications/phurl/controller/PhabricatorPhurlURLEditController.php', + 'PhabricatorPhurlURLEditEngine' => 'applications/phurl/editor/PhabricatorPhurlURLEditEngine.php', 'PhabricatorPhurlURLEditor' => 'applications/phurl/editor/PhabricatorPhurlURLEditor.php', 'PhabricatorPhurlURLListController' => 'applications/phurl/controller/PhabricatorPhurlURLListController.php', 'PhabricatorPhurlURLMailReceiver' => 'applications/phurl/mail/PhabricatorPhurlURLMailReceiver.php', @@ -8097,6 +8098,7 @@ phutil_register_library_map(array( 'PhabricatorPhurlURLCommentController' => 'PhabricatorPhurlController', 'PhabricatorPhurlURLCreateCapability' => 'PhabricatorPolicyCapability', 'PhabricatorPhurlURLEditController' => 'PhabricatorPhurlController', + 'PhabricatorPhurlURLEditEngine' => 'PhabricatorEditEngine', 'PhabricatorPhurlURLEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorPhurlURLListController' => 'PhabricatorPhurlController', 'PhabricatorPhurlURLMailReceiver' => 'PhabricatorObjectMailReceiver', diff --git a/src/applications/paste/application/PhabricatorPasteApplication.php b/src/applications/paste/application/PhabricatorPasteApplication.php index d644224e6e..26e4b1740e 100644 --- a/src/applications/paste/application/PhabricatorPasteApplication.php +++ b/src/applications/paste/application/PhabricatorPasteApplication.php @@ -38,7 +38,6 @@ final class PhabricatorPasteApplication extends PhabricatorApplication { => 'PhabricatorPasteViewController', '/paste/' => array( '(query/(?P[^/]+)/)?' => 'PhabricatorPasteListController', - 'create/' => 'PhabricatorPasteEditController', $this->getEditRoutePattern('edit/') => 'PhabricatorPasteEditController', 'raw/(?P[1-9]\d*)/' => 'PhabricatorPasteRawController', 'archive/(?P[1-9]\d*)/' => 'PhabricatorPasteArchiveController', diff --git a/src/applications/phurl/application/PhabricatorPhurlApplication.php b/src/applications/phurl/application/PhabricatorPhurlApplication.php index 59f8f1546f..38d5d4459a 100644 --- a/src/applications/phurl/application/PhabricatorPhurlApplication.php +++ b/src/applications/phurl/application/PhabricatorPhurlApplication.php @@ -46,9 +46,7 @@ final class PhabricatorPhurlApplication extends PhabricatorApplication { '(?:query/(?P[^/]+)/)?' => 'PhabricatorPhurlURLListController', 'url/' => array( - 'create/' - => 'PhabricatorPhurlURLEditController', - 'edit/(?P[1-9]\d*)/' + $this->getEditRoutePattern('edit/') => 'PhabricatorPhurlURLEditController', 'comment/(?P[1-9]\d*)/' => 'PhabricatorPhurlURLCommentController', diff --git a/src/applications/phurl/controller/PhabricatorPhurlController.php b/src/applications/phurl/controller/PhabricatorPhurlController.php index b85e0bbbbe..84b6a07a90 100644 --- a/src/applications/phurl/controller/PhabricatorPhurlController.php +++ b/src/applications/phurl/controller/PhabricatorPhurlController.php @@ -3,17 +3,10 @@ abstract class PhabricatorPhurlController extends PhabricatorController { protected function buildApplicationCrumbs() { - $can_create = $this->hasApplicationCapability( - PhabricatorPhurlURLCreateCapability::CAPABILITY); - $crumbs = parent::buildApplicationCrumbs(); - $crumbs->addAction( - id(new PHUIListItemView()) - ->setName(pht('Shorten URL')) - ->setHref($this->getApplicationURI().'url/create/') - ->setIcon('fa-plus-square') - ->setDisabled(!$can_create) - ->setWorkflow(!$can_create)); + id(new PhabricatorPhurlURLEditEngine()) + ->setViewer($this->getViewer()) + ->addActionToCrumbs($crumbs); return $crumbs; } diff --git a/src/applications/phurl/controller/PhabricatorPhurlURLEditController.php b/src/applications/phurl/controller/PhabricatorPhurlURLEditController.php index 73ba3474fe..2464c0af37 100644 --- a/src/applications/phurl/controller/PhabricatorPhurlURLEditController.php +++ b/src/applications/phurl/controller/PhabricatorPhurlURLEditController.php @@ -4,263 +4,8 @@ final class PhabricatorPhurlURLEditController extends PhabricatorPhurlController { public function handleRequest(AphrontRequest $request) { - $id = $request->getURIData('id'); - $is_create = !$id; - - $viewer = $this->getViewer(); - $user_phid = $viewer->getPHID(); - $error_long_url = true; - $error_alias = null; - $validation_exception = null; - - $next_workflow = $request->getStr('next'); - $uri_query = $request->getStr('query'); - - if ($is_create) { - $this->requireApplicationCapability( - PhabricatorPhurlURLCreateCapability::CAPABILITY); - - $url = PhabricatorPhurlURL::initializeNewPhurlURL( - $viewer); - $submit_label = pht('Create'); - $page_title = pht('Shorten URL'); - $header_icon = 'fa-plus-square'; - $subscribers = array(); - $cancel_uri = $this->getApplicationURI(); - } else { - $url = id(new PhabricatorPhurlURLQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - - if (!$url) { - return new Aphront404Response(); - } - - $submit_label = pht('Update'); - $page_title = pht('Edit URL: %s', $url->getName()); - $header_icon = 'fa-pencil'; - - $subscribers = PhabricatorSubscribersQuery::loadSubscribersForPHID( - $url->getPHID()); - - $cancel_uri = '/U'.$url->getID(); - } - - if ($is_create) { - $projects = array(); - } else { - $projects = PhabricatorEdgeQuery::loadDestinationPHIDs( - $url->getPHID(), - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); - $projects = array_reverse($projects); - } - - $name = $url->getName(); - $long_url = $url->getLongURL(); - $alias = $url->getAlias(); - $description = $url->getDescription(); - $edit_policy = $url->getEditPolicy(); - $view_policy = $url->getViewPolicy(); - $space = $url->getSpacePHID(); - - if ($request->isFormPost()) { - $xactions = array(); - $name = $request->getStr('name'); - $long_url = $request->getStr('longURL'); - $alias = $request->getStr('alias'); - $projects = $request->getArr('projects'); - $description = $request->getStr('description'); - $subscribers = $request->getArr('subscribers'); - $edit_policy = $request->getStr('editPolicy'); - $view_policy = $request->getStr('viewPolicy'); - $space = $request->getStr('spacePHID'); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType( - PhabricatorPhurlURLTransaction::TYPE_NAME) - ->setNewValue($name); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType( - PhabricatorPhurlURLTransaction::TYPE_URL) - ->setNewValue($long_url); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType( - PhabricatorPhurlURLTransaction::TYPE_ALIAS) - ->setNewValue($alias); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType( - PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue(array('=' => array_fuse($subscribers))); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType( - PhabricatorPhurlURLTransaction::TYPE_DESCRIPTION) - ->setNewValue($description); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY) - ->setNewValue($view_policy); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY) - ->setNewValue($edit_policy); - - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_SPACE) - ->setNewValue($space); - - $editor = id(new PhabricatorPhurlURLEditor()) - ->setActor($viewer) - ->setContentSourceFromRequest($request) - ->setContinueOnNoEffect(true); - - try { - $proj_edge_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; - $xactions[] = id(new PhabricatorPhurlURLTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) - ->setMetadataValue('edge:type', $proj_edge_type) - ->setNewValue(array('=' => array_fuse($projects))); - - $xactions = $editor->applyTransactions($url, $xactions); - return id(new AphrontRedirectResponse()) - ->setURI($url->getURI()); - } catch (PhabricatorApplicationTransactionValidationException $ex) { - $validation_exception = $ex; - $error_long_url = $ex->getShortMessage( - PhabricatorPhurlURLTransaction::TYPE_URL); - $error_alias = $ex->getShortMessage( - PhabricatorPhurlURLTransaction::TYPE_ALIAS); - } - } - - $current_policies = id(new PhabricatorPolicyQuery()) - ->setViewer($viewer) - ->setObject($url) - ->execute(); - - $name = id(new AphrontFormTextControl()) - ->setLabel(pht('Name')) - ->setName('name') - ->setValue($name); - - $long_url = id(new AphrontFormTextControl()) - ->setLabel(pht('URL')) - ->setName('longURL') - ->setValue($long_url) - ->setError($error_long_url); - - $alias = id(new AphrontFormTextControl()) - ->setLabel(pht('Alias')) - ->setName('alias') - ->setValue($alias) - ->setError($error_alias); - - $projects = id(new AphrontFormTokenizerControl()) - ->setLabel(pht('Tags')) - ->setName('projects') - ->setValue($projects) - ->setUser($viewer) - ->setDatasource(new PhabricatorProjectDatasource()); - - $description = id(new PhabricatorRemarkupControl()) - ->setLabel(pht('Description')) - ->setName('description') - ->setValue($description) - ->setUser($viewer); - - $view_policies = id(new AphrontFormPolicyControl()) - ->setUser($viewer) - ->setValue($view_policy) - ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) - ->setPolicyObject($url) - ->setPolicies($current_policies) - ->setSpacePHID($space) - ->setName('viewPolicy'); - $edit_policies = id(new AphrontFormPolicyControl()) - ->setUser($viewer) - ->setValue($edit_policy) - ->setCapability(PhabricatorPolicyCapability::CAN_EDIT) - ->setPolicyObject($url) - ->setPolicies($current_policies) - ->setName('editPolicy'); - - $subscribers = id(new AphrontFormTokenizerControl()) - ->setLabel(pht('Subscribers')) - ->setName('subscribers') - ->setValue($subscribers) - ->setUser($viewer) - ->setDatasource(new PhabricatorMetaMTAMailableDatasource()); - - $form = id(new AphrontFormView()) - ->setUser($viewer) - ->appendChild($name) - ->appendChild($long_url) - ->appendChild($alias) - ->appendControl($view_policies) - ->appendControl($edit_policies) - ->appendControl($subscribers) - ->appendChild($projects) - ->appendChild($description); - - - if ($request->isAjax()) { - return $this->newDialog() - ->setTitle($page_title) - ->setWidth(AphrontDialogView::WIDTH_FULL) - ->appendForm($form) - ->addCancelButton($cancel_uri) - ->addSubmitButton($submit_label); - } - - $submit = id(new AphrontFormSubmitControl()) - ->addCancelButton($cancel_uri) - ->setValue($submit_label); - - $form->appendChild($submit); - - $form_box = id(new PHUIObjectBoxView()) - ->setHeaderText($page_title) - ->setForm($form); - - $crumbs = $this->buildApplicationCrumbs(); - - if (!$is_create) { - $crumbs->addTextCrumb($url->getMonogram(), $url->getURI()); - } else { - $crumbs->addTextCrumb(pht('Create URL')); - } - - $crumbs->addTextCrumb($page_title); - $crumbs->setBorder(true); - - $object_box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('URL')) - ->setValidationException($validation_exception) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($form); - - $header = id(new PHUIHeaderView()) - ->setHeader($page_title) - ->setHeaderIcon($header_icon); - - $view = id(new PHUITwoColumnView()) - ->setHeader($header) - ->setFooter(array( - $object_box, - )); - - return $this->newPage() - ->setTitle($page_title) - ->setCrumbs($crumbs) - ->appendChild($view); + return id(new PhabricatorPhurlURLEditEngine()) + ->setController($this) + ->buildResponse(); } } diff --git a/src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php b/src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php new file mode 100644 index 0000000000..3efabcceee --- /dev/null +++ b/src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php @@ -0,0 +1,104 @@ +getViewer()); + } + + protected function newObjectQuery() { + return new PhabricatorPhurlURLQuery(); + } + + protected function getObjectCreateTitleText($object) { + return pht('Create New URL'); + } + + protected function getObjectEditTitleText($object) { + return pht('Edit URL: %s', $object->getName()); + } + + protected function getObjectEditShortText($object) { + return $object->getName(); + } + + protected function getObjectCreateShortText() { + return pht('Create URL'); + } + + protected function getObjectName() { + return pht('URL'); + } + + protected function getObjectCreateCancelURI($object) { + return $this->getApplication()->getApplicationURI('/'); + } + + protected function getEditorURI() { + return $this->getApplication()->getApplicationURI('url/edit/'); + } + + protected function getObjectViewURI($object) { + return $object->getURI(); + } + + protected function getCreateNewObjectPolicy() { + return $this->getApplication()->getPolicy( + PhabricatorPhurlURLCreateCapability::CAPABILITY); + } + + protected function buildCustomEditFields($object) { + + return array( + id(new PhabricatorTextEditField()) + ->setKey('name') + ->setLabel(pht('Name')) + ->setDescription(pht('URL name.')) + ->setConduitTypeDescription(pht('New URL name.')) + ->setTransactionType(PhabricatorPhurlURLTransaction::TYPE_NAME) + ->setValue($object->getName()), + id(new PhabricatorTextEditField()) + ->setKey('url') + ->setLabel(pht('URL')) + ->setDescription(pht('The URL to shorten.')) + ->setConduitTypeDescription(pht('New URL.')) + ->setValue($object->getLongURL()) + ->setIsRequired(true) + ->setTransactionType(PhabricatorPhurlURLTransaction::TYPE_URL), + id(new PhabricatorTextEditField()) + ->setKey('alias') + ->setLabel(pht('Alias')) + ->setTransactionType(PhabricatorPhurlURLTransaction::TYPE_ALIAS) + ->setDescription(pht('The alias to give the URL.')) + ->setConduitTypeDescription(pht('New alias.')) + ->setValue($object->getAlias()), + id(new PhabricatorRemarkupEditField()) + ->setKey('description') + ->setLabel(pht('Description')) + ->setDescription(pht('URL long description.')) + ->setConduitTypeDescription(pht('New URL description.')) + ->setTransactionType(PhabricatorPhurlURLTransaction::TYPE_DESCRIPTION) + ->setValue($object->getDescription()), + ); + } + +} From f8c22252686e5f53cc0cae26d990eb89aef09f72 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 20 Sep 2016 07:30:42 -0700 Subject: [PATCH 13/23] Use persistent database connections from web contexts Summary: Ref T11672. Depends on D16577. When establishing a connection from a webserver context, try to use persistent connections. The hope is that this will fix outbound port exhaustion issues experienced on repository hosts handling large queue volumes. Test Plan: Added this to a page: ```lang=php $tables = array( new PhabricatorUser(), new ManiphestTask(), new DifferentialRevision(), new PhabricatorRepository(), new PhabricatorPaste(), ); $ids = array(); foreach ($tables as $table) { $conn = $table->establishConnection('r'); $cid = queryfx_one( $conn, 'SELECT CONNECTION_ID() cid'); $ids[get_class($table)] = $cid['cid']; } var_dump($ids); ``` Reloaded the page a bunch of times and saw no reissued connections (the pool seems to keep a particular connection bound to a particular database), but did see connection reuse across requests. That is, across reloads the same connection IDs appeared, but the same connection ID never appeared twice in the same request. This is what we want. Also googled for issues with persistent connections, but everything I found was unconcerning and obscure (local variables and other very complex state that we don't use), and a bunch of the docs are reassuring (transactions, etc., get reset properly). Reviewers: chad Reviewed By: chad Maniphest Tasks: T11672 Differential Revision: https://secure.phabricator.com/D16578 --- src/infrastructure/storage/lisk/PhabricatorLiskDAO.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php index 2fbabf037a..feae254095 100644 --- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php +++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php @@ -75,10 +75,13 @@ abstract class PhabricatorLiskDAO extends LiskDAO { $connection->setReadOnly(true); } - // Unless this is a script running from the CLI, prevent any query from - // running for more than 30 seconds. See T10849 for discussion. + // Unless this is a script running from the CLI: + // - (T10849) Prevent any query from running for more than 30 seconds. + // - (T11672) Use persistent connections. if (php_sapi_name() != 'cli') { - $connection->setQueryTimeout(30); + $connection + ->setQueryTimeout(30) + ->setPersistent(true); } return $connection; From af218564e51a131938245a4813a2024ab3c8982d Mon Sep 17 00:00:00 2001 From: Brendan Zerr Date: Wed, 21 Sep 2016 15:02:45 -0700 Subject: [PATCH 14/23] Backport fix from php-mime-mail-parser to fix attachment parsing Summary: - Allow proper parsing of attachments with missing Content-Disposition header Test Plan: - Create application email for Maniphest. - Send example broken email from Outlook 2007 to that address {F1842816} Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D16584 --- .../mimemailparser/MimeMailParser.class.php | 102 +++++++++++------- 1 file changed, 62 insertions(+), 40 deletions(-) diff --git a/externals/mimemailparser/MimeMailParser.class.php b/externals/mimemailparser/MimeMailParser.class.php index a192f2b266..914f50888e 100644 --- a/externals/mimemailparser/MimeMailParser.class.php +++ b/externals/mimemailparser/MimeMailParser.class.php @@ -111,13 +111,13 @@ class MimeMailParser { * @param $data String */ public function setText($data) { - // NOTE: This has been modified for Phabricator. If the input data does not - // end in a newline, Mailparse fails to include the last line in the mail - // body. This happens somewhere deep, deep inside the mailparse extension, - // so adding a newline here seems like the most straightforward fix. - if (!preg_match('/\n\z/', $data)) { - $data = $data."\n"; - } + // NOTE: This has been modified for Phabricator. If the input data does not + // end in a newline, Mailparse fails to include the last line in the mail + // body. This happens somewhere deep, deep inside the mailparse extension, + // so adding a newline here seems like the most straightforward fix. + if (!preg_match('/\n\z/', $data)) { + $data = $data."\n"; + } $this->resource = mailparse_msg_create(); // does not parse incrementally, fast memory hog might explode @@ -203,23 +203,23 @@ class MimeMailParser { ); if (in_array($type, array_keys($mime_types))) { foreach($this->parts as $part) { - $disposition = $this->getPartContentDisposition($part); - if ($disposition == 'attachment') { - // text/plain parts with "Content-Disposition: attachment" are - // attachments, not part of the text body. - continue; - } + $disposition = $this->getPartContentDisposition($part); + if ($disposition == 'attachment') { + // text/plain parts with "Content-Disposition: attachment" are + // attachments, not part of the text body. + continue; + } if ($this->getPartContentType($part) == $mime_types[$type]) { - $headers = $this->getPartHeaders($part); - // Concatenate all the matching parts into the body text. For example, - // if a user sends a message with some text, then an image, and then - // some more text, the text body of the email gets split over several - // attachments. + $headers = $this->getPartHeaders($part); + // Concatenate all the matching parts into the body text. For example, + // if a user sends a message with some text, then an image, and then + // some more text, the text body of the email gets split over several + // attachments. $body .= $this->decode( $this->getPartBody($part), array_key_exists('content-transfer-encoding', $headers) - ? $headers['content-transfer-encoding'] - : ''); + ? $headers['content-transfer-encoding'] + : ''); } } } else { @@ -251,20 +251,42 @@ class MimeMailParser { return $headers; } - /** * Returns the attachments contents in order of appearance * @return Array * @param $type Object[optional] */ public function getAttachments() { + // NOTE: This has been modified for Phabricator. Some mail clients do not + // send attachments with "Content-Disposition" headers. $attachments = array(); $dispositions = array("attachment","inline"); - foreach($this->parts as $part) { + $non_attachment_types = array("text/plain", "text/html"); + $nonameIter = 0; + foreach ($this->parts as $part) { $disposition = $this->getPartContentDisposition($part); - if (in_array($disposition, $dispositions)) { + $filename = 'noname'; + if (isset($part['disposition-filename'])) { + $filename = $part['disposition-filename']; + } elseif (isset($part['content-name'])) { + // if we have no disposition but we have a content-name, it's a valid attachment. + // we simulate the presence of an attachment disposition with a disposition filename + $filename = $part['content-name']; + $disposition = 'attachment'; + } elseif (!in_array($part['content-type'], $non_attachment_types, true) + && substr($part['content-type'], 0, 10) !== 'multipart/' + ) { + // if we cannot get it with getMessageBody, we assume it is an attachment + $disposition = 'attachment'; + } + + if (in_array($disposition, $dispositions) && isset($filename) === true) { + if ($filename == 'noname') { + $nonameIter++; + $filename = 'noname'.$nonameIter; + } $attachments[] = new MimeMailParser_attachment( - $part['disposition-filename'], + $filename, $this->getPartContentType($part), $this->getAttachmentStream($part), $disposition, @@ -413,7 +435,7 @@ class MimeMailParser { private function getAttachmentStream(&$part) { $temp_fp = tmpfile(); - array_key_exists('content-transfer-encoding', $part['headers']) ? $encoding = $part['headers']['content-transfer-encoding'] : $encoding = ''; + array_key_exists('content-transfer-encoding', $part['headers']) ? $encoding = $part['headers']['content-transfer-encoding'] : $encoding = ''; if ($temp_fp) { if ($this->stream) { @@ -445,21 +467,21 @@ class MimeMailParser { } - /** - * Decode the string depending on encoding type. - * @return String the decoded string. - * @param $encodedString The string in its original encoded state. - * @param $encodingType The encoding type from the Content-Transfer-Encoding header of the part. - */ - private function decode($encodedString, $encodingType) { - if (strtolower($encodingType) == 'base64') { - return base64_decode($encodedString); - } else if (strtolower($encodingType) == 'quoted-printable') { - return quoted_printable_decode($encodedString); - } else { - return $encodedString; - } - } + /** + * Decode the string depending on encoding type. + * @return String the decoded string. + * @param $encodedString The string in its original encoded state. + * @param $encodingType The encoding type from the Content-Transfer-Encoding header of the part. + */ + private function decode($encodedString, $encodingType) { + if (strtolower($encodingType) == 'base64') { + return base64_decode($encodedString); + } else if (strtolower($encodingType) == 'quoted-printable') { + return quoted_printable_decode($encodedString); + } else { + return $encodedString; + } + } } From 8941bbfcea7d333482a93ba052824c8dc5e0f230 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Sep 2016 14:30:59 -0700 Subject: [PATCH 15/23] Make "text" custom fields appear in ApplicationSearch again Summary: Fixes T11675. This capability was erroneously (probably?) removed in D14766. This search implementation (which uses exact match) probably isn't perfect for all cases of "text" fields, but empirically it seems to be what a significant number of users are after. Test Plan: Searched for a custom text field value. {F1843383} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11675 Differential Revision: https://secure.phabricator.com/D16582 --- .../standard/PhabricatorStandardCustomFieldText.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php index 8a4ae97bcf..8242c477fd 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php @@ -72,10 +72,6 @@ final class PhabricatorStandardCustomFieldText return new AphrontStringHTTPParameterType(); } - public function shouldAppearInApplicationSearch() { - return false; - } - public function getConduitEditParameterType() { return new ConduitStringParameterType(); } From db2425b300fb167f13fd004316e147a3e791fae5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Sep 2016 15:16:14 -0700 Subject: [PATCH 16/23] Do initial repository imports at a lower priority and finish importing commits before starting new ones Summary: Fixes T11677. This makes two minor adjustments to the repository import daemons: - The first step ("Message") now queues at a slightly-lower-than-default (for already-imported repositories) or very-low (for newly importing repositories) priority level. - The other steps now queue at "default" priority level. This is actually what they already did, but without this change their behavior would be to inherit the priority level of their parents. This has two effects: - When adding new repositories to an existing install, they shouldn't block other things from happening anymore. - The daemons will tend to start one commit and run through all of its steps before starting another commit. This makes progress through the queue more even and predictable. - Before, they did ALL the message tasks, then ALL the change tasks, etc. This works fine but is confusing/uneven/less-predictable because each type of task takes a different amount of time. Test Plan: - Added a new repository. - Saw all of its "message" steps queue at priority 4000. - Saw followups queue at priority 2000. - Saw progress generally "finish what you started" -- go through the queue one commit at a time, instead of one type of task at a time. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11677 Differential Revision: https://secure.phabricator.com/D16585 --- .../PhabricatorRepositoryDiscoveryEngine.php | 27 ++++++++++++++++++- ...torRepositoryCommitMessageParserWorker.php | 8 ++++++ .../daemon/workers/PhabricatorWorker.php | 1 + 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index 9ccc72b8de..c36f5a7199 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -682,7 +682,32 @@ final class PhabricatorRepositoryDiscoveryEngine $data['commitID'] = $commit->getID(); - PhabricatorWorker::scheduleTask($class, $data); + // If the repository is importing for the first time, we schedule tasks + // at IMPORT priority, which is very low. Making progress on importing a + // new repository for the first time is less important than any other + // daemon task. + + // If the repostitory has finished importing and we're just catching up + // on recent commits, we schedule discovery at COMMIT priority, which is + // slightly below the default priority. + + // Note that followup tasks and triggered tasks (like those generated by + // Herald or Harbormaster) will queue at DEFAULT priority, so that each + // commit tends to fully import before we start the next one. This tends + // to give imports fairly predictable progress. See T11677 for some + // discussion. + + if ($repository->isImporting()) { + $task_priority = PhabricatorWorker::PRIORITY_IMPORT; + } else { + $task_priority = PhabricatorWorker::PRIORITY_COMMIT; + } + + $options = array( + 'priority' => $task_priority, + ); + + PhabricatorWorker::scheduleTask($class, $data, $options); } private function isInitialImport(array $refs) { diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index e28cd78cc7..f340fbf91f 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -47,6 +47,14 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $this->getFollowupTaskClass(), array( 'commitID' => $commit->getID(), + ), + array( + // We queue followup tasks at default priority so that the queue + // finishes work it has started before starting more work. If + // followups are queued at the same priority level, we do all + // message parses first, then all change parses, etc. This makes + // progress uneven. See T11677 for discussion. + 'priority' => PhabricatorWorker::PRIORITY_DEFAULT, )); } } diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php index b974f9989d..1b9821b68d 100644 --- a/src/infrastructure/daemon/workers/PhabricatorWorker.php +++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php @@ -16,6 +16,7 @@ abstract class PhabricatorWorker extends Phobject { const PRIORITY_ALERTS = 1000; const PRIORITY_DEFAULT = 2000; + const PRIORITY_COMMIT = 2500; const PRIORITY_BULK = 3000; const PRIORITY_IMPORT = 4000; From 66c7f22c273c0bcd351095c3d6c8cb4a85a09f8f Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Sep 2016 14:41:52 -0700 Subject: [PATCH 17/23] Truncate and scroll task graph tables instead of fitting task titles to the display Summary: Fixes T11676. Instead of trying to fit task titles to the display, truncate them and let the table scroll. Test Plan: Table now scrolls when cramped: {F1843396} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11676 Differential Revision: https://secure.phabricator.com/D16583 --- resources/celerity/map.php | 6 +++--- src/infrastructure/graph/ManiphestTaskGraph.php | 11 ++++++++--- webroot/rsrc/css/aphront/table-view.css | 4 ++++ 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d4fd4641a9..802fe48b7f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '80a3fcb3', 'conpherence.pkg.js' => '89b4837e', - 'core.pkg.css' => 'eb1298d4', + 'core.pkg.css' => 'f7b03076', 'core.pkg.js' => '1d376fa9', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '3fb7f532', @@ -27,7 +27,7 @@ return array( 'rsrc/css/aphront/notification.css' => '3f6c89c9', 'rsrc/css/aphront/panel-view.css' => '8427b78d', 'rsrc/css/aphront/phabricator-nav-view.css' => 'b29426e9', - 'rsrc/css/aphront/table-view.css' => '832656fd', + 'rsrc/css/aphront/table-view.css' => '3225137a', 'rsrc/css/aphront/tokenizer.css' => '056da01b', 'rsrc/css/aphront/tooltip.css' => '1a07aea8', 'rsrc/css/aphront/typeahead-browse.css' => '8904346a', @@ -607,7 +607,7 @@ return array( 'aphront-list-filter-view-css' => '5d6f0526', 'aphront-multi-column-view-css' => 'fd18389d', 'aphront-panel-view-css' => '8427b78d', - 'aphront-table-view-css' => '832656fd', + 'aphront-table-view-css' => '3225137a', 'aphront-tokenizer-control-css' => '056da01b', 'aphront-tooltip-css' => '1a07aea8', 'aphront-typeahead-control-css' => 'd4f16145', diff --git a/src/infrastructure/graph/ManiphestTaskGraph.php b/src/infrastructure/graph/ManiphestTaskGraph.php index 2fe62af599..6e92feb241 100644 --- a/src/infrastructure/graph/ManiphestTaskGraph.php +++ b/src/infrastructure/graph/ManiphestTaskGraph.php @@ -51,12 +51,19 @@ final class ManiphestTaskGraph $assigned = phutil_tag('em', array(), pht('None')); } + $full_title = $object->getTitle(); + + $title = id(new PhutilUTF8StringTruncator()) + ->setMaximumGlyphs(80) + ->truncateString($full_title); + $link = phutil_tag( 'a', array( 'href' => $object->getURI(), + 'title' => $full_title, ), - $object->getTitle()); + $title); $link = array( phutil_tag( @@ -95,8 +102,6 @@ final class ManiphestTaskGraph )); } - $link = AphrontTableView::renderSingleDisplayLine($link); - return array( $marker, $trace, diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css index bf40c0e352..980118d31a 100644 --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -228,6 +228,10 @@ span.single-display-line-content { position: static; } +.aphront-table-view td.object-link { + white-space: nowrap; +} + .aphront-table-view tr.closed td.object-link .object-name, .aphront-table-view tr.alt-closed td.object-link .object-name { text-decoration: line-through; From 396be07c15b90538acdbc05ec189050767bb127d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Sep 2016 12:32:16 -0700 Subject: [PATCH 18/23] Add a setup issue about small "max_connections" settings Summary: Fixes T11683. Likely as a result of the persitent connections change, more users are seeing MySQL connection limit errors. The persistent connections change means we use //fewer// connections at the high end, but I'm guessing PHP is keeping some more connections around in the pool, so while high-traffic hosts use fewer connections, low-traffic hosts now use more. Raise an explicit setup warning about this. Users should be adjusting it anyway, there's no value to leaving it at extremely low default and connections are baiscally free until you run out of outbound ports. Test Plan: {F1844630} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11683 Differential Revision: https://secure.phabricator.com/D16586 --- .../check/PhabricatorMySQLSetupCheck.php | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index 1990e7f09f..f42009057a 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -44,6 +44,40 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { ->addMySQLConfig('max_allowed_packet'); } + $max_connections = self::loadRawConfigValue('max_connections'); + + // A common default is 150, but we're fairly liberal about the number of + // connections we open and it's easy for us to run far over this limit. + + $warning_threshold = 256; + if ($max_connections < $warning_threshold) { + $message = pht( + 'MySQL is configured with a small "%s" (%d) limit, which may cause '. + 'connection failures long before any resources near exhaustion. '. + 'There is normally very little benefit to enforcing a connection '. + 'limit, and most installs should increase it substantially.'. + "\n\n". + 'You can compute a specific connection limit for your install by '. + 'doing a lot of math with MySQL buffer sizes and RAM available on '. + 'the machine, or just set it to a huge number. In nearly every case, '. + 'setting it to a huge number is entirely reasonable.'. + "\n\n". + 'You can raise this limit by adding this to your %s file (in the %s '. + 'section) and then restarting %s:'. + "\n\n%s", + 'max_connections', + $max_connections, + phutil_tag('tt', array(), 'my.cnf'), + phutil_tag('tt', array(), '[mysqld]'), + phutil_tag('tt', array(), 'mysqld'), + phutil_tag('pre', array(), 'max_connections=100000')); + + $this->newIssue('mysql.max_connections') + ->setName(pht('Small MySQL "%s"', 'max_connections')) + ->setMessage($message) + ->addMySQLConfig('max_connections'); + } + $modes = self::loadRawConfigValue('sql_mode'); $modes = explode(',', $modes); From 88ff486aaeee6665eaae42cdf94290d3cc718b37 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Sep 2016 14:40:13 -0700 Subject: [PATCH 19/23] Fix URI for Phurl NUX Summary: Fixes T11685. We missed this one straggler the recent conversion of Phurl to EditEngine, in T10673. Test Plan: Visited `/phurl/?nux=1`, clicked "Shorten a URL". Reviewers: chad, jcox Reviewed By: jcox Maniphest Tasks: T11685 Differential Revision: https://secure.phabricator.com/D16587 --- .../phurl/query/PhabricatorPhurlURLSearchEngine.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/applications/phurl/query/PhabricatorPhurlURLSearchEngine.php b/src/applications/phurl/query/PhabricatorPhurlURLSearchEngine.php index db40c263ad..9c91adf17d 100644 --- a/src/applications/phurl/query/PhabricatorPhurlURLSearchEngine.php +++ b/src/applications/phurl/query/PhabricatorPhurlURLSearchEngine.php @@ -99,10 +99,13 @@ final class PhabricatorPhurlURLSearchEngine } protected function getNewUserBody() { + $create_uri = id(new PhabricatorPhurlURLEditEngine()) + ->getEditURI(); + $create_button = id(new PHUIButtonView()) ->setTag('a') ->setText(pht('Shorten a URL')) - ->setHref('/phurl/url/create/') + ->setHref($create_uri) ->setColor(PHUIButtonView::GREEN); $icon = $this->getApplication()->getIcon(); From a799d0a8933282a16991ab6a9564c31191ff873c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Sep 2016 15:05:50 -0700 Subject: [PATCH 20/23] Give Phragment a sort of tetris block thing as a title glyph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Fixes T11679. This application is probably vanishing into the aether eventually, but stop it from fataling for now. Here's the glyph: ▛ It's like a fragment of a block of file data! Right? Obviously. Test Plan: Visited `/phragment/` with glpyhs on, saw the glyph. Reviewers: chad, avivey Reviewed By: avivey Subscribers: avivey, hach-que Maniphest Tasks: T11679 Differential Revision: https://secure.phabricator.com/D16588 --- .../phragment/application/PhabricatorPhragmentApplication.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/phragment/application/PhabricatorPhragmentApplication.php b/src/applications/phragment/application/PhabricatorPhragmentApplication.php index c5959f715f..0ca2e5c1e2 100644 --- a/src/applications/phragment/application/PhabricatorPhragmentApplication.php +++ b/src/applications/phragment/application/PhabricatorPhragmentApplication.php @@ -19,7 +19,7 @@ final class PhabricatorPhragmentApplication extends PhabricatorApplication { } public function getTitleGlyph() { - return "\xE2\x26\xB6"; + return "\xE2\x96\x9B"; } public function getApplicationGroup() { From 01afa791ab007878f39837622433130aa14611fc Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 23 Sep 2016 08:49:42 -0400 Subject: [PATCH 21/23] Don't lock subscription in PhameBlog Summary: Ref T11687. Subscription to Blogs comes with many additional features, don't lock people in. Test Plan: Saw I was no longer subscribed. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T11687 Differential Revision: https://secure.phabricator.com/D16589 --- src/applications/phame/storage/PhameBlog.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/phame/storage/PhameBlog.php b/src/applications/phame/storage/PhameBlog.php index dc91672465..32cb6db543 100644 --- a/src/applications/phame/storage/PhameBlog.php +++ b/src/applications/phame/storage/PhameBlog.php @@ -361,7 +361,7 @@ final class PhameBlog extends PhameDAO public function isAutomaticallySubscribed($phid) { - return ($this->creatorPHID == $phid); + return false; } From eea540c5e462422ac8d7866ff7f12d7987e4b18a Mon Sep 17 00:00:00 2001 From: Josh Cox Date: Tue, 20 Sep 2016 19:29:50 -0400 Subject: [PATCH 22/23] Endpoint+controller for a remarkup image proxy Summary: Ref T4190. Currently only have the endpoint and controller working. I added caching so subsequent attempts to proxy the same image should result in the same redirect URL. Still need to: - Write a remarkup rule that uses the endpoint Test Plan: Hit /file/imageproxy/?uri=http://i.imgur.com/nTvVrYN.jpg and are served the picture Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley, yelirekim Maniphest Tasks: T4190 Differential Revision: https://secure.phabricator.com/D16581 --- .../20160921.fileexternalrequest.sql | 14 +++ src/__phutil_library_map__.php | 9 ++ .../PhabricatorFilesApplication.php | 2 +- .../PhabricatorFileImageProxyController.php | 118 ++++++++++++++++++ ...torFileExternalRequestGarbageCollector.php | 28 +++++ .../PhabricatorFileExternalRequest.php | 63 ++++++++++ 6 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20160921.fileexternalrequest.sql create mode 100644 src/applications/files/controller/PhabricatorFileImageProxyController.php create mode 100644 src/applications/files/garbagecollector/PhabricatorFileExternalRequestGarbageCollector.php create mode 100644 src/applications/files/storage/PhabricatorFileExternalRequest.php diff --git a/resources/sql/autopatches/20160921.fileexternalrequest.sql b/resources/sql/autopatches/20160921.fileexternalrequest.sql new file mode 100644 index 0000000000..4c1beaab9e --- /dev/null +++ b/resources/sql/autopatches/20160921.fileexternalrequest.sql @@ -0,0 +1,14 @@ +CREATE TABLE {$NAMESPACE}_file.file_externalrequest ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + filePHID VARBINARY(64), + ttl INT UNSIGNED NOT NULL, + uri LONGTEXT NOT NULL, + uriIndex BINARY(12) NOT NULL, + isSuccessful BOOL NOT NULL, + responseMessage LONGTEXT, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_uriindex` (uriIndex), + KEY `key_ttl` (ttl), + KEY `key_file` (filePHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 646efbdd80..8e67eea1b9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2553,10 +2553,13 @@ phutil_register_library_map(array( 'PhabricatorFileDropUploadController' => 'applications/files/controller/PhabricatorFileDropUploadController.php', 'PhabricatorFileEditController' => 'applications/files/controller/PhabricatorFileEditController.php', 'PhabricatorFileEditor' => 'applications/files/editor/PhabricatorFileEditor.php', + 'PhabricatorFileExternalRequest' => 'applications/files/storage/PhabricatorFileExternalRequest.php', + 'PhabricatorFileExternalRequestGarbageCollector' => 'applications/files/garbagecollector/PhabricatorFileExternalRequestGarbageCollector.php', 'PhabricatorFileFilePHIDType' => 'applications/files/phid/PhabricatorFileFilePHIDType.php', 'PhabricatorFileHasObjectEdgeType' => 'applications/files/edge/PhabricatorFileHasObjectEdgeType.php', 'PhabricatorFileIconSetSelectController' => 'applications/files/controller/PhabricatorFileIconSetSelectController.php', 'PhabricatorFileImageMacro' => 'applications/macro/storage/PhabricatorFileImageMacro.php', + 'PhabricatorFileImageProxyController' => 'applications/files/controller/PhabricatorFileImageProxyController.php', 'PhabricatorFileImageTransform' => 'applications/files/transform/PhabricatorFileImageTransform.php', 'PhabricatorFileInfoController' => 'applications/files/controller/PhabricatorFileInfoController.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', @@ -7368,6 +7371,11 @@ phutil_register_library_map(array( 'PhabricatorFileDropUploadController' => 'PhabricatorFileController', 'PhabricatorFileEditController' => 'PhabricatorFileController', 'PhabricatorFileEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorFileExternalRequest' => array( + 'PhabricatorFileDAO', + 'PhabricatorDestructibleInterface', + ), + 'PhabricatorFileExternalRequestGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorFileFilePHIDType' => 'PhabricatorPHIDType', 'PhabricatorFileHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorFileIconSetSelectController' => 'PhabricatorFileController', @@ -7379,6 +7387,7 @@ phutil_register_library_map(array( 'PhabricatorTokenReceiverInterface', 'PhabricatorPolicyInterface', ), + 'PhabricatorFileImageProxyController' => 'PhabricatorFileController', 'PhabricatorFileImageTransform' => 'PhabricatorFileTransform', 'PhabricatorFileInfoController' => 'PhabricatorFileController', 'PhabricatorFileLinkView' => 'AphrontView', diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php index a94709d400..4247de5bd4 100644 --- a/src/applications/files/application/PhabricatorFilesApplication.php +++ b/src/applications/files/application/PhabricatorFilesApplication.php @@ -78,7 +78,7 @@ final class PhabricatorFilesApplication extends PhabricatorApplication { 'delete/(?P[1-9]\d*)/' => 'PhabricatorFileDeleteController', 'edit/(?P[1-9]\d*)/' => 'PhabricatorFileEditController', 'info/(?P[^/]+)/' => 'PhabricatorFileInfoController', - 'proxy/' => 'PhabricatorFileProxyController', + 'imageproxy/' => 'PhabricatorFileImageProxyController', 'transforms/(?P[1-9]\d*)/' => 'PhabricatorFileTransformListController', 'uploaddialog/(?Psingle/)?' diff --git a/src/applications/files/controller/PhabricatorFileImageProxyController.php b/src/applications/files/controller/PhabricatorFileImageProxyController.php new file mode 100644 index 0000000000..5155229b7f --- /dev/null +++ b/src/applications/files/controller/PhabricatorFileImageProxyController.php @@ -0,0 +1,118 @@ +getViewer(); + $img_uri = $request->getStr('uri'); + + // Validate the URI before doing anything + PhabricatorEnv::requireValidRemoteURIForLink($img_uri); + $uri = new PhutilURI($img_uri); + $proto = $uri->getProtocol(); + if (!in_array($proto, array('http', 'https'))) { + throw new Exception( + pht('The provided image URI must be either http or https')); + } + + // Check if we already have the specified image URI downloaded + $cached_request = id(new PhabricatorFileExternalRequest())->loadOneWhere( + 'uriIndex = %s', + PhabricatorHash::digestForIndex($img_uri)); + + if ($cached_request) { + return $this->getExternalResponse($cached_request); + } + + $ttl = PhabricatorTime::getNow() + phutil_units('7 days in seconds'); + $external_request = id(new PhabricatorFileExternalRequest()) + ->setURI($img_uri) + ->setTTL($ttl); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + // Cache missed so we'll need to validate and download the image + 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( + 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, + 'canCDN' => true, + )); + if (!$file->isViewableImage()) { + $mime_type = $file->getMimeType(); + $engine = new PhabricatorDestructionEngine(); + $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->save(); + } + + $external_request->setIsSuccessful(true) + ->setFilePHID($file->getPHID()) + ->save(); + unset($unguarded); + return $this->getExternalResponse($external_request); + } catch (HTTPFutureHTTPResponseStatus $status) { + $external_request->setIsSuccessful(false) + ->setResponseMessage($status->getMessage()) + ->save(); + return $this->getExternalResponse($external_request); + } catch (Exception $ex) { + // Not actually saving the request in this case + $external_request->setResponseMessage($ex->getMessage()); + return $this->getExternalResponse($external_request); + } + } + + private function getExternalResponse( + PhabricatorFileExternalRequest $request) { + if ($request->getIsSuccessful()) { + $file = id(new PhabricatorFileQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($request->getFilePHID())) + ->executeOne(); + if (!file) { + throw new Exception(pht( + 'The underlying file does not exist, but the cached request was '. + 'successful. This likely means the file record was manually deleted '. + 'by an administrator.')); + } + return id(new AphrontRedirectResponse()) + ->setIsExternal(true) + ->setURI($file->getViewURI()); + } else { + throw new Exception(pht( + "The request to get the external file from '%s' was unsuccessful:\n %s", + $request->getURI(), + $request->getResponseMessage())); + } + } +} diff --git a/src/applications/files/garbagecollector/PhabricatorFileExternalRequestGarbageCollector.php b/src/applications/files/garbagecollector/PhabricatorFileExternalRequestGarbageCollector.php new file mode 100644 index 0000000000..c7e618fb70 --- /dev/null +++ b/src/applications/files/garbagecollector/PhabricatorFileExternalRequestGarbageCollector.php @@ -0,0 +1,28 @@ +loadAllWhere( + 'ttl < %d LIMIT 100', + PhabricatorTime::getNow()); + $engine = new PhabricatorDestructionEngine(); + foreach ($file_requests as $request) { + $engine->destroyObject($request); + } + + return (count($file_requests) == 100); + } + +} diff --git a/src/applications/files/storage/PhabricatorFileExternalRequest.php b/src/applications/files/storage/PhabricatorFileExternalRequest.php new file mode 100644 index 0000000000..347d78c0dd --- /dev/null +++ b/src/applications/files/storage/PhabricatorFileExternalRequest.php @@ -0,0 +1,63 @@ + array( + 'uri' => 'text', + 'uriIndex' => 'bytes12', + 'ttl' => 'epoch', + 'filePHID' => 'phid?', + 'isSuccessful' => 'bool', + 'responseMessage' => 'text?', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_uriindex' => array( + 'columns' => array('uriIndex'), + 'unique' => true, + ), + 'key_ttl' => array( + 'columns' => array('ttl'), + ), + 'key_file' => array( + 'columns' => array('filePHID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function save() { + $hash = PhabricatorHash::digestForIndex($this->getURI()); + $this->setURIIndex($hash); + return parent::save(); + } + +/* -( PhabricatorDestructibleInterface )----------------------------------- */ + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + + $file_phid = $this->getFilePHID(); + if ($file_phid) { + $file = id(new PhabricatorFileQuery()) + ->setViewer($engine->getViewer()) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if ($file) { + $engine->destroyObject($file); + } + } + $this->delete(); + } + +} From 38b10f05a224d29c723c033dec0820f977aa8026 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Sep 2016 12:36:48 -0700 Subject: [PATCH 23/23] For now, disable persistent connections and the "max_connections" setup warning Summary: Ref T11672. At low loads, this causes us to use more connections, which is pushing some installs over the default limits. Rather than trying to walk users through changing `max_connections`, `open_files_limit`, `fs.file-max`, `ulimit`, etc., just put things back for now. After T11044 we should have headroom to use persistent connections within the default limits on all reasonable systems.. Test Plan: Loaded Phabricator, poked around. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11672 Differential Revision: https://secure.phabricator.com/D16591 --- .../check/PhabricatorMySQLSetupCheck.php | 34 ------------------- .../storage/lisk/PhabricatorLiskDAO.php | 8 ++++- 2 files changed, 7 insertions(+), 35 deletions(-) diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index f42009057a..1990e7f09f 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -44,40 +44,6 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { ->addMySQLConfig('max_allowed_packet'); } - $max_connections = self::loadRawConfigValue('max_connections'); - - // A common default is 150, but we're fairly liberal about the number of - // connections we open and it's easy for us to run far over this limit. - - $warning_threshold = 256; - if ($max_connections < $warning_threshold) { - $message = pht( - 'MySQL is configured with a small "%s" (%d) limit, which may cause '. - 'connection failures long before any resources near exhaustion. '. - 'There is normally very little benefit to enforcing a connection '. - 'limit, and most installs should increase it substantially.'. - "\n\n". - 'You can compute a specific connection limit for your install by '. - 'doing a lot of math with MySQL buffer sizes and RAM available on '. - 'the machine, or just set it to a huge number. In nearly every case, '. - 'setting it to a huge number is entirely reasonable.'. - "\n\n". - 'You can raise this limit by adding this to your %s file (in the %s '. - 'section) and then restarting %s:'. - "\n\n%s", - 'max_connections', - $max_connections, - phutil_tag('tt', array(), 'my.cnf'), - phutil_tag('tt', array(), '[mysqld]'), - phutil_tag('tt', array(), 'mysqld'), - phutil_tag('pre', array(), 'max_connections=100000')); - - $this->newIssue('mysql.max_connections') - ->setName(pht('Small MySQL "%s"', 'max_connections')) - ->setMessage($message) - ->addMySQLConfig('max_connections'); - } - $modes = self::loadRawConfigValue('sql_mode'); $modes = explode(',', $modes); diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php index feae254095..d3a287259a 100644 --- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php +++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php @@ -79,9 +79,15 @@ abstract class PhabricatorLiskDAO extends LiskDAO { // - (T10849) Prevent any query from running for more than 30 seconds. // - (T11672) Use persistent connections. if (php_sapi_name() != 'cli') { + + // TODO: For now, disable this until after T11044: it's better at high + // load, but causes us to use slightly more connections at low load and + // is pushing users over limits like MySQL "max_connections". + $use_persistent = false; + $connection ->setQueryTimeout(30) - ->setPersistent(true); + ->setPersistent($use_persistent); } return $connection;