diff --git a/src/conduit/ArcanistConduitCallFuture.php b/src/conduit/ArcanistConduitCallFuture.php index 187b4c27..2f3dad07 100644 --- a/src/conduit/ArcanistConduitCallFuture.php +++ b/src/conduit/ArcanistConduitCallFuture.php @@ -72,15 +72,18 @@ final class ArcanistConduitCallFuture } protected function didReceiveException($exception) { - switch ($exception->getErrorCode()) { - case 'ERR-INVALID-SESSION': - if (!$this->getEngine()->getConduitToken()) { - $this->raiseLoginRequired(); - } - break; - case 'ERR-INVALID-AUTH': - $this->raiseInvalidAuth(); - break; + + if ($exception instanceof ConduitClientException) { + switch ($exception->getErrorCode()) { + case 'ERR-INVALID-SESSION': + if (!$this->getEngine()->getConduitToken()) { + $this->raiseLoginRequired(); + } + break; + case 'ERR-INVALID-AUTH': + $this->raiseInvalidAuth(); + break; + } } throw $exception; diff --git a/src/conduit/ConduitFuture.php b/src/conduit/ConduitFuture.php index 351d2d11..5966f42e 100644 --- a/src/conduit/ConduitFuture.php +++ b/src/conduit/ConduitFuture.php @@ -62,9 +62,14 @@ final class ConduitFuture extends FutureProxy { } if ($data['error_code']) { + $message = pht( + '<%s> %s', + $this->conduitMethod, + $data['error_info']); + throw new ConduitClientException( $data['error_code'], - $data['error_info']); + $message); } $result = $data['result']; diff --git a/src/future/FutureIterator.php b/src/future/FutureIterator.php index 028c02da..96a41e3d 100644 --- a/src/future/FutureIterator.php +++ b/src/future/FutureIterator.php @@ -194,7 +194,14 @@ final class FutureIterator * @task iterator */ public function next() { - $this->key = null; + // See T13572. If we preivously resolved and returned a Future, release + // it now. This prevents us from holding Futures indefinitely when callers + // like FuturePool build long-lived iterators and keep adding new Futures + // to them. + if ($this->key !== null) { + unset($this->futures[$this->key]); + $this->key = null; + } $this->updateWorkingSet(); diff --git a/src/future/exec/execx.php b/src/future/exec/execx.php index bde37d29..ee920aff 100644 --- a/src/future/exec/execx.php +++ b/src/future/exec/execx.php @@ -47,7 +47,7 @@ function exec_manual($cmd /* , ... */) { */ function phutil_passthru($cmd /* , ... */) { $args = func_get_args(); - return newv('PhutilExecPassthru', $args)->execute(); + return newv('PhutilExecPassthru', $args)->resolve(); } diff --git a/src/future/http/HTTPSFuture.php b/src/future/http/HTTPSFuture.php index e1f37cce..c217c112 100644 --- a/src/future/http/HTTPSFuture.php +++ b/src/future/http/HTTPSFuture.php @@ -132,7 +132,7 @@ final class HTTPSFuture extends BaseHTTPFuture { * @return string|false */ public static function loadContent($uri, $timeout = null) { - $future = new HTTPSFuture($uri); + $future = new self($uri); if ($timeout !== null) { $future->setTimeout($timeout); } @@ -151,7 +151,8 @@ final class HTTPSFuture extends BaseHTTPFuture { throw new Exception( pht( 'Specified download path "%s" already exists, refusing to '. - 'overwrite.')); + 'overwrite.', + $download_path)); } return $this; diff --git a/src/hardpoint/ArcanistHardpointList.php b/src/hardpoint/ArcanistHardpointList.php index bc44bdde..1b93a1d7 100644 --- a/src/hardpoint/ArcanistHardpointList.php +++ b/src/hardpoint/ArcanistHardpointList.php @@ -27,7 +27,9 @@ final class ArcanistHardpointList pht( 'Hardpoint (at index "%s") has the same key ("%s") as an earlier '. 'hardpoint. Each hardpoint must have a key that is unique '. - 'among hardpoints on the object.')); + 'among hardpoints on the object.', + $idx, + $key)); } $map[$key] = $hardpoint; diff --git a/src/land/engine/ArcanistLandEngine.php b/src/land/engine/ArcanistLandEngine.php index 1bafeb23..5722073b 100644 --- a/src/land/engine/ArcanistLandEngine.php +++ b/src/land/engine/ArcanistLandEngine.php @@ -1459,6 +1459,7 @@ abstract class ArcanistLandEngine 'Merge strategy "%s" specified in "%s" configuration is '. 'unknown. Supported merge strategies are: %s.', $strategy, + $this->getStrategyConfigurationKey(), $strategy_list)); } diff --git a/src/lint/linter/xhpast/rules/ArcanistFormattedStringXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistFormattedStringXHPASTLinterRule.php index 333d7ab1..7b5ed3bb 100644 --- a/src/lint/linter/xhpast/rules/ArcanistFormattedStringXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistFormattedStringXHPASTLinterRule.php @@ -82,7 +82,12 @@ final class ArcanistFormattedStringXHPASTLinterRule } $format = $parameters->getChildByIndex($start); - if ($format->getTypeName() != 'n_STRING_SCALAR') { + if (!$format->isConstantString()) { + + // TODO: When this parameter is not a constant string, the call may + // be unsafe. We should make some attempt to warn about this for + // "qsprintf()" and other security-sensitive functions. + continue; } diff --git a/src/lint/linter/xhpast/rules/__tests__/formatted-string/formatted-string.lint-test b/src/lint/linter/xhpast/rules/__tests__/formatted-string/formatted-string.lint-test index 388d6d63..f9e5a11b 100644 --- a/src/lint/linter/xhpast/rules/__tests__/formatted-string/formatted-string.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/formatted-string/formatted-string.lint-test @@ -11,12 +11,28 @@ fprintf(null, 'x'); queryfx(null, 'x', 'y'); foobar(null, null, '%s'); + +pht('x %s y'); +pht('x %s y'.'z'); + +pht(<<getChildByIndex(0)->evalStatic(); break; case 'n_STRING_SCALAR': - return (string)$this->getStringLiteralValue(); + return phutil_string_cast($this->getStringLiteralValue()); + case 'n_HEREDOC': + return phutil_string_cast($this->getStringLiteralValue()); case 'n_NUMERIC_SCALAR': $value = $this->getSemanticString(); if (preg_match('/^0x/i', $value)) { @@ -186,31 +188,51 @@ final class XHPASTNode extends AASTNode { } public function getStringLiteralValue() { - if ($this->getTypeName() != 'n_STRING_SCALAR') { - return null; + $type_name = $this->getTypeName(); + + if ($type_name === 'n_HEREDOC') { + $value = $this->getSemanticString(); + $value = phutil_split_lines($value); + $value = array_slice($value, 1, -1); + $value = implode('', $value); + + // Strip the final newline from value, this isn't part of the string + // literal. + $value = preg_replace('/(\r|\n|\r\n)\z/', '', $value); + + return $this->newStringLiteralFromSemanticString($value); } - $value = $this->getSemanticString(); - $type = $value[0]; - $value = preg_replace('/^b?[\'"]|[\'"]$/i', '', $value); - $esc = false; - $len = strlen($value); - $out = ''; + if ($type_name === 'n_STRING_SCALAR') { + $value = $this->getSemanticString(); + $type = $value[0]; + $value = preg_replace('/^b?[\'"]|[\'"]$/i', '', $value); - if ($type == "'") { - // Single quoted strings treat everything as a literal except "\\" and - // "\'". - return str_replace( - array('\\\\', '\\\''), - array('\\', "'"), - $value); + if ($type == "'") { + // Single quoted strings treat everything as a literal except "\\" and + // "\'". + return str_replace( + array('\\\\', '\\\''), + array('\\', "'"), + $value); + } + + return $this->newStringLiteralFromSemanticString($value); } + return null; + } + + private function newStringLiteralFromSemanticString($value) { // Double quoted strings treat "\X" as a literal if X isn't specifically // a character which needs to be escaped -- e.g., "\q" and "\'" are // literally "\q" and "\'". stripcslashes() is too aggressive, so find // all these under-escaped backslashes and escape them. + $len = strlen($value); + $esc = false; + $out = ''; + for ($ii = 0; $ii < $len; $ii++) { $c = $value[$ii]; if ($esc) { diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index cbc6b5a0..e5c2078b 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -778,7 +778,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { pht( "'%s' has been amended with 'Differential Revision:', ". "as specified by '%s' in your %s 'base' configuration.", - '.'. + '.', $rule, $source)); // NOTE: This should be safe because Mercurial doesn't support diff --git a/src/runtime/ArcanistRuntime.php b/src/runtime/ArcanistRuntime.php index 87d574a5..35099b2c 100644 --- a/src/runtime/ArcanistRuntime.php +++ b/src/runtime/ArcanistRuntime.php @@ -553,6 +553,7 @@ final class ArcanistRuntime { 'workflow in a given toolset must have a unique name.', get_class($workflow), get_class($map[$key]), + $key, get_class($toolset), $toolset->getToolsetKey())); } diff --git a/src/toolset/ArcanistAliasEngine.php b/src/toolset/ArcanistAliasEngine.php index cba6d723..baa4ab56 100644 --- a/src/toolset/ArcanistAliasEngine.php +++ b/src/toolset/ArcanistAliasEngine.php @@ -72,8 +72,8 @@ final class ArcanistAliasEngine pht( 'Configuration source ("%s") defines an invalid alias, which '. 'will be ignored: %s', - $alias->getConfigurationSource()->getSourceDisplayName()), - $exception->getMessage()); + $alias->getConfigurationSource()->getSourceDisplayName(), + $exception->getMessage())); } $command = array_shift($argv); diff --git a/src/workflow/ArcanistCloseRevisionWorkflow.php b/src/workflow/ArcanistCloseRevisionWorkflow.php index 363b5dbe..8ed435a1 100644 --- a/src/workflow/ArcanistCloseRevisionWorkflow.php +++ b/src/workflow/ArcanistCloseRevisionWorkflow.php @@ -35,8 +35,7 @@ EOTEXT 'help' => pht( "Close only if the repository is untracked and the revision is ". "accepted. Continue even if the close can't happen. This is a soft ". - "version of '' used by other workflows.", - 'close-revision'), + "version of 'close-revision' used by other workflows."), ), 'quiet' => array( 'help' => pht('Do not print a success message.'),