[RFC] Improve UX of Process #27796

Toflar · 2018-07-02T07:55:06Z

Since Symfony 3.3, the Process component accepts an array of arguments as the $commandline which will then enable autoescaping automatically. See symfony/symfony-docs#9988 and the tweet by @nicolas-grekas. Without that tweet I would have probably completely missed out on that feature and I think a lot of devs are. But I think in probably 99% of all cases you would want to have this feature enabled. However, right now, nothing tells you that you're probably doing things wrong. It will just - maybe even silently - fail on different platforms. And that's not a good situation.
I think most developers still choose new Process('string commandline'); because that's how it used to work ever since even though they would prefer having the arguments escaped. This is too much magic for me (pass a string -> behaves like this, pass an array -> behaves differently but you never get an error). I think if a developer really wants to handle the underlying details for stream redirection etc. they should explicitly opt-in for that, not opt-out.

So I think the first constructor argument of Process should only accept an array and if you really want to pass in a string, you should tell the Process that you know what you're doing. There are multiple solutions how we could achieve that:

Adding a new method to disable escaping and checking in the start() method if argument escaping was disabled explicitly:

$process = new Process('string command');
$process->start(); // Raises deprecation, in 5.0 exception

$process = new Process('string command');
$process->disableArgumentEscaping(); // Developer now explicitly disables escaping which is what we want
$process->start(); // This is fine now

New class:

$process = new Process('string command'); // deprecated, use ShellProcess if you want to disable argument escaping, in 5.0 use array as typehint

$process = new Process(['string', 'command']); // correct
$process = new ShellProcess('string command'); // correct

Wdyt?

The text was updated successfully, but these errors were encountered:

nicolas-grekas · 2018-07-02T08:00:37Z

$process = new ShellProcess('string command'); // correct

I like this one, with a deprecation when "new Process" is called with a string as argument.
Autoescaping is wrong btw: when there is no shell, there is no concept of escaping at all, because there is no parsing of anything, thus... nothing to escape. The array are just arguments passed to the command-executor of the kernel.

Toflar · 2018-07-02T08:03:55Z

I see, I'll leave a comment in symfony/symfony-docs#9988 then :-)

Toflar · 2018-07-02T08:18:24Z

when there is no shell, there is no concept of escaping at all, because there is no parsing of anything, thus... nothing to escape.

Also, this is kind of an argument against having ShellProcess, no? If there is a shell, there is a concept of escaping = why would ShellProcess accept a string that's not escaped then? 😄

nicolas-grekas · 2018-07-02T08:20:18Z

why would ShellProcess accept a string that's not escaped then?

I'm sorry i don't get this sentence, it doesn't make sense to me.

Toflar · 2018-07-02T08:23:19Z

Sorry, trying again. You said:

when there is no shell, there is no concept of escaping at all, because there is no parsing of anything, thus... nothing to escape.

So imho it should be the other way around:

$process = new Process('direct command'); // Never escapes anything because there is no shell

$process = new ShellProcess(['ls', '-la']); // Escapes and then passes the string to the parent

nicolas-grekas · 2018-07-02T08:26:47Z

it's the opposite: when using an array there is no shell involved, and when using a string, there is a shell that is used to parse it.

Toflar · 2018-07-02T08:32:06Z

I see. I think I need to see where the whole escaping takes place. But I guess the discussion shows that there's room for improvement here 😄

javiereguiluz · 2018-07-02T08:57:46Z

I'm not sure it's worth it to create a whole new ShellProcess class. The docs are clear about this and easy to understand for the differences of using array/strings with Process.

linaori · 2018-07-02T10:49:29Z

I agree that writing a new class for this might not be a good idea if it does the same but with a different constructor. Out of curiosity though, would it be interesting to make the Process class internal and expose different light-weight wrappers instead? It always felt odd to initiate a new process, which could (afaik) accept multiple commands inside.

This is just a brainfart:

$interactiveShell = new InteractiveShell(); // nothing needed as it's interactive
$shell1 = new SimpleShell('ls -la');
$shell2 = new PreparedShell('ls', ['-la']);
$command = new SymfonyCommandShell('debug:container', ['--show-private']);
$customCommand = new SomeComplexCommandWithLotsOfArgumentsForMyDomain([...]);

Internally they could all use the process and probably expose a very similar API, but it would make it possible to use a consistent abstraction layer to run commands/tools/scripts etc. with a nicer UX (imo).

stof · 2018-07-02T10:54:26Z

@iltar does it actually make things simpler ? Most of the public methods of the Process object would then have to be proxied. Users would then have to ask themselves "which one should I be using?" all the time.

linaori · 2018-07-02T11:00:58Z

@stof Yeah, that's the only downside, but I don't know how to solve that cleanly without delegation.

One advantage though, is that you can narrow down the public API per "shell". A process that pipes output all the time that needs to be parsed, only needs to expose methods to do just that, where as a simple one time only output command, doesn't have to do this and can just get the output when done. Some custom commands might not have any output at all and you only want to know the return type.

Writing those might be a bit more difficult and will also narrow down the possibilities. However, as writer I've often noticed that I'm at a loss on which methods I need to call on the process, making it more of a trial and error to see what (kinda) works.

Toflar · 2018-07-02T11:01:19Z

"which one should I be using?"

Which is what they should. I mean, not talking about the proxy but this is why I've created this issue. Right now devs don't ask. They just use new Process('string') and it might cause issues with escaping. I think it would be a good thing to make them decide rather than just making a difference between passing a string or an array. That doesn't feel like devs are deciding. Only if you pass an array you are deciding and imho passing an array is normally what you want. I agree that docs of Symfony are good but it could probably also be improved by the interfaces provided.

nicolas-grekas · 2018-07-02T11:03:02Z

I think deprecating new Process(string) is the way to go: it will force people to consider the issue and migrate to using arrays. If they really want the shell, new ShellProcessor will be provided.
I don't think there is anything else to seek for.

nicolas-grekas · 2018-07-02T11:05:11Z

We can also not add any new class and add a static constructor instead for the shell!

nicolas-grekas · 2018-07-03T12:10:06Z

See #27821

…ings (nicolas-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Process][Console] deprecated defining commands as strings | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #27796 | License | MIT | Doc PR | - * Added the `Process::fromShellCommandline()` static constructor to define shell command-lines * Allowed passing commands as `array($process, 'ENV_VAR' => 'value')` to `ProcessHelper::run()` * Deprecated passing commands as strings when creating a `Process` instance. * Deprecated the `Process::setCommandline()` and the `PhpProcess::setPhpBinary()` methods. * Deprecated passing a command as a string to `ProcessHelper::run()`, pass it the command as an array of arguments instead. * Made the `ProcessHelper` class final Commits ------- 8895bc1 [Process][Console] deprecated defining commands as strings

Toflar mentioned this issue Jul 2, 2018

[Process] document command-as-arrays symfony/symfony-docs#9988

Merged

javiereguiluz added Process RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 2, 2018

nicolas-grekas mentioned this issue Jul 3, 2018

[Process][Console] deprecated defining commands as strings #27821

Merged

fabpot closed this as completed Jul 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Improve UX of Process #27796

[RFC] Improve UX of Process #27796