run-tests.php fixes #11480

divinity76 · 2023-06-19T13:51:21Z

if php was compiled with
./configure --disable-cgi --enable-cli
then this would happen previously:

PHP Deprecated: escapeshellarg(): Passing null to parameter #1 ($arg) of type string is deprecated in /home/hans/projects/php-src/run-tests.php on line 687
PHP Deprecated: escapeshellarg(): Passing null to parameter #1 ($arg) of type string is deprecated in /home/hans/projects/php-src/run-tests.php on line 688

also did some miscellaneous fixes/improvements, for example

$pass_options .= " -d opcache.preload=" . $preload_filename;

would fail if $preload_filename had whitespace in it.
and replaced some regex-number-checks with ~~FILTER_VALIDATE_INT~~ update: is_numeric() + (int).
and fixed an issue where command-line-arguments were escaped with addslashes() instead of escapeshellarg()
(btw i have issues with escapeshellarg() which you can read more about here https://github.com/divinity76/phpquoteshellarg , but i don't intend to address that issue in this PR)

if php was compiled with ./configure --disable-all --disable-cgi --enable-cli then this would happen previously: PHP Deprecated: escapeshellarg(): Passing null to parameter php#1 ($arg) of type string is deprecated in /home/hans/projects/php-src/run-tests.php on line 687 PHP Deprecated: escapeshellarg(): Passing null to parameter php#1 ($arg) of type string is deprecated in /home/hans/projects/php-src/run-tests.php on line 688

run-tests.php

iluuu1994 · 2023-06-20T19:40:46Z

I haven't had a proper look at this yet, just one thing I noticed.

and replaced some regex-number-checks with FILTER_VALIDATE_INT.

That might be because pcre is a required extension while filter is not.

divinity76 · 2023-06-20T21:01:39Z

That might be because pcre is a required extension while filter is not.

oh crap, definitely don't want to break --disable-all builds, gotta undo that.

using filter_var() would break ./configure --disable-all and --disable-filter builds.

divinity76 · 2023-06-21T07:45:07Z

ok replaced the filter_var with a is_numeric() + (int) approach

divinity76 · 2023-06-21T08:00:38Z

run-tests.php

@@ -2971,24 +2972,31 @@ function settings2params(array $ini_settings): string
    $settings = '';

    foreach ($ini_settings as $name => $value) {


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 👍

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).

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.

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.

@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.

@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.

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 <.< )

@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)

cmd too loking good

@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 ^^

iluuu1994 · 2023-06-21T08:46:48Z

run-tests.php

+                    $workers_raw = substr($argv[$i], strlen("-j"));
+                    $workers = try_string_to_int($workers_raw, 1);
+                    if ($workers === null) {
+                        error("'{$workers_raw}' is not a valid number of workers, try e.g. -j16 for 16 workers");


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.

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 {} ?

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.

iluuu1994 · 2023-06-21T08:50:33Z

run-tests.php

+
+function try_string_to_int($value, int $min_value = PHP_INT_MIN): ?int
+{
+    if(is_string($value) && is_numeric($value) && strpos($value, '.') === false) {


https://3v4l.org/VqVh0 This seems a bit simpler and closer to the original behavior.

does 2684091 look ok to you?

hmm actually that doesn't seem quite right either: https://3v4l.org/iQ6VB

is there something specific you're worried about with the is_numeric($value) && strpos($value, '.') === false approach?

actually never mind, the old regex never supported scientific notation either. lets keep the $value === (string)(int) $value approach

The original regex didn't allow scientific notation. There are no worries with is_numeric/strpos($value, '.'), the alternative is just seems simpler. It's not important.

iluuu1994 · 2023-06-21T09:17:28Z

run-tests.php

@@ -2971,24 +2972,31 @@ function settings2params(array $ini_settings): string
    $settings = '';

    foreach ($ini_settings as $name => $value) {


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).

thanks to iluuu1994 @ php#11480 (comment)

using GitHub CI to test it on Windows for me..

This reverts commit 8461490. turns out using escapeshellarg() on windows breaks random tests due to bugs in escapeshellarg() itself, for example on Windows ```php escapeshellarg("!E_ERROR"); ``` returns ``` "E_ERROR" ``` due to a bug/limitation in escapeshellarg() on windows, and that in turns breaks the test tests/func/011.phpt (among others)

highlyprofessionalscum · 2023-07-11T17:22:31Z

merge plz

divinity76 added 2 commits June 19, 2023 14:08

misc run-tests improvements

4309e98

divinity76 commented Jun 19, 2023

View reviewed changes

run-tests.php Show resolved Hide resolved

devnexen added the Category: Tests label Jun 19, 2023

devnexen requested a review from iluuu1994 June 20, 2023 11:47

do not depend on filter extension

e0832e9

using filter_var() would break ./configure --disable-all and --disable-filter builds.

divinity76 commented Jun 21, 2023

View reviewed changes

iluuu1994 reviewed Jun 21, 2023

View reviewed changes

divinity76 added 3 commits June 22, 2023 10:49

improved string-to-int

2684091

thanks to iluuu1994 @ php#11480 (comment)

simplify settings2params

8461490

using GitHub CI to test it on Windows for me..

		@@ -2971,24 +2972,31 @@ function settings2params(array $ini_settings): string
		$settings = '';

		foreach ($ini_settings as $name => $value) {

run-tests.php fixes #11480

Are you sure you want to change the base?

run-tests.php fixes #11480

Uh oh!

Conversation

divinity76 commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

iluuu1994 commented Jun 20, 2023

Uh oh!

divinity76 commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divinity76 Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divinity76 Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highlyprofessionalscum Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divinity76 Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highlyprofessionalscum Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

divinity76 Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highlyprofessionalscum commented Jul 11, 2023

Uh oh!

Uh oh!

divinity76 commented Jun 19, 2023 •

edited

Loading

divinity76 commented Jun 20, 2023 •

edited

Loading

divinity76 commented Jun 21, 2023 •

edited

Loading

divinity76 Jun 21, 2023 •

edited

Loading

divinity76 Jun 23, 2023 •

edited

Loading

divinity76 Jun 23, 2023 •

edited

Loading

highlyprofessionalscum Jul 4, 2023 •

edited

Loading

divinity76 Jul 4, 2023 •

edited

Loading

highlyprofessionalscum Jul 4, 2023 •

edited

Loading

divinity76 Jun 21, 2023 •

edited

Loading