-
Notifications
You must be signed in to change notification settings - Fork 7.9k
tests: do not use a shell in proc_open if not really needed #13778
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
Conversation
1f7ff8d to
fca5f71
Compare
It is seldom that the tests are used to perform operations that require a shell. remove all implicit shell uses where appropiate.
fca5f71 to
ca5fbcf
Compare
Is this change meant to speedup the command or reduce memory use? Or any other motivation?
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..
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.
It is seldom that the tests are used to perform operations that require a shell.
remove all implicit shell uses where appropiate.