Skip to content
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.

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

tests: do not use a shell in proc_open if not really needed #13778

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crrodriguez
Copy link
Contributor

It is seldom that the tests are used to perform operations that require a shell.
remove all implicit shell uses where appropiate.

It is seldom that the tests are used to perform operations that
require a shell.
remove all implicit shell uses where appropiate.
@staabm
Copy link
Contributor

staabm commented Mar 22, 2024

Is this change meant to speedup the command or reduce memory use? Or any other motivation?

@crrodriguez
Copy link
Contributor Author

Is this change meant to speedup the command or reduce memory use? Or any other motivation?

Well yes, executing things without a shell is at least 2x faster or more depending on the OS..

@@ -61,7 +61,22 @@ if ($serverCert === false
$port = 14430;

// set up local server
$cmd = "openssl s_server -key $serverKeyPath -cert $serverCertPath -accept $port -www -CAfile $clientCertPath -verify_return_error -Verify 1";
$cmd = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I am also playing arround with this kind of array-type arg passing to proc_open.

I wonder how its possible to make stderr to stdout redirection work in this situation. usually I would use '2>&1' but it seems in the array-type notation, this arg gets escaped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The command is executed as is by the operating system, there is no shell , ergo no redirection.
proc_open implements the redirect option in descriptorspec (which is unfortunately not documented) see the general_functions/proc_open_redirect.phpt test for examples on how to do the 2>&1

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for taking the time to answer my silly questions. really appreciate it.

@bwoebi
Copy link
Member

bwoebi commented Mar 25, 2024

While it certainly makes the tests a bit faster, having the shell used in the tests makes some of the tests more readable (in others using the array makes it more readable), which is a bit more important than the minor time savings this brings.

So, in particular something like the openssl command in ext/curl/tests/curl_setopt_ssl.phpt should not be changed.

Something like sapi/cli/tests/023.phpt or sapi/cli/tests/022.phpt where it saves escaping it makes a lot of sense to change it in favour of readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants