From 51d2d06e3722ad807be588dc894e8d0f0e8be076 Mon Sep 17 00:00:00 2001 From: linead Date: Wed, 4 Jul 2012 12:10:38 +1000 Subject: [PATCH 01/52] Added ldap import controller --- src/__phutil_library_map__.php | 2 + ...AphrontDefaultApplicationConfiguration.php | 1 + .../auth/ldap/PhabricatorLDAPProvider.php | 56 ++++- .../PhabricatorPeopleLdapController.php | 224 ++++++++++++++++++ .../PhabricatorPeopleListController.php | 10 + 5 files changed, 288 insertions(+), 5 deletions(-) create mode 100644 src/applications/people/controller/PhabricatorPeopleLdapController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4be33f8125..a693d6e391 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -822,6 +822,7 @@ phutil_register_library_map(array( 'PhabricatorPasteViewController' => 'applications/paste/controller/PhabricatorPasteViewController.php', 'PhabricatorPeopleController' => 'applications/people/controller/PhabricatorPeopleController.php', 'PhabricatorPeopleEditController' => 'applications/people/controller/PhabricatorPeopleEditController.php', + 'PhabricatorPeopleLdapController' => 'applications/people/controller/PhabricatorPeopleLdapController.php', 'PhabricatorPeopleListController' => 'applications/people/controller/PhabricatorPeopleListController.php', 'PhabricatorPeopleLogsController' => 'applications/people/controller/PhabricatorPeopleLogsController.php', 'PhabricatorPeopleProfileController' => 'applications/people/controller/PhabricatorPeopleProfileController.php', @@ -1792,6 +1793,7 @@ phutil_register_library_map(array( 'PhabricatorPasteViewController' => 'PhabricatorPasteController', 'PhabricatorPeopleController' => 'PhabricatorController', 'PhabricatorPeopleEditController' => 'PhabricatorPeopleController', + 'PhabricatorPeopleLdapController' => 'PhabricatorPeopleController', 'PhabricatorPeopleListController' => 'PhabricatorPeopleController', 'PhabricatorPeopleLogsController' => 'PhabricatorPeopleController', 'PhabricatorPeopleProfileController' => 'PhabricatorPeopleController', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index 4f56a77cfe..ec0a84b356 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -72,6 +72,7 @@ class AphrontDefaultApplicationConfiguration 'logs/' => 'PhabricatorPeopleLogsController', 'edit/(?:(?P\d+)/(?:(?P\w+)/)?)?' => 'PhabricatorPeopleEditController', + 'ldap/' => 'PhabricatorPeopleLdapController', ), '/p/(?P[\w._-]+)/(?:(?P\w+)/)?' => 'PhabricatorPeopleProfileController', diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index d1d0b29c31..3f9241b897 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -53,22 +53,26 @@ final class PhabricatorLDAPProvider { public function retrieveUserEmail() { return $this->userData['mail'][0]; } - + public function retrieveUserRealName() { + return $this->retrieveUserRealNameFromData($this->userData); + } + + public function retrieveUserRealNameFromData($data) { $name_attributes = PhabricatorEnv::getEnvConfig( 'ldap.real_name_attributes'); $real_name = ''; if (is_array($name_attributes)) { foreach ($name_attributes AS $attribute) { - if (isset($this->userData[$attribute][0])) { - $real_name .= $this->userData[$attribute][0] . ' '; + if (isset($data[$attribute][0])) { + $real_name .= $data[$attribute][0] . ' '; } } trim($real_name); - } else if (isset($this->userData[$name_attributes][0])) { - $real_name = $this->userData[$name_attributes][0]; + } else if (isset($data[$name_attributes][0])) { + $real_name = $data[$name_attributes][0]; } if ($real_name == '') { @@ -146,4 +150,46 @@ final class PhabricatorLDAPProvider { return $entries[0]; } + + public function search($query) { + $result = ldap_search($this->getConnection(), $this->getBaseDN(), + $query); + + if (!$result) { + throw new Exception('Search failed. Please check your LDAP and HTTP '. + 'logs for more information.'); + } + + $entries = ldap_get_entries($this->getConnection(), $result); + + if ($entries === false) { + throw new Exception('Could not get entries'); + } + + if ($entries['count'] == 0) { + throw new Exception('No results found'); + } + + + $rows = array(); + + for($i = 0; $i < $entries['count']; $i++) { + $row = array(); + $entry = $entries[$i]; + // Get username, email and realname + $username = $entry[$this->getSearchAttribute()][0]; + if(empty($username)) { + continue; + } + $row[] = $username; + $row[] = $entry['mail'][0]; + $row[] = $this->retrieveUserRealNameFromData($entry); + + + $rows[] = $row; + } + + return $rows; + + } } diff --git a/src/applications/people/controller/PhabricatorPeopleLdapController.php b/src/applications/people/controller/PhabricatorPeopleLdapController.php new file mode 100644 index 0000000000..e0e4b3a3eb --- /dev/null +++ b/src/applications/people/controller/PhabricatorPeopleLdapController.php @@ -0,0 +1,224 @@ +view = idx($data, 'view'); + } + + public function processRequest() { + + $request = $this->getRequest(); + $admin = $request->getUser(); + + $base_uri = '/people/edit/'; + + $content = array(); + + + $response = $this->processBasicRequest(); + + if ($response instanceof AphrontResponse) { + return $response; + } + + $content[] = $response; + + + return $this->buildStandardPageResponse( + $content, + array( + 'title' => 'Import Ldap Users', + )); + } + + /** + * Displays a ldap login form, as we need to auth before we can search + */ + private function processBasicRequest() { + $panels = array(); + + $request = $this->getRequest(); + + $admin = $request->getUser(); + + $form = id(new AphrontFormView()) + ->setUser($admin) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel('LDAP username') + ->setName('username')) + ->appendChild( + id(new AphrontFormPasswordControl()) + ->setLabel('Password') + ->setName('password')) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel('LDAP query') + ->setName('query')) + ->setAction($request->getRequestURI()->alter('search', 'true')->alter('import', null)) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue('Search')); + + $panel = new AphrontPanelView(); + $panel->setHeader('Import Ldap Users'); + $panel->appendChild($form); + + + if($request->getStr('import')) { + $panels[] = $this->processImportRequest($request); + } + + $panels[] = $panel; + + if($request->getStr('search')) { + $panels[] = $this->processSearchRequest($request); + } + + return $panels; + + } + + private function processImportRequest($request) { + $admin = $request->getUser(); + $usernames = $request->getArr('usernames'); + $emails = $request->getArr('email'); + $names = $request->getArr('name'); + + $panel = new AphrontErrorView(); + $panel->setSeverity(AphrontErrorView::SEVERITY_NOTICE); + $panel->setTitle("Import Successful"); + $errors = array("Successfully imported users from ldap"); + + + foreach($usernames as $username) { + $user = new PhabricatorUser(); + $user->setUsername($username); + $user->setRealname($names[$username]); + + $email_obj = id(new PhabricatorUserEmail()) + ->setAddress($emails[$username]) + ->setIsVerified(1); + try { + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->createNewUser($user, $email_obj); + + $ldap_info = new PhabricatorUserLDAPInfo(); + $ldap_info->setLDAPUsername($username); + $ldap_info->setUserID($user->getID()); + $ldap_info->save(); + $errors[] = 'Succesfully added ' . $username; + } catch (Exception $ex) { + $errors[] = 'Failed to add ' . $username . ' ' . $ex->getMessage(); + } + } + + $panel->setErrors($errors); + return $panel; + + } + + private function processSearchRequest($request) { + $panel = new AphrontPanelView(); + + $admin = $request->getUser(); + + $username = $request->getStr('username'); + $password = $request->getStr('password'); + $search = $request->getStr('query'); + + try { + $ldapProvider = new PhabricatorLDAPProvider(); + $ldapProvider->auth($username, $password); + $results = $ldapProvider->search($search); + foreach($results as $key => $result) { + $results[$key][] = $this->renderUserInputs($result); + } + + $form = id(new AphrontFormView()) + ->setUser($admin); + + $table = new AphrontTableView($results); + $table->setHeaders( + array( + 'Username', + 'Email', + 'RealName', + '', + )); + $form->appendChild($table); + $form->setAction($request->getRequestURI()->alter('import', 'true')->alter('search', null)) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue('Import')); + + + $panel->appendChild($form); + } catch (Exception $ex) { + $error_view = new AphrontErrorView(); + $error_view->setTitle('LDAP Search Failed'); + $error_view->setErrors(array($ex->getMessage())); + return $error_view; + } + return $panel; + + } + + private function renderUserInputs($user) { + $username = $user[0]; + $inputs = phutil_render_tag( + 'input', + array( + 'type' => 'checkbox', + 'name' => 'usernames[]', + 'value' =>$username, + ), + ''); + + $inputs .= phutil_render_tag( + 'input', + array( + 'type' => 'hidden', + 'name' => "email[$username]", + 'value' =>$user[1], + ), + ''); + + $inputs .= phutil_render_tag( + 'input', + array( + 'type' => 'hidden', + 'name' => "name[$username]", + 'value' =>$user[2], + ), + ''); + + return $inputs; + + } +} diff --git a/src/applications/people/controller/PhabricatorPeopleListController.php b/src/applications/people/controller/PhabricatorPeopleListController.php index cb41eab371..a9577f5003 100644 --- a/src/applications/people/controller/PhabricatorPeopleListController.php +++ b/src/applications/people/controller/PhabricatorPeopleListController.php @@ -130,6 +130,16 @@ final class PhabricatorPeopleListController 'class' => 'button green', ), 'Create New Account')); + if (PhabricatorEnv::getEnvConfig('ldap.auth-enabled')) { + $panel->addButton( + phutil_render_tag( + 'a', + array( + 'href' => '/people/ldap', + 'class' => 'button green' + ), + 'Import from Ldap')); + } } return $this->buildStandardPageResponse($panel, array( From 67691c196e1901b35d75b4649d62b708f5f75825 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 5 Jul 2012 15:43:14 -0700 Subject: [PATCH 02/52] Phame - add an "Everyone but Me" view and make published posts truly accessible to the whole world. Summary: accessibility covers not only a given post but also the various "published" views. to keep the code relative clean, this diff also splits up the post list controller logic quite a bit. this also feels like good preparation for some other work around introducing "blogs" which are collections of published posts from bloggers with some fancy features around that. Test Plan: clicked around various parts of the Phame application as a logged in user, a logged in user with no personal posts, and without any user logged in at all. various views all seemed reasonable. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T1373 Differential Revision: https://secure.phabricator.com/D2898 --- src/__phutil_library_map__.php | 8 +- ...AphrontDefaultApplicationConfiguration.php | 8 +- .../phame/controller/PhameController.php | 11 +- .../post/PhamePostEditController.php | 2 +- .../post/PhamePostViewController.php | 6 + .../PhameAllBloggersPostListController.php | 95 ++++++++++++++ .../list/PhameBloggerPostListController.php | 74 +++++++++++ .../post/list/PhameDraftListController.php | 30 ++++- .../post/list/PhamePostListBaseController.php | 123 ++++++++---------- .../post/list/PhamePostListController.php | 29 ----- .../post/list/PhameUserPostListController.php | 89 +++++++++++++ .../phame/query/PhamePostQuery.php | 18 +++ .../phame/view/PhamePostDetailView.php | 10 +- .../phame/view/PhamePostListView.php | 7 +- 14 files changed, 394 insertions(+), 116 deletions(-) create mode 100644 src/applications/phame/controller/post/list/PhameAllBloggersPostListController.php create mode 100644 src/applications/phame/controller/post/list/PhameBloggerPostListController.php delete mode 100644 src/applications/phame/controller/post/list/PhamePostListController.php create mode 100644 src/applications/phame/controller/post/list/PhameUserPostListController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cd786089ed..b2564116cb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1040,6 +1040,8 @@ phutil_register_library_map(array( 'PhabricatorXHProfProfileSymbolView' => 'applications/xhprof/view/PhabricatorXHProfProfileSymbolView.php', 'PhabricatorXHProfProfileTopLevelView' => 'applications/xhprof/view/PhabricatorXHProfProfileTopLevelView.php', 'PhabricatorXHProfProfileView' => 'applications/xhprof/view/PhabricatorXHProfProfileView.php', + 'PhameAllBloggersPostListController' => 'applications/phame/controller/post/list/PhameAllBloggersPostListController.php', + 'PhameBloggerPostListController' => 'applications/phame/controller/post/list/PhameBloggerPostListController.php', 'PhameController' => 'applications/phame/controller/PhameController.php', 'PhameDAO' => 'applications/phame/storage/PhameDAO.php', 'PhameDraftListController' => 'applications/phame/controller/post/list/PhameDraftListController.php', @@ -1048,11 +1050,11 @@ phutil_register_library_map(array( 'PhamePostDetailView' => 'applications/phame/view/PhamePostDetailView.php', 'PhamePostEditController' => 'applications/phame/controller/post/PhamePostEditController.php', 'PhamePostListBaseController' => 'applications/phame/controller/post/list/PhamePostListBaseController.php', - 'PhamePostListController' => 'applications/phame/controller/post/list/PhamePostListController.php', 'PhamePostListView' => 'applications/phame/view/PhamePostListView.php', 'PhamePostPreviewController' => 'applications/phame/controller/post/PhamePostPreviewController.php', 'PhamePostQuery' => 'applications/phame/query/PhamePostQuery.php', 'PhamePostViewController' => 'applications/phame/controller/post/PhamePostViewController.php', + 'PhameUserPostListController' => 'applications/phame/controller/post/list/PhameUserPostListController.php', 'PhortuneMonthYearExpiryControl' => 'applications/phortune/control/PhortuneMonthYearExpiryControl.php', 'PhortuneStripeBaseController' => 'applications/phortune/stripe/controller/PhortuneStripeBaseController.php', 'PhortuneStripePaymentFormView' => 'applications/phortune/stripe/view/PhortuneStripePaymentFormView.php', @@ -2001,6 +2003,8 @@ phutil_register_library_map(array( 'PhabricatorXHProfProfileSymbolView' => 'PhabricatorXHProfProfileView', 'PhabricatorXHProfProfileTopLevelView' => 'PhabricatorXHProfProfileView', 'PhabricatorXHProfProfileView' => 'AphrontView', + 'PhameAllBloggersPostListController' => 'PhamePostListBaseController', + 'PhameBloggerPostListController' => 'PhamePostListBaseController', 'PhameController' => 'PhabricatorController', 'PhameDAO' => 'PhabricatorLiskDAO', 'PhameDraftListController' => 'PhamePostListBaseController', @@ -2009,11 +2013,11 @@ phutil_register_library_map(array( 'PhamePostDetailView' => 'AphrontView', 'PhamePostEditController' => 'PhameController', 'PhamePostListBaseController' => 'PhameController', - 'PhamePostListController' => 'PhamePostListBaseController', 'PhamePostListView' => 'AphrontView', 'PhamePostPreviewController' => 'PhameController', 'PhamePostQuery' => 'PhabricatorOffsetPagedQuery', 'PhamePostViewController' => 'PhameController', + 'PhameUserPostListController' => 'PhamePostListBaseController', 'PhortuneMonthYearExpiryControl' => 'AphrontFormControl', 'PhortuneStripeBaseController' => 'PhabricatorController', 'PhortuneStripePaymentFormView' => 'AphrontView', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index 300dcdd3eb..c250beaf12 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -381,9 +381,9 @@ class AphrontDefaultApplicationConfiguration ), '/phame/' => array( - '' => 'PhamePostListController', + '' => 'PhameAllBloggersPostListController', 'post/' => array( - '' => 'PhamePostListController', + '' => 'PhameUserPostListController', 'delete/(?P[^/]+)/' => 'PhamePostDeleteController', 'edit/(?P[^/]+)/' => 'PhamePostEditController', 'new/' => 'PhamePostEditController', @@ -395,8 +395,8 @@ class AphrontDefaultApplicationConfiguration 'new/' => 'PhamePostEditController', ), 'posts/' => array( - '' => 'PhamePostListController', - '(?P\w+)/' => 'PhamePostListController', + '' => 'PhameUserPostListController', + '(?P\w+)/' => 'PhameBloggerPostListController', '(?P\w+)/(?P.+/)' => 'PhamePostViewController', ), diff --git a/src/applications/phame/controller/PhameController.php b/src/applications/phame/controller/PhameController.php index 67ce808a90..2f5c451667 100644 --- a/src/applications/phame/controller/PhameController.php +++ b/src/applications/phame/controller/PhameController.php @@ -60,8 +60,9 @@ abstract class PhameController extends PhabricatorController { } private function renderSideNavFilterView($filter) { + $base_uri = new PhutilURI('/phame/'); $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/phame/')); + $nav->setBaseURI($base_uri); $nav->addLabel('Drafts'); $nav->addFilter('post/new', 'New Draft'); @@ -71,12 +72,16 @@ abstract class PhameController extends PhabricatorController { $nav->addLabel('Posts'); $nav->addFilter('post', 'My Posts'); + $nav->addFilter('everyone', + 'Everyone', + $base_uri); foreach ($this->getSideNavExtraPostFilters() as $post_filter) { $nav->addFilter($post_filter['key'], - $post_filter['name']); + $post_filter['name'], + idx($post_filter, 'uri')); } - $nav->selectFilter($filter, 'post'); + $nav->selectFilter($filter); return $nav; } diff --git a/src/applications/phame/controller/post/PhamePostEditController.php b/src/applications/phame/controller/post/PhamePostEditController.php index 6b80fdc42f..3d1c005c96 100644 --- a/src/applications/phame/controller/post/PhamePostEditController.php +++ b/src/applications/phame/controller/post/PhamePostEditController.php @@ -99,7 +99,7 @@ extends PhameController { $post = id(new PhamePost()) ->setBloggerPHID($user->getPHID()) ->setVisibility(PhamePost::VISIBILITY_DRAFT); - $cancel_uri = '/phame'; + $cancel_uri = '/phame/'; $submit_button = 'Create Post'; $delete_button = null; $page_title = 'Create Post'; diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index 94db9b16ad..9122ea682e 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -60,6 +60,12 @@ extends PhameController { return $filters; } + public function shouldRequireLogin() { + // TODO -- get policy logic going + // return PhabricatorEnv::getEnvConfig('policy.allow-public'); + return true; + } + public function willProcessRequest(array $data) { $this->setPostPHID(idx($data, 'phid')); $this->setPhameTitle(idx($data, 'phametitle')); diff --git a/src/applications/phame/controller/post/list/PhameAllBloggersPostListController.php b/src/applications/phame/controller/post/list/PhameAllBloggersPostListController.php new file mode 100644 index 0000000000..c851ca092a --- /dev/null +++ b/src/applications/phame/controller/post/list/PhameAllBloggersPostListController.php @@ -0,0 +1,95 @@ +getRequest()->getUser(); + + $new_link = phutil_render_tag( + 'a', + array( + 'href' => '/phame/post/new/', + 'class' => 'button green', + ), + 'write a blog post' + ); + + $remarkup_link = phutil_render_tag( + 'a', + array( + 'href' => + PhabricatorEnv::getDoclink('article/Remarkup_Reference.html'), + ), + 'remarkup' + ); + + $guide_link = phutil_render_tag( + 'a', + array( + 'href' => PhabricatorEnv::getDoclink('article/Phame_User_Guide.html'), + ), + 'Phame user guide' + ); + + $notices = array( + 'Seek phame and '.$new_link, + 'Use '.$remarkup_link.' for maximal elegance, grace, and style. ', + 'If you need more help try the '.$guide_link.'.', + ); + + $notice_view = id(new AphrontErrorView()) + ->setSeverity(AphrontErrorView::SEVERITY_NOTICE) + ->setTitle('Meta thoughts and feelings'); + foreach ($notices as $notice) { + $notice_view->appendChild('

'.$notice.'

'); + } + + return $notice_view; + } + + public function processRequest() { + $user = $this->getRequest()->getUser(); + + $query = new PhamePostQuery(); + $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $this->setPhamePostQuery($query); + + $this->setActions(array('view')); + + $page_title = 'Posts by Everyone'; + $this->setPageTitle($page_title); + + $this->setShowSideNav(true); + + return $this->buildPostListPageResponse(); + } + +} diff --git a/src/applications/phame/controller/post/list/PhameBloggerPostListController.php b/src/applications/phame/controller/post/list/PhameBloggerPostListController.php new file mode 100644 index 0000000000..d6932caa4f --- /dev/null +++ b/src/applications/phame/controller/post/list/PhameBloggerPostListController.php @@ -0,0 +1,74 @@ +bloggerName = $blogger_name; + return $this; + } + private function getBloggerName() { + return $this->bloggerName; + } + + public function shouldRequireLogin() { + // TODO -- get policy logic going + // return PhabricatorEnv::getEnvConfig('policy.allow-public'); + return true; + } + + public function willProcessRequest(array $data) { + $this->setBloggerName(idx($data, 'bloggername')); + } + + public function processRequest() { + $user = $this->getRequest()->getUser(); + + $blogger = id(new PhabricatorUser())->loadOneWhere( + 'username = %s', + $this->getBloggerName()); + if (!$blogger) { + return new Aphront404Response(); + } + $blogger_phid = $blogger->getPHID(); + if ($blogger_phid == $user->getPHID()) { + $actions = array('view', 'edit'); + } else { + $actions = array('view'); + } + $this->setActions($actions); + + $query = new PhamePostQuery(); + $query->withBloggerPHID($blogger_phid); + $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $this->setPhamePostQuery($query); + + $page_title = 'Posts by '.$this->getBloggerName(); + $this->setPageTitle($page_title); + + $this->setShowSideNav(false); + + return $this->buildPostListPageResponse(); + } +} diff --git a/src/applications/phame/controller/post/list/PhameDraftListController.php b/src/applications/phame/controller/post/list/PhameDraftListController.php index c8bb668944..7eccbf9f14 100644 --- a/src/applications/phame/controller/post/list/PhameDraftListController.php +++ b/src/applications/phame/controller/post/list/PhameDraftListController.php @@ -22,8 +22,34 @@ final class PhameDraftListController extends PhamePostListBaseController { + public function shouldRequireLogin() { + return true; + } + + protected function getSideNavFilter() { + return 'draft'; + } + + protected function isDraft() { + return true; + } + public function processRequest() { - $this->setIsDraft(true); - return parent::processRequest(); + $user = $this->getRequest()->getUser(); + $phid = $user->getPHID(); + + $query = new PhamePostQuery(); + $query->withBloggerPHID($phid); + $query->withVisibility(PhamePost::VISIBILITY_DRAFT); + $this->setPhamePostQuery($query); + + $actions = array('view', 'edit'); + $this->setActions($actions); + + $this->setPageTitle('My Drafts'); + + $this->setShowSideNav(true); + + return $this->buildPostListPageResponse(); } } diff --git a/src/applications/phame/controller/post/list/PhamePostListBaseController.php b/src/applications/phame/controller/post/list/PhamePostListBaseController.php index 00f8d053bb..21da11b2a5 100644 --- a/src/applications/phame/controller/post/list/PhamePostListBaseController.php +++ b/src/applications/phame/controller/post/list/PhamePostListBaseController.php @@ -22,106 +22,87 @@ abstract class PhamePostListBaseController extends PhameController { - private $bloggerName; - private $isDraft; + private $phamePostQuery; + private $actions; + private $pageTitle; - private function setBloggerName($blogger_name) { - $this->bloggerName = $blogger_name; + protected function setPageTitle($page_title) { + $this->pageTitle = $page_title; return $this; } - private function getBloggerName() { - return $this->bloggerName; + private function getPageTitle() { + return $this->pageTitle; } - protected function getSideNavExtraPostFilters() { - if ($this->isDraft() || !$this->getBloggerName()) { - return array(); - } - - return - array(array('key' => $this->getSideNavFilter(), - 'name' => 'Posts by '.$this->getBloggerName())); - } - - protected function getSideNavFilter() { - if ($this->getBloggerName()) { - $filter = 'posts/'.$this->getBloggerName(); - } else if ($this->isDraft()) { - $filter = 'draft'; - } else { - $filter = 'posts'; - } - return $filter; - } - - private function isDraft() { - return (bool) $this->isDraft; - } - protected function setIsDraft($is_draft) { - $this->isDraft = $is_draft; + protected function setActions($actions) { + $this->actions = $actions; return $this; } - - public function willProcessRequest(array $data) { - $this->setBloggerName(idx($data, 'bloggername')); + private function getActions() { + return $this->actions; } - public function processRequest() { + protected function setPhamePostQuery(PhamePostQuery $query) { + $this->phamePostQuery = $query; + return $this; + } + private function getPhamePostQuery() { + return $this->phamePostQuery; + } + + protected function isDraft() { + return false; + } + + protected function getPager() { $request = $this->getRequest(); - $user = $request->getUser(); $pager = new AphrontPagerView(); $page_size = 50; $pager->setURI($request->getRequestURI(), 'offset'); $pager->setPageSize($page_size); $pager->setOffset($request->getInt('offset')); - if ($this->getBloggerName()) { - $blogger = id(new PhabricatorUser())->loadOneWhere( - 'username = %s', - $this->getBloggerName()); - if (!$blogger) { - return new Aphront404Response(); - } - $page_title = 'Posts by '.$this->getBloggerName(); - if ($blogger->getPHID() == $user->getPHID()) { - $actions = array('view', 'edit'); - } else { - $actions = array('view'); - } - $this->setShowSideNav(false); - } else { - $blogger = $user; - $page_title = 'Posts by '.$user->getUserName(); - $actions = array('view', 'edit'); - $this->setShowSideNav(true); + return $pager; + } + + protected function getNoticeView() { + return null; + } + + private function loadBloggersFromPosts(array $posts) { + assert_instances_of($posts, 'PhamePost'); + if (empty($posts)) { + return array(); } - $phid = $blogger->getPHID(); - // user gets to see their own unpublished stuff - if ($phid == $user->getPHID() && $this->isDraft()) { - $post_visibility = PhamePost::VISIBILITY_DRAFT; - } else { - $post_visibility = PhamePost::VISIBILITY_PUBLISHED; - } - $query = new PhamePostQuery(); - $query->withBloggerPHID($phid); - $query->withVisibility($post_visibility); - $posts = $query->executeWithPager($pager); - $bloggers = array($blogger->getPHID() => $blogger); + + $blogger_phids = mpull($posts, 'getBloggerPHID', 'getBloggerPHID'); + + return + id(new PhabricatorObjectHandleData($blogger_phids))->loadHandles(); + } + + protected function buildPostListPageResponse() { + $pager = $this->getPager(); + $query = $this->getPhamePostQuery(); + $posts = $query->executeWithPager($pager); + + $bloggers = $this->loadBloggersFromPosts($posts); $panel = id(new PhamePostListView()) - ->setUser($user) + ->setUser($this->getRequest()->getUser()) ->setBloggers($bloggers) ->setPosts($posts) - ->setActions($actions) + ->setActions($this->getActions()) ->setDraftList($this->isDraft()); return $this->buildStandardPageResponse( array( + $this->getNoticeView(), $panel, $pager ), array( - 'title' => $page_title, + 'title' => $this->getPageTitle(), )); } } diff --git a/src/applications/phame/controller/post/list/PhamePostListController.php b/src/applications/phame/controller/post/list/PhamePostListController.php deleted file mode 100644 index 7923e52b56..0000000000 --- a/src/applications/phame/controller/post/list/PhamePostListController.php +++ /dev/null @@ -1,29 +0,0 @@ -setIsDraft(false); - return parent::processRequest(); - } -} diff --git a/src/applications/phame/controller/post/list/PhameUserPostListController.php b/src/applications/phame/controller/post/list/PhameUserPostListController.php new file mode 100644 index 0000000000..ea25cf6390 --- /dev/null +++ b/src/applications/phame/controller/post/list/PhameUserPostListController.php @@ -0,0 +1,89 @@ +getRequest()->getUser(); + + $new_link = phutil_render_tag( + 'a', + array( + 'href' => '/phame/post/new/', + 'class' => 'button green', + ), + 'write another blog post' + ); + + $pretty_uri = PhabricatorEnv::getProductionURI( + '/phame/posts/'.$user->getUserName().'/'); + $pretty_link = phutil_render_tag( + 'a', + array( + 'href' => (string) $pretty_uri + ), + (string) $pretty_uri + ); + + $notices = array( + 'Seek even more phame and '.$new_link, + 'Published posts also appear at the awesome, world-accessible '. + 'URI: '.$pretty_link + ); + + $notice_view = id(new AphrontErrorView()) + ->setSeverity(AphrontErrorView::SEVERITY_NOTICE) + ->setTitle('Meta thoughts and feelings'); + foreach ($notices as $notice) { + $notice_view->appendChild('

'.$notice.'

'); + } + + return $notice_view; + } + + public function processRequest() { + $user = $this->getRequest()->getUser(); + $phid = $user->getPHID(); + + $query = new PhamePostQuery(); + $query->withBloggerPHID($phid); + $query->withVisibility(PhamePost::VISIBILITY_PUBLISHED); + $this->setPhamePostQuery($query); + + $actions = array('view', 'edit'); + $this->setActions($actions); + + $this->setPageTitle('My Posts'); + + $this->setShowSideNav(true); + + return $this->buildPostListPageResponse(); + } +} diff --git a/src/applications/phame/query/PhamePostQuery.php b/src/applications/phame/query/PhamePostQuery.php index aa3f95c150..31c587efcb 100644 --- a/src/applications/phame/query/PhamePostQuery.php +++ b/src/applications/phame/query/PhamePostQuery.php @@ -19,12 +19,24 @@ final class PhamePostQuery extends PhabricatorOffsetPagedQuery { private $bloggerPHID; + private $withoutBloggerPHID; private $visibility; + /** + * Mutually exlusive with @{method:withoutBloggerPHID}. + * + * @{method:withBloggerPHID} wins because being positive and inclusive is + * cool. + */ public function withBloggerPHID($blogger_phid) { $this->bloggerPHID = $blogger_phid; return $this; } + public function withoutBloggerPHID($blogger_phid) { + $this->withoutBloggerPHID = $blogger_phid; + return $this; + } + public function withVisibility($visibility) { $this->visibility = $visibility; return $this; @@ -60,6 +72,12 @@ final class PhamePostQuery extends PhabricatorOffsetPagedQuery { 'bloggerPHID = %s', $this->bloggerPHID ); + } else if ($this->withoutBloggerPHID) { + $where[] = qsprintf( + $conn_r, + 'bloggerPHID != %s', + $this->withoutBloggerPHID + ); } if ($this->visibility !== null) { diff --git a/src/applications/phame/view/PhamePostDetailView.php b/src/applications/phame/view/PhamePostDetailView.php index e010dd1918..ebf5089acb 100644 --- a/src/applications/phame/view/PhamePostDetailView.php +++ b/src/applications/phame/view/PhamePostDetailView.php @@ -79,7 +79,7 @@ final class PhamePostDetailView extends AphrontView { $uri = '/phame/draft/'; $label = 'Back to Your Drafts'; } else { - $uri = '/phame/posts/'.$blogger->getUsername(); + $uri = '/phame/posts/'.$blogger->getUsername().'/'; $label = 'More Posts by '.phutil_escape_html($blogger->getUsername()); } $button = phutil_render_tag( @@ -101,6 +101,14 @@ final class PhamePostDetailView extends AphrontView { phabricator_datetime($post->getDateModified(), $user); } + $caption .= ' by '.phutil_render_tag( + 'a', + array( + 'href' => new PhutilURI('/p/'.$blogger->getUsername().'/'), + ), + phutil_escape_html($blogger->getUsername()) + ).'.'; + if ($this->isPreview()) { $width = AphrontPanelView::WIDTH_FULL; } else { diff --git a/src/applications/phame/view/PhamePostListView.php b/src/applications/phame/view/PhamePostListView.php index ebda5bde9f..5726cdafea 100644 --- a/src/applications/phame/view/PhamePostListView.php +++ b/src/applications/phame/view/PhamePostListView.php @@ -59,7 +59,7 @@ final class PhamePostListView extends AphrontView { return $this->posts; } public function setBloggers(array $bloggers) { - assert_instances_of($bloggers, 'PhabricatorUser'); + assert_instances_of($bloggers, 'PhabricatorObjectHandle'); $this->bloggers = $bloggers; return $this; } @@ -99,17 +99,18 @@ final class PhamePostListView extends AphrontView { foreach ($posts as $post) { $blogger_phid = $post->getBloggerPHID(); $blogger = $bloggers[$blogger_phid]; + $blogger_link = $blogger->renderLink(); $updated = phabricator_datetime($post->getDateModified(), $user); $body = $engine->markupText($post->getBody()); $panel = id(new AphrontPanelView()) ->setHeader(phutil_escape_html($post->getTitle())) - ->setCaption('Last updated '.$updated) + ->setCaption('Last updated '.$updated.' by '.$blogger_link.'.') ->appendChild('
'.$body.'
'); foreach ($actions as $action) { switch ($action) { case 'view': - $uri = $post->getViewURI($blogger->getUsername()); + $uri = $post->getViewURI($blogger->getName()); $label = 'View '.$noun; break; case 'edit': From ce360926b7283dd475c75001bb7fa21488b3d3c3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Jul 2012 16:03:43 -0700 Subject: [PATCH 03/52] Allow PhabricatorGlobalLock to block Summary: See D2924. Test Plan: Ran locks with blocking timeouts. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2925 --- src/infrastructure/util/PhabricatorGlobalLock.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php index 02e04e48eb..d074526f95 100644 --- a/src/infrastructure/util/PhabricatorGlobalLock.php +++ b/src/infrastructure/util/PhabricatorGlobalLock.php @@ -63,7 +63,7 @@ final class PhabricatorGlobalLock extends PhutilLock { /* -( Implementation )----------------------------------------------------- */ - protected function doLock() { + protected function doLock($wait) { $conn = $this->conn; if (!$conn) { // NOTE: Using the 'repository' database somewhat arbitrarily, mostly @@ -87,9 +87,9 @@ final class PhabricatorGlobalLock extends PhutilLock { $result = queryfx_one( $conn, - 'SELECT GET_LOCK(%s, %d)', + 'SELECT GET_LOCK(%s, %f)', 'phabricator:'.$this->lockname, - 0); + $wait); $ok = head($result); if (!$ok) { From ae6661ac3f0698a03eb4eda71585c5eeb09c0c4d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Jul 2012 16:03:49 -0700 Subject: [PATCH 04/52] Document event --trace added by D2918 Summary: Also fix a doc typo. Test Plan: Read. Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran Maniphest Tasks: T1092 Differential Revision: https://secure.phabricator.com/D2919 --- src/docs/userguide/events.diviner | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/docs/userguide/events.diviner b/src/docs/userguide/events.diviner index 64c11e51fc..3ca182fe96 100644 --- a/src/docs/userguide/events.diviner +++ b/src/docs/userguide/events.diviner @@ -206,10 +206,12 @@ If you're having problems with your listener, try these steps: events instead, to test that your listener reacts to them properly. You might have to use fake data, but this gives you an easy way to test the at least the basics. + - For scripts, you can run under `--trace` to see which events are emitted + and how many handlers are listening to each event. = Next Steps = Continue by: - taking a look at @{class:PhabricatorExampleEventListener}; or - - building a library with @{article:@{article:libphutil Libraries User Guide}. \ No newline at end of file + - building a library with @{article:libphutil Libraries User Guide}. \ No newline at end of file From 63be89ba00c9346348ebf8268fc4a96cceec2dd9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Jul 2012 16:03:58 -0700 Subject: [PATCH 05/52] Improve error message for error 2006 Summary: See discussion here: https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=21186 Basically, MySQL usually raises a good error if we exceed "max_allowed_packet": EXCEPTION: (AphrontQueryException) #1153: Got a packet bigger than 'max_allowed_packet' bytes But sometimes it gives us a #2006 instead. This is documented, at least: >"With some clients, you may also get a Lost connection to MySQL server during query error if the communication packet is too large." http://dev.mysql.com/doc/refman//5.5/en/packet-too-large.html Try to improve the error message to point at this as a possible explanation. Test Plan: Faked an error, had it throw, read exception message. See also chatlog. Reviewers: btrahan, skrul Reviewed By: skrul CC: aran Differential Revision: https://secure.phabricator.com/D2923 --- .../connection/mysql/AphrontMySQLDatabaseConnectionBase.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/infrastructure/storage/connection/mysql/AphrontMySQLDatabaseConnectionBase.php b/src/infrastructure/storage/connection/mysql/AphrontMySQLDatabaseConnectionBase.php index 8224592f5f..476e6049a2 100644 --- a/src/infrastructure/storage/connection/mysql/AphrontMySQLDatabaseConnectionBase.php +++ b/src/infrastructure/storage/connection/mysql/AphrontMySQLDatabaseConnectionBase.php @@ -217,8 +217,11 @@ abstract class AphrontMySQLDatabaseConnectionBase switch ($errno) { case 2013: // Connection Dropped - case 2006: // Gone Away throw new AphrontQueryConnectionLostException($exmsg); + case 2006: // Gone Away + $more = "This error may occur if your MySQL 'wait_timeout' ". + "or 'max_allowed_packet' configuration values are set too low."; + throw new AphrontQueryConnectionLostException("{$exmsg}\n\n{$more}"); case 1213: // Deadlock case 1205: // Lock wait timeout exceeded throw new AphrontQueryDeadlockException($exmsg); From 78d2f08fcd8014fed1fddcb324560a8f055b0e97 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Jul 2012 16:04:04 -0700 Subject: [PATCH 06/52] Add an Aphlict CLI client Summary: Depends on D2926. Adds a simple CLI client for Aphlict to make it easier to debug stuff. Test Plan: Ran client, saw debug messages. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2927 --- src/docs/userguide/notifications.diviner | 4 ++ .../aphlict/client/aphlict_test_client.php | 72 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100755 support/aphlict/client/aphlict_test_client.php diff --git a/src/docs/userguide/notifications.diviner b/src/docs/userguide/notifications.diviner index f94828c541..0842cf8335 100644 --- a/src/docs/userguide/notifications.diviner +++ b/src/docs/userguide/notifications.diviner @@ -76,6 +76,10 @@ You can run `aphlict` in the foreground to get output to your console: phabricator/ $ ./bin/aphlict --foreground +You can run `support/aphlict/client/aphlict_test_client.php` to connect to the +Aphlict server from the command line. Messages the client receives will be +printed to stdout. + You can set `notification.debug` in your configuration to get additional output in your browser. diff --git a/support/aphlict/client/aphlict_test_client.php b/support/aphlict/client/aphlict_test_client.php new file mode 100755 index 0000000000..7131ef4736 --- /dev/null +++ b/support/aphlict/client/aphlict_test_client.php @@ -0,0 +1,72 @@ +#!/usr/bin/env php +setTagline('test client for Aphlict server'); +$args->setSynopsis(<<parseStandardArguments(); +$args->parse( + array( + array( + 'name' => 'server', + 'param' => 'uri', + 'default' => PhabricatorEnv::getEnvConfig('notification.client-uri'), + 'help' => 'Connect to __uri__ instead of the default server.', + ), + )); +$console = PhutilConsole::getConsole(); + +$errno = null; +$errstr = null; + +$uri = $args->getArg('server'); +$uri = new PhutilURI($uri); +$uri->setProtocol('tcp'); + +$console->writeErr("Connecting...\n"); +$socket = stream_socket_client( + $uri, + $errno, + $errstr); + +if (!$socket) { + $console->writeErr( + "Unable to connect to Aphlict (at '$uri'). Error #{$errno}: {$errstr}"); + exit(1); +} else { + $console->writeErr("Connected.\n"); +} + +$io_channel = new PhutilSocketChannel($socket); +$proto_channel = new PhutilJSONProtocolChannel($io_channel); + +$json = new PhutilJSON(); +while (true) { + $message = $proto_channel->waitForMessage(); + $console->writeOut($json->encodeFormatted($message)); +} + From 731e0df2b5ccc2749646a46f865662b11739e823 Mon Sep 17 00:00:00 2001 From: Alan Huang Date: Thu, 5 Jul 2012 16:47:04 -0700 Subject: [PATCH 07/52] Add a differential.abandon Conduit method. Summary: Brazenly copied from differential.close. Ulterior motive is to be able to automate discarding of useless commits from arcanist testing. Test Plan: Called method in phabricator sandbox. Revisions were successfully abandoned. Reviewers: epriestley Reviewed By: epriestley CC: nh, aran, Korvin Differential Revision: https://secure.phabricator.com/D2928 --- .../ConduitAPI_differential_createcomment_Method.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php index 43226e21cb..b986e9834a 100644 --- a/src/applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php @@ -29,7 +29,8 @@ final class ConduitAPI_differential_createcomment_Method public function defineParamTypes() { return array( 'revision_id' => 'required revisionid', - 'message' => 'required string', + 'message' => 'optional string', + 'action' => 'optional string', ); } @@ -54,10 +55,15 @@ final class ConduitAPI_differential_createcomment_Method PhabricatorContentSource::SOURCE_CONDUIT, array()); + $action = $request->getValue('action'); + if (!$action) { + $action = 'none'; + } + $editor = new DifferentialCommentEditor( $revision, $request->getUser()->getPHID(), - DifferentialAction::ACTION_COMMENT); + $action); $editor->setContentSource($content_source); $editor->setMessage($request->getValue('message')); $editor->save(); From 1b8ed98ddcc9537b1e7d38cc1687b742acc4a0d6 Mon Sep 17 00:00:00 2001 From: Miles Shang Date: Wed, 4 Jul 2012 14:04:45 -0700 Subject: [PATCH 08/52] Multi-line highlighting in Diffusion. Summary: Currently, Diffusion supports highlighting of line ranges passed in the URI. It would be helpful to be able to highlight multiple line ranges. Test Plan: Accessed directly via URL in my sandbox. Seems to work. I'm not sure what other components use this functionality, but this change should be backwards compatible. Reviewers: epriestley Reviewed By: epriestley CC: aran, epriestley, Korvin Differential Revision: https://secure.phabricator.com/D2921 --- .../DiffusionBrowseFileController.php | 36 ++++++++++++------- .../diffusion/request/DiffusionRequest.php | 2 +- .../__tests__/DiffusionURITestCase.php | 13 +++++++ 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index 6207cb9e09..e6822d8667 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -293,15 +293,22 @@ final class DiffusionBrowseFileController extends DiffusionController { $epoch_range = ($epoch_max - $epoch_min) + 1; } - $min_line = 0; - $line = $drequest->getLine(); - if (strpos($line, '-') !== false) { - list($min, $max) = explode('-', $line, 2); - $min_line = min($min, $max); - $max_line = max($min, $max); - } else if (strlen($line)) { - $min_line = $line; - $max_line = $line; + $line_arr = array(); + $line_str = $drequest->getLine(); + $ranges = explode(',', $line_str); + foreach ($ranges as $range) { + if (strpos($range, '-') !== false) { + list($min, $max) = explode('-', $range, 2); + $line_arr[] = array( + 'min' => min($min, $max), + 'max' => max($min, $max), + ); + } else if (strlen($range)) { + $line_arr[] = array( + 'min' => $range, + 'max' => $range, + ); + } } $display = array(); @@ -366,12 +373,15 @@ final class DiffusionBrowseFileController extends DiffusionController { } } - if ($min_line) { - if ($line_number == $min_line) { + if ($line_arr) { + if ($line_number == $line_arr[0]['min']) { $display_line['target'] = true; } - if ($line_number >= $min_line && $line_number <= $max_line) { - $display_line['highlighted'] = true; + foreach ($line_arr as $range) { + if ($line_number >= $range['min'] && + $line_number <= $range['max']) { + $display_line['highlighted'] = true; + } } } diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index e179c38d12..5a02a88fb8 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -475,7 +475,7 @@ abstract class DiffusionRequest { // Consume the back part of the URI, up to the first "$". Use a negative // lookbehind to prevent matching '$$'. We double the '$' symbol when // encoding so that files with names like "money/$100" will survive. - $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d-]+)$@'; + $pattern = '@(?:(?:^|[^$])(?:[$][$])*)[$]([\d-,]+)$@'; if (preg_match($pattern, $blob, $matches)) { $result['line'] = $matches[1]; $blob = substr($blob, 0, -(strlen($matches[1]) + 1)); diff --git a/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php b/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php index 84e18a3d33..3a4e3cce32 100644 --- a/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php +++ b/src/applications/diffusion/request/__tests__/DiffusionURITestCase.php @@ -55,6 +55,12 @@ final class DiffusionURITestCase extends ArcanistPhutilTestCase { 'commit' => '$;;semicolon;;$$', 'line' => '100', ), + 'branch/path.ext;abc$3-5,7-12,14' => array( + 'branch' => 'branch', + 'path' => 'path.ext', + 'commit' => 'abc', + 'line' => '3-5,7-12,14', + ), ); foreach ($map as $input => $expect) { @@ -140,6 +146,13 @@ final class DiffusionURITestCase extends ArcanistPhutilTestCase { 'path' => 'path/to/file.ext', 'commit' => 'abc', ), + '/diffusion/A/browse/branch/path.ext$3-5%2C7-12%2C14' => array( + 'action' => 'browse', + 'callsign' => 'A', + 'branch' => 'branch', + 'path' => 'path.ext', + 'line' => '3-5,7-12,14', + ), ); foreach ($map as $expect => $input) { From 18cfab0c36967d1b90e92d946a2c3c49d32ebd28 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Jul 2012 15:39:43 -0700 Subject: [PATCH 09/52] Allow configuration of the default monospaced font style Summary: This is a fairly contentious default that we can easily move to configuration. Test Plan: Changed the default, changed my user setting, reverted my user setting, verified the "settings" page. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2935 --- conf/default.conf.php | 5 +++ ...rUserPreferenceSettingsPanelController.php | 6 ++-- src/view/page/PhabricatorStandardPageView.php | 36 +++++++++---------- .../application/base/standard-page-view.css | 5 --- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index c0a80ce958..e09b4dfc37 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -1154,4 +1154,9 @@ return array( '@\.arcconfig$@' => 'js', ), + // Set the default monospaced font style for users who haven't set a custom + // style. + 'style.monospace' => '10px "Menlo", "Consolas", "Monaco", monospace', + + ); diff --git a/src/applications/people/controller/settings/panels/PhabricatorUserPreferenceSettingsPanelController.php b/src/applications/people/controller/settings/panels/PhabricatorUserPreferenceSettingsPanelController.php index 1edd725ae1..f650a76008 100644 --- a/src/applications/people/controller/settings/panels/PhabricatorUserPreferenceSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/PhabricatorUserPreferenceSettingsPanelController.php @@ -59,6 +59,9 @@ EXAMPLE; ), 'User Guide: Configuring an External Editor'); + $font_default = PhabricatorEnv::getEnvConfig('style.monospace'); + $font_default = phutil_escape_html($font_default); + $form = id(new AphrontFormView()) ->setUser($user) ->setAction('/settings/page/preferences/') @@ -90,8 +93,7 @@ EXAMPLE; ->setName($pref_monospaced) ->setCaption( 'Overrides default fonts in tools like Differential. '. - '(Default: 10px "Menlo", "Consolas", "Monaco", '. - 'monospace)') + '(Default: '.$font_default.')') ->setValue($preferences->getPreference($pref_monospaced))) ->appendChild( id(new AphrontFormMarkupControl()) diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index cdc21e0dcb..ba4ef9fb33 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -209,33 +209,31 @@ final class PhabricatorStandardPageView extends AphrontPageView { } $response = CelerityAPI::getStaticResourceResponse(); + + $monospaced = PhabricatorEnv::getEnvConfig('style.monospace'); + + $request = $this->getRequest(); + if ($request) { + $user = $request->getUser(); + if ($user) { + $monospaced = nonempty( + $user->loadPreferences()->getPreference( + PhabricatorUserPreferences::PREFERENCE_MONOSPACED), + $monospaced); + } + } + $head = ''. $response->renderResourcesOfType('css'). + ''. $response->renderSingleResource('javelin-magical-init'); - $request = $this->getRequest(); - if ($request) { - $user = $request->getUser(); - if ($user) { - $monospaced = $user->loadPreferences()->getPreference( - PhabricatorUserPreferences::PREFERENCE_MONOSPACED - ); - - if (strlen($monospaced)) { - $head .= - ''; - } - } - } - return $head; } diff --git a/webroot/rsrc/css/application/base/standard-page-view.css b/webroot/rsrc/css/application/base/standard-page-view.css index 5793d5bac2..2861d69357 100644 --- a/webroot/rsrc/css/application/base/standard-page-view.css +++ b/webroot/rsrc/css/application/base/standard-page-view.css @@ -207,11 +207,6 @@ a.handle-disabled { background-image: url(/rsrc/image/icon/fatcow/bullet_black.png); } -.PhabricatorMonospaced { - font-family: "Menlo", "Consolas", "Monaco", monospace; - font-size: 10px; -} - .aphront-developer-error-callout { padding: 2em; background: #aa0000; From 3e15c3580daa90a54c0771dec0f8d7d574fb5c0d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:38:25 -0700 Subject: [PATCH 10/52] Simplify build file from data-or-hash code Summary: We have a bit more copy-paste than we need, consolidate a bit. (Also switch Mercurial to download git diffs, which it handles well; we use them in "arc patch".) Test Plan: - Downloaded a raw diff from Differential. - Downloaded a raw change from Diffusion. - Downloaded a raw file from Diffusion. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2942 --- .../DifferentialRevisionViewController.php | 51 +++++++------------ .../DiffusionBrowseFileController.php | 24 ++------- .../controller/DiffusionCommitController.php | 22 ++------ .../files/storage/PhabricatorFile.php | 39 ++++++++++++++ 4 files changed, 68 insertions(+), 68 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 3b5a157d29..5b5c824969 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -940,49 +940,36 @@ final class DifferentialRevisionViewController extends DifferentialController { $vcs = $repository ? $repository->getVersionControlSystem() : null; switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: $raw_diff = $bundle->toGitPatch(); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: default: $raw_diff = $bundle->toUnifiedDiff(); break; } - $hash = PhabricatorHash::digest($raw_diff); + $request_uri = $this->getRequest()->getRequestURI(); - $file = id(new PhabricatorFile())->loadOneWhere( - 'contentHash = %s LIMIT 1', - $hash); - - if (!$file) { - $request_uri = $this->getRequest()->getRequestURI(); - - // this ends up being something like - // D123.diff - // or the verbose - // D123.vs123.id123.whitespaceignore-all.diff - // lame but nice to include these options - $file_name = ltrim($request_uri->getPath(), '/') . '.'; - foreach ($request_uri->getQueryParams() as $key => $value) { - if ($key == 'download') { - continue; - } - $file_name .= $key . $value . '.'; + // this ends up being something like + // D123.diff + // or the verbose + // D123.vs123.id123.whitespaceignore-all.diff + // lame but nice to include these options + $file_name = ltrim($request_uri->getPath(), '/').'.'; + foreach ($request_uri->getQueryParams() as $key => $value) { + if ($key == 'download') { + continue; } - $file_name .= 'diff'; - - // We're just caching the data; this is always safe. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileData( - $raw_diff, - array( - 'name' => $file_name, - )); - - unset($unguarded); + $file_name .= $key.$value.'.'; } + $file_name .= 'diff'; + + $file = PhabricatorFile::buildFromFileDataOrHash( + $raw_diff, + array( + 'name' => $file_name, + )); return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index e6822d8667..cb04b5eb06 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -639,25 +639,11 @@ final class DiffusionBrowseFileController extends DiffusionController { } private function loadFileForData($path, $data) { - $hash = PhabricatorHash::digest($data); - - $file = id(new PhabricatorFile())->loadOneWhere( - 'contentHash = %s LIMIT 1', - $hash); - if (!$file) { - // We're just caching the data; this is always safe. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileData( - $data, - array( - 'name' => basename($path), - )); - - unset($unguarded); - } - - return $file; + return PhabricatorFile::buildFromFileDataOrHash( + $data, + array( + 'name' => basename($path), + )); } private function buildRawResponse($path, $data) { diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 893f0c308b..62d8b7023e 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -872,23 +872,11 @@ final class DiffusionCommitController extends DiffusionController { $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); $raw_diff = $raw_query->loadRawDiff(); - $hash = PhabricatorHash::digest($raw_diff); - - $file = id(new PhabricatorFile())->loadOneWhere( - 'contentHash = %s LIMIT 1', - $hash); - if (!$file) { - // We're just caching the data; this is always safe. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileData( - $raw_diff, - array( - 'name' => $drequest->getCommit().'.diff', - )); - - unset($unguarded); - } + $file = PhabricatorFile::buildFromFileDataOrHash( + $raw_diff, + array( + 'name' => $drequest->getCommit().'.diff', + )); return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index c0be163cbd..b55d402fbc 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -101,6 +101,45 @@ final class PhabricatorFile extends PhabricatorFileDAO { } } + + /** + * Given a block of data, try to load an existing file with the same content + * if one exists. If it does not, build a new file. + * + * This method is generally used when we have some piece of semi-trusted data + * like a diff or a file from a repository that we want to show to the user. + * We can't just dump it out because it may be dangerous for any number of + * reasons; instead, we need to serve it through the File abstraction so it + * ends up on the CDN domain if one is configured and so on. However, if we + * simply wrote a new file every time we'd potentially end up with a lot + * of redundant data in file storage. + * + * To solve these problems, we use file storage as a cache and reuse the + * same file again if we've previously written it. + * + * NOTE: This method unguards writes. + * + * @param string Raw file data. + * @param dict Dictionary of file information. + */ + public static function buildFromFileDataOrHash( + $data, + array $params = array()) { + + $file = id(new PhabricatorFile())->loadOneWhere( + 'contentHash = %s LIMIT 1', + PhabricatorHash::digest($data)); + + if (!$file) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $file = PhabricatorFile::newFromFileData($data, $params); + unset($unguarded); + } + + return $file; + } + + public static function newFromFileData($data, array $params = array()) { $selector = PhabricatorEnv::newObjectFromConfig('storage.engine-selector'); From bb714a2beffb66cd384886641f298b988f0c0348 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:38:32 -0700 Subject: [PATCH 11/52] Update GitHub oauth docs Summary: GitHub moved these pages around and made it easier to find your application list. Test Plan: Read docs, verified links are correct. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2941 --- .../configuring_accounts_and_registration.diviner | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/docs/configuration/configuring_accounts_and_registration.diviner b/src/docs/configuration/configuring_accounts_and_registration.diviner index d2d933b861..f23786f808 100644 --- a/src/docs/configuration/configuring_accounts_and_registration.diviner +++ b/src/docs/configuration/configuring_accounts_and_registration.diviner @@ -93,7 +93,7 @@ nothing (the default). To configure GitHub OAuth, create a new GitHub Application: -https://github.com/account/applications/new +https://github.com/settings/applications/new You should set these things in your application: @@ -116,11 +116,6 @@ set these keys: - **github.auth-permanent**: set to ##true## to prevent unlinking Phabricator accounts from GitHub accounts. -Note that you can see a list of your GitHub applications here, although it's not -immediately clear how to get there via the UI: - -https://github.com/account/applications/ - = Configuring Google OAuth = You can configure Google OAuth to allow login, login and registration, or From 7cf6313be9576fea2d37989a515191ec99dc2c05 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:39:14 -0700 Subject: [PATCH 12/52] Add a generic object for unit tests Summary: A later diff adds unit tests against edges, but we need real objects to connect with edges. Add some trivial objects to the Harbormaster database to compliment the similar HarbormasterScratchTable. On its own, this does nothing interesting. Test Plan: Built unit tests on this in a followup. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1162 Differential Revision: https://secure.phabricator.com/D2937 --- resources/sql/patches/harbormasterobject.sql | 23 ++++++++++++ src/__phutil_library_map__.php | 2 ++ .../storage/HarbormasterObject.php | 35 +++++++++++++++++++ .../phid/PhabricatorPHIDConstants.php | 2 ++ .../patch/PhabricatorBuiltinPatchList.php | 4 +++ 5 files changed, 66 insertions(+) create mode 100644 resources/sql/patches/harbormasterobject.sql create mode 100644 src/applications/harbormaster/storage/HarbormasterObject.php diff --git a/resources/sql/patches/harbormasterobject.sql b/resources/sql/patches/harbormasterobject.sql new file mode 100644 index 0000000000..c1eb93cb2c --- /dev/null +++ b/resources/sql/patches/harbormasterobject.sql @@ -0,0 +1,23 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_object ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARCHAR(64) NOT NULL COLLATE utf8_bin, + name VARCHAR(255) COLLATE utf8_general_ci, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL +); + +CREATE TABLE {$NAMESPACE}_harbormaster.edge ( + src VARCHAR(64) NOT NULL COLLATE utf8_bin, + type VARCHAR(64) NOT NULL COLLATE utf8_bin, + dst VARCHAR(64) NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + seq INT UNSIGNED NOT NULL, + dataID INT UNSIGNED, + PRIMARY KEY (src, type, dst), + KEY (src, type, dateCreated, seq) +) ENGINE=InnoDB, COLLATE utf8_general_ci; + +CREATE TABLE {$NAMESPACE}_harbormaster.edgedata ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + data LONGTEXT NOT NULL COLLATE utf8_bin +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b2564116cb..b2a7ae4559 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -439,6 +439,7 @@ phutil_register_library_map(array( 'DrydockSSHCommandInterface' => 'applications/drydock/interface/command/DrydockSSHCommandInterface.php', 'DrydockWebrootInterface' => 'applications/drydock/interface/webroot/DrydockWebrootInterface.php', 'HarbormasterDAO' => 'applications/harbormaster/storage/HarbormasterDAO.php', + 'HarbormasterObject' => 'applications/harbormaster/storage/HarbormasterObject.php', 'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php', 'HeraldAction' => 'applications/herald/storage/HeraldAction.php', 'HeraldActionConfig' => 'applications/herald/config/HeraldActionConfig.php', @@ -1488,6 +1489,7 @@ phutil_register_library_map(array( 'DrydockSSHCommandInterface' => 'DrydockCommandInterface', 'DrydockWebrootInterface' => 'DrydockInterface', 'HarbormasterDAO' => 'PhabricatorLiskDAO', + 'HarbormasterObject' => 'HarbormasterDAO', 'HarbormasterScratchTable' => 'HarbormasterDAO', 'HeraldAction' => 'HeraldDAO', 'HeraldApplyTranscript' => 'HeraldDAO', diff --git a/src/applications/harbormaster/storage/HarbormasterObject.php b/src/applications/harbormaster/storage/HarbormasterObject.php new file mode 100644 index 0000000000..f4bac23631 --- /dev/null +++ b/src/applications/harbormaster/storage/HarbormasterObject.php @@ -0,0 +1,35 @@ + true, + ) + parent::getConfiguration(); + } + + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + PhabricatorPHIDConstants::PHID_TYPE_TOBJ); + } + +} diff --git a/src/applications/phid/PhabricatorPHIDConstants.php b/src/applications/phid/PhabricatorPHIDConstants.php index 57677d6a28..f8bd3985bb 100644 --- a/src/applications/phid/PhabricatorPHIDConstants.php +++ b/src/applications/phid/PhabricatorPHIDConstants.php @@ -40,4 +40,6 @@ final class PhabricatorPHIDConstants { const PHID_TYPE_OASC = 'OASC'; const PHID_TYPE_OASA = 'OASA'; const PHID_TYPE_POST = 'POST'; + const PHID_TYPE_TOBJ = 'TOBJ'; + } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index ccfc1200aa..b78fed7c8a 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -899,6 +899,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('differentialbookmarks.sql'), ), + 'harbormasterobject.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('harbormasterobject.sql'), + ), ); } From d86c4e03664e4b476942eb76993584668a4203c5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:39:21 -0700 Subject: [PATCH 13/52] Store forced connections in the Lisk connection cache Summary: In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally. Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them. Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1162 Differential Revision: https://secure.phabricator.com/D2938 --- src/infrastructure/storage/lisk/LiskDAO.php | 26 ++++++++++++++------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 6382094540..a1d832d241 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -302,9 +302,18 @@ abstract class LiskDAO { */ protected function setEstablishedConnection( $mode, - AphrontDatabaseConnection $connection) { + AphrontDatabaseConnection $connection, + $force_unique = false) { $key = $this->getConnectionNamespace().':'.$mode; + + if ($force_unique) { + $key .= ':unique'; + while (isset(self::$connections[$key])) { + $key .= '!'; + } + } + self::$connections[$key] = $connection; return $this; } @@ -483,7 +492,7 @@ abstract class LiskDAO { * * @task load */ - public function loadAllWhere($pattern/*, $arg, $arg, $arg ... */) { + public function loadAllWhere($pattern/* , $arg, $arg, $arg ... */) { $args = func_get_args(); array_unshift($args, null); $data = call_user_func_array( @@ -503,7 +512,7 @@ abstract class LiskDAO { * * @task load */ - public function loadColumnsWhere(array $columns, $pattern/*, $args... */) { + public function loadColumnsWhere(array $columns, $pattern/* , $args... */) { if (!$this->getConfigOption(self::CONFIG_PARTIAL_OBJECTS)) { throw new BadMethodCallException( "This class does not support partial objects."); @@ -528,7 +537,7 @@ abstract class LiskDAO { * * @task load */ - public function loadOneWhere($pattern/*, $arg, $arg, $arg ... */) { + public function loadOneWhere($pattern/* , $arg, $arg, $arg ... */) { $args = func_get_args(); array_unshift($args, null); $data = call_user_func_array( @@ -549,7 +558,7 @@ abstract class LiskDAO { } - protected function loadRawDataWhere($columns, $pattern/*, $args... */) { + protected function loadRawDataWhere($columns, $pattern/* , $args... */) { $connection = $this->establishConnection('r'); $lock_clause = ''; @@ -1009,9 +1018,10 @@ abstract class LiskDAO { if (self::shouldIsolateAllLiskEffectsToTransactions()) { $connection->openTransaction(); } - if (!$force_new) { - $this->setEstablishedConnection($mode, $connection); - } + $this->setEstablishedConnection( + $mode, + $connection, + $force_unique = $force_new); } return $connection; From bf8cbf55b1f5c9baf291770bea3c6e5cedf7ca81 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:39:30 -0700 Subject: [PATCH 14/52] Namespace GlobalLocks to storage namespaces Summary: Currently, multiple unit tests that acquire global locks will interfere with each other. Namespace the locks so they don't. (Possibly we should also rename this to PhabricatorStorageNamespaceLock or something since it's not really global any more, but that's kind of unwieldy...) Test Plan: Acquired locks with --trace and verified they were namespaced properly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1162 Differential Revision: https://secure.phabricator.com/D2939 --- src/infrastructure/util/PhabricatorGlobalLock.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php index d074526f95..b5384a2298 100644 --- a/src/infrastructure/util/PhabricatorGlobalLock.php +++ b/src/infrastructure/util/PhabricatorGlobalLock.php @@ -35,6 +35,10 @@ * do_contentious_things(); * $lock->unlock(); * + * NOTE: This lock is not completely global; it is namespaced to the active + * storage namespace so that unit tests running in separate table namespaces + * are isolated from one another. + * * @task construct Constructing Locks * @task impl Implementation */ @@ -48,7 +52,8 @@ final class PhabricatorGlobalLock extends PhutilLock { public static function newLock($name) { - $full_name = 'global:'.$name; + $namespace = PhabricatorLiskDAO::getStorageNamespace(); + $full_name = 'global:'.$namespace.':'.$name; $lock = self::getLock($full_name); if (!$lock) { From 1089a48d4a66f01401049816ed88e3be034f379b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:39:38 -0700 Subject: [PATCH 15/52] Allow edges to be configured to prevent cycles Summary: Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races. Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type: - We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined. - After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit. - When we're done, we release the edge type locks. This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way. Test Plan: Ran unit tests. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1162 Differential Revision: https://secure.phabricator.com/D2940 --- src/__phutil_library_map__.php | 6 + .../__tests__/PhabricatorEdgeTestCase.php | 81 ++++++++++++ .../edges/constants/PhabricatorEdgeConfig.php | 10 ++ .../edges/editor/PhabricatorEdgeEditor.php | 121 ++++++++++++++++-- .../PhabricatorEdgeCycleException.php | 42 ++++++ .../edges/util/PhabricatorEdgeGraph.php | 50 ++++++++ 6 files changed, 298 insertions(+), 12 deletions(-) create mode 100644 src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php create mode 100644 src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php create mode 100644 src/infrastructure/edges/util/PhabricatorEdgeGraph.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b2a7ae4559..60ca2aa07f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -621,8 +621,11 @@ phutil_register_library_map(array( 'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', + 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', 'PhabricatorEdgeEditor' => 'infrastructure/edges/editor/PhabricatorEdgeEditor.php', + 'PhabricatorEdgeGraph' => 'infrastructure/edges/util/PhabricatorEdgeGraph.php', 'PhabricatorEdgeQuery' => 'infrastructure/edges/query/PhabricatorEdgeQuery.php', + 'PhabricatorEdgeTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php', 'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php', 'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php', 'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php', @@ -1643,7 +1646,10 @@ phutil_register_library_map(array( 'PhabricatorDraft' => 'PhabricatorDraftDAO', 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', + 'PhabricatorEdgeCycleException' => 'Exception', + 'PhabricatorEdgeGraph' => 'AbstractDirectedGraph', 'PhabricatorEdgeQuery' => 'PhabricatorQuery', + 'PhabricatorEdgeTestCase' => 'PhabricatorTestCase', 'PhabricatorEmailLoginController' => 'PhabricatorAuthController', 'PhabricatorEmailTokenController' => 'PhabricatorAuthController', 'PhabricatorEmailVerificationController' => 'PhabricatorPeopleController', diff --git a/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php new file mode 100644 index 0000000000..ac07e3aaeb --- /dev/null +++ b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php @@ -0,0 +1,81 @@ + true, + ); + } + + public function testCycleDetection() { + + // The editor should detect that this introduces a cycle and prevent the + // edit. + + $user = new PhabricatorUser(); + + $obj1 = id(new HarbormasterObject())->save(); + $obj2 = id(new HarbormasterObject())->save(); + $phid1 = $obj1->getPHID(); + $phid2 = $obj2->getPHID(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + + + // The first edit should go through (no cycle), bu the second one should + // fail (it introduces a cycle). + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->save(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + } + + +} diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 0dfa2e34f9..627aa2277a 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -24,6 +24,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_TASK_HAS_COMMIT = 1; const TYPE_COMMIT_HAS_TASK = 2; + const TYPE_TEST_NO_CYCLE = 9000; + public static function getInverse($edge_type) { static $map = array( self::TYPE_TASK_HAS_COMMIT => self::TYPE_COMMIT_HAS_TASK, @@ -33,6 +35,13 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return idx($map, $edge_type); } + public static function shouldPreventCycles($edge_type) { + static $map = array( + self::TYPE_TEST_NO_CYCLE => true, + ); + return isset($map[$edge_type]); + } + public static function establishConnection($phid_type, $conn_type) { static $class_map = array( PhabricatorPHIDConstants::PHID_TYPE_TASK => 'ManiphestTask', @@ -43,6 +52,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { PhabricatorPHIDConstants::PHID_TYPE_PROJ => 'PhabricatorProject', PhabricatorPHIDConstants::PHID_TYPE_MLST => 'PhabricatorMetaMTAMailingList', + PhabricatorPHIDConstants::PHID_TYPE_TOBJ => 'HarbormasterObject', ); $class = idx($class_map, $phid_type); diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index 77653e89be..f4f7c811c3 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -32,6 +32,7 @@ * ->save(); * * @task edit Editing Edges + * @task cycles Cycle Prevention * @task internal Internals */ final class PhabricatorEdgeEditor { @@ -113,26 +114,61 @@ final class PhabricatorEdgeEditor { */ public function save() { - // NOTE: We write edge data first, before doing any transactions, since - // it's OK if we just leave it hanging out in space unattached to anything. + $cycle_types = $this->getPreventCyclesEdgeTypes(); - $this->writeEdgeData(); + $locks = array(); + $caught = null; + try { - static $id = 0; - $id++; + // NOTE: We write edge data first, before doing any transactions, since + // it's OK if we just leave it hanging out in space unattached to + // anything. + $this->writeEdgeData(); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); + // If we're going to perform cycle detection, lock the edge type before + // doing edits. + if ($cycle_types) { + $src_phids = ipull($this->addEdges, 'src'); + foreach ($cycle_types as $cycle_type) { + $key = 'edge.cycle:'.$cycle_type; + $locks[] = PhabricatorGlobalLock::newLock($key)->lock(15); + } + } - // NOTE: Removes first, then adds, so that "remove + add" is a useful - // operation meaning "overwrite". + static $id = 0; + $id++; - $this->executeRemoves(); - $this->executeAdds(); + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + // NOTE: Removes first, then adds, so that "remove + add" is a useful + // operation meaning "overwrite". + + $this->executeRemoves(); + $this->executeAdds(); + + foreach ($cycle_types as $cycle_type) { + $this->detectCycles($src_phids, $cycle_type); + } + + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + + $this->saveTransactions(); + } catch (Exception $ex) { + $caught = $ex; + } - $this->saveTransactions(); + if ($caught) { + $this->killTransactions(); + } + + foreach ($locks as $lock) { + $lock->unlock(); + } + + if ($caught) { + throw $caught; + } } @@ -327,6 +363,13 @@ final class PhabricatorEdgeEditor { } } + private function killTransactions() { + foreach ($this->openTransactions as $key => $conn_w) { + $conn_w->killTransaction(); + unset($this->openTransactions[$key]); + } + } + private function sendEvent($edit_id, $event_type) { $event = new PhabricatorEvent( $event_type, @@ -339,4 +382,58 @@ final class PhabricatorEdgeEditor { PhutilEventEngine::dispatchEvent($event); } + +/* -( Cycle Prevention )--------------------------------------------------- */ + + + /** + * Get a list of all edge types which are being added, and which we should + * prevent cycles on. + * + * @return list List of edge types which should have cycles prevented. + * @task cycle + */ + private function getPreventCyclesEdgeTypes() { + $edge_types = array(); + foreach ($this->addEdges as $edge) { + $edge_types[$edge['type']] = true; + } + foreach ($edge_types as $type => $ignored) { + if (!PhabricatorEdgeConfig::shouldPreventCycles($type)) { + unset($edge_types[$type]); + } + } + return array_keys($edge_types); + } + + + /** + * Detect graph cycles of a given edge type. If the edit introduces a cycle, + * a @{class:PhabricatorEdgeCycleException} is thrown with details. + * + * @return void + * @task cycle + */ + private function detectCycles(array $phids, $edge_type) { + // For simplicity, we just seed the graph with the affected nodes rather + // than seeding it with their edges. To do this, we just add synthetic + // edges from an imaginary '' node to the known edges. + + + $graph = id(new PhabricatorEdgeGraph()) + ->setEdgeType($edge_type) + ->addNodes( + array( + '' => $phids, + )) + ->loadGraph(); + + foreach ($phids as $phid) { + $cycle = $graph->detectCycles($phid); + if ($cycle) { + throw new PhabricatorEdgeCycleException($edge_type, $cycle); + } + } + } + } diff --git a/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php new file mode 100644 index 0000000000..98c7561748 --- /dev/null +++ b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php @@ -0,0 +1,42 @@ +cycleEdgeType = $cycle_edge_type; + $this->cycle = $cycle; + + $cycle_list = implode(', ', $cycle); + + parent::__construct( + "Graph cycle detected (type={$cycle_edge_type}, cycle={$cycle_list})."); + } + + public function getCycle() { + return $this->cycle; + } + + public function getCycleEdgeType() { + return $this->cycleEdgeType; + } + +} diff --git a/src/infrastructure/edges/util/PhabricatorEdgeGraph.php b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php new file mode 100644 index 0000000000..5cd3bfc5bb --- /dev/null +++ b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php @@ -0,0 +1,50 @@ +edgeType = $edge_type; + return $this; + } + + protected function loadEdges(array $nodes) { + if (!$this->edgeType) { + throw new Exception("Set edge type before loading graph!"); + } + + $edges = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($nodes) + ->withEdgeTypes(array($this->edgeType)) + ->execute(); + + $results = array_fill_keys($nodes, array()); + foreach ($edges as $src => $types) { + foreach ($types as $type => $dsts) { + foreach ($dsts as $dst => $edge) { + $results[$src][] = $dst; + } + } + } + + return $results; + } + +} From 918ac5755c782f39b32e833efd20c853c312c616 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:51:46 -0700 Subject: [PATCH 16/52] Limit maximum size of Owners package queries Summary: Currently, a change may affect a very large number of paths. When we run the OwnersWorker on it, we'll execute a query which looks up packages for the paths. This may exceed "max_allowed_packet". Instead, break the list of paths into smaller chunks. This is mostly to unblock r4nt / llvm, I'm going to add a more finessed approach to array_chunk() shortly. Test Plan: Ran `reparse.php --owners` on a revision which affected packages, verified results are the same before and after the change. Set chunk size to 1, verified query results aggregated properly. Reviewers: btrahan, jungejason, nh Reviewed By: jungejason CC: aran Differential Revision: https://secure.phabricator.com/D2943 --- .../storage/PhabricatorOwnersPackage.php | 63 +++++++++++-------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 82e2deae3a..aad671425c 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -142,40 +142,51 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO { $path = new PhabricatorOwnersPath(); $conn = $package->establishConnection('r'); - $repository_clause = qsprintf($conn, 'AND p.repositoryPHID = %s', + $repository_clause = qsprintf( + $conn, + 'AND p.repositoryPHID = %s', $repository->getPHID()); - $limit_clause = ''; - if (!empty($limit)) { - $limit_clause = qsprintf($conn, 'LIMIT %d', $limit); - } + // NOTE: The list of $paths may be very large if we're coming from + // the OwnersWorker and processing, e.g., an SVN commit which created a new + // branch. Break it apart so that it will fit within 'max_allowed_packet', + // and then merge results in PHP. - $data = queryfx_all( - $conn, - 'SELECT pkg.id FROM %T pkg JOIN %T p ON p.packageID = pkg.id - WHERE p.path IN (%Ls) %Q ORDER BY LENGTH(p.path) DESC %Q', - $package->getTableName(), - $path->getTableName(), - $paths, - $repository_clause, - $limit_clause); + $ids = array(); + foreach (array_chunk($paths, 128) as $chunk) { + $rows = queryfx_all( + $conn, + 'SELECT pkg.id id, LENGTH(p.path) len + FROM %T pkg JOIN %T p ON p.packageID = pkg.id + WHERE p.path IN (%Ls) %Q', + $package->getTableName(), + $path->getTableName(), + $chunk, + $repository_clause); - $ids = ipull($data, 'id'); - - if (empty($ids)) { - return array(); - } - - $order = array(); - foreach ($ids as $id) { - if (empty($order[$id])) { - $order[$id] = true; + foreach ($rows as $row) { + $id = (int)$row['id']; + $len = (int)$row['len']; + if (isset($ids[$id])) { + $ids[$id] = max($len, $ids[$id]); + } else { + $ids[$id] = $len; + } } } - $packages = $package->loadAllWhere('id in (%Ld)', array_keys($order)); + if (!$ids) { + return array(); + } - $packages = array_select_keys($packages, array_keys($order)); + arsort($ids); + if ($limit) { + $ids = array_slice($ids, 0, $limit, $preserve_keys = true); + } + $ids = array_keys($ids); + + $packages = $package->loadAllWhere('id in (%Ld)', array_keys($ids)); + $packages = array_select_keys($packages, array_keys($ids)); return $packages; } From 2b0b9a15734ca7b885fd2fe249c7f65a16d72cba Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 15:20:56 -0700 Subject: [PATCH 17/52] Add a generic multistep Markup cache Summary: The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive. The broader issue is that out markup caches aren't very good right now. They have three major problems: **Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different. To solve this, I created a dedicated cache database that I plan to move all markup caches to use. **Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks. To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases. This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering. **Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward. Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered). I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON. Test Plan: - Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table). - Verified that published documents come out of cache. - Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1366 Differential Revision: https://secure.phabricator.com/D2945 --- resources/sql/patches/harbormasterobject.sql | 2 +- resources/sql/patches/markupcache.sql | 10 + src/__phutil_library_map__.php | 11 +- .../cache/storage/PhabricatorCacheDAO.php | 25 ++ .../cache/storage/PhabricatorMarkupCache.php | 34 ++ .../controller/PhrictionHistoryController.php | 4 +- .../phriction/storage/PhrictionContent.php | 66 +++- .../markup/PhabricatorMarkupEngine.php | 291 +++++++++++++++++- .../markup/PhabricatorMarkupInterface.php | 103 +++++++ .../patch/PhabricatorBuiltinPatchList.php | 8 + 10 files changed, 534 insertions(+), 20 deletions(-) create mode 100644 resources/sql/patches/markupcache.sql create mode 100644 src/applications/cache/storage/PhabricatorCacheDAO.php create mode 100644 src/applications/cache/storage/PhabricatorMarkupCache.php create mode 100644 src/infrastructure/markup/PhabricatorMarkupInterface.php diff --git a/resources/sql/patches/harbormasterobject.sql b/resources/sql/patches/harbormasterobject.sql index c1eb93cb2c..f252ae8e5e 100644 --- a/resources/sql/patches/harbormasterobject.sql +++ b/resources/sql/patches/harbormasterobject.sql @@ -4,7 +4,7 @@ CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_object ( name VARCHAR(255) COLLATE utf8_general_ci, dateCreated INT UNSIGNED NOT NULL, dateModified INT UNSIGNED NOT NULL -); +) ENGINE=InnoDB, COLLATE utf8_general_ci; CREATE TABLE {$NAMESPACE}_harbormaster.edge ( src VARCHAR(64) NOT NULL COLLATE utf8_bin, diff --git a/resources/sql/patches/markupcache.sql b/resources/sql/patches/markupcache.sql new file mode 100644 index 0000000000..e13a236f25 --- /dev/null +++ b/resources/sql/patches/markupcache.sql @@ -0,0 +1,10 @@ +CREATE TABLE {$NAMESPACE}_cache.cache_markupcache ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + cacheKey VARCHAR(128) NOT NULL collate utf8_bin, + cacheData LONGTEXT NOT NULL COLLATE utf8_bin, + metadata LONGTEXT NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY (cacheKey), + KEY (dateCreated) +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 60ca2aa07f..a7510a5d9d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -563,6 +563,7 @@ phutil_register_library_map(array( 'PhabricatorAuthController' => 'applications/auth/controller/PhabricatorAuthController.php', 'PhabricatorBaseEnglishTranslation' => 'infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php', 'PhabricatorBuiltinPatchList' => 'infrastructure/storage/patch/PhabricatorBuiltinPatchList.php', + 'PhabricatorCacheDAO' => 'applications/cache/storage/PhabricatorCacheDAO.php', 'PhabricatorCalendarBrowseController' => 'applications/calendar/controller/PhabricatorCalendarBrowseController.php', 'PhabricatorCalendarController' => 'applications/calendar/controller/PhabricatorCalendarController.php', 'PhabricatorCalendarDAO' => 'applications/calendar/storage/PhabricatorCalendarDAO.php', @@ -738,7 +739,9 @@ phutil_register_library_map(array( 'PhabricatorMailImplementationSendGridAdapter' => 'applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php', 'PhabricatorMailImplementationTestAdapter' => 'applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php', 'PhabricatorMailReplyHandler' => 'applications/metamta/replyhandler/PhabricatorMailReplyHandler.php', + 'PhabricatorMarkupCache' => 'applications/cache/storage/PhabricatorMarkupCache.php', 'PhabricatorMarkupEngine' => 'infrastructure/markup/PhabricatorMarkupEngine.php', + 'PhabricatorMarkupInterface' => 'infrastructure/markup/PhabricatorMarkupInterface.php', 'PhabricatorMercurialGraphStream' => 'applications/repository/daemon/PhabricatorMercurialGraphStream.php', 'PhabricatorMetaMTAAttachment' => 'applications/metamta/storage/PhabricatorMetaMTAAttachment.php', 'PhabricatorMetaMTAController' => 'applications/metamta/controller/PhabricatorMetaMTAController.php', @@ -1590,6 +1593,7 @@ phutil_register_library_map(array( 'PhabricatorAuthController' => 'PhabricatorController', 'PhabricatorBaseEnglishTranslation' => 'PhabricatorTranslation', 'PhabricatorBuiltinPatchList' => 'PhabricatorSQLPatchList', + 'PhabricatorCacheDAO' => 'PhabricatorLiskDAO', 'PhabricatorCalendarBrowseController' => 'PhabricatorCalendarController', 'PhabricatorCalendarController' => 'PhabricatorController', 'PhabricatorCalendarDAO' => 'PhabricatorLiskDAO', @@ -1742,6 +1746,7 @@ phutil_register_library_map(array( 'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'PhabricatorMailImplementationAdapter', 'PhabricatorMailImplementationSendGridAdapter' => 'PhabricatorMailImplementationAdapter', 'PhabricatorMailImplementationTestAdapter' => 'PhabricatorMailImplementationAdapter', + 'PhabricatorMarkupCache' => 'PhabricatorCacheDAO', 'PhabricatorMetaMTAController' => 'PhabricatorController', 'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO', 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase', @@ -2037,7 +2042,11 @@ phutil_register_library_map(array( 'PhrictionDAO' => 'PhabricatorLiskDAO', 'PhrictionDeleteController' => 'PhrictionController', 'PhrictionDiffController' => 'PhrictionController', - 'PhrictionDocument' => 'PhrictionDAO', + 'PhrictionDocument' => + array( + 0 => 'PhrictionDAO', + 1 => 'PhabricatorMarkupInterface', + ), 'PhrictionDocumentController' => 'PhrictionController', 'PhrictionDocumentPreviewController' => 'PhrictionController', 'PhrictionDocumentStatus' => 'PhrictionConstants', diff --git a/src/applications/cache/storage/PhabricatorCacheDAO.php b/src/applications/cache/storage/PhabricatorCacheDAO.php new file mode 100644 index 0000000000..4d4dc4a72f --- /dev/null +++ b/src/applications/cache/storage/PhabricatorCacheDAO.php @@ -0,0 +1,25 @@ + array( + 'cacheData' => self::SERIALIZATION_PHP, + 'metadata' => self::SERIALIZATION_JSON, + ), + ) + parent::getConfiguration(); + } + +} diff --git a/src/applications/phriction/controller/PhrictionHistoryController.php b/src/applications/phriction/controller/PhrictionHistoryController.php index c6af894098..e266a6d7a0 100644 --- a/src/applications/phriction/controller/PhrictionHistoryController.php +++ b/src/applications/phriction/controller/PhrictionHistoryController.php @@ -61,7 +61,7 @@ final class PhrictionHistoryController $rows = array(); foreach ($history as $content) { - $uri = PhrictionDocument::getSlugURI($document->getSlug()); + $slug_uri = PhrictionDocument::getSlugURI($document->getSlug()); $version = $content->getVersion(); $diff_uri = new PhutilURI('/phriction/diff/'.$document->getID().'/'); @@ -102,7 +102,7 @@ final class PhrictionHistoryController phutil_render_tag( 'a', array( - 'href' => $uri.'?v='.$version, + 'href' => $slug_uri.'?v='.$version, ), 'Version '.$version), $handles[$content->getAuthorPHID()]->renderLink(), diff --git a/src/applications/phriction/storage/PhrictionContent.php b/src/applications/phriction/storage/PhrictionContent.php index b03e731906..da84fb1ce0 100644 --- a/src/applications/phriction/storage/PhrictionContent.php +++ b/src/applications/phriction/storage/PhrictionContent.php @@ -17,9 +17,14 @@ */ /** + * @task markup Markup Interface + * * @group phriction */ -final class PhrictionContent extends PhrictionDAO { +final class PhrictionContent extends PhrictionDAO + implements PhabricatorMarkupInterface { + + const MARKUP_FIELD_BODY = 'markup:body'; protected $id; protected $documentID; @@ -35,11 +40,55 @@ final class PhrictionContent extends PhrictionDAO { protected $changeRef; public function renderContent() { - $engine = PhabricatorMarkupEngine::newPhrictionMarkupEngine(); - $markup = $engine->markupText($this->getContent()); + return PhabricatorMarkupEngine::renderOneObject( + $this, + self::MARKUP_FIELD_BODY); + } + + +/* -( Markup Interface )--------------------------------------------------- */ + + + /** + * @task markup + */ + public function getMarkupFieldKey($field) { + if ($this->shouldUseMarkupCache($field)) { + $id = $this->getID(); + } else { + $id = PhabricatorHash::digest($this->getMarkupText($field)); + } + return "phriction:{$field}:{$id}"; + } + + + /** + * @task markup + */ + public function getMarkupText($field) { + return $this->getContent(); + } + + + /** + * @task markup + */ + public function newMarkupEngine($field) { + return PhabricatorMarkupEngine::newPhrictionMarkupEngine(); + } + + + /** + * @task markup + */ + public function didMarkupText( + $field, + $output, + PhutilMarkupEngine $engine) { $toc = PhutilRemarkupEngineRemarkupHeaderBlockRule::renderTableOfContents( $engine); + if ($toc) { $toc = '
'. @@ -53,8 +102,17 @@ final class PhrictionContent extends PhrictionDAO { return '
'. $toc. - $markup. + $output. '
'; } + + /** + * @task markup + */ + public function shouldUseMarkupCache($field) { + return (bool)$this->getID(); + } + + } diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index c64527167e..fd03ef3178 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -16,41 +16,268 @@ * limitations under the License. */ -class PhabricatorMarkupEngine { +/** + * Manages markup engine selection, configuration, application, caching and + * pipelining. + * + * @{class:PhabricatorMarkupEngine} can be used to render objects which + * implement @{interface:PhabricatorMarkupInterface} in a batched, cache-aware + * way. For example, if you have a list of comments written in remarkup (and + * the objects implement the correct interface) you can render them by first + * building an engine and adding the fields with @{method:addObject}. + * + * $field = 'field:body'; // Field you want to render. Each object exposes + * // one or more fields of markup. + * + * $engine = new PhabricatorMarkupEngine(); + * foreach ($comments as $comment) { + * $engine->addObject($comment, $field); + * } + * + * Now, call @{method:process} to perform the actual cache/rendering + * step. This is a heavyweight call which does batched data access and + * transforms the markup into output. + * + * $engine->process(); + * + * Finally, do something with the results: + * + * $results = array(); + * foreach ($comments as $comment) { + * $results[] = $engine->getOutput($comment, $field); + * } + * + * If you have a single object to render, you can use the convenience method + * @{method:renderOneObject}. + * + * @task markup Markup Pipeline + * @task engine Engine Construction + */ +final class PhabricatorMarkupEngine { - public static function extractPHIDsFromMentions(array $content_blocks) { - $mentions = array(); + private $objects = array(); - $engine = self::newDifferentialMarkupEngine(); - foreach ($content_blocks as $content_block) { - $engine->markupText($content_block); - $phids = $engine->getTextMetadata( - PhabricatorRemarkupRuleMention::KEY_MENTIONED, - array()); - $mentions += $phids; - } +/* -( Markup Pipeline )---------------------------------------------------- */ - return $mentions; + + /** + * Convenience method for pushing a single object through the markup + * pipeline. + * + * @param PhabricatorMarkupInterface The object to render. + * @param string The field to render. + * @return string Marked up output. + * @task markup + */ + public static function renderOneObject( + PhabricatorMarkupInterface $object, + $field) { + return id(new PhabricatorMarkupEngine()) + ->addObject($object, $field) + ->process() + ->getOutput($object, $field); } + + /** + * Queue an object for markup generation when @{method:process} is + * called. You can retrieve the output later with @{method:getOutput}. + * + * @param PhabricatorMarkupInterface The object to render. + * @param string The field to render. + * @return this + * @task markup + */ + public function addObject(PhabricatorMarkupInterface $object, $field) { + $key = $this->getMarkupFieldKey($object, $field); + $this->objects[$key] = array( + 'object' => $object, + 'field' => $field, + ); + + return $this; + } + + + /** + * Process objects queued with @{method:addObject}. You can then retrieve + * the output with @{method:getOutput}. + * + * @return this + * @task markup + */ + public function process() { + $keys = array(); + foreach ($this->objects as $key => $info) { + if (!isset($info['markup'])) { + $keys[] = $key; + } + } + + if (!$keys) { + return; + } + + $objects = array_select_keys($this->objects, $keys); + + // Build all the markup engines. We need an engine for each field whether + // we have a cache or not, since we still need to postprocess the cache. + $engines = array(); + foreach ($objects as $key => $info) { + $engines[$key] = $info['object']->newMarkupEngine($info['field']); + } + + // Load or build the preprocessor caches. + $blocks = $this->loadPreprocessorCaches($engines, $objects); + + // Finalize the output. + foreach ($objects as $key => $info) { + $data = $blocks[$key]->getCacheData(); + $engine = $engines[$key]; + $field = $info['field']; + $object = $info['object']; + + $output = $engine->postprocessText($data); + $output = $object->didMarkupText($field, $output, $engine); + $this->objects[$key]['output'] = $output; + } + + return $this; + } + + + /** + * Get the output of markup processing for a field queued with + * @{method:addObject}. Before you can call this method, you must call + * @{method:process}. + * + * @param PhabricatorMarkupInterface The object to retrieve. + * @param string The field to retrieve. + * @return string Processed output. + * @task markup + */ + public function getOutput(PhabricatorMarkupInterface $object, $field) { + $key = $this->getMarkupFieldKey($object, $field); + + if (empty($this->objects[$key])) { + throw new Exception( + "Call addObject() before getOutput() (key = '{$key}')."); + } + + if (!isset($this->objects[$key]['output'])) { + throw new Exception( + "Call process() before getOutput()."); + } + + return $this->objects[$key]['output']; + } + + + /** + * @task markup + */ + private function getMarkupFieldKey( + PhabricatorMarkupInterface $object, + $field) { + return $object->getMarkupFieldKey($field); + } + + + /** + * @task markup + */ + private function loadPreprocessorCaches(array $engines, array $objects) { + $blocks = array(); + + $use_cache = array(); + foreach ($objects as $key => $info) { + if ($info['object']->shouldUseMarkupCache($info['field'])) { + $use_cache[$key] = true; + } + } + + if ($use_cache) { + $blocks = id(new PhabricatorMarkupCache())->loadAllWhere( + 'cacheKey IN (%Ls)', + array_keys($use_cache)); + $blocks = mpull($blocks, null, 'getCacheKey'); + } + + foreach ($objects as $key => $info) { + if (isset($blocks[$key])) { + // If we already have a preprocessing cache, we don't need to rebuild + // it. + continue; + } + + $text = $info['object']->getMarkupText($info['field']); + $data = $engines[$key]->preprocessText($text); + + // NOTE: This is just debugging information to help sort out cache issues. + // If one machine is misconfigured and poisoning caches you can use this + // field to hunt it down. + + $metadata = array( + 'host' => php_uname('n'), + ); + + $blocks[$key] = id(new PhabricatorMarkupCache()) + ->setCacheKey($key) + ->setCacheData($data) + ->setMetadata($metadata); + + if (isset($use_cache[$key])) { + // This is just filling a cache and always safe, even on a read pathway. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + try { + $blocks[$key]->save(); + } catch (AphrontQueryDuplicateKeyException $ex) { + // Ignore this, we just raced to write the cache. + } + unset($unguarded); + } + } + + return $blocks; + } + + +/* -( Engine Construction )------------------------------------------------ */ + + + /** + * @task engine + */ public static function newManiphestMarkupEngine() { return self::newMarkupEngine(array( )); } + + /** + * @task engine + */ public static function newPhrictionMarkupEngine() { return self::newMarkupEngine(array( 'header.generate-toc' => true, )); } + + /** + * @task engine + */ public static function newPhameMarkupEngine() { return self::newMarkupEngine(array( 'macros' => false, )); } + + /** + * @task engine + */ public static function newFeedMarkupEngine() { return self::newMarkupEngine( array( @@ -61,6 +288,10 @@ class PhabricatorMarkupEngine { )); } + + /** + * @task engine + */ public static function newDifferentialMarkupEngine(array $options = array()) { return self::newMarkupEngine(array( 'custom-inline' => PhabricatorEnv::getEnvConfig( @@ -71,21 +302,37 @@ class PhabricatorMarkupEngine { )); } + + /** + * @task engine + */ public static function newDiffusionMarkupEngine(array $options = array()) { return self::newMarkupEngine(array( )); } + + /** + * @task engine + */ public static function newProfileMarkupEngine() { return self::newMarkupEngine(array( )); } + + /** + * @task engine + */ public static function newSlowvoteMarkupEngine() { return self::newMarkupEngine(array( )); } + + /** + * @task engine + */ private static function getMarkupEngineDefaultConfiguration() { return array( 'pygments' => PhabricatorEnv::getEnvConfig('pygments.enabled'), @@ -104,6 +351,10 @@ class PhabricatorMarkupEngine { ); } + + /** + * @task engine + */ private static function newMarkupEngine(array $options) { $options += self::getMarkupEngineDefaultConfiguration(); @@ -198,4 +449,20 @@ class PhabricatorMarkupEngine { return $engine; } + public static function extractPHIDsFromMentions(array $content_blocks) { + $mentions = array(); + + $engine = self::newDifferentialMarkupEngine(); + + foreach ($content_blocks as $content_block) { + $engine->markupText($content_block); + $phids = $engine->getTextMetadata( + PhabricatorRemarkupRuleMention::KEY_MENTIONED, + array()); + $mentions += $phids; + } + + return $mentions; + } + } diff --git a/src/infrastructure/markup/PhabricatorMarkupInterface.php b/src/infrastructure/markup/PhabricatorMarkupInterface.php new file mode 100644 index 0000000000..a3c96449c2 --- /dev/null +++ b/src/infrastructure/markup/PhabricatorMarkupInterface.php @@ -0,0 +1,103 @@ + 'db', 'name' => 'xhpastview', ), + 'db.cache' => array( + 'type' => 'db', + 'name' => 'cache', + ), '0000.legacy.sql' => array( 'type' => 'sql', 'name' => $this->getPatchPath('0000.legacy.sql'), @@ -903,6 +907,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('harbormasterobject.sql'), ), + 'markupcache.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('markupcache.sql'), + ), ); } From e2e9aed4fa14dcf3e88701a06168e322d2fa251b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 17:51:42 -0700 Subject: [PATCH 18/52] Fix symbol handling in symbol query and IRC "Where is x?" handler Summary: If a symbol's project has no linked repository, we currently explode. Instead, decline to generate a URI and fall back gracefully. Test Plan: https://secure.phabricator.com/chatlog/channel/%23phabricator/?at=22345 Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1465 Differential Revision: https://secure.phabricator.com/D2948 --- .../diffusion/ConduitAPI_diffusion_findsymbols_Method.php | 8 +++++++- .../repository/storage/PhabricatorRepositorySymbol.php | 7 +++++++ .../irc/handler/PhabricatorIRCObjectNameHandler.php | 7 ++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_findsymbols_Method.php b/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_findsymbols_Method.php index 8be4467fec..e818063df8 100644 --- a/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_findsymbols_Method.php +++ b/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_findsymbols_Method.php @@ -70,15 +70,21 @@ final class ConduitAPI_diffusion_findsymbols_Method $results = $query->execute(); + $response = array(); foreach ($results as $result) { + $uri = $result->getURI(); + if ($uri) { + $uri = PhabricatorEnv::getProductionURI($uri); + } + $response[] = array( 'name' => $result->getSymbolName(), 'type' => $result->getSymbolType(), 'language' => $result->getSymbolLanguage(), 'path' => $result->getPath(), 'line' => $result->getLineNumber(), - 'uri' => PhabricatorEnv::getProductionURI($result->getURI()), + 'uri' => $uri, ); } diff --git a/src/applications/repository/storage/PhabricatorRepositorySymbol.php b/src/applications/repository/storage/PhabricatorRepositorySymbol.php index 250a1100cb..12c4726389 100644 --- a/src/applications/repository/storage/PhabricatorRepositorySymbol.php +++ b/src/applications/repository/storage/PhabricatorRepositorySymbol.php @@ -45,6 +45,13 @@ final class PhabricatorRepositorySymbol extends PhabricatorRepositoryDAO { } public function getURI() { + if (!$this->repository) { + // This symbol is in the index, but we don't know which Repository it's + // part of. Usually this means the Arcanist Project hasn't been linked + // to a Repository. We can't generate a URI, so just fail. + return null; + } + $request = DiffusionRequest::newFromDictionary( array( 'repository' => $this->getRepository(), diff --git a/src/infrastructure/daemon/irc/handler/PhabricatorIRCObjectNameHandler.php b/src/infrastructure/daemon/irc/handler/PhabricatorIRCObjectNameHandler.php index 234e837352..5ced711c94 100644 --- a/src/infrastructure/daemon/irc/handler/PhabricatorIRCObjectNameHandler.php +++ b/src/infrastructure/daemon/irc/handler/PhabricatorIRCObjectNameHandler.php @@ -240,16 +240,17 @@ final class PhabricatorIRCObjectNameHandler extends PhabricatorIRCHandler { 'name' => $symbol, )); + $default_uri = $this->getURI('/diffusion/symbol/'.$symbol.'/'); + if (count($results) > 1) { - $uri = $this->getURI('/diffusion/symbol/'.$symbol.'/'); - $response = "Multiple symbols named '{$symbol}': {$uri}"; + $response = "Multiple symbols named '{$symbol}': {$default_uri}"; } else if (count($results) == 1) { $result = head($results); $response = $result['type'].' '. $result['name'].' '. '('.$result['language'].'): '. - $result['uri']; + nonempty($result['uri'], $default_uri); } else { $response = "No symbol '{$symbol}' found anywhere."; } From 42dfee38e734097ccbe488b714cea3502fa0e58d Mon Sep 17 00:00:00 2001 From: Daniel Fullarton Date: Wed, 11 Jul 2012 00:56:38 +1000 Subject: [PATCH 19/52] Refactor methods, fix style issues --- .../PhabricatorPeopleLdapController.php | 105 +++++++----------- .../PhabricatorPeopleListController.php | 2 +- 2 files changed, 40 insertions(+), 67 deletions(-) diff --git a/src/applications/people/controller/PhabricatorPeopleLdapController.php b/src/applications/people/controller/PhabricatorPeopleLdapController.php index e0e4b3a3eb..333f8f373d 100644 --- a/src/applications/people/controller/PhabricatorPeopleLdapController.php +++ b/src/applications/people/controller/PhabricatorPeopleLdapController.php @@ -25,47 +25,16 @@ final class PhabricatorPeopleLdapController private $view; - public function willProcessRequest(array $data) { - $this->view = idx($data, 'view'); - } - public function processRequest() { $request = $this->getRequest(); $admin = $request->getUser(); - $base_uri = '/people/edit/'; - $content = array(); - - $response = $this->processBasicRequest(); - - if ($response instanceof AphrontResponse) { - return $response; - } - - $content[] = $response; - - - return $this->buildStandardPageResponse( - $content, - array( - 'title' => 'Import Ldap Users', - )); - } - - /** - * Displays a ldap login form, as we need to auth before we can search - */ - private function processBasicRequest() { - $panels = array(); - - $request = $this->getRequest(); - - $admin = $request->getUser(); - $form = id(new AphrontFormView()) + ->setAction($request->getRequestURI() + ->alter('search', 'true')->alter('import', null)) ->setUser($admin) ->appendChild( id(new AphrontFormTextControl()) @@ -74,12 +43,12 @@ final class PhabricatorPeopleLdapController ->appendChild( id(new AphrontFormPasswordControl()) ->setLabel('Password') - ->setName('password')) + ->setName('password')) ->appendChild( id(new AphrontFormTextControl()) ->setLabel('LDAP query') + ->setCaption('A filter such as (objectClass=*)') ->setName('query')) - ->setAction($request->getRequestURI()->alter('search', 'true')->alter('import', null)) ->appendChild( id(new AphrontFormSubmitControl()) ->setValue('Search')); @@ -88,38 +57,41 @@ final class PhabricatorPeopleLdapController $panel->setHeader('Import Ldap Users'); $panel->appendChild($form); - - if($request->getStr('import')) { - $panels[] = $this->processImportRequest($request); - } - - $panels[] = $panel; - if($request->getStr('search')) { - $panels[] = $this->processSearchRequest($request); + if ($request->getStr('import')) { + $content[] = $this->processImportRequest($request); } - return $panels; + $content[] = $panel; + if ($request->getStr('search')) { + $content[] = $this->processSearchRequest($request); + } + + return $this->buildStandardPageResponse( + $content, + array( + 'title' => 'Import Ldap Users', + )); } private function processImportRequest($request) { $admin = $request->getUser(); - $usernames = $request->getArr('usernames'); - $emails = $request->getArr('email'); - $names = $request->getArr('name'); - + $usernames = $request->getArr('usernames'); + $emails = $request->getArr('email'); + $names = $request->getArr('name'); + $panel = new AphrontErrorView(); $panel->setSeverity(AphrontErrorView::SEVERITY_NOTICE); $panel->setTitle("Import Successful"); - $errors = array("Successfully imported users from ldap"); + $errors = array("Successfully imported users from LDAP"); - foreach($usernames as $username) { + foreach ($usernames as $username) { $user = new PhabricatorUser(); $user->setUsername($username); $user->setRealname($names[$username]); - + $email_obj = id(new PhabricatorUserEmail()) ->setAddress($emails[$username]) ->setIsVerified(1); @@ -127,19 +99,19 @@ final class PhabricatorPeopleLdapController id(new PhabricatorUserEditor()) ->setActor($admin) ->createNewUser($user, $email_obj); - + $ldap_info = new PhabricatorUserLDAPInfo(); $ldap_info->setLDAPUsername($username); $ldap_info->setUserID($user->getID()); $ldap_info->save(); - $errors[] = 'Succesfully added ' . $username; + $errors[] = 'Successfully added ' . $username; } catch (Exception $ex) { $errors[] = 'Failed to add ' . $username . ' ' . $ex->getMessage(); } - } + } $panel->setErrors($errors); - return $panel; + return $panel; } @@ -153,10 +125,10 @@ final class PhabricatorPeopleLdapController $search = $request->getStr('query'); try { - $ldapProvider = new PhabricatorLDAPProvider(); - $ldapProvider->auth($username, $password); - $results = $ldapProvider->search($search); - foreach($results as $key => $result) { + $ldap_provider = new PhabricatorLDAPProvider(); + $ldap_provider->auth($username, $password); + $results = $ldap_provider->search($search); + foreach ($results as $key => $result) { $results[$key][] = $this->renderUserInputs($result); } @@ -172,7 +144,8 @@ final class PhabricatorPeopleLdapController '', )); $form->appendChild($table); - $form->setAction($request->getRequestURI()->alter('import', 'true')->alter('search', null)) + $form->setAction($request->getRequestURI() + ->alter('import', 'true')->alter('search', null)) ->appendChild( id(new AphrontFormSubmitControl()) ->setValue('Import')); @@ -188,15 +161,15 @@ final class PhabricatorPeopleLdapController return $panel; } - + private function renderUserInputs($user) { $username = $user[0]; - $inputs = phutil_render_tag( + $inputs = phutil_render_tag( 'input', array( 'type' => 'checkbox', 'name' => 'usernames[]', - 'value' =>$username, + 'value' =>$username, ), ''); @@ -205,16 +178,16 @@ final class PhabricatorPeopleLdapController array( 'type' => 'hidden', 'name' => "email[$username]", - 'value' =>$user[1], + 'value' =>$user[1], ), ''); - + $inputs .= phutil_render_tag( 'input', array( 'type' => 'hidden', 'name' => "name[$username]", - 'value' =>$user[2], + 'value' =>$user[2], ), ''); diff --git a/src/applications/people/controller/PhabricatorPeopleListController.php b/src/applications/people/controller/PhabricatorPeopleListController.php index a9577f5003..4ff527ea9e 100644 --- a/src/applications/people/controller/PhabricatorPeopleListController.php +++ b/src/applications/people/controller/PhabricatorPeopleListController.php @@ -135,7 +135,7 @@ final class PhabricatorPeopleListController phutil_render_tag( 'a', array( - 'href' => '/people/ldap', + 'href' => '/people/ldap/', 'class' => 'button green' ), 'Import from Ldap')); From 176b4a68a8cf3b25c3dff09e9f586c1e1a46906b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Jul 2012 10:36:33 -0700 Subject: [PATCH 20/52] Fix some mercurial edge cases Summary: - Old versions of Mercurial give different output for `hg log -- ''` and `hg log`. Just use `hg log`. - Branch names with spaces can't be specified in `--rev`. I talked with hstuart in #mercurial and apparently am not crazy. Test Plan: - Viewed history of a repository. - Viewed history of a file. - Viewed branch `m m m m m 2:ffffffffffff (inactive)` - Learned that you checkout this branch with `hg checkout ':m m m m m 2:ffffffffffff (inactive)'` Reviewers: dschleimer, btrahan Reviewed By: dschleimer CC: aran Maniphest Tasks: T1268 Differential Revision: https://secure.phabricator.com/D2950 --- .../DiffusionMercurialHistoryQuery.php | 28 +++++++++++++++---- .../request/DiffusionMercurialRequest.php | 16 ++++++++++- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/applications/diffusion/query/history/DiffusionMercurialHistoryQuery.php b/src/applications/diffusion/query/history/DiffusionMercurialHistoryQuery.php index c1aafb5e2e..7b6add1adb 100644 --- a/src/applications/diffusion/query/history/DiffusionMercurialHistoryQuery.php +++ b/src/applications/diffusion/query/history/DiffusionMercurialHistoryQuery.php @@ -26,20 +26,36 @@ final class DiffusionMercurialHistoryQuery extends DiffusionHistoryQuery { $commit_hash = $drequest->getStableCommitName(); $path = DiffusionPathIDQuery::normalizePath($path); + $path = ltrim($path, '/'); - // NOTE: Using '' as a default path produces the correct behavior if HEAD - // is a merge commit; using '.' does not (the merge commit is not included - // in the log). - $default_path = ''; + // NOTE: Older versions of Mercurial give different results for these + // commands (see T1268): + // + // $ hg log -- '' + // $ hg log + // + // All versions of Mercurial give different results for these commands + // (merge commits are excluded with the "." version): + // + // $ hg log -- . + // $ hg log + // + // If we don't have a path component in the query, omit it from the command + // entirely to avoid these inconsistencies. + + $path_arg = ''; + if (strlen($path)) { + $path_arg = csprintf('-- %s', $path); + } // NOTE: --branch used to be called --only-branch; use -b for compatibility. list($stdout) = $repository->execxLocalCommand( - 'log --debug --template %s --limit %d -b %s --rev %s:0 -- %s', + 'log --debug --template %s --limit %d -b %s --rev %s:0 %C', '{node};{parents}\\n', ($this->getOffset() + $this->getLimit()), // No '--skip' in Mercurial. $drequest->getBranch(), $commit_hash, - nonempty(ltrim($path, '/'), $default_path)); + $path_arg); $lines = explode("\n", trim($stdout)); $lines = array_slice($lines, $this->getOffset()); diff --git a/src/applications/diffusion/request/DiffusionMercurialRequest.php b/src/applications/diffusion/request/DiffusionMercurialRequest.php index 99d6baf720..b2ef171813 100644 --- a/src/applications/diffusion/request/DiffusionMercurialRequest.php +++ b/src/applications/diffusion/request/DiffusionMercurialRequest.php @@ -59,8 +59,22 @@ final class DiffusionMercurialRequest extends DiffusionRequest { if ($this->commit) { $this->stableCommitName = $this->commit; } else { + + // NOTE: For branches with spaces in their name like "a b", this + // does not work properly: + // + // $ hg log --rev 'a b' + // + // We can use revsets instead: + // + // $ hg log --rev branch('a b') + // + // ...but they require a somewhat newer version of Mercurial. Instead, + // use "-b" flag with limit 1 for greatest compatibility across + // versions. + list($this->stableCommitName) = $this->repository->execxLocalCommand( - 'log --template=%s --rev %s', + 'log --template=%s -b %s --limit 1', '{node}', $this->getBranch()); } From eac8e0e7f3fd7ece1ba684f7a2a5719e1ee6d3ac Mon Sep 17 00:00:00 2001 From: Andrew Gallagher Date: Thu, 5 Jul 2012 19:39:30 -0700 Subject: [PATCH 21/52] Support postponed lint status in creatediff conduit call Test Plan: Created diff with a postponed linter and the lint status set to such via arcanist. Verified lint status showed as postponed in diff view. Reviewers: epriestley, vrana Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T1332 Differential Revision: https://secure.phabricator.com/D2932 --- .../ConduitAPI_differential_creatediff_Method.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_creatediff_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_creatediff_Method.php index 8780afc012..de70a1027d 100644 --- a/src/applications/conduit/method/differential/ConduitAPI_differential_creatediff_Method.php +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_creatediff_Method.php @@ -41,9 +41,9 @@ final class ConduitAPI_differential_creatediff_Method extends ConduitAPIMethod { 'arcanistProject' => 'optional string', 'repositoryUUID' => 'optional string', 'lintStatus' => - 'required enum', + 'required enum', 'unitStatus' => - 'required enum', + 'required enum', ); } @@ -121,6 +121,9 @@ final class ConduitAPI_differential_creatediff_Method extends ConduitAPIMethod { case 'fail': $diff->setLintStatus(DifferentialLintStatus::LINT_FAIL); break; + case 'postponed': + $diff->setLintStatus(DifferentialLintStatus::LINT_POSTPONED); + break; case 'none': default: $diff->setLintStatus(DifferentialLintStatus::LINT_NONE); From 523395590bb4e75a08bc79cbe164766a514ec081 Mon Sep 17 00:00:00 2001 From: Felix Fung Date: Tue, 10 Jul 2012 23:09:39 -0700 Subject: [PATCH 22/52] Fixed 'Sever' Typo --- .../panels/PhabricatorUserProfileSettingsPanelController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/people/controller/settings/panels/PhabricatorUserProfileSettingsPanelController.php b/src/applications/people/controller/settings/panels/PhabricatorUserProfileSettingsPanelController.php index 6cda66107c..26f0fa5910 100644 --- a/src/applications/people/controller/settings/panels/PhabricatorUserProfileSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/PhabricatorUserProfileSettingsPanelController.php @@ -137,7 +137,7 @@ final class PhabricatorUserProfileSettingsPanelController asort($translations); $default = PhabricatorEnv::newObjectFromConfig('translation.provider'); $translations = array( - '' => 'Sever Default ('.$default->getName().')', + '' => 'Server Default ('.$default->getName().')', ) + $translations; $form = new AphrontFormView(); From 5d8b75b4da9771fbfcc4cda264fffbb19b671733 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jul 2012 11:40:10 -0700 Subject: [PATCH 23/52] Use the unified markup cache for Maniphest Summary: - See D2945. - Drop `cache` field from ManiphestTransaction. - Render task descriptions and transactions through PhabricatorMarkupEngine. - Also pull the list of macros more lazily. Test Plan: - Verified transactions and transaction preview work correctly and interact with cache correctly. - Verified tasks descriptions and task preview work correctly. - Verified we don't hit the imagemacro table when we're rendering everything from cache anymore. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2946 --- resources/sql/patches/maniphestxcache.sql | 2 + scripts/util/purge_cache.php | 24 +------- ...iphestTaskDescriptionPreviewController.php | 9 ++- .../ManiphestTaskDetailController.php | 13 ++++- .../ManiphestTransactionPreviewController.php | 4 +- .../maniphest/storage/ManiphestTask.php | 53 ++++++++++++++++- .../storage/ManiphestTransaction.php | 58 ++++++++++++++++++- .../view/ManiphestTransactionDetailView.php | 20 ++----- .../view/ManiphestTransactionListView.php | 2 +- .../PhabricatorRemarkupRuleImageMacro.php | 15 ++--- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 11 files changed, 150 insertions(+), 54 deletions(-) create mode 100644 resources/sql/patches/maniphestxcache.sql diff --git a/resources/sql/patches/maniphestxcache.sql b/resources/sql/patches/maniphestxcache.sql new file mode 100644 index 0000000000..131bb52bc1 --- /dev/null +++ b/resources/sql/patches/maniphestxcache.sql @@ -0,0 +1,2 @@ +ALTER TABLE `{$NAMESPACE}_maniphest`.`maniphest_transaction` + DROP `cache`; diff --git a/scripts/util/purge_cache.php b/scripts/util/purge_cache.php index 45c91040c1..0a273a72a2 100755 --- a/scripts/util/purge_cache.php +++ b/scripts/util/purge_cache.php @@ -22,7 +22,6 @@ require_once $root.'/scripts/__init_script__.php'; $purge_changesets = false; $purge_differential = false; -$purge_maniphest = false; $args = array_slice($argv, 1); if (!$args) { @@ -36,7 +35,6 @@ for ($ii = 0; $ii < $len; $ii++) { case '--all': $purge_changesets = true; $purge_differential = true; - $purge_maniphest = true; break; case '--changesets': $purge_changesets = true; @@ -52,9 +50,6 @@ for ($ii = 0; $ii < $len; $ii++) { case '--differential': $purge_differential = true; break; - case '--maniphest': - $purge_maniphest = true; - break; case '--help': return help(); default: @@ -98,16 +93,6 @@ if ($purge_differential) { echo "Done.\n"; } -if ($purge_maniphest) { - echo "Purging Maniphest comment cache...\n"; - $table = new ManiphestTransaction(); - queryfx( - $table->establishConnection('w'), - 'UPDATE %T SET cache = NULL', - $table->getTableName()); - echo "Done.\n"; -} - echo "Ok, caches purged.\n"; function usage($message) { @@ -122,7 +107,6 @@ function help() { **SUMMARY** **purge_cache.php** - [--maniphest] [--differential] [--changesets [changeset_id ...]] **purge_cache.php** --all @@ -136,9 +120,8 @@ function help() { syntax highlighting, you may want to purge the changeset cache (with "--changesets") so your changes are reflected in older diffs. - If you change Remarkup rules, you may want to purge the Maniphest or - Differential comment caches ("--maniphest", "--differential") so older - comments pick up the new rules. + If you change Remarkup rules, you may want to purge the Differential + comment caches ("--differential") so older comments pick up the new rules. __--all__ Purge all long-lived caches. @@ -151,9 +134,6 @@ function help() { __--differential__ Purge Differential comment formatting cache. - __--maniphest__: show this help - Purge Maniphest comment formatting cache. - __--help__: show this help diff --git a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php b/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php index a01e15a349..17a89f6991 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDescriptionPreviewController.php @@ -27,11 +27,16 @@ final class ManiphestTaskDescriptionPreviewController $request = $this->getRequest(); $description = $request->getStr('description'); - $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine(); + $task = new ManiphestTask(); + $task->setDescription($description); + + $output = PhabricatorMarkupEngine::renderOneObject( + $task, + ManiphestTask::MARKUP_FIELD_DESCRIPTION); $content = '
'. - $engine->markupText($description). + $output. '
'; return id(new AphrontAjaxResponse()) diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 6353a0f19f..311e537f13 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -88,8 +88,6 @@ final class ManiphestTaskDetailController extends ManiphestController { $handles = id(new PhabricatorObjectHandleData($phids)) ->loadHandles(); - $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine(); - $dict = array(); $dict['Status'] = ''. @@ -305,9 +303,18 @@ final class ManiphestTaskDetailController extends ManiphestController { $headsup_panel->setActionList($action_list); $headsup_panel->setProperties($dict); + $engine = new PhabricatorMarkupEngine(); + $engine->addObject($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION); + foreach ($transactions as $xaction) { + if ($xaction->hasComments()) { + $engine->addObject($xaction, ManiphestTransaction::MARKUP_FIELD_BODY); + } + } + $engine->process(); + $headsup_panel->appendChild( '
'. - $engine->markupText($task->getDescription()). + $engine->getOutput($task, ManiphestTask::MARKUP_FIELD_DESCRIPTION). '
'); $transaction_types = ManiphestTransactionType::getTransactionTypeMap(); diff --git a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php index cb9e3fcfed..94e6c49b0d 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionPreviewController.php @@ -119,7 +119,9 @@ final class ManiphestTransactionPreviewController extends ManiphestController { $transactions = array(); $transactions[] = $transaction; - $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine(); + $engine = new PhabricatorMarkupEngine(); + $engine->addObject($transaction, ManiphestTransaction::MARKUP_FIELD_BODY); + $engine->process(); $transaction_view = new ManiphestTransactionListView(); $transaction_view->setTransactions($transactions); diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index cf0d88d77e..97a269d4f7 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -19,7 +19,10 @@ /** * @group maniphest */ -final class ManiphestTask extends ManiphestDAO { +final class ManiphestTask extends ManiphestDAO + implements PhabricatorMarkupInterface { + + const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; protected $phid; protected $authorPHID; @@ -214,4 +217,52 @@ final class ManiphestTask extends ManiphestDAO { } } + +/* -( Markup Interface )--------------------------------------------------- */ + + + /** + * @task markup + */ + public function getMarkupFieldKey($field) { + $hash = PhabricatorHash::digest($this->getMarkupText($field)); + $id = $this->getID(); + return "maniphest:T{$id}:{$field}:{$hash}"; + } + + + /** + * @task markup + */ + public function getMarkupText($field) { + return $this->getDescription(); + } + + + /** + * @task markup + */ + public function newMarkupEngine($field) { + return PhabricatorMarkupEngine::newManiphestMarkupEngine(); + } + + + /** + * @task markup + */ + public function didMarkupText( + $field, + $output, + PhutilMarkupEngine $engine) { + return $output; + } + + + /** + * @task markup + */ + public function shouldUseMarkupCache($field) { + return (bool)$this->getID(); + } + } diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index bbdd2c2756..c106e37904 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -17,9 +17,13 @@ */ /** + * @task markup Markup Interface * @group maniphest */ -final class ManiphestTransaction extends ManiphestDAO { +final class ManiphestTransaction extends ManiphestDAO + implements PhabricatorMarkupInterface { + + const MARKUP_FIELD_BODY = 'markup:body'; protected $taskID; protected $authorPHID; @@ -27,7 +31,6 @@ final class ManiphestTransaction extends ManiphestDAO { protected $oldValue; protected $newValue; protected $comments; - protected $cache; protected $metadata = array(); protected $contentSource; @@ -143,4 +146,55 @@ final class ManiphestTransaction extends ManiphestDAO { return PhabricatorContentSource::newFromSerialized($this->contentSource); } + +/* -( Markup Interface )--------------------------------------------------- */ + + + /** + * @task markup + */ + public function getMarkupFieldKey($field) { + if ($this->shouldUseMarkupCache($field)) { + $id = $this->getID(); + } else { + $id = PhabricatorHash::digest($this->getMarkupText($field)); + } + return "maniphest:x:{$field}:{$id}"; + } + + + /** + * @task markup + */ + public function getMarkupText($field) { + return $this->getComments(); + } + + + /** + * @task markup + */ + public function newMarkupEngine($field) { + return PhabricatorMarkupEngine::newManiphestMarkupEngine(); + } + + + /** + * @task markup + */ + public function didMarkupText( + $field, + $output, + PhutilMarkupEngine $engine) { + return $output; + } + + + /** + * @task markup + */ + public function shouldUseMarkupCache($field) { + return (bool)$this->getID(); + } + } diff --git a/src/applications/maniphest/view/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/ManiphestTransactionDetailView.php index 8f8662e9e7..ccb4c7444f 100644 --- a/src/applications/maniphest/view/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/ManiphestTransactionDetailView.php @@ -57,7 +57,7 @@ final class ManiphestTransactionDetailView extends ManiphestView { return $this; } - public function setMarkupEngine(PhutilMarkupEngine $engine) { + public function setMarkupEngine(PhabricatorMarkupEngine $engine) { $this->markupEngine = $engine; return $this; } @@ -199,22 +199,12 @@ final class ManiphestTransactionDetailView extends ManiphestView { } if ($comment_transaction && $comment_transaction->hasComments()) { - $comments = $comment_transaction->getCache(); - if (!strlen($comments)) { - $comments = $comment_transaction->getComments(); - if (strlen($comments)) { - $comments = $this->markupEngine->markupText($comments); - $comment_transaction->setCache($comments); - if ($comment_transaction->getID() && !$this->preview) { - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $comment_transaction->save(); - unset($unguarded); - } - } - } + $comment_block = $this->markupEngine->getOutput( + $comment_transaction, + ManiphestTransaction::MARKUP_FIELD_BODY); $comment_block = '
'. - $comments. + $comment_block. '
'; } else { $comment_block = null; diff --git a/src/applications/maniphest/view/ManiphestTransactionListView.php b/src/applications/maniphest/view/ManiphestTransactionListView.php index 3ecda00487..001eee94c5 100644 --- a/src/applications/maniphest/view/ManiphestTransactionListView.php +++ b/src/applications/maniphest/view/ManiphestTransactionListView.php @@ -45,7 +45,7 @@ final class ManiphestTransactionListView extends ManiphestView { return $this; } - public function setMarkupEngine(PhutilMarkupEngine $engine) { + public function setMarkupEngine(PhabricatorMarkupEngine $engine) { $this->markupEngine = $engine; return $this; } diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php index 96248b38a1..714c539152 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php +++ b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php @@ -24,13 +24,6 @@ final class PhabricatorRemarkupRuleImageMacro private $images = array(); - public function __construct() { - $rows = id(new PhabricatorFileImageMacro())->loadAll(); - foreach ($rows as $row) { - $this->images[$row->getName()] = $row->getFilePHID(); - } - } - public function apply($text) { return preg_replace_callback( '@^([a-zA-Z0-9_\-]+)$@m', @@ -39,6 +32,14 @@ final class PhabricatorRemarkupRuleImageMacro } public function markupImageMacro($matches) { + if ($this->images === null) { + $this->images = array(); + $rows = id(new PhabricatorFileImageMacro())->loadAll(); + foreach ($rows as $row) { + $this->images[$row->getName()] = $row->getFilePHID(); + } + } + if (array_key_exists($matches[1], $this->images)) { $phid = $this->images[$matches[1]]; diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index b67804920d..e4138845b8 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -911,6 +911,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('markupcache.sql'), ), + 'maniphestxcache.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('maniphestxcache.sql'), + ), ); } From fcd04708d2b5df78174a234813c14fbb6aa41c94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jul 2012 11:40:18 -0700 Subject: [PATCH 24/52] Add markup cache collection to the GC daemon Summary: Allow the GC daemon to collect the new markup cache. Test Plan: Ran gc daemon in "debug" mode, saw it collect cache entries. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2947 --- conf/default.conf.php | 1 + .../PhabricatorGarbageCollectorDaemon.php | 30 +++++++++++++++---- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index e09b4dfc37..c0fde81573 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -1007,6 +1007,7 @@ return array( 'gcdaemon.ttl.herald-transcripts' => 30 * (24 * 60 * 60), 'gcdaemon.ttl.daemon-logs' => 7 * (24 * 60 * 60), 'gcdaemon.ttl.differential-parse-cache' => 14 * (24 * 60 * 60), + 'gcdaemon.ttl.markup-cache' => 30 * (24 * 60 * 60), // -- Feed ------------------------------------------------------------------ // diff --git a/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php b/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php index 0ad87fb617..c763a99b6d 100644 --- a/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php +++ b/src/infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php @@ -46,7 +46,7 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { if ($now < $start || $now > ($start + $run_for)) { if ($just_ran) { - echo "Stopped garbage collector.\n"; + $this->log("Stopped garbage collector."); $just_ran = false; } // The configuration says we can't collect garbage right now, so @@ -56,27 +56,26 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { } if (!$just_ran) { - echo "Started garbage collector.\n"; + $this->log("Started garbage collector."); $just_ran = true; } $n_herald = $this->collectHeraldTranscripts(); $n_daemon = $this->collectDaemonLogs(); $n_parse = $this->collectParseCaches(); + $n_markup = $this->collectMarkupCaches(); $collected = array( 'Herald Transcript' => $n_herald, 'Daemon Log' => $n_daemon, 'Differential Parse Cache' => $n_parse, + 'Markup Cache' => $n_markup, ); $collected = array_filter($collected); foreach ($collected as $thing => $count) { - if ($thing == 'Daemon Log' && !$this->getTraceMode()) { - continue; - } $count = number_format($count); - echo "Garbage collected {$count} '{$thing}' objects.\n"; + $this->log("Garbage collected {$count} '{$thing}' objects."); } $total = array_sum($collected); @@ -154,4 +153,23 @@ final class PhabricatorGarbageCollectorDaemon extends PhabricatorDaemon { return $conn_w->getAffectedRows(); } + private function collectMarkupCaches() { + $key = 'gcdaemon.ttl.markup-cache'; + $ttl = PhabricatorEnv::getEnvConfig($key); + if ($ttl <= 0) { + return 0; + } + + $table = new PhabricatorMarkupCache(); + $conn_w = $table->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE dateCreated < %d LIMIT 100', + $table->getTableName(), + time() - $ttl); + + return $conn_w->getAffectedRows(); + } + } From dc75e79cb532eb0d1ae786bc1f450e49034192d4 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 11 Jul 2012 16:27:41 -0700 Subject: [PATCH 25/52] Make IRC Bot connect on both successful end of MOTD (376) and non-successful MOTD (422) Summary: http://www.networksorcery.com/enp/protocol/irc.htm Test Plan: augment code with an additional debug line (phlog('hi');) so I can see my case was trigged and it will fall through. setup an ill-configured IRC server with ngircd. Configure an ircbot to connect to said ill-configured IRC server. verify ircbot connected to channel. verify in irc bot logs that debug line was invoked. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T1452 Differential Revision: https://secure.phabricator.com/D2962 --- .../daemon/irc/handler/PhabricatorIRCProtocolHandler.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/infrastructure/daemon/irc/handler/PhabricatorIRCProtocolHandler.php b/src/infrastructure/daemon/irc/handler/PhabricatorIRCProtocolHandler.php index daf2958a68..383bccab0f 100644 --- a/src/infrastructure/daemon/irc/handler/PhabricatorIRCProtocolHandler.php +++ b/src/infrastructure/daemon/irc/handler/PhabricatorIRCProtocolHandler.php @@ -25,6 +25,7 @@ final class PhabricatorIRCProtocolHandler extends PhabricatorIRCHandler { public function receiveMessage(PhabricatorIRCMessage $message) { switch ($message->getCommand()) { + case '422': // Error - no MOTD case '376': // End of MOTD $join = $this->getConfig('join'); if (!$join) { From ff4774a970030a73b07db664c647e8437d8f8e5d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jul 2012 17:02:15 -0700 Subject: [PATCH 26/52] Don't fatal on missing commit in DiffusionBrowseFileController Summary: Still not 100% sure what the repro case here is, but we do something similar on line 438 above. One case could be an SVN repo with a subpath specified, I think. In any case, don't fatal if we're missing the commit. Test Plan: Loaded some browse views, although I don't have a repro. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2959 --- .../diffusion/controller/DiffusionBrowseFileController.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index cb04b5eb06..8ac7b2e5cc 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -463,7 +463,11 @@ final class DiffusionBrowseFileController extends DiffusionController { ), phutil_escape_html(phutil_utf8_shorten($line['commit'], 9, ''))); - $revision_id = idx($revision_ids, $commits[$commit]->getPHID()); + $revision_id = null; + if (idx($commits, $commit)) { + $revision_id = idx($revision_ids, $commits[$commit]->getPHID()); + } + if ($revision_id) { $revision = idx($revisions, $revision_id); if (!$revision) { From 6f20809a5101d3c18f2a6e9d2a363e9a94824cef Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Jul 2012 17:02:26 -0700 Subject: [PATCH 27/52] Improve Windows editor documentation with specific examples for popular editors Summary: See D2955, D2956, D2957. Test Plan: Read documentation. Ran all these commands from Git Bash and cmd.exe and used the specified editors to edit blocks of text. Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran Maniphest Tasks: T1309 Differential Revision: https://secure.phabricator.com/D2958 --- src/docs/userguide/arcanist_windows.diviner | 47 +++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/src/docs/userguide/arcanist_windows.diviner b/src/docs/userguide/arcanist_windows.diviner index 35a9e3570c..550a975b1e 100644 --- a/src/docs/userguide/arcanist_windows.diviner +++ b/src/docs/userguide/arcanist_windows.diviner @@ -27,12 +27,53 @@ Then, configure: `php`, `arc`, or (for example) `git` from the command line, they should all do something. - Your EDITOR environmental variable should point at some valid CLI editor, - like the Git Bash `vim`. (Under `cmd.exe`, you need to point to the actual - `vim.exe`, not just the `bin/vim` symlink which runs it under Git Bash - since `cmd.exe` does not know how to run the symlink.) + like the Git Bash `vim`. You can set this in `arc` if you prefer. + See below for details. You can set environmental variables somewhere in the `Advanced` tab of the `System` control panel. Now you should be able to run `arc` normally (either from `cmd.exe` or Git Bash) and it should work more-or-less properly. + += Configuring an Editor = + +NOTE: You **can not** use Notepad as your editor, because it does not have a +blocking mode. You can use GitPad instead. + +Some arc workflows prompt you to edit large blocks of text using a text editor. +You can configure various programs for this purpose, depending on which text +editor you prefer. Some editors that will work are: + + - [[ http://notepad-plus-plus.org/ | Notepad++ ]], a good all-around editor. + - **vim**, which comes with Git Bash. + - [[ https://github.com/github/gitpad | GitPad ]], which allows you to use + Notepad as your editor. + +Other editors may also work, but they must have a blocking edit mode. + +To configure an editor, either set the `EDITOR` environmental variable to point +at it, or run: + + $ arc set-config editor "\"C:\path\to\some\editor.exe\"" + +NOTE: Note the use of quotes. Paths with spaces in them must be quoted, and +these quotes must be escaped when passed to `arc set-config`, as in the examples +below. + +Specifically, you can use this command for **Notepad++** (adjusting the path for +your machine): + + name=Notepad++ + $ arc set-config editor "\"C:\Program Files (x86)\Notepad++\notepad++.exe\" -multiInst -nosession" + +And this command for Vim (you may need to adjust the path): + + name=vim + $ arc set-config editor "\"C:\Program Files (x86)\Git\share\vim\vim73\vim.exe\"" + +And this for GitPad (you may need to adjust the path): + + name=GitPad + $ arc set-config editor "\"C:\Users\yourusername\AppData\Roaming\GitPad\GitPad.exe\"" + From 154f8b1984749350deff97edddbd3ab2db5f2617 Mon Sep 17 00:00:00 2001 From: Felix Fung Date: Wed, 11 Jul 2012 00:27:35 -0700 Subject: [PATCH 28/52] Correcting README Summary: Custom logos are 220px in width - make README consistent Test Plan: Read it. Reviewers: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D2954 --- webroot/rsrc/image/custom/README | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webroot/rsrc/image/custom/README b/webroot/rsrc/image/custom/README index bab43c10c9..dbe339a3b7 100644 --- a/webroot/rsrc/image/custom/README +++ b/webroot/rsrc/image/custom/README @@ -1,3 +1,3 @@ -To customize your Phabricator install's appearance, create a 240x80 PNG like +To customize your Phabricator install's appearance, create a 220x80 PNG like the examples in this folder, name it "custom.png" in this folder, and then set 'phabricator.custom.logo' to a link URI. From 2b690372dedcf98f66a22723c109cb3f9dab879d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Jul 2012 13:33:26 -0700 Subject: [PATCH 29/52] Fix several issues with Differential exception email Summary: - Assigning $cc_phids clobbers the correct value assigned on line 80. - Remove stack trace noise, the trace is always meaningless and well-known. - Actually show the original body. Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body. Reviewers: btrahan, jungejason Reviewed By: jungejason CC: aran Differential Revision: https://secure.phabricator.com/D2969 --- src/applications/differential/DifferentialReplyHandler.php | 2 +- .../differential/mail/DifferentialExceptionMail.php | 5 +---- src/applications/differential/mail/DifferentialMail.php | 1 - .../metamta/storage/PhabricatorMetaMTAReceivedMail.php | 4 ++++ 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/DifferentialReplyHandler.php b/src/applications/differential/DifferentialReplyHandler.php index b16dbde854..3104c04b26 100644 --- a/src/applications/differential/DifferentialReplyHandler.php +++ b/src/applications/differential/DifferentialReplyHandler.php @@ -162,7 +162,7 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler { $exception_mail = new DifferentialExceptionMail( $this->getMailReceiver(), $ex, - $body); + $this->receivedMail->getRawTextBody()); $exception_mail->setToPHIDs(array($this->getActor()->getPHID())); $exception_mail->send(); diff --git a/src/applications/differential/mail/DifferentialExceptionMail.php b/src/applications/differential/mail/DifferentialExceptionMail.php index d168302054..90ceed2e8e 100644 --- a/src/applications/differential/mail/DifferentialExceptionMail.php +++ b/src/applications/differential/mail/DifferentialExceptionMail.php @@ -45,16 +45,13 @@ final class DifferentialExceptionMail extends DifferentialMail { $original_body = $this->originalBody; $message = $exception->getMessage(); - $trace = $exception->getTraceAsString(); return <<, <', $reviewer_phids).'>'); } - $cc_phids = $revision->getCCPHIDs(); if ($cc_phids) { $template->addPHIDHeaders('X-Differential-CC', $cc_phids); $template->addHeader( diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index f7d2122f20..4a05119952 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -222,6 +222,10 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return $parser->stripTextBody($body); } + public function getRawTextBody() { + return idx($this->bodies, 'text'); + } + public static function loadReceiverObject($receiver_name) { if (!$receiver_name) { return null; From 9574b91e55d446cd0f2155cdf8d86804011aafd6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Jul 2012 13:33:35 -0700 Subject: [PATCH 30/52] Add options to include patches inline or attached for Diffusion commit emails Summary: See D818 for an older attempt at this. Support code has matured to the point where the patch is pretty straightforward. @tido, this was a long-standing request from Aditya back in the day. Test Plan: Used `reparse.php --herald` to send myself a bunch of emails with various patch configurations. Confirmed that limits are respected, reasonable errors arise when they're violated, etc. (Timeout is a little funky but that's out of scope here, I think.) Reviewers: btrahan Reviewed By: btrahan CC: tido, aran Maniphest Tasks: T456 Differential Revision: https://secure.phabricator.com/D2967 --- conf/default.conf.php | 20 ++++ ...habricatorRepositoryCommitHeraldWorker.php | 101 +++++++++++++++++- 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index c0fde81573..8e07f39cce 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -396,6 +396,26 @@ return array( // affects Diffusion. 'metamta.diffusion.reply-handler' => 'PhabricatorAuditReplyHandler', + // Set this to true if you want patches to be attached to commit notifications + // from Diffusion. This won't work with SendGrid. + 'metamta.diffusion.attach-patches' => false, + + // To include patches in Diffusion email bodies, set this to a positive + // integer. Patches will be inlined if they are at most that many lines. + // By default, patches are not inlined. + 'metamta.diffusion.inline-patches' => 0, + + // If you've enabled attached patches or inline patches for commit emails, you + // can establish a hard byte limit on their size. You should generally set + // reasonable byte and time limits (defaults are 1MB and 60 seconds) to avoid + // sending ridiculously enormous email for changes like "importing an external + // library" or "accidentally committed this full-length movie as text". + 'metamta.diffusion.byte-limit' => 1024 * 1024, + + // If you've enabled attached patches or inline patches for commit emails, you + // can establish a hard time limit on generating them. + 'metamta.diffusion.time-limit' => 60, + // Prefix prepended to mail sent by Package. 'metamta.package.subject-prefix' => '[Package]', diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 23be0ead94..c58032c3f2 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -130,6 +130,9 @@ final class PhabricatorRepositoryCommitHeraldWorker " ".$reply_instructions."\n"; } + $template = new PhabricatorMetaMTAMail(); + + $inline_patch_text = $this->buildPatch($template, $repository, $commit); $body = <<setRelatedPHID($commit->getPHID()); $template->setSubject("{$commit_name}: {$name}"); $template->setSubjectPrefix($prefix); @@ -314,4 +316,99 @@ EOBODY; $publisher->publish(); } + private function buildPatch( + PhabricatorMetaMTAMail $template, + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + + $attach_key = 'metamta.diffusion.attach-patches'; + $inline_key = 'metamta.diffusion.inline-patches'; + + $attach_patches = PhabricatorEnv::getEnvConfig($attach_key); + $inline_patches = PhabricatorEnv::getEnvConfig($inline_key); + + if (!$attach_patches && !$inline_patches) { + return; + } + + $encoding = $repository->getDetail('encoding', 'utf-8'); + + $result = null; + $patch_error = null; + + try { + $raw_patch = $this->loadRawPatchText($repository, $commit); + if ($attach_patches) { + $commit_name = $repository->formatCommitName( + $commit->getCommitIdentifier()); + + $template->addAttachment( + new PhabricatorMetaMTAAttachment( + $raw_patch, + $commit_name.'.patch', + 'text/x-patch; charset='.$encoding)); + } + } catch (Exception $ex) { + phlog($ex); + $patch_error = 'Unable to generate: '.$ex->getMessage(); + } + + if ($patch_error) { + $result = $patch_error; + } else if ($inline_patches) { + $len = substr_count($raw_patch, "\n"); + if ($len <= $inline_patches) { + // We send email as utf8, so we need to convert the text to utf8 if + // we can. + if (strtolower($encoding) != 'utf-8' && + function_exists('mb_convert_encoding')) { + $raw_patch = mb_convert_encoding($raw_patch, 'utf-8', $encoding); + } + $result = phutil_utf8ize($raw_patch); + } + } + + if ($result) { + $result = "\nPATCH\n\n{$result}\n"; + } + + return $result; + } + + private function loadRawPatchText( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + + $drequest = DiffusionRequest::newFromDictionary( + array( + 'repository' => $repository, + 'commit' => $commit->getCommitIdentifier(), + )); + + $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); + $raw_query->setLinesOfContext(3); + + $time_key = 'metamta.diffusion.time-limit'; + $byte_key = 'metamta.diffusion.byte-limit'; + $time_limit = PhabricatorEnv::getEnvConfig($time_key); + $byte_limit = PhabricatorEnv::getEnvConfig($byte_key); + + if ($time_limit) { + $raw_query->setTimeout($time_limit); + } + + $raw_diff = $raw_query->loadRawDiff(); + + $size = strlen($raw_diff); + if ($byte_limit && $size > $byte_limit) { + $pretty_size = phabricator_format_bytes($size); + $pretty_limit = phabricator_format_bytes($byte_limit); + throw new Exception( + "Patch size of {$pretty_size} exceeds configured byte size limit of ". + "{$pretty_limit}."); + } + + return $raw_diff; + } + } From 621bac74c7212cd552caaff0cf92627f7503d331 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Jul 2012 21:34:53 -0700 Subject: [PATCH 31/52] Unfatal the "show change" link in Maniphest description edits Summary: I missed this callsite in D2946. Transition it to the new markup cache. Test Plan: Clicked "show change" on a description edit transaction, got the change instead of a fatal. Reviewers: alanh, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1502 Differential Revision: https://secure.phabricator.com/D2972 --- .../ManiphestTaskDescriptionChangeController.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskDescriptionChangeController.php b/src/applications/maniphest/controller/ManiphestTaskDescriptionChangeController.php index 262a729101..0680ac20c6 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDescriptionChangeController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDescriptionChangeController.php @@ -56,14 +56,16 @@ final class ManiphestTaskDescriptionChangeController $transactions = array($transaction); $phids = array(); - foreach ($transactions as $transaction) { - foreach ($transaction->extractPHIDs() as $phid) { + foreach ($transactions as $xaction) { + foreach ($xaction->extractPHIDs() as $phid) { $phids[$phid] = $phid; } } $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); - $engine = PhabricatorMarkupEngine::newManiphestMarkupEngine(); + $engine = new PhabricatorMarkupEngine(); + $engine->addObject($transaction, ManiphestTransaction::MARKUP_FIELD_BODY); + $engine->process(); $view = new ManiphestTransactionDetailView(); $view->setTransactionGroup($transactions); From 226cf288e97c02005d5e62c16d48e52efa30accd Mon Sep 17 00:00:00 2001 From: Avishay Lavie Date: Fri, 13 Jul 2012 15:16:16 +0300 Subject: [PATCH 32/52] Add active-directory domain-based ldap authentication support Summary: Add active-directory domain-based ldap authentication support Test Plan: Tested on a live install against Active Directory on a Windows Server Reviewers: epriestley CC: aran, epriestley Maniphest Tasks: T1496 Differential Revision: https://secure.phabricator.com/D2966 --- conf/default.conf.php | 4 ++++ .../auth/ldap/PhabricatorLDAPProvider.php | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index c0fde81573..2d5bd9c3b1 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -604,6 +604,10 @@ return array( // the array will be joined 'ldap.real_name_attributes' => array(), + // A domain name to use when authenticating against Active Directory + // (e.g. 'example.com') + 'ldap.activedirectory_domain' => '', + // The LDAP version 'ldap.version' => 3, diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index 3f9241b897..d73659c5fb 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -111,10 +111,17 @@ final class PhabricatorLDAPProvider { throw new Exception('Username and/or password can not be empty'); } - $result = ldap_bind($this->getConnection(), - $this->getSearchAttribute() . '=' . $username . ',' . - $this->getBaseDN(), - $password); + $activeDirectoryDomain = + PhabricatorEnv::getEnvConfig('ldap.activedirectory_domain'); + + if ($activeDirectoryDomain) { + $dn = $username . '@' . $activeDirectoryDomain; + } else { + $dn = $this->getSearchAttribute() . '=' . $username . ',' . + $this->getBaseDN(); + } + + $result = ldap_bind($this->getConnection(), $dn, $password); if (!$result) { throw new Exception('Bad username/password.'); @@ -176,6 +183,7 @@ final class PhabricatorLDAPProvider { for($i = 0; $i < $entries['count']; $i++) { $row = array(); $entry = $entries[$i]; + // Get username, email and realname $username = $entry[$this->getSearchAttribute()][0]; if(empty($username)) { From e0d4793e74ed28a59a92ae87956e58202657e65d Mon Sep 17 00:00:00 2001 From: Jason Ge Date: Tue, 3 Jul 2012 18:24:58 -0700 Subject: [PATCH 33/52] Show responsiveness' in differential stats Summary: For "accept" and "reject" action, find the time between the action and last time the revision was updated by a new diff. Show it as the responsiveness row. Test Plan: view it for a couple of engineers. Reviewers: epriestley, vii Reviewed By: epriestley CC: vrana, nh, aran, Korvin Differential Revision: https://secure.phabricator.com/D2970 --- .../DifferentialRevisionStatsController.php | 17 +++ .../view/DifferentialRevisionStatsView.php | 102 +++++++++++++++--- 2 files changed, 107 insertions(+), 12 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionStatsController.php b/src/applications/differential/controller/DifferentialRevisionStatsController.php index 1389ba7d66..613ba1e4b2 100644 --- a/src/applications/differential/controller/DifferentialRevisionStatsController.php +++ b/src/applications/differential/controller/DifferentialRevisionStatsController.php @@ -67,6 +67,20 @@ final class DifferentialRevisionStatsController extends DifferentialController { return $table->loadAllFromArray($rows); } + + private function loadDiffs(array $revisions) { + if (!$revisions) { + return array(); + } + + $diff_teml = new DifferentialDiff(); + $diffs = $diff_teml->loadAllWhere( + 'revisionID in (%Ld)', + array_keys($revisions) + ); + return $diffs; + } + public function willProcessRequest(array $data) { $this->filter = idx($data, 'filter'); } @@ -127,13 +141,16 @@ final class DifferentialRevisionStatsController extends DifferentialController { $comments = $this->loadComments($params['phid']); $revisions = $this->loadRevisions($params['phid']); + $diffs = $this->loadDiffs($revisions); $panel = new AphrontPanelView(); $panel->setHeader('Differential rate analysis'); $panel->appendChild( id(new DifferentialRevisionStatsView()) ->setComments($comments) + ->setFilter($this->filter) ->setRevisions($revisions) + ->setDiffs($diffs) ->setUser($user)); $panels[] = $panel; diff --git a/src/applications/differential/view/DifferentialRevisionStatsView.php b/src/applications/differential/view/DifferentialRevisionStatsView.php index 66eb51aecf..a455a1aeae 100644 --- a/src/applications/differential/view/DifferentialRevisionStatsView.php +++ b/src/applications/differential/view/DifferentialRevisionStatsView.php @@ -22,7 +22,9 @@ final class DifferentialRevisionStatsView extends AphrontView { private $comments; private $revisions; + private $diffs; private $user; + private $filter; public function setRevisions(array $revisions) { assert_instances_of($revisions, 'DifferentialRevision'); @@ -36,6 +38,17 @@ final class DifferentialRevisionStatsView extends AphrontView { return $this; } + public function setDiffs(array $diffs) { + assert_instances_of($diffs, 'DifferentialDiff'); + $this->diffs = $diffs; + return $this; + } + + public function setFilter($filter) { + $this->filter = $filter; + return $this; + } + public function setUser($user) { $this->user = $user; return $this; @@ -56,9 +69,10 @@ final class DifferentialRevisionStatsView extends AphrontView { $dates = array(); $counts = array(); $lines = array(); - $boosts = array(); $days_with_diffs = array(); $count_active = array(); + $response_time = array(); + $response_count = array(); $now = time(); $row_array = array(); @@ -72,33 +86,52 @@ final class DifferentialRevisionStatsView extends AphrontView { $counts[$age] = 0; $lines[$age] = 0; $count_active[$age] = 0; + $response_time[$age] = array(); + } + + $revision_diffs_map = mgroup($this->diffs, 'getRevisionID'); + foreach ($revision_diffs_map as $revision_id => $diffs) { + $revision_diffs_map[$revision_id] = msort($diffs, 'getID'); } foreach ($this->comments as $comment) { - $rev_date = $comment->getDateCreated(); + $comment_date = $comment->getDateCreated(); - $day = phabricator_date($rev_date, $user); + $day = phabricator_date($comment_date, $user); $old_daycount = idx($days_with_diffs, $day, 0); $days_with_diffs[$day] = $old_daycount + 1; $rev_id = $comment->getRevisionID(); if (idx($revisions_seen, $rev_id)) { - continue; + $revision_seen = true; + $rev = null; + } else { + $revision_seen = false; + $rev = $id_to_revision_map[$rev_id]; + $revisions_seen[$rev_id] = true; } - $rev = $id_to_revision_map[$rev_id]; - $revisions_seen[$rev_id] = true; foreach ($dates as $age => $cutoff) { - if ($cutoff >= $rev_date) { + if ($cutoff >= $comment_date) { continue; } - if ($rev) { - $lines[$age] += $rev->getLineCount(); + + if (!$revision_seen) { + if ($rev) { + $lines[$age] += $rev->getLineCount(); + } + $counts[$age]++; + if (!$old_daycount) { + $count_active[$age]++; + } } - $counts[$age]++; - if (!$old_daycount) { - $count_active[$age]++; + + $diffs = $revision_diffs_map[$rev_id]; + $target_diff = $this->findTargetDiff($diffs, $comment); + if ($target_diff) { + $response_time[$age][] = + $comment_date - $target_diff->getDateCreated(); } } } @@ -123,6 +156,30 @@ final class DifferentialRevisionStatsView extends AphrontView { ($counts[$age] + 0.0001)), 'Active days' => number_format($count_active[$age]), ); + + switch ($this->filter) { + case DifferentialAction::ACTION_CLOSE: + case DifferentialAction::ACTION_UPDATE: + case DifferentialAction::ACTION_COMMENT: + break; + case DifferentialAction::ACTION_ACCEPT: + case DifferentialAction::ACTION_REJECT: + $count = count($response_time[$age]); + if ($count) { + rsort($response_time[$age]); + $median = $response_time[$age][round($count / 2) - 1]; + $average = array_sum($response_time[$age]) / $count; + } else { + $median = 0; + $average = 0; + } + + $row_array[$age]['Response hours (median|average)'] = + number_format($median / 3600, 1). + ' | '. + number_format($average / 3600, 1); + break; + } } $rows = array(); @@ -153,4 +210,25 @@ final class DifferentialRevisionStatsView extends AphrontView { return $table->render(); } + + private function findTargetDiff(array $diffs, + DifferentialComment $comment) { + switch ($this->filter) { + case DifferentialAction::ACTION_CLOSE: + case DifferentialAction::ACTION_UPDATE: + case DifferentialAction::ACTION_COMMENT: + return null; + case DifferentialAction::ACTION_ACCEPT: + case DifferentialAction::ACTION_REJECT: + $result = head($diffs); + foreach ($diffs as $diff) { + if ($diff->getDateCreated() >= $comment->getDateCreated()) { + break; + } + $result = $diff; + } + + return $result; + } + } } From 23f2ffb81c5ba393b9e5aa23ff0f125a72655b25 Mon Sep 17 00:00:00 2001 From: vrana Date: Sun, 15 Jul 2012 21:12:05 -0700 Subject: [PATCH 34/52] Add footer link to report a bug Summary: According to http://www.phabricator.com/docs/phabricator/article/Give_Feedback!_Get_Support!.html, we love feedback. But there is no way to provide it without reading the docs. Test Plan: Clicked the link. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin, ldemailly Differential Revision: https://secure.phabricator.com/D2975 --- src/view/page/PhabricatorStandardPageView.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index ba4ef9fb33..fcbcebaac6 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -323,6 +323,11 @@ final class PhabricatorStandardPageView extends AphrontPageView { $version = PhabricatorEnv::getEnvConfig('phabricator.version'); $foot_links[] = phutil_escape_html('Phabricator '.$version); + $foot_links[] = + ''. + 'Report a Bug'. + ''; + if (PhabricatorEnv::getEnvConfig('darkconsole.enabled') && !PhabricatorEnv::getEnvConfig('darkconsole.always-on')) { if ($console) { From 8fd97f4352354cf2ab538c04d8d72b2e337feada Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jul 2012 09:40:10 -0700 Subject: [PATCH 35/52] When users submit a no-op edit in Phriction, don't update the document Summary: See T1501. When users mash "save", stop them if they didn't change anything. Also, don't default-fill the "edit notes" field with the previous notes. This is meant to be more like a commit message for your changes. Test Plan: Edited a document with no changes, got a dialog. Edited a document with a title change only and a description change only, things worked. Edited a document with a previous "Edit notes", got a blank default fill. Reviewers: alanh, btrahan, vrana Reviewed By: vrana CC: aran Maniphest Tasks: T1501 Differential Revision: https://secure.phabricator.com/D2978 --- .../controller/PhrictionEditController.php | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/applications/phriction/controller/PhrictionEditController.php b/src/applications/phriction/controller/PhrictionEditController.php index d896169b0a..935fff78db 100644 --- a/src/applications/phriction/controller/PhrictionEditController.php +++ b/src/applications/phriction/controller/PhrictionEditController.php @@ -103,10 +103,12 @@ final class PhrictionEditController require_celerity_resource('phriction-document-css'); $e_title = true; + $notes = null; $errors = array(); if ($request->isFormPost()) { $title = $request->getStr('title'); + $notes = $request->getStr('description'); if (!strlen($title)) { $e_title = 'Required'; @@ -115,12 +117,27 @@ final class PhrictionEditController $e_title = null; } + if ($document->getID()) { + if ($content->getTitle() == $title && + $content->getContent() == $request->getStr('content')) { + + $dialog = new AphrontDialogView(); + $dialog->setUser($user); + $dialog->setTitle('No Edits'); + $dialog->appendChild( + '

You did not make any changes to the document.

'); + $dialog->addCancelButton($request->getRequestURI()); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + } + if (!count($errors)) { $editor = id(PhrictionDocumentEditor::newForSlug($document->getSlug())) ->setUser($user) ->setTitle($title) ->setContent($request->getStr('content')) - ->setDescription($request->getStr('description')); + ->setDescription($notes); $editor->save(); @@ -195,6 +212,7 @@ final class PhrictionEditController $form = id(new AphrontFormView()) ->setUser($user) + ->setWorkflow(true) ->setAction($request->getRequestURI()->getPath()) ->addHiddenInput('slug', $document->getSlug()) ->addHiddenInput('nodraft', $request->getBool('nodraft')) @@ -220,7 +238,7 @@ final class PhrictionEditController ->appendChild( id(new AphrontFormTextControl()) ->setLabel('Edit Notes') - ->setValue($content->getDescription()) + ->setValue($notes) ->setError(null) ->setName('description')) ->appendChild( From cb8120551f728f8072882f90c7c818db7233b422 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jul 2012 09:50:23 -0700 Subject: [PATCH 36/52] Don't special-case LiskDAO->load(0) Summary: Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists. This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example. On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object. Test Plan: Reasoned about program behavior. Reviewers: alanh, btrahan, vrana Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2977 --- src/infrastructure/storage/lisk/LiskDAO.php | 4 ++-- .../lisk/__tests__/LiskFixtureTestCase.php | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index a1d832d241..d980f4835b 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -442,8 +442,8 @@ abstract class LiskDAO { * @task load */ public function load($id) { - if (!($id = (int)$id)) { - throw new Exception("Bogus ID provided to load()."); + if (!$id || (!is_int($id) && !ctype_digit($id))) { + return null; } return $this->loadOneWhere( diff --git a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php index 750d997837..d20dfae1ee 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php @@ -93,6 +93,23 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { } } + public function testGarbageLoadCalls() { + $obj = new HarbormasterObject(); + $obj->save(); + $id = $obj->getID(); + + $load = new HarbormasterObject(); + + $this->assertEqual(null, $load->load(0)); + $this->assertEqual(null, $load->load(-1)); + $this->assertEqual(null, $load->load(9999)); + $this->assertEqual(null, $load->load('')); + $this->assertEqual(null, $load->load('cow')); + $this->assertEqual(null, $load->load($id."cow")); + + $this->assertEqual(true, (bool)$load->load((int)$id)); + $this->assertEqual(true, (bool)$load->load((string)$id)); + } } From 22660cff2a755d50b4331c4470224b2dbad4bc61 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jul 2012 10:35:36 -0700 Subject: [PATCH 37/52] OMG Summary: NOOOO Test Plan: Image macros work again. Reviewers: kdeggelman, btrahan, vrana Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2979 --- .../markup/rule/PhabricatorRemarkupRuleImageMacro.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php index 714c539152..c7ad58033d 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php +++ b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleImageMacro.php @@ -22,7 +22,7 @@ final class PhabricatorRemarkupRuleImageMacro extends PhutilRemarkupRule { - private $images = array(); + private $images; public function apply($text) { return preg_replace_callback( From 8065d728996644e82fb9fed5cb909e0546d5c1d4 Mon Sep 17 00:00:00 2001 From: Alan Huang Date: Mon, 16 Jul 2012 11:49:06 -0700 Subject: [PATCH 38/52] Add a differential.createinline method to Conduit Summary: a silly thing because I was bored Test Plan: and I said "arc call-conduit", and there were comments Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T1500 Differential Revision: https://secure.phabricator.com/D2971 --- src/__phutil_library_map__.php | 26 ++-- ...itAPI_differential_createinline_Method.php | 130 ++++++++++++++++++ 2 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dd3af5b8d9..e398a3c9c5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -135,6 +135,7 @@ phutil_register_library_map(array( 'ConduitAPI_differential_close_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_close_Method.php', 'ConduitAPI_differential_createcomment_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_createcomment_Method.php', 'ConduitAPI_differential_creatediff_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_creatediff_Method.php', + 'ConduitAPI_differential_createinline_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php', 'ConduitAPI_differential_createrawdiff_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_createrawdiff_Method.php', 'ConduitAPI_differential_createrevision_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_createrevision_Method.php', 'ConduitAPI_differential_find_Method' => 'applications/conduit/method/differential/ConduitAPI_differential_find_Method.php', @@ -1228,6 +1229,7 @@ phutil_register_library_map(array( 'ConduitAPI_differential_close_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_createcomment_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_creatediff_Method' => 'ConduitAPIMethod', + 'ConduitAPI_differential_createinline_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_createrawdiff_Method' => 'ConduitAPI_differential_Method', 'ConduitAPI_differential_createrevision_Method' => 'ConduitAPIMethod', 'ConduitAPI_differential_find_Method' => 'ConduitAPIMethod', @@ -1549,7 +1551,11 @@ phutil_register_library_map(array( 'ManiphestSavedQueryEditController' => 'ManiphestController', 'ManiphestSavedQueryListController' => 'ManiphestController', 'ManiphestSubpriorityController' => 'ManiphestController', - 'ManiphestTask' => 'ManiphestDAO', + 'ManiphestTask' => + array( + 0 => 'ManiphestDAO', + 1 => 'PhabricatorMarkupInterface', + ), 'ManiphestTaskAuxiliaryStorage' => 'ManiphestDAO', 'ManiphestTaskDescriptionChangeController' => 'ManiphestController', 'ManiphestTaskDescriptionPreviewController' => 'ManiphestController', @@ -1564,7 +1570,11 @@ phutil_register_library_map(array( 'ManiphestTaskStatus' => 'ManiphestConstants', 'ManiphestTaskSubscriber' => 'ManiphestDAO', 'ManiphestTaskSummaryView' => 'ManiphestView', - 'ManiphestTransaction' => 'ManiphestDAO', + 'ManiphestTransaction' => + array( + 0 => 'ManiphestDAO', + 1 => 'PhabricatorMarkupInterface', + ), 'ManiphestTransactionDetailView' => 'ManiphestView', 'ManiphestTransactionListView' => 'ManiphestView', 'ManiphestTransactionPreviewController' => 'ManiphestController', @@ -2039,16 +2049,16 @@ phutil_register_library_map(array( 'PhortuneStripeTestPaymentFormController' => 'PhortuneStripeBaseController', 'PhrictionActionConstants' => 'PhrictionConstants', 'PhrictionChangeType' => 'PhrictionConstants', - 'PhrictionContent' => 'PhrictionDAO', - 'PhrictionController' => 'PhabricatorController', - 'PhrictionDAO' => 'PhabricatorLiskDAO', - 'PhrictionDeleteController' => 'PhrictionController', - 'PhrictionDiffController' => 'PhrictionController', - 'PhrictionDocument' => + 'PhrictionContent' => array( 0 => 'PhrictionDAO', 1 => 'PhabricatorMarkupInterface', ), + 'PhrictionController' => 'PhabricatorController', + 'PhrictionDAO' => 'PhabricatorLiskDAO', + 'PhrictionDeleteController' => 'PhrictionController', + 'PhrictionDiffController' => 'PhrictionController', + 'PhrictionDocument' => 'PhrictionDAO', 'PhrictionDocumentController' => 'PhrictionController', 'PhrictionDocumentPreviewController' => 'PhrictionController', 'PhrictionDocumentStatus' => 'PhrictionConstants', diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php new file mode 100644 index 0000000000..0ffa7fd30c --- /dev/null +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php @@ -0,0 +1,130 @@ + 'optional revisionid', + 'diffID' => 'optional diffid', + 'filePath' => 'required string', + 'isNewFile' => 'required bool', + 'lineStart' => 'required int', + 'lineCount' => 'optional int', + 'message' => 'required string', + ); + } + + public function defineReturnType() { + return 'nonempty dict'; + } + + public function defineErrorTypes() { + return array( + 'ERR-BAD-REVISION' => 'Bad revision ID.', + 'ERR-BAD-DIFF' => 'Bad diff ID, or diff does not belong to revision.', + 'ERR-NEED-DIFF' => 'Neither revision ID nor diff ID was provided.', + 'ERR-NEED-FILE' => 'A file path was not provided.', + 'ERR-BAD-FILE' => "Requested file doesn't exist in this revision." + ); + } + + protected function execute(ConduitAPIRequest $request) { + $rid = $request->getValue('revisionID'); + $did = $request->getValue('diffID'); + + if ($rid) { + // Given both a revision and a diff, check that they match. + // Given only a revision, find the active diff. + $revision = id(new DifferentialRevision())->load($rid); + if (!$revision) { + throw new ConduitException('ERR-BAD-REVISION'); + } + + if (!$did) { // did not! + $diff = $revision->loadActiveDiff(); + $did = $diff->getID(); + } else { // did too! + $diff = id(new DifferentialDiff())->load($did); + if (!$diff || $diff->getRevisionID() != $rid) { + throw new ConduitException('ERR-BAD-DIFF'); + } + } + } else if ($did) { + // Given only a diff, find the parent revision. + $diff = id(new DifferentialDiff())->load($did); + if (!$diff) { + throw new ConduitException('ERR-BAD-DIFF'); + } + $rid = $diff->getRevisionID(); + } else { + // Given neither, bail. + throw new ConduitException('ERR-NEED-DIFF'); + } + + $file = $request->getValue('filePath'); + if (!$file) { + throw new ConduitException('ERR-NEED-FILE'); + } + $changes = id(new DifferentialChangeset())->loadAllWhere( + 'diffID = %d', + $did); + $cid = null; + foreach ($changes as $id => $change) { + if ($file == $change->getFilename()) { + $cid = $id; + } + } + if ($cid == null) { + throw new ConduitException('ERR-BAD-FILE'); + } + + $inline = id(new DifferentialInlineComment()) + ->setRevisionID($rid) + ->setChangesetID($cid) + ->setAuthorPHID($request->getUser()->getPHID()) + ->setContent($request->getValue('message')) + ->setIsNewFile($request->getValue('isNewFile')) + ->setLineNumber($request->getValue('lineStart')) + ->setLineLength($request->getValue('lineCount', 0)) + ->save(); + + // Load everything again, just to be safe. + $changeset = id(new DifferentialChangeset()) + ->load($inline->getChangesetID()); + return array( + 'filePath' => ($inline->getIsNewFile() ? + $changeset->getFilename() : + $changeset->getOldFile()), + 'isNewFile' => $inline->getIsNewFile(), + 'lineNumber' => $inline->getLineNumber(), + 'lineLength' => $inline->getLineLength(), + 'diffID' => $changeset->getDiffID(), + 'content' => $inline->getContent(), + ); + } + +} From 5daf08048be0e0812d6e399082710ba4620d0d0c Mon Sep 17 00:00:00 2001 From: vrana Date: Mon, 16 Jul 2012 12:44:14 -0700 Subject: [PATCH 39/52] Fix path autocomplete Summary: Paths autocompleter sometimes omits `/`. Test Plan: # Go to https://secure.phabricator.com/owners/new/. # Write `/specs/h` to path. # Delete the second slash resulting in `/specsh`. # Don't find `/specshistorytest.html` in autocomplete. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D2981 --- .../controller/DiffusionPathCompleteController.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionPathCompleteController.php b/src/applications/diffusion/controller/DiffusionPathCompleteController.php index 69ed227b71..67108c72b3 100644 --- a/src/applications/diffusion/controller/DiffusionPathCompleteController.php +++ b/src/applications/diffusion/controller/DiffusionPathCompleteController.php @@ -34,15 +34,12 @@ final class DiffusionPathCompleteController extends DiffusionController { } $query_path = $request->getStr('q'); - $query_path = ltrim($query_path, '/'); if (preg_match('@/$@', $query_path)) { $query_dir = $query_path; } else { - $query_dir = dirname($query_path); - if ($query_dir == '.') { - $query_dir = ''; - } + $query_dir = dirname($query_path).'/'; } + $query_dir = ltrim($query_dir, '/'); $drequest = DiffusionRequest::newFromDictionary( array( From 508f1b19f9666960668b46545c398f50fc257a4a Mon Sep 17 00:00:00 2001 From: mkedia Date: Fri, 22 Jun 2012 14:38:01 -0700 Subject: [PATCH 40/52] [easy] Add limit param to recent commits Summary: as title Test Plan: https://phabricator.fb.com/conduit/method/diffusion.getrecentcommitsbypath/ Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D2838 --- ...duitAPI_diffusion_getrecentcommitsbypath_Method.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php b/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php index c46c49ef74..a5d9014333 100644 --- a/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php +++ b/src/applications/conduit/method/diffusion/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php @@ -22,7 +22,7 @@ final class ConduitAPI_diffusion_getrecentcommitsbypath_Method extends ConduitAPIMethod { - const RESULT_LIMIT = 10; + const DEFAULT_LIMIT = 10; public function getMethodDescription() { return "Get commit identifiers for recent commits affecting a given path."; @@ -32,6 +32,7 @@ final class ConduitAPI_diffusion_getrecentcommitsbypath_Method return array( 'callsign' => 'required string', 'path' => 'required string', + 'limit' => 'optional int', ); } @@ -51,8 +52,13 @@ final class ConduitAPI_diffusion_getrecentcommitsbypath_Method 'path' => $request->getValue('path'), )); + $limit = nonempty( + $request->getValue('limit'), + self::DEFAULT_LIMIT + ); + $history = DiffusionHistoryQuery::newFromDiffusionRequest($drequest) - ->setLimit(self::RESULT_LIMIT) + ->setLimit($limit) ->needDirectChanges(true) ->needChildChanges(true) ->loadHistory(); From f3dec13431b0d1dceed25b9c5660523108460431 Mon Sep 17 00:00:00 2001 From: vrana Date: Mon, 16 Jul 2012 18:36:24 -0700 Subject: [PATCH 41/52] Highlight .divinerconfig as JavaScript Test Plan: Displayed `.arcconfig` and verified that it is still highlighted. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Differential Revision: https://secure.phabricator.com/D2987 --- conf/default.conf.php | 1 + 1 file changed, 1 insertion(+) diff --git a/conf/default.conf.php b/conf/default.conf.php index ec55ea3814..df9cf35c69 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -1177,6 +1177,7 @@ return array( // '@\\.([^.]+)\\.bak$@' => 1, '@\.arcconfig$@' => 'js', + '@\.divinerconfig$@' => 'js', ), // Set the default monospaced font style for users who haven't set a custom From 378feb3ffb3fa4123ea3cd80da0c8c43cfb7606c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jul 2012 19:01:43 -0700 Subject: [PATCH 42/52] Centralize rendering of application mail bodies Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is. Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible. Reviewers: btrahan, vrana Reviewed By: vrana CC: aran Maniphest Tasks: T931 Differential Revision: https://secure.phabricator.com/D2986 --- .divinerconfig | 3 +- src/__phutil_library_map__.php | 3 + .../editor/DifferentialRevisionEditor.php | 3 +- .../differential/mail/DifferentialMail.php | 27 +--- .../editor/ManiphestTransactionEditor.php | 26 +--- .../view/PhabricatorMetaMTAMailBody.php | 125 ++++++++++++++++++ .../PhabricatorMetaMTAMailBodyTestCase.php | 57 ++++++++ ...habricatorRepositoryCommitHeraldWorker.php | 46 ++----- 8 files changed, 215 insertions(+), 75 deletions(-) create mode 100644 src/applications/metamta/view/PhabricatorMetaMTAMailBody.php create mode 100644 src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php diff --git a/.divinerconfig b/.divinerconfig index ef6c805d91..8757fdb13b 100644 --- a/.divinerconfig +++ b/.divinerconfig @@ -23,7 +23,8 @@ "search" : "Search", "daemon" : "Daemons, Tasks and Workers", "irc" : "IRC", - "markup" : "Remarkup Extensions" + "markup" : "Remarkup Extensions", + "metamta" : "MetaMTA (Mail)" }, "engines" : [ ["DivinerArticleEngine", {}], diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e398a3c9c5..5f5de293ac 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -751,6 +751,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php', 'PhabricatorMetaMTAListController' => 'applications/metamta/controller/PhabricatorMetaMTAListController.php', 'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php', + 'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php', + 'PhabricatorMetaMTAMailBodyTestCase' => 'applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php', 'PhabricatorMetaMTAMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php', 'PhabricatorMetaMTAMailingList' => 'applications/metamta/storage/PhabricatorMetaMTAMailingList.php', 'PhabricatorMetaMTAMailingListEditController' => 'applications/metamta/controller/PhabricatorMetaMTAMailingListEditController.php', @@ -1763,6 +1765,7 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTAListController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO', + 'PhabricatorMetaMTAMailBodyTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTAMailingList' => 'PhabricatorMetaMTADAO', 'PhabricatorMetaMTAMailingListEditController' => 'PhabricatorMetaMTAController', diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index eab033a6c4..62c1c3b64e 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -251,8 +251,7 @@ final class DifferentialRevisionEditor { $adapter->setForbiddenCCs($revision->getUnsubscribedPHIDs()); $xscript = HeraldEngine::loadAndApplyRules($adapter); - $xscript_uri = PhabricatorEnv::getProductionURI( - '/herald/transcript/'.$xscript->getID().'/'); + $xscript_uri = '/herald/transcript/'.$xscript->getID().'/'; $xscript_phid = $xscript->getPHID(); $xscript_header = $xscript->getXHeraldRulesHeader(); diff --git a/src/applications/differential/mail/DifferentialMail.php b/src/applications/differential/mail/DifferentialMail.php index 1820ec1a89..f232186222 100644 --- a/src/applications/differential/mail/DifferentialMail.php +++ b/src/applications/differential/mail/DifferentialMail.php @@ -260,34 +260,21 @@ abstract class DifferentialMail { } protected function buildBody() { + $main_body = $this->renderBody(); - $body = $this->renderBody(); + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection($main_body); $reply_handler = $this->getReplyHandler(); - $reply_instructions = $reply_handler->getReplyHandlerInstructions(); - if ($reply_instructions) { - $body .= - "\nREPLY HANDLER ACTIONS\n". - " {$reply_instructions}\n"; - } + $body->addReplySection($reply_handler->getReplyHandlerInstructions()); if ($this->getHeraldTranscriptURI() && $this->isFirstMailToRecipients()) { - $manage_uri = PhabricatorEnv::getProductionURI( - '/herald/view/differential/'); - + $manage_uri = '/herald/view/differential/'; $xscript_uri = $this->getHeraldTranscriptURI(); - $body .= <<addHeraldSection($manage_uri, $xscript_uri); } - return $body; + return $body->render(); } /** diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 55a7f714c6..8ced7f62c3 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -245,7 +245,7 @@ final class ManiphestTransactionEditor { $view->setTransactionGroup($transactions); $view->setHandles($handles); $view->setAuxiliaryFields($this->auxiliaryFields); - list($action, $body) = $view->renderForEmail($with_date = false); + list($action, $main_body) = $view->renderForEmail($with_date = false); $is_create = $this->isCreate($transactions); @@ -253,25 +253,13 @@ final class ManiphestTransactionEditor { $reply_handler = $this->buildReplyHandler($task); + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection($main_body); if ($is_create) { - $body .= - "\n\n". - "TASK DESCRIPTION\n". - " ".$task->getDescription(); - } - - $body .= - "\n\n". - "TASK DETAIL\n". - " ".$task_uri."\n"; - - $reply_instructions = $reply_handler->getReplyHandlerInstructions(); - if ($reply_instructions) { - $body .= - "\n". - "REPLY HANDLER ACTIONS\n". - " ".$reply_instructions."\n"; + $body->addTextSection(pht('TASK DESCRIPTION'), $task->getDescription()); } + $body->addTextSection(pht('TASK DETAIL'), $task_uri); + $body->addReplySection($reply_handler->getReplyHandlerInstructions()); $thread_id = 'maniphest-task-'.$task->getPHID(); $task_id = $task->getID(); @@ -290,7 +278,7 @@ final class ManiphestTransactionEditor { ->setRelatedPHID($task->getPHID()) ->setIsBulk(true) ->setMailTags($mailtags) - ->setBody($body); + ->setBody($body->render()); $mails = $reply_handler->multiplexMail( $template, diff --git a/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php new file mode 100644 index 0000000000..3bf292670f --- /dev/null +++ b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php @@ -0,0 +1,125 @@ +sections[] = rtrim($text); + } + return $this; + } + + + /** + * Add a block of text with a section header. This is rendered like this: + * + * HEADER + * Text is indented. + * + * @param string Header text. + * @param string Section text. + * @return this + * @task compose + */ + public function addTextSection($header, $text) { + $this->sections[] = $header."\n".$this->indent($text); + return $this; + } + + + /** + * Add a Herald section with a rule management URI and a transcript URI. + * + * @param string URI to rule management. + * @param string URI to rule transcripts. + * @return this + * @task compose + */ + public function addHeraldSection($rules_uri, $xscript_uri) { + $this->addTextSection( + pht('MANAGE HERALD RULES'), + PhabricatorEnv::getProductionURI($rules_uri)); + $this->addTextSection( + pht('WHY DID I GET THIS EMAIL?'), + PhabricatorEnv::getProductionURI($xscript_uri)); + return $this; + } + + + /** + * Add a section with reply handler instructions. + * + * @param string Reply handler instructions. + * @return this + * @task compose + */ + public function addReplySection($instructions) { + if (strlen($instructions)) { + $this->addTextSection(pht('REPLY HANDLER ACTIONS'), $instructions); + } + return $this; + } + + +/* -( Rendering )---------------------------------------------------------- */ + + + /** + * Render the email body. + * + * @return string Rendered body. + * @task render + */ + public function render() { + return implode("\n\n", $this->sections)."\n"; + } + + + /** + * Indent a block of text for rendering under a section heading. + * + * @param string Text to indent. + * @return string Indented text. + * @task render + */ + private function indent($text) { + return rtrim(" ".str_replace("\n", "\n ", $text)); + } + +} diff --git a/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php new file mode 100644 index 0000000000..abbb2dbe93 --- /dev/null +++ b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php @@ -0,0 +1,57 @@ +overrideEnvConfig('phabricator.base-uri', 'http://test.com/'); + + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection("salmon"); + $body->addTextSection("HEADER", "bass\ntrout\n"); + $body->addHeraldSection("/rules/", "/xscript/"); + $body->addReplySection("pike"); + + $expect = <<assertEqual($expect, $body->render()); + } + + +} diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index c58032c3f2..b73bab67e8 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -111,49 +111,29 @@ final class PhabricatorRepositoryCommitHeraldWorker $files = $adapter->loadAffectedPaths(); sort($files); - $files = implode("\n ", $files); + $files = implode("\n", $files); $xscript_id = $xscript->getID(); - $manage_uri = PhabricatorEnv::getProductionURI('/herald/view/commits/'); - $why_uri = PhabricatorEnv::getProductionURI( - '/herald/transcript/'.$xscript_id.'/'); + $manage_uri = '/herald/view/commits/'; + $why_uri = '/herald/transcript/'.$xscript_id.'/'; $reply_handler = PhabricatorAuditCommentEditor::newReplyHandlerForCommit( $commit); - $reply_instructions = $reply_handler->getReplyHandlerInstructions(); - if ($reply_instructions) { - $reply_instructions = - "\n". - "REPLY HANDLER ACTIONS\n". - " ".$reply_instructions."\n"; - } - $template = new PhabricatorMetaMTAMail(); $inline_patch_text = $this->buildPatch($template, $repository, $commit); - $body = <<addRawSection($description); + $body->addTextSection(pht('DETAILS'), $commit_uri); + $body->addTextSection(pht('DIFFERENTIAL REVISION'), $differential); + $body->addTextSection(pht('AFFECTED FILES'), $files); + $body->addReplySection($reply_handler->getReplyHandlerInstructions()); + $body->addHeraldSection($manage_uri, $why_uri); + $body->addRawSection($inline_patch_text); + $body = $body->render(); $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); @@ -369,7 +349,7 @@ EOBODY; } if ($result) { - $result = "\nPATCH\n\n{$result}\n"; + $result = "PATCH\n\n{$result}\n"; } return $result; From 02f40fd7ef48a24eabdb54c47d7ce88302fc512e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jul 2012 19:02:06 -0700 Subject: [PATCH 43/52] Allow noncritical blocks in email bodies to be disabled via config Summary: See T931. LLVM users would also prefer simpler mail, and have similarly harsh opinions about the current state of affairs. Allow installs to disable the hint blocks if they don't want them. Test Plan: Sent myself mail with these settings on/off, got mail with/without the blocks. Reviewers: btrahan, vrana Reviewed By: vrana CC: aran Maniphest Tasks: T931 Differential Revision: https://secure.phabricator.com/D2968 --- conf/default.conf.php | 14 +++++ .../editor/PhabricatorAuditCommentEditor.php | 25 +++----- .../PhabricatorMailReplyHandler.php | 14 +++-- .../view/PhabricatorMetaMTAMailBody.php | 15 ++++- .../PhabricatorMetaMTAMailBodyTestCase.php | 63 ++++++++++++++++--- 5 files changed, 99 insertions(+), 32 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index df9cf35c69..426d00f774 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -466,6 +466,20 @@ return array( // address will be stored in an 'From Email' field on the task. 'metamta.maniphest.default-public-author' => null, + // You can disable the Herald hints in email if users prefer smaller messages. + // These are the links under the headers "MANAGE HERALD RULES" and + // "WHY DID I GET THIS EMAIL?". If you set this to true, they will not appear + // in any mail. Users can still navigate to the links via the web interface. + 'metamta.herald.show-hints' => true, + + // You can disable the hints under "REPLY HANDLER ACTIONS" if users prefer + // smaller messages. The actions themselves will still work properly. + 'metamta.reply.show-hints' => true, + + // You can disable the "To:" and "Cc:" footers in mail if users prefer + // smaller messages. + 'metamta.recipients.show-hints' => true, + // If this option is enabled, Phabricator will add a "Precedence: bulk" // header to transactional mail (e.g., Differential, Maniphest and Herald // notifications). This may improve the behavior of some auto-responder diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 53f76bec3f..8a352a3a72 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -486,12 +486,9 @@ final class PhabricatorAuditCommentEditor { $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb( $comment->getAction()); - $body = array(); - $body[] = "{$name} {$verb} commit {$cname}."; - - if ($comment->getContent()) { - $body[] = $comment->getContent(); - } + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection("{$name} {$verb} commit {$cname}."); + $body->addRawSection($comment->getContent()); if ($inline_comments) { $block = array(); @@ -518,18 +515,16 @@ final class PhabricatorAuditCommentEditor { $content = $inline->getContent(); $block[] = "{$path}:{$range} {$content}"; } - $body[] = "INLINE COMMENTS\n ".implode("\n ", $block); + + $body->addTextSection(pht('INLINE COMMENTS'), implode("\n", $block)); } - $body[] = "COMMIT\n ".PhabricatorEnv::getProductionURI($handle->getURI()); + $body->addTextSection( + pht('COMMIT'), + PhabricatorEnv::getProductionURI($handle->getURI())); + $body->addReplySection($reply_handler->getReplyHandlerInstructions()); - - $reply_instructions = $reply_handler->getReplyHandlerInstructions(); - if ($reply_instructions) { - $body[] = "REPLY HANDLER ACTIONS\n ".$reply_instructions; - } - - return implode("\n\n", $body)."\n"; + return $body->render(); } } diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index 8c78a513f9..18a2640f3c 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -78,12 +78,16 @@ abstract class PhabricatorMailReplyHandler { assert_instances_of($cc_handles, 'PhabricatorObjectHandle'); $body = ''; - if ($to_handles) { - $body .= "To: ".implode(', ', mpull($to_handles, 'getName'))."\n"; - } - if ($cc_handles) { - $body .= "Cc: ".implode(', ', mpull($cc_handles, 'getName'))."\n"; + + if (PhabricatorEnv::getEnvConfig('metamta.recipients.show-hints')) { + if ($to_handles) { + $body .= "To: ".implode(', ', mpull($to_handles, 'getName'))."\n"; + } + if ($cc_handles) { + $body .= "Cc: ".implode(', ', mpull($cc_handles, 'getName'))."\n"; + } } + return $body; } diff --git a/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php index 3bf292670f..556a0d0448 100644 --- a/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php +++ b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php @@ -72,12 +72,17 @@ final class PhabricatorMetaMTAMailBody { * @task compose */ public function addHeraldSection($rules_uri, $xscript_uri) { + if (!PhabricatorEnv::getEnvConfig('metamta.herald.show-hints')) { + return $this; + } + $this->addTextSection( pht('MANAGE HERALD RULES'), PhabricatorEnv::getProductionURI($rules_uri)); $this->addTextSection( pht('WHY DID I GET THIS EMAIL?'), PhabricatorEnv::getProductionURI($xscript_uri)); + return $this; } @@ -90,9 +95,15 @@ final class PhabricatorMetaMTAMailBody { * @task compose */ public function addReplySection($instructions) { - if (strlen($instructions)) { - $this->addTextSection(pht('REPLY HANDLER ACTIONS'), $instructions); + if (!PhabricatorEnv::getEnvConfig('metamta.reply.show-hints')) { + return $this; } + if (!strlen($instructions)) { + return $this; + } + + $this->addTextSection(pht('REPLY HANDLER ACTIONS'), $instructions); + return $this; } diff --git a/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php index abbb2dbe93..ca8d293f5f 100644 --- a/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php +++ b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php @@ -21,17 +21,8 @@ */ final class PhabricatorMetaMTAMailBodyTestCase extends PhabricatorTestCase { + public function testBodyRender() { - - $env = PhabricatorEnv::beginScopedEnv(); - $env->overrideEnvConfig('phabricator.base-uri', 'http://test.com/'); - - $body = new PhabricatorMetaMTAMailBody(); - $body->addRawSection("salmon"); - $body->addTextSection("HEADER", "bass\ntrout\n"); - $body->addHeraldSection("/rules/", "/xscript/"); - $body->addReplySection("pike"); - $expect = <<assertEmail($expect, true, true); + } + + + public function testBodyRenderNoHerald() { + $expect = <<assertEmail($expect, false, true); + } + + + public function testBodyRenderNoReply() { + $expect = <<assertEmail($expect, true, false); + } + + private function assertEmail($expect, $herald_hints, $reply_hints) { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('phabricator.base-uri', 'http://test.com/'); + $env->overrideEnvConfig('metamta.herald.show-hints', $herald_hints); + $env->overrideEnvConfig('metamta.reply.show-hints', $reply_hints); + + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection("salmon"); + $body->addTextSection("HEADER", "bass\ntrout\n"); + $body->addHeraldSection("/rules/", "/xscript/"); + $body->addReplySection("pike"); + $this->assertEqual($expect, $body->render()); } From 420d6426f96077d162b178ae8f3800f01bb00f5a Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 17 Jul 2012 13:39:32 -0400 Subject: [PATCH 44/52] 'phd status' should exit with non-zero if daemons are not running. 'phd status' may be invoked by tools (such as puppet) which need to make automated decisions about whether to start/restart the daemon. To enable this, 'phd status' now exits with 0 if all daemons are running, 1 if no daemons are running, and 2 if some (but not all) daemons are running. --- src/infrastructure/daemon/PhabricatorDaemonControl.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/infrastructure/daemon/PhabricatorDaemonControl.php b/src/infrastructure/daemon/PhabricatorDaemonControl.php index 33059d4cbe..b2b2f0cd79 100644 --- a/src/infrastructure/daemon/PhabricatorDaemonControl.php +++ b/src/infrastructure/daemon/PhabricatorDaemonControl.php @@ -41,9 +41,10 @@ final class PhabricatorDaemonControl { if (!$daemons) { echo "There are no running Phabricator daemons.\n"; - return 0; + return 1; } + $status = 0; printf( "%-5s\t%-24s\t%s\n", "PID", @@ -52,6 +53,7 @@ final class PhabricatorDaemonControl { foreach ($daemons as $daemon) { $name = $daemon->getName(); if (!$daemon->isRunning()) { + $status = 2; $name = ' '.$name; if ($daemon->getPIDFile()) { Filesystem::remove($daemon->getPIDFile()); @@ -66,7 +68,7 @@ final class PhabricatorDaemonControl { $name); } - return 0; + return $status; } public function executeStopCommand($pids = null) { From 883e11f761fd1d2fc189fbd5101afc3d3366a640 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 17 Jul 2012 13:44:15 -0400 Subject: [PATCH 45/52] Retain pid files for dead daemons in 'phd status'. 'phd status' should have a stable result when invoked multiple times. Automatically removing PID files for dead daemons every time 'phd status' is invoked prevents tools from noticing that a daemon has died if something happens to invoke 'phd status' before the tool looks. This affects Puppet noticably, since it probably runs the status command every half hour. --- src/infrastructure/daemon/PhabricatorDaemonControl.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/infrastructure/daemon/PhabricatorDaemonControl.php b/src/infrastructure/daemon/PhabricatorDaemonControl.php index b2b2f0cd79..f4b6f80508 100644 --- a/src/infrastructure/daemon/PhabricatorDaemonControl.php +++ b/src/infrastructure/daemon/PhabricatorDaemonControl.php @@ -55,9 +55,6 @@ final class PhabricatorDaemonControl { if (!$daemon->isRunning()) { $status = 2; $name = ' '.$name; - if ($daemon->getPIDFile()) { - Filesystem::remove($daemon->getPIDFile()); - } } printf( "%5s\t%-24s\t%s\n", From 9d26ef4248546310a7135cbd426beaf50ca57d12 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Jul 2012 12:06:18 -0700 Subject: [PATCH 46/52] Improve `differential.createinline` Summary: - Share code between `createinline` and `getcomments` - Make them both extend the base class. - Sync up the parameters (this is more or less nonbreaking since the call is ~6 hours old). Test Plan: Created and fetched inlines via the API. Reviewers: alanh, vrana, btrahan Reviewed By: vrana CC: aran Maniphest Tasks: T1500 Differential Revision: https://secure.phabricator.com/D2988 --- .../ConduitAPI_differential_Method.php | 27 +++++++++++++++++++ ...itAPI_differential_createinline_Method.php | 25 ++++++----------- ...ifferential_getrevisioncomments_Method.php | 21 +++------------ 3 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_Method.php index 32d78cf88e..51c933ca81 100644 --- a/src/applications/conduit/method/differential/ConduitAPI_differential_Method.php +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_Method.php @@ -32,4 +32,31 @@ abstract class ConduitAPI_differential_Method extends ConduitAPIMethod { } + protected function buildInlineInfoDictionary( + DifferentialInlineComment $inline, + DifferentialChangeset $changeset = null) { + + $file_path = null; + $diff_id = null; + if ($changeset) { + $file_path = $inline->getIsNewFile() + ? $changeset->getFilename() + : $changeset->getOldFile(); + + $diff_id = $changeset->getDiffID(); + } + + return array( + 'id' => $inline->getID(), + 'authorPHID' => $inline->getAuthorPHID(), + 'filePath' => $file_path, + 'isNewFile' => $inline->getIsNewFile(), + 'lineNumber' => $inline->getLineNumber(), + 'lineLength' => $inline->getLineLength(), + 'diffID' => $diff_id, + 'content' => $inline->getContent(), + ); + } + + } diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php index 0ffa7fd30c..a9dfa7d743 100644 --- a/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_createinline_Method.php @@ -20,7 +20,7 @@ * @group conduit */ final class ConduitAPI_differential_createinline_Method - extends ConduitAPIMethod { + extends ConduitAPI_differential_Method { public function getMethodDescription() { return "Add an inline comment to a Differential revision."; @@ -32,9 +32,9 @@ final class ConduitAPI_differential_createinline_Method 'diffID' => 'optional diffid', 'filePath' => 'required string', 'isNewFile' => 'required bool', - 'lineStart' => 'required int', - 'lineCount' => 'optional int', - 'message' => 'required string', + 'lineNumber' => 'required int', + 'lineLength' => 'optional int', + 'content' => 'required string', ); } @@ -106,25 +106,16 @@ final class ConduitAPI_differential_createinline_Method ->setRevisionID($rid) ->setChangesetID($cid) ->setAuthorPHID($request->getUser()->getPHID()) - ->setContent($request->getValue('message')) + ->setContent($request->getValue('content')) ->setIsNewFile($request->getValue('isNewFile')) - ->setLineNumber($request->getValue('lineStart')) - ->setLineLength($request->getValue('lineCount', 0)) + ->setLineNumber($request->getValue('lineNumber')) + ->setLineLength($request->getValue('lineLength', 0)) ->save(); // Load everything again, just to be safe. $changeset = id(new DifferentialChangeset()) ->load($inline->getChangesetID()); - return array( - 'filePath' => ($inline->getIsNewFile() ? - $changeset->getFilename() : - $changeset->getOldFile()), - 'isNewFile' => $inline->getIsNewFile(), - 'lineNumber' => $inline->getLineNumber(), - 'lineLength' => $inline->getLineLength(), - 'diffID' => $changeset->getDiffID(), - 'content' => $inline->getContent(), - ); + return $this->buildInlineInfoDictionary($inline, $changeset); } } diff --git a/src/applications/conduit/method/differential/ConduitAPI_differential_getrevisioncomments_Method.php b/src/applications/conduit/method/differential/ConduitAPI_differential_getrevisioncomments_Method.php index 5f09a73fb8..8d71da0149 100644 --- a/src/applications/conduit/method/differential/ConduitAPI_differential_getrevisioncomments_Method.php +++ b/src/applications/conduit/method/differential/ConduitAPI_differential_getrevisioncomments_Method.php @@ -20,7 +20,7 @@ * @group conduit */ final class ConduitAPI_differential_getrevisioncomments_Method - extends ConduitAPIMethod { + extends ConduitAPI_differential_Method { public function getMethodDescription() { return "Retrieve Differential Revision Comments."; @@ -81,23 +81,10 @@ final class ConduitAPI_differential_getrevisioncomments_Method if ($with_inlines) { $result['inlines'] = array(); foreach (idx($inlines, $comment->getID(), array()) as $inline) { - $file_path = null; - $diff_id = null; $changeset = idx($changesets, $inline->getChangesetID()); - if ($changeset) { - $file_path = ($inline->getIsNewFile() ? - $changeset->getFilename() : - $changeset->getOldFile()); - $diff_id = $changeset->getDiffID(); - } - $result['inlines'][] = array( - 'filePath' => $file_path, - 'isNewFile' => $inline->getIsNewFile(), - 'lineNumber' => $inline->getLineNumber(), - 'lineLength' => $inline->getLineLength(), - 'diffID' => $diff_id, - 'content' => $inline->getContent(), - ); + $result['inlines'][] = $this->buildInlineInfoDictionary( + $inline, + $changeset); } // TODO: Put synthetic inlines without an attached comment somewhere. } From ae2e73ce809ff1141c24d0e4cb2aa1de49bcfa96 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Jul 2012 12:06:25 -0700 Subject: [PATCH 47/52] Add "stop on redirect" and "always profile" debugging options Summary: Currently, it's hard to debug performance issues on POST pages. Add flags to stop redirects and always collect profiles. Also fix an issue with "all" profiles. This feature is mostly just for profiling DarkConsole itself and is rarely used, I think it's been broken for some time. There's no way to get to it with the UI. NOTE: Some JS workflows don't stop on redirect because they use JS/AJAX redirects. Test Plan: Enabled options, browsed, got stopped on redirects and had profiles generated. Disabled options and verified redirects and profiles work normally. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2990 --- conf/default.conf.php | 17 ++++++ .../plugin/DarkConsoleXHProfPlugin.php | 4 +- .../xhprof/DarkConsoleXHProfPluginAPI.php | 59 +++++++++++++++---- .../response/AphrontRedirectResponse.php | 38 +++++++++++- src/aphront/writeguard/AphrontWriteGuard.php | 11 ++++ webroot/index.php | 4 +- 6 files changed, 115 insertions(+), 18 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 426d00f774..6b856ea69f 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -1199,4 +1199,21 @@ return array( 'style.monospace' => '10px "Menlo", "Consolas", "Monaco", monospace', +// -- Debugging ------------------------------------------------------------- // + + // Enable this to change HTTP redirects into normal pages with a link to the + // redirection target. For example, after you submit a form you'll get a page + // saying "normally, you'd be redirected...". This is useful to examine + // service or profiler information on write pathways, or debug redirects. It + // also makes the UX horrible for normal use, so you should enable it only + // when debugging. + // + // NOTE: This does not currently work for forms with Javascript "workflow", + // since the redirect happens in Javascript. + 'debug.stop-on-redirect' => false, + + // Enable this to always profile every page. This is very slow! You should + // only enable it when debugging. + 'debug.profile-every-request' => false, + ); diff --git a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php index e2a03d7804..0c38b60f99 100644 --- a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php +++ b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php @@ -103,8 +103,8 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { public function willShutdown() { - if (isset($_REQUEST['__profile__']) && - $_REQUEST['__profile__'] != 'all') { + if (DarkConsoleXHProfPluginAPI::isProfilerRequested() && + (DarkConsoleXHProfPluginAPI::isProfilerRequested() !== 'all')) { $this->xhprofID = DarkConsoleXHProfPluginAPI::stopProfiler(); } } diff --git a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php index 38bc063aed..8ab312c4df 100644 --- a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php +++ b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php @@ -29,6 +29,18 @@ final class DarkConsoleXHProfPluginAPI { return extension_loaded('xhprof'); } + public static function isProfilerRequested() { + if (!empty($_REQUEST['__profile__'])) { + return $_REQUEST['__profile__']; + } + + if (PhabricatorEnv::getEnvConfig('debug.profile-every-request')) { + return PhabricatorEnv::getEnvConfig('debug.profile-every-request'); + } + + return false; + } + public static function includeXHProfLib() { // TODO: this is incredibly stupid, but we may not have Phutil metamodule // stuff loaded yet so we can't just phutil_get_library_root() our way @@ -41,8 +53,9 @@ final class DarkConsoleXHProfPluginAPI { require_once $root.'/externals/xhprof/xhprof_lib.php'; } + public static function hookProfiler() { - if (empty($_REQUEST['__profile__'])) { + if (!self::isProfilerRequested()) { return; } @@ -71,17 +84,41 @@ final class DarkConsoleXHProfPluginAPI { $data = serialize($data); $file_class = 'PhabricatorFile'; - // Since these happen on GET we can't do guarded writes. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + // Since these happen on GET we can't do guarded writes. These also + // sometimes happen after we've disposed of the write guard; in this + // case we need to disable the whole mechanism. - $file = call_user_func( - array($file_class, 'newFromFileData'), - $data, - array( - 'mime-type' => 'application/xhprof', - 'name' => 'profile.xhprof', - )); - return $file->getPHID(); + $use_scope = AphrontWriteGuard::isGuardActive(); + if ($use_scope) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(true); + } + + $caught = null; + try { + $file = call_user_func( + array($file_class, 'newFromFileData'), + $data, + array( + 'mime-type' => 'application/xhprof', + 'name' => 'profile.xhprof', + )); + } catch (Exception $ex) { + $caught = $ex; + } + + if ($use_scope) { + unset($unguarded); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(false); + } + + if ($caught) { + throw $caught; + } else { + return $file->getPHID(); + } } else { return null; } diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php index f49681436b..7d060bb16b 100644 --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -34,15 +34,47 @@ class AphrontRedirectResponse extends AphrontResponse { return (string)$this->uri; } + public function shouldStopForDebugging() { + return PhabricatorEnv::getEnvConfig('debug.stop-on-redirect'); + } + public function getHeaders() { - $headers = array( - array('Location', $this->uri), - ); + $headers = array(); + if (!$this->shouldStopForDebugging()) { + $headers[] = array('Location', $this->uri); + } $headers = array_merge(parent::getHeaders(), $headers); return $headers; } public function buildResponseString() { + if ($this->shouldStopForDebugging()) { + $view = new PhabricatorStandardPageView(); + $view->setRequest($this->getRequest()); + $view->setApplicationName('Debug'); + $view->setTitle('Stopped on Redirect'); + + $error = new AphrontErrorView(); + $error->setSeverity(AphrontErrorView::SEVERITY_NOTICE); + $error->setTitle('Stopped on Redirect'); + + $link = phutil_render_tag( + 'a', + array( + 'href' => $this->getURI(), + ), + 'Continue to: '.phutil_escape_html($this->getURI())); + + $error->appendChild( + '

You were stopped here because debug.stop-on-redirect '. + 'is set in your configuration.

'. + '

'.$link.'

'); + + $view->appendChild($error); + + return $view->render(); + } + return ''; } diff --git a/src/aphront/writeguard/AphrontWriteGuard.php b/src/aphront/writeguard/AphrontWriteGuard.php index 38d6ce7ad2..e0a61284cf 100644 --- a/src/aphront/writeguard/AphrontWriteGuard.php +++ b/src/aphront/writeguard/AphrontWriteGuard.php @@ -108,6 +108,17 @@ final class AphrontWriteGuard { } + /** + * Determine if there is an active write guard. + * + * @return bool + * @task manage + */ + public static function isGuardActive() { + return (bool)self::$instance; + } + + /* -( Protecting Writes )-------------------------------------------------- */ diff --git a/webroot/index.php b/webroot/index.php index 1b5076134a..cfca18e57a 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -220,8 +220,8 @@ $headers = array_merge($headers, $response->getHeaders()); $sink->writeHeaders($headers); // TODO: This shouldn't be possible in a production-configured environment. -if (isset($_REQUEST['__profile__']) && - ($_REQUEST['__profile__'] == 'all')) { +if (DarkConsoleXHProfPluginAPI::isProfilerRequested() && + DarkConsoleXHProfPluginAPI::isProfilerRequested() === 'all') { $profile = DarkConsoleXHProfPluginAPI::stopProfiler(); $profile = '
openTransaction(); $editor->makeAdminUser($user, $set_admin); if ($changed_pass !== false) { - $editor->changePassword($user, $changed_pass); + $envelope = new PhutilOpaqueEnvelope($changed_pass); + $editor->changePassword($user, $envelope); } $user->saveTransaction(); diff --git a/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php b/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php index 98d1801596..142accdc84 100644 --- a/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php +++ b/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php @@ -29,6 +29,10 @@ final class DarkConsoleErrorLogPluginAPI { self::$discardMode = true; } + public static function disableDiscardMode() { + self::$discardMode = false; + } + public static function getErrors() { return self::$errors; } diff --git a/src/applications/auth/controller/PhabricatorLDAPLoginController.php b/src/applications/auth/controller/PhabricatorLDAPLoginController.php index 0525625373..524f52ef13 100644 --- a/src/applications/auth/controller/PhabricatorLDAPLoginController.php +++ b/src/applications/auth/controller/PhabricatorLDAPLoginController.php @@ -37,9 +37,8 @@ final class PhabricatorLDAPLoginController extends PhabricatorAuthController { if ($request->isFormPost()) { try { - $this->provider->auth($request->getStr('username'), - $request->getStr('password')); - + $envelope = new PhutilOpaqueEnvelope($request->getStr('password')); + $this->provider->auth($request->getStr('username'), $envelope); } catch (Exception $e) { $errors[] = $e->getMessage(); } diff --git a/src/applications/auth/controller/PhabricatorLoginController.php b/src/applications/auth/controller/PhabricatorLoginController.php index c48b287233..2af18468dc 100644 --- a/src/applications/auth/controller/PhabricatorLoginController.php +++ b/src/applications/auth/controller/PhabricatorLoginController.php @@ -119,7 +119,10 @@ final class PhabricatorLoginController if (!$errors) { // Perform username/password tests only if we didn't get rate limited // by the CAPTCHA. - if (!$user || !$user->comparePassword($request->getStr('password'))) { + + $envelope = new PhutilOpaqueEnvelope($request->getStr('password')); + + if (!$user || !$user->comparePassword($envelope)) { $errors[] = 'Bad username/password.'; } } diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index d73659c5fb..166b312a5d 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -53,7 +53,7 @@ final class PhabricatorLDAPProvider { public function retrieveUserEmail() { return $this->userData['mail'][0]; } - + public function retrieveUserRealName() { return $this->retrieveUserRealNameFromData($this->userData); } @@ -106,9 +106,9 @@ final class PhabricatorLDAPProvider { return $this->userData; } - public function auth($username, $password) { - if (strlen(trim($username)) == 0 || strlen(trim($password)) == 0) { - throw new Exception('Username and/or password can not be empty'); + public function auth($username, PhutilOpaqueEnvelope $password) { + if (strlen(trim($username)) == 0) { + throw new Exception('Username can not be empty'); } $activeDirectoryDomain = @@ -121,10 +121,17 @@ final class PhabricatorLDAPProvider { $this->getBaseDN(); } - $result = ldap_bind($this->getConnection(), $dn, $password); + $conn = $this->getConnection(); + + // NOTE: It is very important we suppress any messages that occur here, + // because it logs passwords if it reaches an error log of any sort. + DarkConsoleErrorLogPluginAPI::enableDiscardMode(); + $result = @ldap_bind($conn, $dn, $password->openEnvelope()); + DarkConsoleErrorLogPluginAPI::disableDiscardMode(); if (!$result) { - throw new Exception('Bad username/password.'); + throw new Exception( + "LDAP Error #".ldap_errno($conn).": ".ldap_error($conn)); } $this->userData = $this->getUser($username); @@ -184,14 +191,14 @@ final class PhabricatorLDAPProvider { $row = array(); $entry = $entries[$i]; - // Get username, email and realname + // Get username, email and realname $username = $entry[$this->getSearchAttribute()][0]; if(empty($username)) { continue; } $row[] = $username; $row[] = $entry['mail'][0]; - $row[] = $this->retrieveUserRealNameFromData($entry); + $row[] = $this->retrieveUserRealNameFromData($entry); $rows[] = $row; diff --git a/src/applications/people/PhabricatorUserEditor.php b/src/applications/people/PhabricatorUserEditor.php index 0752a4a937..1ad29c0919 100644 --- a/src/applications/people/PhabricatorUserEditor.php +++ b/src/applications/people/PhabricatorUserEditor.php @@ -127,7 +127,10 @@ final class PhabricatorUserEditor { /** * @task edit */ - public function changePassword(PhabricatorUser $user, $password) { + public function changePassword( + PhabricatorUser $user, + PhutilOpaqueEnvelope $envelope) { + if (!$user->getID()) { throw new Exception("User has not been created yet!"); } @@ -135,7 +138,7 @@ final class PhabricatorUserEditor { $user->openTransaction(); $user->reload(); - $user->setPassword($password); + $user->setPassword($envelope); $user->save(); $log = PhabricatorUserLog::newLog( diff --git a/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php b/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php index 3f2ca1021a..af2e8d9b93 100644 --- a/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php @@ -59,7 +59,8 @@ final class PhabricatorUserPasswordSettingsPanelController $errors = array(); if ($request->isFormPost()) { if (!$valid_token) { - if (!$user->comparePassword($request->getStr('old_pw'))) { + $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw')); + if (!$user->comparePassword($envelope)) { $errors[] = 'The old password you entered is incorrect.'; $e_old = 'Invalid'; } @@ -85,9 +86,10 @@ final class PhabricatorUserPasswordSettingsPanelController // is changed here the CSRF token check will fail. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $envelope = new PhutilOpaqueEnvelope($pass); id(new PhabricatorUserEditor()) ->setActor($user) - ->changePassword($user, $pass); + ->changePassword($user, $envelope); unset($unguarded); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 8786196b89..71383fc17f 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -74,18 +74,18 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson { PhabricatorPHIDConstants::PHID_TYPE_USER); } - public function setPassword($password) { + public function setPassword(PhutilOpaqueEnvelope $envelope) { if (!$this->getPHID()) { throw new Exception( "You can not set a password for an unsaved user because their PHID ". "is a salt component in the password hash."); } - if (!strlen($password)) { + if (!strlen($envelope->openEnvelope())) { $this->setPasswordHash(''); } else { $this->setPasswordSalt(md5(mt_rand())); - $hash = $this->hashPassword($password); + $hash = $this->hashPassword($envelope); $this->setPasswordHash($hash); } return $this; @@ -129,26 +129,26 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson { return Filesystem::readRandomCharacters(255); } - public function comparePassword($password) { - if (!strlen($password)) { + public function comparePassword(PhutilOpaqueEnvelope $envelope) { + if (!strlen($envelope->openEnvelope())) { return false; } if (!strlen($this->getPasswordHash())) { return false; } - $password = $this->hashPassword($password); - return ($password === $this->getPasswordHash()); + $password_hash = $this->hashPassword($envelope); + return ($password_hash === $this->getPasswordHash()); } - private function hashPassword($password) { - $password = $this->getUsername(). - $password. - $this->getPHID(). - $this->getPasswordSalt(); + private function hashPassword(PhutilOpaqueEnvelope $envelope) { + $hash = $this->getUsername(). + $envelope->openEnvelope(). + $this->getPHID(). + $this->getPasswordSalt(); for ($ii = 0; $ii < 1000; $ii++) { - $password = md5($password); + $hash = md5($hash); } - return $password; + return $hash; } const CSRF_CYCLE_FREQUENCY = 3600; From e098f5d275144168041ca927f5fd5cb312c76796 Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 17 Jul 2012 15:20:53 -0400 Subject: [PATCH 49/52] Mention non-zero exit from 'phd status' in 'phd help'. --- src/infrastructure/daemon/PhabricatorDaemonControl.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/infrastructure/daemon/PhabricatorDaemonControl.php b/src/infrastructure/daemon/PhabricatorDaemonControl.php index f4b6f80508..b2c886c303 100644 --- a/src/infrastructure/daemon/PhabricatorDaemonControl.php +++ b/src/infrastructure/daemon/PhabricatorDaemonControl.php @@ -173,7 +173,8 @@ final class PhabricatorDaemonControl { List available daemons. **status** - List running daemons. + List running daemons. This command will exit with a non-zero exit + status if any daemons are not running. **help** Show this help. From 30deacdbaf487afcc74c08aba8564313a7afbe45 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Jul 2012 14:05:26 -0700 Subject: [PATCH 50/52] Fix some more ldap issues Summary: - LDAP import needs to use envelopes. - Use ldap_sprintf(). Test Plan: Configured an LDAP server. Added an account. Imported it; logged in with it. Tried to login with accounts like ",", etc., got good errors. Reviewers: vrana, btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2995 --- .../PhabricatorLDAPLoginController.php | 5 +- .../auth/ldap/PhabricatorLDAPProvider.php | 19 ++++-- .../PhabricatorPeopleLdapController.php | 61 ++++++++++--------- 3 files changed, 48 insertions(+), 37 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorLDAPLoginController.php b/src/applications/auth/controller/PhabricatorLDAPLoginController.php index 524f52ef13..6b3c1e86da 100644 --- a/src/applications/auth/controller/PhabricatorLDAPLoginController.php +++ b/src/applications/auth/controller/PhabricatorLDAPLoginController.php @@ -35,10 +35,12 @@ final class PhabricatorLDAPLoginController extends PhabricatorAuthController { $current_user = $this->getRequest()->getUser(); $request = $this->getRequest(); + $ldap_username = $request->getCookie('phusr'); if ($request->isFormPost()) { + $ldap_username = $request->getStr('username'); try { $envelope = new PhutilOpaqueEnvelope($request->getStr('password')); - $this->provider->auth($request->getStr('username'), $envelope); + $this->provider->auth($ldap_username, $envelope); } catch (Exception $e) { $errors[] = $e->getMessage(); } @@ -124,7 +126,6 @@ final class PhabricatorLDAPLoginController extends PhabricatorAuthController { } } - $ldap_username = $request->getCookie('phusr'); $ldap_form = new AphrontFormView(); $ldap_form ->setUser($request->getUser()) diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index 166b312a5d..2fbaa99581 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -117,8 +117,11 @@ final class PhabricatorLDAPProvider { if ($activeDirectoryDomain) { $dn = $username . '@' . $activeDirectoryDomain; } else { - $dn = $this->getSearchAttribute() . '=' . $username . ',' . - $this->getBaseDN(); + $dn = ldap_sprintf( + '%Q=%s,%Q', + $this->getSearchAttribute(), + $username, + $this->getBaseDN()); } $conn = $this->getConnection(); @@ -139,15 +142,21 @@ final class PhabricatorLDAPProvider { } private function getUser($username) { - $result = ldap_search($this->getConnection(), $this->getBaseDN(), - $this->getSearchAttribute() . '=' . $username); + $conn = $this->getConnection(); + + $query = ldap_sprintf( + '%Q=%S', + $this->getSearchAttribute(), + $username); + + $result = ldap_search($conn, $this->getBaseDN(), $query); if (!$result) { throw new Exception('Search failed. Please check your LDAP and HTTP '. 'logs for more information.'); } - $entries = ldap_get_entries($this->getConnection(), $result); + $entries = ldap_get_entries($conn, $result); if ($entries === false) { throw new Exception('Could not get entries'); diff --git a/src/applications/people/controller/PhabricatorPeopleLdapController.php b/src/applications/people/controller/PhabricatorPeopleLdapController.php index 333f8f373d..57d47ae29b 100644 --- a/src/applications/people/controller/PhabricatorPeopleLdapController.php +++ b/src/applications/people/controller/PhabricatorPeopleLdapController.php @@ -54,7 +54,7 @@ final class PhabricatorPeopleLdapController ->setValue('Search')); $panel = new AphrontPanelView(); - $panel->setHeader('Import Ldap Users'); + $panel->setHeader('Import LDAP Users'); $panel->appendChild($form); @@ -126,7 +126,8 @@ final class PhabricatorPeopleLdapController try { $ldap_provider = new PhabricatorLDAPProvider(); - $ldap_provider->auth($username, $password); + $envelope = new PhutilOpaqueEnvelope($password); + $ldap_provider->auth($username, $envelope); $results = $ldap_provider->search($search); foreach ($results as $key => $result) { $results[$key][] = $this->renderUserInputs($result); @@ -141,7 +142,7 @@ final class PhabricatorPeopleLdapController 'Username', 'Email', 'RealName', - '', + 'Import?', )); $form->appendChild($table); $form->setAction($request->getRequestURI() @@ -163,35 +164,35 @@ final class PhabricatorPeopleLdapController } private function renderUserInputs($user) { - $username = $user[0]; - $inputs = phutil_render_tag( - 'input', - array( - 'type' => 'checkbox', - 'name' => 'usernames[]', - 'value' =>$username, - ), - ''); + $username = $user[0]; + $inputs = phutil_render_tag( + 'input', + array( + 'type' => 'checkbox', + 'name' => 'usernames[]', + 'value' =>$username, + ), + ''); - $inputs .= phutil_render_tag( - 'input', - array( - 'type' => 'hidden', - 'name' => "email[$username]", - 'value' =>$user[1], - ), - ''); + $inputs .= phutil_render_tag( + 'input', + array( + 'type' => 'hidden', + 'name' => "email[$username]", + 'value' =>$user[1], + ), + ''); - $inputs .= phutil_render_tag( - 'input', - array( - 'type' => 'hidden', - 'name' => "name[$username]", - 'value' =>$user[2], - ), - ''); - - return $inputs; + $inputs .= phutil_render_tag( + 'input', + array( + 'type' => 'hidden', + 'name' => "name[$username]", + 'value' =>$user[2], + ), + ''); + return $inputs; } + } From 0d162e15f649801e5e95107b5dc714bbdcce8b9c Mon Sep 17 00:00:00 2001 From: vrana Date: Tue, 17 Jul 2012 17:13:03 -0700 Subject: [PATCH 51/52] Make Diffusion view sticky Summary: Blame can be slow. Test Plan: Viewed a file with no preference, saw blame. Changed view, saw it. Viewed a file, saw the changed view. Viewed a file as raw document. Reviewers: Two9A, epriestley Reviewed By: epriestley CC: aran, epriestley, Korvin, btrahan Maniphest Tasks: T1278 Differential Revision: https://secure.phabricator.com/D3000 --- .../DiffusionBrowseFileController.php | 34 ++++++++++++------- .../storage/PhabricatorUserPreferences.php | 2 ++ 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index 8ac7b2e5cc..ed31b2191d 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -31,11 +31,20 @@ final class DiffusionBrowseFileController extends DiffusionController { } $path = $drequest->getPath(); + $selected = $request->getStr('view'); - // If requested without a view, assume that blame is required (see T1278). + $preferences = $request->getUser()->loadPreferences(); if (!$selected) { - $selected = 'blame'; + $selected = $preferences->getPreference( + PhabricatorUserPreferences::PREFERENCE_DIFFUSION_VIEW, + 'highlighted'); + } else if ($request->isFormPost() && $selected != 'raw') { + $preferences->setPreference( + PhabricatorUserPreferences::PREFERENCE_DIFFUSION_VIEW, + $selected); + $preferences->save(); } + $needs_blame = ($selected == 'blame' || $selected == 'plainblame'); $file_query = DiffusionFileContentQuery::newFromDiffusionRequest( @@ -60,7 +69,7 @@ final class DiffusionBrowseFileController extends DiffusionController { require_celerity_resource('diffusion-source-css'); if ($this->corpusType == 'text') { - $view_select_panel = $this->renderViewSelectPanel(); + $view_select_panel = $this->renderViewSelectPanel($selected); } else { $view_select_panel = null; } @@ -217,17 +226,17 @@ final class DiffusionBrowseFileController extends DiffusionController { return $corpus; } - private function renderViewSelectPanel() { + private function renderViewSelectPanel($selected) { $request = $this->getRequest(); $select = AphrontFormSelectControl::renderSelectTag( - $request->getStr('view'), + $selected, array( - 'blame' => 'View as Highlighted Text with Blame', 'highlighted' => 'View as Highlighted Text', - 'plainblame' => 'View as Plain Text with Blame', + 'blame' => 'View as Highlighted Text with Blame', 'plain' => 'View as Plain Text', + 'plainblame' => 'View as Plain Text with Blame', 'raw' => 'View as raw document', ), array( @@ -235,11 +244,11 @@ final class DiffusionBrowseFileController extends DiffusionController { )); $view_select_panel = new AphrontPanelView(); - $view_select_form = phutil_render_tag( - 'form', + $view_select_form = phabricator_render_form( + $request->getUser(), array( - 'action' => $request->getRequestURI(), - 'method' => 'get', + 'action' => $request->getRequestURI()->alter('view', null), + 'method' => 'post', 'class' => 'diffusion-browse-type-form', ), $select. @@ -420,10 +429,9 @@ final class DiffusionBrowseFileController extends DiffusionController { 'action' => 'browse', 'line' => $line['line'], 'stable' => true, + 'params' => array('view' => $selected), )); - $line_href->setQueryParams($request->getRequestURI()->getQueryParams()); - $blame = array(); if ($line['color']) { $color = $line['color']; diff --git a/src/applications/people/storage/PhabricatorUserPreferences.php b/src/applications/people/storage/PhabricatorUserPreferences.php index c1512a0a00..d569d8929a 100644 --- a/src/applications/people/storage/PhabricatorUserPreferences.php +++ b/src/applications/people/storage/PhabricatorUserPreferences.php @@ -30,6 +30,8 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO { const PREFERENCE_SEARCHBAR_JUMP = 'searchbar-jump'; const PREFERENCE_SEARCH_SHORTCUT = 'search-shortcut'; + const PREFERENCE_DIFFUSION_VIEW = 'diffusion-view'; + protected $userPHID; protected $preferences = array(); From 7081923bd73c271faa57e375ab942cf7ad5a1797 Mon Sep 17 00:00:00 2001 From: Neal Poole Date: Tue, 17 Jul 2012 18:18:16 -0700 Subject: [PATCH 52/52] Adding 'Secure Browsing' config setting to Facebook OAuth. Summary: The Graph API exposes a new field, security_settings, which allows applications to see whether a user has enabled Secure Browsing. This diff adds a configuration setting to Phabricator which forces users to have Secure Browsing enabled when logging in via Facebook. Test Plan: With the configuration setting off, verify that secure browsing does not affect the ability to log in. With the configuration setting on and secure browsing off, verify that the login attempts is rejected. Then verify that the login attempt succeeds when secure browsing is enabled. Reviewers: epriestley Reviewed By: epriestley CC: arice, aran, Korvin Maniphest Tasks: T1487 Differential Revision: https://secure.phabricator.com/D2964 --- conf/default.conf.php | 3 +++ .../PhabricatorOAuthLoginController.php | 10 ++++++++-- .../provider/PhabricatorOAuthProviderFacebook.php | 15 ++++++++++++++- .../auth/view/PhabricatorOAuthFailureView.php | 11 +++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 6b856ea69f..4176dcb9b5 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -582,6 +582,9 @@ return array( // The Facebook "Application Secret" to use for Facebook API access. 'facebook.application-secret' => null, + // Should Phabricator reject requests made by users with + // Secure Browsing disabled? + 'facebook.require-https-auth' => false, // -- GitHub OAuth ---------------------------------------------------------- // diff --git a/src/applications/auth/controller/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/PhabricatorOAuthLoginController.php index 62da43d8c7..d87e7a2cd4 100644 --- a/src/applications/auth/controller/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/PhabricatorOAuthLoginController.php @@ -70,7 +70,7 @@ final class PhabricatorOAuthLoginController } $provider->setUserData($user_data); } catch (PhabricatorOAuthProviderException $e) { - return $this->buildErrorResponse(new PhabricatorOAuthFailureView()); + return $this->buildErrorResponse(new PhabricatorOAuthFailureView(), $e); } $provider->setAccessToken($this->accessToken); @@ -243,12 +243,18 @@ final class PhabricatorOAuthLoginController return $this->delegateToController($controller); } - private function buildErrorResponse(PhabricatorOAuthFailureView $view) { + private function buildErrorResponse(PhabricatorOAuthFailureView $view, + Exception $e = null) { + $provider = $this->provider; $provider_name = $provider->getProviderName(); $view->setOAuthProvider($provider); + if ($e) { + $view->setException($e); + } + return $this->buildStandardPageResponse( $view, array( diff --git a/src/applications/auth/oauth/provider/PhabricatorOAuthProviderFacebook.php b/src/applications/auth/oauth/provider/PhabricatorOAuthProviderFacebook.php index ec396d7145..32bc418320 100644 --- a/src/applications/auth/oauth/provider/PhabricatorOAuthProviderFacebook.php +++ b/src/applications/auth/oauth/provider/PhabricatorOAuthProviderFacebook.php @@ -78,7 +78,9 @@ final class PhabricatorOAuthProviderFacebook extends PhabricatorOAuthProvider { } public function getUserInfoURI() { - return 'https://graph.facebook.com/me'; + $fields = array('id', 'name', 'email', 'link', 'security_settings'); + return 'https://graph.facebook.com/me?fields='. + implode(',', $fields); } public function getMinimumScope() { @@ -88,6 +90,17 @@ final class PhabricatorOAuthProviderFacebook extends PhabricatorOAuthProvider { public function setUserData($data) { $data = json_decode($data, true); $this->validateUserData($data); + + if (PhabricatorEnv::getEnvConfig('facebook.require-https-auth')) { + if (!$data['security_settings']['secure_browsing']['enabled']) { + throw new PhabricatorOAuthProviderException( + 'You must enable Secure Browsing on your Facebook account in'. + ' order to log in to Phabricator. For more information, check'. + ' out http://www.facebook.com/help/?faq=215897678434749' + ); + } + } + $this->userData = $data; return $this; } diff --git a/src/applications/auth/view/PhabricatorOAuthFailureView.php b/src/applications/auth/view/PhabricatorOAuthFailureView.php index b1e407ad85..1af0db2925 100644 --- a/src/applications/auth/view/PhabricatorOAuthFailureView.php +++ b/src/applications/auth/view/PhabricatorOAuthFailureView.php @@ -20,6 +20,7 @@ final class PhabricatorOAuthFailureView extends AphrontView { private $request; private $provider; + private $exception; public function setRequest(AphrontRequest $request) { $this->request = $request; @@ -31,6 +32,11 @@ final class PhabricatorOAuthFailureView extends AphrontView { return $this; } + public function setException(Exception $e) { + $this->exception = $e; + return $this; + } + public function render() { $request = $this->request; $provider = $this->provider; @@ -53,6 +59,11 @@ final class PhabricatorOAuthFailureView extends AphrontView { hsprintf( '

Error Reason: %s

', $request->getStr('error_reason'))); + } else if ($this->exception) { + $view->appendChild( + hsprintf( + '

Error Details: %s

', + $this->exception->getMessage())); } else { // TODO: We can probably refine this. $view->appendChild(