1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-28 17:52:42 +01:00

Fix two "implode()" order issues arising from wilds/experimental collapse

Summary:
Ref T13490. There's one simple "implode()" order issue here, and one slightly more complex one that uses "DIRECTORY_SEPARATOR" as glue.

Add test coverage for this, update the lint check to detect constants used as glue, and fix the callsites.

Test Plan:
  - Added a failing test and made it pass.
  - Ran `arc lint --everything` and looked for remaining implode warnings, found none.

Maniphest Tasks: T13490

Differential Revision: https://secure.phabricator.com/D21024
This commit is contained in:
epriestley 2020-02-23 08:30:54 -08:00
parent ee66b15bd4
commit de461bb179
4 changed files with 18 additions and 3 deletions

View file

@ -1119,7 +1119,7 @@ final class Filesystem extends Phobject {
} }
public static function concatenatePaths(array $components) { public static function concatenatePaths(array $components) {
$components = implode($components, DIRECTORY_SEPARATOR); $components = implode(DIRECTORY_SEPARATOR, $components);
// Replace any extra sequences of directory separators with a single // Replace any extra sequences of directory separators with a single
// separator, so we don't end up with "path//to///thing.c". // separator, so we don't end up with "path//to///thing.c".

View file

@ -23,7 +23,17 @@ final class ArcanistImplodeArgumentOrderXHPASTLinterRule
} }
$parameter = $parameters->getChildByIndex(1); $parameter = $parameters->getChildByIndex(1);
if (!$parameter->isStaticScalar()) {
// If the value is a static scalar, like a string literal, it's probably
// the glue.
$is_scalar = $parameter->isStaticScalar();
// If the value is a constant, like "DIRECTORY_SEPARATOR", it's probably
// the glue.
$is_constant = ($parameter->getTypeName() === 'n_SYMBOL_NAME');
$looks_like_glue = ($is_scalar || $is_constant);
if (!$looks_like_glue) {
continue; continue;
} }

View file

@ -14,5 +14,10 @@ implode();
implode($x); implode($x);
implode($x, $y, $z); implode($x, $y, $z);
// This should raise a warning by guessing that "DIRECTORY_SEPARATOR" is
// glue.
implode($path_list, DIRECTORY_SEPARATOR);
~~~~~~~~~~ ~~~~~~~~~~
error:7:1:XHP129:Implode With Glue First error:7:1:XHP129:Implode With Glue First
error:19:1:XHP129:Implode With Glue First

View file

@ -90,7 +90,7 @@ EOTEXT
"%s\n", "%s\n",
pht( pht(
"Valid status options are:\n\t%s", "Valid status options are:\n\t%s",
implode($this->getStatusOptions(), ', '))); implode(', ', $this->getStatusOptions())));
return 0; return 0;
} }
$ids = $this->getArgument('task_id'); $ids = $this->getArgument('task_id');