From 13275860b18eea348bee375074c05a9e7bafe3ee Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Nov 2013 14:38:29 -0800 Subject: [PATCH 1/3] When stopping on redirect, show a full stack trace Summary: Ref T4140. Provide more debugging information so we can figure out what's going on with redirect loops. Test Plan: {F83868} Reviewers: chad, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4140 Differential Revision: https://secure.phabricator.com/D7620 --- src/__phutil_library_map__.php | 2 + ...AphrontDefaultApplicationConfiguration.php | 110 +--------------- .../response/AphrontRedirectResponse.php | 53 +++++--- src/view/widget/AphrontStackTraceView.php | 120 ++++++++++++++++++ 4 files changed, 162 insertions(+), 123 deletions(-) create mode 100644 src/view/widget/AphrontStackTraceView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 82bbb6263a..3ec33cc4f8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -81,6 +81,7 @@ phutil_register_library_map(array( 'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php', 'AphrontResponse' => 'aphront/response/AphrontResponse.php', 'AphrontSideNavFilterView' => 'view/layout/AphrontSideNavFilterView.php', + 'AphrontStackTraceView' => 'view/widget/AphrontStackTraceView.php', 'AphrontTableView' => 'view/control/AphrontTableView.php', 'AphrontTagView' => 'view/AphrontTagView.php', 'AphrontTokenizerTemplateView' => 'view/control/AphrontTokenizerTemplateView.php', @@ -2401,6 +2402,7 @@ phutil_register_library_map(array( 'AphrontRequestFailureView' => 'AphrontView', 'AphrontRequestTestCase' => 'PhabricatorTestCase', 'AphrontSideNavFilterView' => 'AphrontView', + 'AphrontStackTraceView' => 'AphrontView', 'AphrontTableView' => 'AphrontView', 'AphrontTagView' => 'AphrontView', 'AphrontTokenizerTemplateView' => 'AphrontView', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index e114f8d663..e1124568a7 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -243,7 +243,9 @@ class AphrontDefaultApplicationConfiguration } if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { - $trace = $this->renderStackTrace($ex->getTrace(), $user); + $trace = id(new AphrontStackTraceView()) + ->setUser($user) + ->setTrace($ex->getTrace()); } else { $trace = null; } @@ -290,110 +292,4 @@ class AphrontDefaultApplicationConfiguration )); } - private function renderStackTrace($trace, PhabricatorUser $user) { - - $libraries = PhutilBootloader::getInstance()->getAllLibraries(); - - // TODO: Make this configurable? - $path = 'https://secure.phabricator.com/diffusion/%s/browse/master/src/'; - - $callsigns = array( - 'arcanist' => 'ARC', - 'phutil' => 'PHU', - 'phabricator' => 'P', - ); - - $rows = array(); - $depth = count($trace); - foreach ($trace as $part) { - $lib = null; - $file = idx($part, 'file'); - $relative = $file; - foreach ($libraries as $library) { - $root = phutil_get_library_root($library); - if (Filesystem::isDescendant($file, $root)) { - $lib = $library; - $relative = Filesystem::readablePath($file, $root); - break; - } - } - - $where = ''; - if (isset($part['class'])) { - $where .= $part['class'].'::'; - } - if (isset($part['function'])) { - $where .= $part['function'].'()'; - } - - if ($file) { - if (isset($callsigns[$lib])) { - $attrs = array('title' => $file); - try { - $attrs['href'] = $user->loadEditorLink( - '/src/'.$relative, - $part['line'], - $callsigns[$lib]); - } catch (Exception $ex) { - // The database can be inaccessible. - } - if (empty($attrs['href'])) { - $attrs['href'] = sprintf($path, $callsigns[$lib]). - str_replace(DIRECTORY_SEPARATOR, '/', $relative). - '$'.$part['line']; - $attrs['target'] = '_blank'; - } - $file_name = phutil_tag( - 'a', - $attrs, - $relative); - } else { - $file_name = phutil_tag( - 'span', - array( - 'title' => $file, - ), - $relative); - } - $file_name = hsprintf('%s : %d', $file_name, $part['line']); - } else { - $file_name = phutil_tag('em', array(), '(Internal)'); - } - - - $rows[] = array( - $depth--, - $lib, - $file_name, - $where, - ); - } - $table = new AphrontTableView($rows); - $table->setHeaders( - array( - 'Depth', - 'Library', - 'File', - 'Where', - )); - $table->setColumnClasses( - array( - 'n', - '', - '', - 'wide', - )); - - return phutil_tag( - 'div', - array('class' => 'exception-trace'), - array( - phutil_tag( - 'div', - array('class' => 'exception-trace-header'), - pht('Stack Trace')), - $table->render(), - )); - } - } diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php index 4ee21fb4ee..bab7b7f261 100644 --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -8,6 +8,14 @@ class AphrontRedirectResponse extends AphrontResponse { private $uri; + private $stackWhenCreated; + + public function __construct() { + if ($this->shouldStopForDebugging()) { + // If we're going to stop, capture the stack so we can print it out. + $this->stackWhenCreated = id(new Exception())->getTrace(); + } + } public function setURI($uri) { $this->uri = $uri; @@ -33,31 +41,44 @@ class AphrontRedirectResponse extends AphrontResponse { public function buildResponseString() { if ($this->shouldStopForDebugging()) { + $user = new PhabricatorUser(); + $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'); + $dialog = new AphrontDialogView(); + $dialog->setUser($user); + $dialog->setTitle('Stopped on Redirect'); - $error->appendChild(phutil_tag('p', array(), pht( - 'You were stopped here because %s is set in your configuration.', - phutil_tag('tt', array(), 'debug.stop-on-redirect')))); + $dialog->appendParagraph( + pht( + 'You were stopped here because %s is set in your configuration.', + phutil_tag('tt', array(), 'debug.stop-on-redirect'))); - $link = phutil_tag( - 'a', - array( - 'href' => $this->getURI(), - ), - $this->getURI()); + $dialog->appendParagraph( + pht( + 'You are being redirected to: %s', + phutil_tag('tt', array(), $this->getURI()))); - $error->appendChild(phutil_tag('p', array(), pht( - 'Continue to: %s', - $link))); + $dialog->addCancelButton($this->getURI(), pht('Continue')); - $view->appendChild($error); + $dialog->appendChild(phutil_tag('br')); + + $dialog->appendChild( + id(new AphrontStackTraceView()) + ->setUser($user) + ->setTrace($this->stackWhenCreated)); + + $dialog->setIsStandalone(true); + $dialog->setWidth(AphrontDialogView::WIDTH_FULL); + + $box = id(new PHUIBoxView()) + ->addMargin(PHUI::MARGIN_LARGE) + ->appendChild($dialog); + + $view->appendChild($box); return $view->render(); } diff --git a/src/view/widget/AphrontStackTraceView.php b/src/view/widget/AphrontStackTraceView.php new file mode 100644 index 0000000000..61811a28e9 --- /dev/null +++ b/src/view/widget/AphrontStackTraceView.php @@ -0,0 +1,120 @@ +trace = $trace; + return $this; + } + + public function render() { + $user = $this->getUser(); + $trace = $this->trace; + + $libraries = PhutilBootloader::getInstance()->getAllLibraries(); + + // TODO: Make this configurable? + $path = 'https://secure.phabricator.com/diffusion/%s/browse/master/src/'; + + $callsigns = array( + 'arcanist' => 'ARC', + 'phutil' => 'PHU', + 'phabricator' => 'P', + ); + + $rows = array(); + $depth = count($trace); + foreach ($trace as $part) { + $lib = null; + $file = idx($part, 'file'); + $relative = $file; + foreach ($libraries as $library) { + $root = phutil_get_library_root($library); + if (Filesystem::isDescendant($file, $root)) { + $lib = $library; + $relative = Filesystem::readablePath($file, $root); + break; + } + } + + $where = ''; + if (isset($part['class'])) { + $where .= $part['class'].'::'; + } + if (isset($part['function'])) { + $where .= $part['function'].'()'; + } + + if ($file) { + if (isset($callsigns[$lib])) { + $attrs = array('title' => $file); + try { + $attrs['href'] = $user->loadEditorLink( + '/src/'.$relative, + $part['line'], + $callsigns[$lib]); + } catch (Exception $ex) { + // The database can be inaccessible. + } + if (empty($attrs['href'])) { + $attrs['href'] = sprintf($path, $callsigns[$lib]). + str_replace(DIRECTORY_SEPARATOR, '/', $relative). + '$'.$part['line']; + $attrs['target'] = '_blank'; + } + $file_name = phutil_tag( + 'a', + $attrs, + $relative); + } else { + $file_name = phutil_tag( + 'span', + array( + 'title' => $file, + ), + $relative); + } + $file_name = hsprintf('%s : %d', $file_name, $part['line']); + } else { + $file_name = phutil_tag('em', array(), '(Internal)'); + } + + + $rows[] = array( + $depth--, + $lib, + $file_name, + $where, + ); + } + $table = new AphrontTableView($rows); + $table->setHeaders( + array( + 'Depth', + 'Library', + 'File', + 'Where', + )); + $table->setColumnClasses( + array( + 'n', + '', + '', + 'wide', + )); + + return phutil_tag( + 'div', + array('class' => 'exception-trace'), + array( + phutil_tag( + 'div', + array('class' => 'exception-trace-header'), + pht('Stack Trace')), + $table->render(), + )); + } + +} From 3a035c02e7b910692fefdef3b88c2b83041cfbb7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Nov 2013 14:41:32 -0800 Subject: [PATCH 2/3] Recover more flexibly from an already-verified email Summary: Ref T4140. We could hit a redirect loop for a user with a verified primary email address but no "is verified" flag on their account. This shouldn't be possible since the migration should have set the flag, but we can deal with it more gracefully when it does happen (maybe because users forgot to run `storage/upgrade`, or because of ghosts). In the controller, check the same flag we check before forcing the user to the controller. When verifying, allow the verification if either the email or user flag isn't set. Test Plan: Hit `/login/mustverify/`; verified an address. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4140 Differential Revision: https://secure.phabricator.com/D7621 --- .../auth/controller/PhabricatorEmailVerificationController.php | 2 +- .../auth/controller/PhabricatorMustVerifyEmailController.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorEmailVerificationController.php b/src/applications/auth/controller/PhabricatorEmailVerificationController.php index 87c9c746f1..4bbe9b2dc7 100644 --- a/src/applications/auth/controller/PhabricatorEmailVerificationController.php +++ b/src/applications/auth/controller/PhabricatorEmailVerificationController.php @@ -44,7 +44,7 @@ final class PhabricatorEmailVerificationController 'user. Make sure you followed the link in the email correctly and are '. 'logged in with the user account associated with the email address.'); $continue = pht('Rats!'); - } else if ($email->getIsVerified()) { + } else if ($email->getIsVerified() && $user->getIsEmailVerified()) { $title = pht('Address Already Verified'); $content = pht( 'This email address has already been verified.'); diff --git a/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php b/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php index beae85a3a9..efb4a3987e 100644 --- a/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php +++ b/src/applications/auth/controller/PhabricatorMustVerifyEmailController.php @@ -19,7 +19,7 @@ final class PhabricatorMustVerifyEmailController $email = $user->loadPrimaryEmail(); - if ($email->getIsVerified()) { + if ($user->getIsEmailVerified()) { return id(new AphrontRedirectResponse())->setURI('/'); } From e99c53da2ea2b1dc7a097d40adc749b625ea7151 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Nov 2013 14:41:38 -0800 Subject: [PATCH 3/3] Fix an issue with SVN path construction in the presence of subpath configuration Summary: D7590 made path construction more consistent, but affected this callsite if a subpath is configured. Currently, we end up with double `@@` in the URI. Test Plan: - Ran unit tests. - Ran `bin/repostitory discover`. Reviewers: staticshock, btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D7619 --- .../engine/PhabricatorRepositoryDiscoveryEngine.php | 5 ++--- .../repository/storage/PhabricatorRepository.php | 4 ++-- .../__tests__/PhabricatorRepositoryTestCase.php | 11 +++++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index 17374a02d6..3134e1eb5f 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -88,10 +88,9 @@ final class PhabricatorRepositoryDiscoveryEngine try { list($xml, $stderr) = $repository->execxRemoteCommand( - 'log --xml --quiet --limit %d %s@%s', + 'log --xml --quiet --limit %d %s', $limit, - $repository->getSubversionBaseURI(), - $at_rev); + $repository->getSubversionBaseURI($at_rev)); } catch (CommandException $ex) { $stderr = $ex->getStdErr(); if (preg_match('/(path|File) not found/', $stderr)) { diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 4597a36fe3..c8601e6729 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -155,12 +155,12 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $this->getDetail('local-path'); } - public function getSubversionBaseURI() { + public function getSubversionBaseURI($commit = null) { $subpath = $this->getDetail('svn-subpath'); if (!strlen($subpath)) { $subpath = null; } - return $this->getSubversionPathURI($subpath); + return $this->getSubversionPathURI($subpath, $commit); } public function getSubversionPathURI($path = null, $commit = null) { diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php index c58e7c2dfe..69f18f454a 100644 --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -94,6 +94,17 @@ final class PhabricatorRepositoryTestCase $this->assertEqual( 'file:///var/repo/SVN/%3F@22', $repo->getSubversionPathURI('?', 22)); + + $repo->setDetail('svn-subpath', 'quack/trunk/'); + + $this->assertEqual( + 'file:///var/repo/SVN/quack/trunk/@', + $repo->getSubversionBaseURI()); + + $this->assertEqual( + 'file:///var/repo/SVN/quack/trunk/@HEAD', + $repo->getSubversionBaseURI('HEAD')); + }