From eccc76dae685ddde5722b98f9f14b4ae85b25acb Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 26 Feb 2011 20:57:21 -0800 Subject: [PATCH] Fix some issues caught by HipHop, and work around some issues caused by HipHop. --- externals/phpmailer/class.phpmailer-lite.php | 2 +- externals/xhprof/xhprof_lib.php | 6 ++-- ...AphrontDefaultApplicationConfiguration.php | 31 +++++++--------- .../default/configuration/__init__.php | 1 + .../oauth/PhabricatorOAuthLoginController.php | 36 +++++++++++++------ ...tAPI_differential_markcommitted_Method.php | 3 +- .../DifferentialRevisionListData.php | 5 ++- .../changeset/DifferentialChangesetParser.php | 2 +- .../view/tasklist/ManiphestTaskListView.php | 2 +- .../ManiphestTransactionListView.php | 2 +- .../PhabricatorRepositoryCreateController.php | 12 +++---- src/infrastructure/env/PhabricatorEnv.php | 2 +- .../base/AphrontDatabaseConnection.php | 10 ++++-- .../mysql/AphrontMySQLDatabaseConnection.php | 11 +++--- .../AphrontQueryDuplicateKeyException.php | 10 ------ src/storage/queryfx/queryfx.php | 22 +++++++----- .../AphrontHeadsupActionListView.php | 2 +- .../layout/headsup/actionlist/__init__.php | 1 + .../standard/PhabricatorStandardPageView.php | 28 +++++++++------ webroot/index.php | 13 +++++++ 20 files changed, 116 insertions(+), 85 deletions(-) diff --git a/externals/phpmailer/class.phpmailer-lite.php b/externals/phpmailer/class.phpmailer-lite.php index f12a8d5844..ad0ea36edb 100755 --- a/externals/phpmailer/class.phpmailer-lite.php +++ b/externals/phpmailer/class.phpmailer-lite.php @@ -370,7 +370,7 @@ class PHPMailerLite { */ private function AddAnAddress($kind, $address, $name = '') { if (!preg_match('/^(to|cc|bcc|ReplyTo)$/', $kind)) { - echo 'Invalid recipient array: ' . kind; + echo 'Invalid recipient array: ' . $kind; return false; } $address = trim($address); diff --git a/externals/xhprof/xhprof_lib.php b/externals/xhprof/xhprof_lib.php index fed9487039..8f8985ed26 100644 --- a/externals/xhprof/xhprof_lib.php +++ b/externals/xhprof/xhprof_lib.php @@ -261,7 +261,7 @@ function xhprof_aggregate_runs($xhprof_runs_impl, $runs, $bad_runs = array(); foreach($runs as $idx => $run_id) { - $raw_data = $xhprof_runs_impl->get_run($run_id, $source, $description); + $raw_data = $xhprof_runs_impl->get_run($run_id, $source, '?'); // use the first run to derive what metrics to aggregate on. if ($idx == 0) { @@ -283,7 +283,7 @@ function xhprof_aggregate_runs($xhprof_runs_impl, $runs, } if ($use_script_name) { - $page = $description; + $page = '?'; // create a fake function '__script::$page', and have and edge from // main() to '__script::$page'. We will also need edges to transfer @@ -589,7 +589,7 @@ function xhprof_prune_run($raw_data, $prune_percent) { $prune_threshold = (($main_info[$prune_metric] * $prune_percent) / 100.0); - init_metrics($raw_data, null, null, false); +// init_metrics($raw_data, null, null, false); $flat_info = xhprof_compute_inclusive_times($raw_data); foreach ($raw_data as $parent_child => $info) { diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 0ef8fd2786..e42fd40b41 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -212,27 +212,22 @@ class AphrontDefaultApplicationConfiguration ''.phutil_escape_html((string)$ex).''. ''; - if ($this->getRequest()->isAjax()) { - $dialog = new AphrontDialogView(); - $dialog - ->setTitle('Exception!') - ->setClass('aphront-exception-dialog') - ->setUser($this->getRequest()->getUser()) - ->appendChild($content) - ->addCancelButton('/'); - - $response = new AphrontDialogResponse(); - $response->setDialog($dialog); - - return $response; + $user = $this->getRequest()->getUser(); + if (!$user) { + // If we hit an exception very early, we won't have a user. + $user = new PhabricatorUser(); } - $view = new PhabricatorStandardPageView(); - $view->setRequest($this->getRequest()); - $view->appendChild($content); + $dialog = new AphrontDialogView(); + $dialog + ->setTitle('Exception!') + ->setClass('aphront-exception-dialog') + ->setUser($user) + ->appendChild($content) + ->addCancelButton('/'); - $response = new AphrontWebpageResponse(); - $response->setContent($view->render()); + $response = new AphrontDialogResponse(); + $response->setDialog($dialog); return $response; } diff --git a/src/aphront/default/configuration/__init__.php b/src/aphront/default/configuration/__init__.php index 62bdc33fb1..8fd005d1e7 100644 --- a/src/aphront/default/configuration/__init__.php +++ b/src/aphront/default/configuration/__init__.php @@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'aphront/response/ajax'); phutil_require_module('phabricator', 'aphront/response/dialog'); phutil_require_module('phabricator', 'aphront/response/webpage'); phutil_require_module('phabricator', 'applications/base/controller/404'); +phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'view/dialog'); phutil_require_module('phabricator', 'view/page/failure'); phutil_require_module('phabricator', 'view/page/standard'); diff --git a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php index 4c79dbfc4b..8386ca91ac 100644 --- a/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php +++ b/src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php @@ -65,21 +65,30 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { 'code' => $code, ); + $post_data = http_build_query($query_data); + $post_length = strlen($post_data); + $stream_context = stream_context_create( array( 'http' => array( 'method' => 'POST', - 'header' => 'Content-type: application/x-www-form-urlencoded', - 'content' => http_build_query($query_data), + 'header' => + "Content-Type: application/x-www-form-urlencoded\r\n". + "Content-Length: {$post_length}\r\n", + 'content' => $post_data, ), )); $stream = fopen($auth_uri, 'r', false, $stream_context); - $meta = stream_get_meta_data($stream); - $response = stream_get_contents($stream); + $response = false; + $meta = null; + if ($stream) { + $meta = stream_get_meta_data($stream); + $response = stream_get_contents($stream); + fclose($stream); + } - fclose($stream); if ($response === false) { return $this->buildErrorResponse(new PhabricatorOAuthFailureView()); @@ -127,7 +136,6 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $user_id); if ($current_user->getPHID()) { - if ($known_oauth) { if ($known_oauth->getUserID() != $current_user->getID()) { $dialog = new AphrontDialogView(); @@ -285,11 +293,19 @@ class PhabricatorOAuthLoginController extends PhabricatorAuthController { $request->setCookie('phsid', $session_key); return id(new AphrontRedirectResponse())->setURI('/'); } catch (AphrontQueryDuplicateKeyException $exception) { - $key = $exception->getDuplicateKey(); - if ($key == 'userName') { + + $same_username = id(new PhabricatorUser())->loadOneWhere( + 'userName = %s', + $user->getUserName()); + + $same_email = id(new PhabricatorUser())->loadOneWhere( + 'email = %s', + $user->getEmail()); + + if ($same_username) { $e_username = 'Duplicate'; - $errors[] = 'That username is not unique.'; - } else if ($key == 'email') { + $errors[] = 'That username or email is not unique.'; + } else if ($same_email) { $e_email = 'Duplicate'; $errors[] = 'That email is not unique.'; } else { diff --git a/src/applications/conduit/method/differential/markcommitted/ConduitAPI_differential_markcommitted_Method.php b/src/applications/conduit/method/differential/markcommitted/ConduitAPI_differential_markcommitted_Method.php index ec8858c28a..ccc959a173 100644 --- a/src/applications/conduit/method/differential/markcommitted/ConduitAPI_differential_markcommitted_Method.php +++ b/src/applications/conduit/method/differential/markcommitted/ConduitAPI_differential_markcommitted_Method.php @@ -60,8 +60,7 @@ class ConduitAPI_differential_markcommitted_Method extends ConduitAPIMethod { $editor = new DifferentialCommentEditor( $revision, $revision->getAuthorPHID(), - DifferentialAction::ACTION_COMMIT, - $inline_comments = array()); + DifferentialAction::ACTION_COMMIT); $editor->save(); $revision->setStatus(DifferentialRevisionStatus::COMMITTED); diff --git a/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php b/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php index d4dbb6eb31..c6c8f80540 100755 --- a/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php +++ b/src/applications/differential/data/revisionlist/DifferentialRevisionListData.php @@ -165,7 +165,7 @@ class DifferentialRevisionListData { $this->revisions = $rev->loadAllFromArray($data); break; - case self::QUERY_BY_PHID: + case self::QUERY_PHIDS: $this->revisions = $this->loadAllWhere( 'revision.phid in (%Ls)', $this->ids); @@ -233,8 +233,11 @@ class DifferentialRevisionListData { } private function loadAllOpenWithCCs(array $ccphids) { + $rev = new DifferentialRevision(); + $revision = new DifferentialRevision(); $data = queryfx_all( + $rev->establishConnection('r'), 'SELECT revision.* FROM %T revision JOIN %T relationship ON relationship.revisionID = revision.id AND relationship.relation = %s diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index 4e693165d1..cd684cfef8 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -516,7 +516,7 @@ class DifferentialChangesetParser { $changeset->getTableName().'_parse_cache', $this->changesetID, $cache); - } catch (QueryException $ex) { + } catch (AphrontQueryException $ex) { // TODO: uhoh } } diff --git a/src/applications/maniphest/view/tasklist/ManiphestTaskListView.php b/src/applications/maniphest/view/tasklist/ManiphestTaskListView.php index ce630dc09e..8234ad3967 100644 --- a/src/applications/maniphest/view/tasklist/ManiphestTaskListView.php +++ b/src/applications/maniphest/view/tasklist/ManiphestTaskListView.php @@ -35,7 +35,7 @@ class ManiphestTaskListView extends AphrontView { $views = array(); foreach ($this->tasks as $task) { - $view = new ManiphestTaskSummaryView($task); + $view = new ManiphestTaskSummaryView(); $view->setTask($task); $view->setHandles($this->handles); $views[] = $view->render(); diff --git a/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php b/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php index 8d010d83f1..4c71d090a9 100644 --- a/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php +++ b/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php @@ -72,7 +72,7 @@ class ManiphestTransactionListView extends AphrontView { } foreach ($groups as $group) { - $view = new ManiphestTransactionDetailView($transaction); + $view = new ManiphestTransactionDetailView(); $view->setTransactionGroup($group); $view->setHandles($this->handles); $view->setMarkupEngine($this->markupEngine); diff --git a/src/applications/repository/controller/create/PhabricatorRepositoryCreateController.php b/src/applications/repository/controller/create/PhabricatorRepositoryCreateController.php index ea29580d89..bbab3011cf 100644 --- a/src/applications/repository/controller/create/PhabricatorRepositoryCreateController.php +++ b/src/applications/repository/controller/create/PhabricatorRepositoryCreateController.php @@ -69,14 +69,10 @@ class PhabricatorRepositoryCreateController extends PhabricatorController { return id(new AphrontRedirectResponse()) ->setURI('/repository/edit/'.$repository->getID().'/'); - } catch (PhabricatorQueryDuplicateKeyException $ex) { - if ($ex->getDuplicateKey() == 'callsign') { - $e_callsign = 'Duplicate'; - $errors[] = 'Callsign must be unique. Another repository already '. - 'uses that callsign.'; - } else { - throw $ex; - } + } catch (AphrontQueryDuplicateKeyException $ex) { + $e_callsign = 'Duplicate'; + $errors[] = 'Callsign must be unique. Another repository already '. + 'uses that callsign.'; } } } diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index ff811248e4..c349913b5c 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -30,7 +30,7 @@ final class PhabricatorEnv { public static function getURI($path) { return rtrim(self::getEnvConfig('phabricator.base-uri'), '/').$path; } - + public static function getAllConfigKeys() { return self::$env; } diff --git a/src/storage/connection/base/AphrontDatabaseConnection.php b/src/storage/connection/base/AphrontDatabaseConnection.php index 5d7fd27e27..150f43b960 100644 --- a/src/storage/connection/base/AphrontDatabaseConnection.php +++ b/src/storage/connection/base/AphrontDatabaseConnection.php @@ -36,15 +36,21 @@ abstract class AphrontDatabaseConnection { abstract public function escapeStringForLikeClause($string); public function queryData($pattern/*, $arg, $arg, ... */) { + if (false) { + // Workaround for the HPHP workaround: ensure we include this module + // since we really are using the function. + queryfx($this, $pattern); + } + $args = func_get_args(); array_unshift($args, $this); - return call_user_func_array('queryfx_all', $args); + return hphp_workaround_call_user_func_array('queryfx_all', $args); } public function query($pattern/*, $arg, $arg, ... */) { $args = func_get_args(); array_unshift($args, $this); - return call_user_func_array('queryfx', $args); + return hphp_workaround_call_user_func_array('queryfx', $args); } // TODO: Probably need to reset these when we catch a connection exception diff --git a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php index 5af7dcb12f..3729954f5a 100644 --- a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php +++ b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php @@ -234,12 +234,11 @@ class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection { case 1205: // Lock wait timeout exceeded throw new AphrontQueryRecoverableException("#{$errno}: {$error}"); case 1062: // Duplicate Key - $matches = null; - $key = null; - if (preg_match('/for key \'(.*)\'$/', $error, $matches)) { - $key = $matches[1]; - } - throw new AphrontQueryDuplicateKeyException($key, "{$errno}: {$error}"); + // NOTE: In some versions of MySQL we get a key name back here, but + // older versions just give us a key index ("key 2") so it's not + // portable to parse the key out of the error and attach it to the + // exception. + throw new AphrontQueryDuplicateKeyException("{$errno}: {$error}"); default: // TODO: 1064 is syntax error, and quite terrible in production. throw new AphrontQueryException("#{$errno}: {$error}"); diff --git a/src/storage/exception/duplicatekey/AphrontQueryDuplicateKeyException.php b/src/storage/exception/duplicatekey/AphrontQueryDuplicateKeyException.php index 0c2a8be94e..496fec6fab 100644 --- a/src/storage/exception/duplicatekey/AphrontQueryDuplicateKeyException.php +++ b/src/storage/exception/duplicatekey/AphrontQueryDuplicateKeyException.php @@ -21,14 +21,4 @@ */ class AphrontQueryDuplicateKeyException extends AphrontQueryException { - private $duplicateKey; - - public function getDuplicateKey() { - return $this->duplicateKey; - } - - public function __construct($duplicate_key, $message) { - $this->duplicateKey = $duplicate_key; - parent::__construct($message); - } } diff --git a/src/storage/queryfx/queryfx.php b/src/storage/queryfx/queryfx.php index 32fa9c6d5d..91cb5bb376 100644 --- a/src/storage/queryfx/queryfx.php +++ b/src/storage/queryfx/queryfx.php @@ -20,9 +20,15 @@ * @group storage */ function queryfx(AphrontDatabaseConnection $conn, $sql/*, ... */) { + if (false) { + // Workaround for the HPHP workaround: ensure we include this module + // since we really are using the function. + qsprintf($conn, $sql); + } + $argv = func_get_args(); - $query = call_user_func_array('qsprintf', $argv); - return $conn->executeRawQuery($query); + $query = hphp_workaround_call_user_func_array('qsprintf', $argv); + $conn->executeRawQuery($query); } /** @@ -30,7 +36,7 @@ function queryfx(AphrontDatabaseConnection $conn, $sql/*, ... */) { */ function vqueryfx($conn, $sql, $argv) { array_unshift($argv, $conn, $sql); - return call_user_func_array('queryfx', $argv); + hphp_workaround_call_user_func_array('queryfx', $argv); } /** @@ -38,8 +44,8 @@ function vqueryfx($conn, $sql, $argv) { */ function queryfx_all($conn, $sql/*, ... */) { $argv = func_get_args(); - $ret = call_user_func_array('queryfx', $argv); - return $conn->selectAllResults($ret); + hphp_workaround_call_user_func_array('queryfx', $argv); + return $conn->selectAllResults(); } /** @@ -47,7 +53,7 @@ function queryfx_all($conn, $sql/*, ... */) { */ function queryfx_one($conn, $sql/*, ... */) { $argv = func_get_args(); - $ret = call_user_func_array('queryfx_all', $argv); + $ret = hphp_workaround_call_user_func_array('queryfx_all', $argv); if (count($ret) > 1) { throw new AphrontQueryCountException( 'Query returned more than one row.'); @@ -59,6 +65,6 @@ function queryfx_one($conn, $sql/*, ... */) { function vqueryfx_all($conn, $sql, array $argv) { array_unshift($argv, $conn, $sql); - $ret = call_user_func_array('queryfx', $argv); - return $conn->selectAllResults($ret); + hphp_workaround_call_user_func_array('queryfx', $argv); + return $conn->selectAllResults(); } diff --git a/src/view/layout/headsup/actionlist/AphrontHeadsupActionListView.php b/src/view/layout/headsup/actionlist/AphrontHeadsupActionListView.php index 5f994b84b5..2eb411024e 100755 --- a/src/view/layout/headsup/actionlist/AphrontHeadsupActionListView.php +++ b/src/view/layout/headsup/actionlist/AphrontHeadsupActionListView.php @@ -25,7 +25,7 @@ final class AphrontHeadsupActionListView extends AphrontView { } public function render() { - + require_celerity_resource('aphront-headsup-action-list-view-css'); $actions = array(); diff --git a/src/view/layout/headsup/actionlist/__init__.php b/src/view/layout/headsup/actionlist/__init__.php index c5f5e313e5..7cf4e4b56d 100644 --- a/src/view/layout/headsup/actionlist/__init__.php +++ b/src/view/layout/headsup/actionlist/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'view/base'); diff --git a/src/view/page/standard/PhabricatorStandardPageView.php b/src/view/page/standard/PhabricatorStandardPageView.php index 1a5655feed..8e9103bced 100755 --- a/src/view/page/standard/PhabricatorStandardPageView.php +++ b/src/view/page/standard/PhabricatorStandardPageView.php @@ -152,9 +152,12 @@ class PhabricatorStandardPageView extends AphrontPageView { $login_stuff = null; $request = $this->getRequest(); + $user = null; if ($request) { $user = $request->getUser(); - if ($user->getPHID()) { + // NOTE: user may not be set here if we caught an exception early + // in the execution workflow. + if ($user && $user->getPHID()) { $login_stuff = 'Logged in as '.phutil_render_tag( 'a', @@ -203,16 +206,19 @@ class PhabricatorStandardPageView extends AphrontPageView { } $foot_links[] = $link; } - // This ends up very early in tab order at the top of the page and there's - // a bunch of junk up there anyway, just shove it down here. - $foot_links[] = phabricator_render_form( - $user, - array( - 'action' => '/logout/', - 'method' => 'post', - 'style' => 'display: inline', - ), - ''); + + if ($user && $user->getPHID()) { + // This ends up very early in tab order at the top of the page and there's + // a bunch of junk up there anyway, just shove it down here. + $foot_links[] = phabricator_render_form( + $user, + array( + 'action' => '/logout/', + 'method' => 'post', + 'style' => 'display: inline', + ), + ''); + } $foot_links = implode(' · ', $foot_links); diff --git a/webroot/index.php b/webroot/index.php index 6a8898304f..37e22d8f3f 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -19,6 +19,11 @@ error_reporting(E_ALL | E_STRICT); $env = getenv('PHABRICATOR_ENV'); // Apache +if (!$env) { + if (isset($_ENV['PHABRICATOR_ENV'])) { + $env = $_ENV['PHABRICATOR_ENV']; + } +} if (!$env) { phabricator_fatal_config_error( @@ -172,3 +177,11 @@ function phabricator_fatal_config_error($msg) { die(); } +/** + * Workaround for HipHop bug, see Facebook Task #503624. + */ +function hphp_workaround_call_user_func_array($func, array $array) { + $f = new ReflectionFunction($func); + return $f->invokeArgs($array); +} +