Support array command in proc_open()

[nikic]

As requested by @nicolas-grekas, this implements support for passing a command array to proc_open(). In this case no shell will be used for the execution, and argument escaping (if it is necessary) will be handled internally. Example:

proc_open(['php', '-r', 'echo "Hello World\n";'], $ds, $pipes);

On Unix this is implemented using execvp. On Windows we use CreateProcessW and escape arguments appropriately.

I did the Windows implementation blind, so it likely doesn't work... someone on Windows will have to check that.

[nicolas-grekas]

Thanks for moving this forward, that's a nice addition to me. I also had a look at the Symfony Process class to review the pile of workarounds we accumulated there. Windows still has a major bug with pipes: https://bugs.php.net/51800 is not fixed, despite it being closed. Looking at the git history, Anatolyi gave it a try a few times but without success, I fear. For this reason, we still need bypass_shell... Making pipes non-blocking on Windows would be another way to work around the issue. That'd be my next request if any Windows-dev would know how to achieve this...

[nikic]

Thanks for moving this forward, that's a nice addition to me. I also had a look at the Symfony Process class to review the pile of workarounds we accumulated there. Windows still has a major bug with pipes: https://bugs.php.net/51800 is not fixed, despite it being closed. Looking at the git history, Anatolyi gave it a try a few times but without success, I fear. For this reason, we still need bypass_shell... Making pipes non-blocking on Windows would be another way to work around the issue. That'd be my next request if any Windows-dev would know how to achieve this...

To be clear, you're saying that this bug does not occur if bypass_shell=1? If so, then it also shouldn't occur with the array command form.

[nicolas-grekas]

Nope, it's more complex than that: to work around this bug, we redirect stdout/stderr to files. But we don't define them using $descriptospec because of what is described in https://bugs.php.net/65650. This means we use shell redirections ($cmd > file). To do so, we bypass the shell and add our owns, because we want to enable delayed expansion (for the case where the command line is defined as a string, with env var refs).
Having non-blocking pipes would allow us to remove the redirections and ignore bug 65650. That'd allow removing a pile of messy code + have proper stream_select logic for async process monitoring...

[i336]

@nikic, wanted to thank you for this; in slightly unbelievable timing, I literally just opened php-src on GitHub to go look up how PHP behaves given that this very feature does not exist (...yet :D)! I got distracted looking at the pull requests, and then my jaw dropped at the first entry in the list... haha.

@nicolas-grekas, just had a look at how you add your own console, and had an entirely different kind of slight jawdrop. Quite amazing the amount of plumbing that must be done in these kinds of legacy/imperfect circumstances; hats off to the work you do. One (uninformed, naive) question: Microsoft has just dropped (as in released) the new Windows Terminal, which IIUC follows a significant rearchitecture of how consoles in Windows work internally. I heard some very positive noises when I talked to the console team 3 years ago; I can only imagine things are a billion times better now ("get a Windows box" still on my todo list ;) )... I wonder if it could be interesting to find out how the new infra works (and the minimum Windows versions) and RFC support into PHP. Unsure if barking up wrong tree or if such ideas are already in a queue somewhere.

[nikic]

/cc @cmb69 @weltling Could you please check over the Windows part of this? The test at least passes...

[cmb69]

Looks good so far, thanks! Only issue I see is that there don't seem to be any explicit NUL byte checks.

[nikic]

@cmb69 Right, I'm currently (implicitly) discarding everything after null bytes. Should I make this an error condition instead (i.e. warning+false)?

[cmb69]

@nikic, IMO it's indeed more reasonable to reject NUL bytes as error in the first place.

[nikic]

@cmb69 Now generating warning+false for nul bytes.

[nikic]

Merged as 8be0510.

Nope, it's more complex than that: to work around this bug, we redirect stdout/stderr to files. But we don't define them using $descriptospec because of what is described in https://bugs.php.net/65650.

Per the last two comments there the original code there is buggy ... is that wrong? If so the issue should be clarified & reopened.

[cmb69]

@nicolas-grekas, please confirm whether bug 65650 has been closed as "not a bug" erroneously.

[nikic]

I'm wondering whether we can use SetNamedPipeHandleState with PIPE_NOWAIT to switch the pipe into non-blocking mode.