Skip to content

run-tests.php fixes #11480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

run-tests.php fixes #11480

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c26b20
escapeshellarg null fixes
divinity76 Jun 19, 2023
4309e98
misc run-tests improvements
divinity76 Jun 19, 2023
e0832e9
do not depend on filter extension
divinity76 Jun 21, 2023
2684091
improved string-to-int
divinity76 Jun 22, 2023
8461490
simplify settings2params
divinity76 Jun 24, 2023
71c7af1
Revert "simplify settings2params"
divinity76 Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 60 additions & 40 deletions run-tests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,11 @@ function main(): void

switch ($switch) {
case 'j':
$workers = substr($argv[$i], 2);
if ($workers == 0 || !preg_match('/^\d+$/', $workers)) {
error("'$workers' is not a valid number of workers, try e.g. -j16 for 16 workers");
$workers_raw = substr($argv[$i], strlen("-j"));
$workers = validate_int($workers_raw, 1);
if ($workers === null) {
error("'{$workers_raw}' is not a valid number of workers, try e.g. -j16 for 16 workers");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a good reason to add {} for simple variables. In general, we should try to avoid changes that aren't clearly superior to avoid ping pong of code styles.

Copy link
Contributor Author

@divinity76 divinity76 Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo using{} is less error-prone, so i pretty much always use {} - take for example concatenating 3 variables with _, people who habitually do not use {} may write it as

$str = "$v1_$v2_$v3";

which is wrong: it tries to concatenate the variable $v1_ and $v2_ and $v3 with no separator, instead of concatenating $v1 and $v2 and $v3 with _ , but it may be difficult to spot the error in code review. people who habitually use {} would write it as

$str = "{$v1}_{$v2}_{$v3}";

which would actually work.

Are you sure I should really remove the {} ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the risks are largely overblown, similar to single line ifs (without macros, that is). Anyway, I don't care which style but we should avoid changing back and forth. Since we have no established standard it's not clear which is preferred.

}
$workers = intval($workers, 10);
// Don't use parallel testing infrastructure if there is only one worker.
if ($workers === 1) {
$workers = null;
Expand Down Expand Up @@ -528,30 +528,31 @@ function main(): void
$just_save_results = true;
break;
case '--set-timeout':
$timeout = $argv[++$i] ?? '';
if (!preg_match('/^\d+$/', $timeout)) {
error("'$timeout' is not a valid number of seconds, try e.g. --set-timeout 60 for 1 minute");
$timeout_raw = $argv[++$i] ?? '';
$timeout = validate_int($timeout_raw, 0);
if ($timeout === null) {
error("'{$timeout_raw}' is not a valid number of seconds, try e.g. --set-timeout 60 for 1 minute");
}
$environment['TEST_TIMEOUT'] = intval($timeout, 10);
$environment['TEST_TIMEOUT'] = $timeout;
break;
case '--context':
$context_line_count = $argv[++$i] ?? '';
if (!preg_match('/^\d+$/', $context_line_count)) {
error("'$context_line_count' is not a valid number of lines of context, try e.g. --context 3 for 3 lines");
$context_line_count_raw = $argv[++$i] ?? '';
$context_line_count = validate_int($context_line_count_raw, 0);
if ($context_line_count === null) {
error("'{$context_line_count_raw}' is not a valid number of lines of context, try e.g. --context 3 for 3 lines");
}
$context_line_count = intval($context_line_count, 10);
break;
case '--show-all':
foreach ($cfgfiles as $file) {
$cfg['show'][$file] = true;
}
break;
case '--show-slow':
$slow_min_ms = $argv[++$i] ?? '';
if (!preg_match('/^\d+$/', $slow_min_ms)) {
error("'$slow_min_ms' is not a valid number of milliseconds, try e.g. --show-slow 1000 for 1 second");
$slow_min_ms_raw = $argv[++$i] ?? '';
$slow_min_ms = validate_int($slow_min_ms_raw, 0);
if ($slow_min_ms === null) {
error("'{$slow_min_ms_raw}' is not a valid number of milliseconds, try e.g. --show-slow 1000 for 1 second");
}
$slow_min_ms = intval($slow_min_ms, 10);
break;
case '--temp-source':
$temp_source = $argv[++$i];
Expand Down Expand Up @@ -683,8 +684,8 @@ function main(): void
$environment['TEST_PHP_EXECUTABLE_ESCAPED'] = escapeshellarg($php);
putenv("TEST_PHP_CGI_EXECUTABLE=$php_cgi");
$environment['TEST_PHP_CGI_EXECUTABLE'] = $php_cgi;
putenv("TEST_PHP_CGI_EXECUTABLE_ESCAPED=" . escapeshellarg($php_cgi));
$environment['TEST_PHP_CGI_EXECUTABLE_ESCAPED'] = escapeshellarg($php_cgi);
putenv("TEST_PHP_CGI_EXECUTABLE_ESCAPED=" . escapeshellarg($php_cgi ?? ''));
$environment['TEST_PHP_CGI_EXECUTABLE_ESCAPED'] = escapeshellarg($php_cgi ?? '');
putenv("TEST_PHPDBG_EXECUTABLE=$phpdbg");
$environment['TEST_PHPDBG_EXECUTABLE'] = $phpdbg;
putenv("TEST_PHPDBG_EXECUTABLE_ESCAPED=" . escapeshellarg($phpdbg ?? ''));
Expand All @@ -694,7 +695,7 @@ function main(): void
if (IS_WINDOWS) {
$pass_options .= " -c " . escapeshellarg($conf_passed);
} else {
$pass_options .= " -c '" . realpath($conf_passed) . "'";
$pass_options .= " -c " . escapeshellarg(realpath($conf_passed));
}
}

Expand Down Expand Up @@ -1189,13 +1190,13 @@ function system_with_timeout(

$descriptorspec = [];
if ($captureStdIn) {
$descriptorspec[0] = ['pipe', 'r'];
$descriptorspec[0] = ['pipe', 'rb'];
}
if ($captureStdOut) {
$descriptorspec[1] = ['pipe', 'w'];
$descriptorspec[1] = ['pipe', 'wb'];
}
if ($captureStdErr) {
$descriptorspec[2] = ['pipe', 'w'];
$descriptorspec[2] = ['pipe', 'wb'];
}
$proc = proc_open($commandline, $descriptorspec, $pipes, TEST_PHP_SRCDIR, $bin_env, ['suppress_errors' => true]);

Expand Down Expand Up @@ -2348,11 +2349,11 @@ function run_test(string $php, $file, array $env): string
$args = $test->hasSection('ARGS') ? ' -- ' . $test->getSection('ARGS') : '';

if ($preload && !empty($test_file)) {
save_text($preload_filename, "<?php opcache_compile_file('$test_file');");
save_text($preload_filename, "<?php opcache_compile_file(" . var_export($test_file, true) . ");");
$local_pass_options = $pass_options;
unset($pass_options);
$pass_options = $local_pass_options;
$pass_options .= " -d opcache.preload=" . $preload_filename;
$pass_options .= " -d opcache.preload=" . escapeshellarg($preload_filename);
}

if ($test->sectionNotEmpty('POST_RAW')) {
Expand Down Expand Up @@ -2971,24 +2972,31 @@ function settings2params(array $ini_settings): string
$settings = '';

foreach ($ini_settings as $name => $value) {
Copy link
Contributor Author

@divinity76 divinity76 Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for people reviewing this: the old loop was much more complex than necessary, and even wrong, for unix. I don't think the windows-loop is correct either, but I don't have a Windows computer to test on, and I'm not comfortable editing the windows-side of the loop. for Unix, all of this stuff was replaced with literally 3 lines of code 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBF it has even more branches now. We could at least apply the same optimization to Windows (handling values and arrays the same), and applying the " to ' fix in both cases. I don't understand what it's for (added in cee9708).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBF it has even more branches now

yeah but the new branch is super simple (3 lines) and correct, while the old branches were complex-ish and incorrect (incorrect for unix at least)

We could at least apply the same optimization to Windows (handling values and arrays the same)

yeah, but then we need to test it. I have no easy way of testing it. If someone else wants to fix it, that's fine, but I'd rather not touch the windows branch.

and applying the " to ' fix in both cases. I don't understand what it's for

the existing logic suggest that removing " is only a special-case for lists/arrays on Windows, not applicable to plain strings and not applicable to unix, and it looks like it's a hack in the first place, i don't think we should add the " thing to plain strings without a deeper understanding of why it exists. I don't understand it either.

Copy link
Contributor Author

@divinity76 divinity76 Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone want to change/optimize/fix the Windows branch, that's fine, but it shouldn't be me. I don't even have a Windows system to test on. I don't want to touch it.

Copy link
Contributor Author

@divinity76 divinity76 Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iluuu1994 i think the whole function could be written as

function settings2params(array $ini_settings): string
{
    $settings = '';
    $deep = null;

    foreach ($ini_settings as $name => $value) {
        if (is_array($value)) {
            $deep = true;
        } else {
            $deep = false;
            $value = [$value];
        }
        foreach ($value as $val) {
            if (IS_WINDOWS && $deep && strlen($val) >= 2 && str_starts_with($val, '"') && str_ends_with($val, '"')) {
                // hack?
                $val = substr($val, 1, -1);
            }
            $settings .= " -d " . escapeshellarg($name) . "=" . escapeshellarg($val);
        }
    }
        return $settings;
}

but i don't know, and i have no easy way of testing it, and if im wrong, i have to push it to github, then wait 2 hours for github actions tell me if it worked or not, and repeat, until i get it right. it can take hours and its super annoying, and it would be much easier for some guy with an actual Windows system to test on. Thus, some guy with a Windows computer should do it, not me.

Copy link

@highlyprofessionalscum highlyprofessionalscum Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divinity76 i think we can remove all escaping. test run by proc_open and the docs says

array|string $command – 
Execute a command and open file pointers for input/output
As of PHP 7.4.0, cmd may be passed as array of command parameters. In this case the process will be opened directly (without going through a shell) and PHP will take care of any necessary argument escaping.

Copy link
Contributor Author

@divinity76 divinity76 Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat, but it would require some significant refactoring, for example the entire settings2params function would need to be changed from returning :string to :array 🤔

PHP will take care of any necessary argument escaping.

it wouldn't surprise me if proc_open's array-of-command-parameters suffers the same bugs as escapeshellarg()-on-windows, you need to check that (i can't be arsed)

  • does this work on windows?
<?php
$descriptorspecs=array();
$pipes=array();
$proc = proc_open(array('php.exe', '-r', 'echo "!!exclamation_marks!!";'), $descriptorspecs, $pipes);
proc_close($proc);
die();

(it wouldn't surprise me if the !! gets stripped, like escapeshellarg()-on-windows does, in which case it would break tests <.< )

Copy link

@highlyprofessionalscum highlyprofessionalscum Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divinity76
dimli@LAPTOP-KUHG2256 MINGW64 /c/OpenServer2/domains/ts/sf/public
$ /c/OpenServer2/modules/php/PHP_7.4/php.exe index.php
!!exclamation_marks!!int(0)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd too loking good
изображение

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@highlyprofessionalscum neat! if you want to update the function, and the code relying on the function, to use arrays instead of strings, that would be better improvement for sure.

But i think it should be done in it's own dedicated PR, not this one ^^

if (is_array($value)) {
foreach ($value as $val) {
$val = addslashes($val);
$settings .= " -d \"$name=$val\"";
}
} else {
if (IS_WINDOWS && !empty($value) && $value[0] == '"') {
$len = strlen($value);

if ($value[$len - 1] == '"') {
$value[0] = "'";
$value[$len - 1] = "'";
if (IS_WINDOWS) {
if (is_array($value)) {
foreach ($value as $val) {
$val = addslashes($val);
$settings .= " -d \"$name=$val\"";
}
} else {
$value = addslashes($value);
}
if (!empty($value) && $value[0] == '"') {
$len = strlen($value);

if ($value[$len - 1] == '"') {
$value[0] = "'";
$value[$len - 1] = "'";
}
} else {
$value = addslashes($value);
}

$settings .= " -d \"$name=$value\"";
$settings .= " -d \"$name=$value\"";
}
} else {
// !IS_WINDOWS
foreach ((is_array($value) ? $value : [$value]) as $val) {
$settings .= " -d " . escapeshellarg($name) . "=" . escapeshellarg($val);
}
}
}

Expand Down Expand Up @@ -3321,7 +3329,7 @@ public function __construct(array $env, int $workerID)
$this->enabled = false;
return;
}
if (!$workerID && !$this->fp = fopen($fileName, 'w')) {
if (!$workerID && !$this->fp = fopen($fileName, 'wb')) {
throw new Exception("Failed to open $fileName for writing.");
}
}
Expand Down Expand Up @@ -3965,6 +3973,18 @@ function bless_failed_tests(array $failedTests): void
proc_open($args, [], $pipes);
}


function validate_int($value, int $min_value = PHP_INT_MIN): ?int
{
$ret = null;
if(is_int($value)) {
$ret = $value;
} elseif (is_string($value) && $value === (string)(int) $value) {
$ret = (int) $value;
}
return ($ret !== null && $ret >= $min_value) ? $ret : null;
}

/*
* BSD 3-Clause License
*
Expand Down