[Process] Pass the commandline as array to proc_open()
#52409
Conversation
|
The failing unit test in PHP 8.3 seems to be a bug in PHP: php/php-src#12589 |
|
It is a new feature and must therefore target 7.1 because of feature freeze period Should we wait wait for a PHP bugfix before merge? |
209588c to
b56c0b0
Compare
Changed the base to 7.0 (there is no 7.1 branch yet)
I think so, yes. |
62a7132 to
185744e
Compare
The PHP bug got closed, the new behavior is intended. I updated the pull request to handle that case. |
proc_open()
4b31327 to
3eca0b9
Compare
|
|
|
Hey! Thanks for your PR. You are targeting branch "7.0" but it seems your PR description refers to branch "7.1". Cheers! Carsonbot |
|
@carsonbot the 7.1 branch does not exist yet. |
| $process = @proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $envPairs, $this->options); | ||
| } finally { | ||
| restore_error_handler(); | ||
| } | ||
|
|
||
| if (!\is_resource($process)) { | ||
| throw new RuntimeException('Unable to launch a new process.'); | ||
| throw new RuntimeException('Unable to launch a new process: '.$lastError); |
There was a problem hiding this comment.
Should we throw a ProcessFailedException here? That would improve backwards compatibility I think.
Or should we maybe create a new exception like ProcessStartFailedException?
There was a problem hiding this comment.
ProcessFailedException would be fine to me
There was a problem hiding this comment.
I created a new ProcessStartFailedException that extends the ProcessFailedException in 833bf1d.
This is a clean API I think and makes the changed behavior in PHP 8.3 a little bit more BC.
There was a problem hiding this comment.
Thanks for working on this!
Can't we use an array on Windows also? Sorry I didn't read everything 😬
While at it, WDYT of the warnings on appveyor :)
'where' is not recognized as an internal or external command,
operable program or batch file.
'where' is not recognized as an internal or external command,
operable program or batch file.
ERROR: Invalid argument or option - '/usr/local/php/bin/php-invalid'.
Type "WHERE /?" for usage help.
ERROR: Invalid argument or option - '/usr/local/php/bin/php-invalid'.
Type "WHERE /?" for usage help.
| $process = @proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $envPairs, $this->options); | ||
| } finally { | ||
| restore_error_handler(); | ||
| } | ||
|
|
||
| if (!\is_resource($process)) { | ||
| throw new RuntimeException('Unable to launch a new process.'); | ||
| throw new RuntimeException('Unable to launch a new process: '.$lastError); |
There was a problem hiding this comment.
ProcessFailedException would be fine to me
d603793 to
833bf1d
Compare
41770b7 to
a300de8
Compare
|
Thank you @ausi. |
I don’t think so as we use some heavy |
Introduced in symfony/symfony#52409 we no longer should use `Illuminate\Support\ProcessUtils::escapeArgument()` unless calling `Process` via `Process::fromShellCommandline()` fixes #51820 Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Introduced in symfony/symfony#52409 we no longer should use `Illuminate\Support\ProcessUtils::escapeArgument()` unless calling `Process` via `Process::fromShellCommandline()` fixes #51820 Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Introduced in symfony/symfony#52409 we no longer should use `Illuminate\Support\ProcessUtils::escapeArgument()` unless calling `Process` via `Process::fromShellCommandline()` fixes #51820 Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Since PHP 7.4,
proc_open()accepts an array which makes it twice as fast (4x as fast with PHP 8.3), see #43162 (comment)This pull request enables calling
proc_open()with an array if:$this->commandlineis an array--enable-sigchild