[Bridge/PhpUnit] Prefer $_SERVER['argv'] over $argv #25304
Conversation
| @@ -230,9 +230,8 @@ if ($components) { | |||
| if (!class_exists('SymfonyBlacklistSimplePhpunit', false)) { | |||
| class SymfonyBlacklistSimplePhpunit {} | |||
| } | |||
| array_splice($argv, 1, 0, array('--colors=always')); | |||
| $_SERVER['argv'] = $argv; | |||
| $_SERVER['argc'] = ++$argc; | |||
There was a problem hiding this comment.
this line should not be removed!
There was a problem hiding this comment.
I forgot to put them back while I was debugging. They are back in 67cc253.
| array_splice($argv, 1, 0, array('--colors=always')); | ||
| $_SERVER['argv'] = $argv; | ||
| $_SERVER['argc'] = ++$argc; | ||
| array_splice($_SERVER['argv'], 1, 0, array('--colors=always')); |
There was a problem hiding this comment.
ok, in fact, this change introduces a bug
both $_SERVER['argv'] and $argv must be updated
this breaks the second
| 8000 | @@ -120,7 +120,7 @@ EOPHP | ||
| } | |||
|
|
|||
| $components = array(); | |||
| $cmd = array_map('escapeshellarg', $argv); | |||
| $cmd = array_map('escapeshellarg', $_SERVER['argv']); | |||
| $exit = 0; | |||
|
|
|||
| if (isset($argv[1]) && 'symfony' === $argv[1] && !file_exists('symfony') && file_exists('src/Symfony')) { | |||
There was a problem hiding this comment.
if you happen to be running this script in a context where $argv is not defined, then this line will always eval to false, which means it's useless
$argv is expected to be always defined in the current code. A better patch would be to always ensure the vars are defined
There was a problem hiding this comment.
So the $cmd should be $argv to be sure it is always defined. That means that this is correct code but leaves $argv still used, not sure where to init the variable $_SERVER['argv'] so that $argv will be intact.
There was a problem hiding this comment.
just start the script with:
$argv = isset($_SERVER['argv']) ? $_SERVER['argv'] : array();
$argc = isset($_SERVER['argc']) ? $_SERVER['argc'] : 0;
There was a problem hiding this comment.
Ah yea. I was thinking way too deep here. Thank you for your suggestion. I reverted everything in 8fbf912 and added the lines at the beginning.
| @@ -119,6 +119,8 @@ EOPHP | |||
|
|
|||
| } | |||
|
|
|||
| $argv = isset($_SERVER['argv']) ? $_SERVER['argv'] : array(); | |||
There was a problem hiding this comment.
onle last thing: before this line, add: global $argv, $argc;
|
No worries @ricknox, I'm taking care of everything. |
|
Thank you @ricknox. |
This PR was submitted for the master branch but it was squashed and merged into the 3.3 branch instead (closes #25304). Discussion ---------- [Bridge/PhpUnit] Prefer $_SERVER['argv'] over $argv | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This makes the script usable even if it is wrapped into another script, which is what some IDEs like PHPStorm do. Commits ------- 1ff22e6 [Bridge/PhpUnit] Prefer ['argv'] over
This makes the script usable even if it is wrapped into another script, which is what some IDEs like PHPStorm do.