[WIP][Process] Add commandline parameter binding support #12488
Conversation
36fbcc1 to
18b41bf
Compare
|
@romainneutron thanks for this proposal. I like it! I have one question: could it be possible to use the exact same syntax used in SQL parameter binding? Positional: $process = new Process('ls ? | grep ? > data.log && curl -XPOST ? 2>&1 --data-binary "@data.log" >/dev/null');
$process->run(null, ['.', '\'', 'http://localhost/endpoint']);Named: $process = new Process('ls :dir | grep :filter > data.log && curl -XPOST :url 2>&1 --data-binary "@data.log" >/dev/null');
$process->run(null, [':dir' => '.', ':filter' => '\'', ':url' => 'http://localhost/endpoint']); |
|
@javiereguiluz I'm not sure about the $process = new Process('ls :dir | grep :filter > data.log && curl -XPOST :url 2>&1 --data-binary "@data.log" >/dev/null');
$process->run(null, array('dir' => '.', 'filter' => '\'', 'url' => 'http://localhost/endpoint')); |
18b41bf to
e8d995c
Compare
|
@iltar thanks for fixing this error; it was obviously a mistake on my side :) I really meant using the exact same syntax as DBAL. My reasoning is that introducing new syntaxes is always a bad idea unless there are technical reasons for doing it. |
|
Hi guys, This is indeed a good idea. I'm gonna update the proposal so it use the same syntax as DBAL |
| * | ||
| * @return string | ||
| */ | ||
| private function escape($string) |
There was a problem hiding this comment.
IMO, this method is not needed. Use the static util directly
|
I see a drawback in this API: it forces to pass parameters when starting the process. This makes it impossible to build a process and then pass it to a separate place running it. |
|
note that otherwise, I find this idea very interesting |
|
Is there something we can do to help finish this PR? I still think this is a nice feature and makes commands very readable when using named params. Thanks! |
…ars, fixing signaling and escaping (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [Process] Accept command line arrays and per-run env vars, fixing signaling and escaping | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #12488, #11972, #10025, #11335, #5759, #5030, #19993, #10486 | License | MIT | Doc PR | - I think I found a way to fix this network of issues once for all. Of all the linked ones, only the last two are still open: the remaining were closed in dead ends. Instead of trying to make `ProcessUtil::escapeArgument` work correctly on Windows - which is impossible as discussed in #21347 - this PR deprecates it in favor of a more powerful approach. Depending on the use case: - when a simple command should be run, `Process` now accepts an array of arguments (the "binary" being the first arg). Making this the responsibility of `Process` (instead of `ProcessBuilder`) gives two benefits: - escape becomes an internal detail that doesn't leak - thus can't be misused ([see here](#21347 (comment))) - since we know we're running a single command, we can prefix it automatically by "exec" - thus fixing a long standing issue with signaling ```php $p = new Process(array('php', '-r', 'echo 123;')); echo $p->getCommandLine(); // displays on Linux: // exec 'php' '-r' 'echo 123;' ``` - when a shell expression is required, passing a string is still allowed. To make it easy and look-like sql prepared statements, env vars can be used when running the command. Since the shell is OS-specific (think Windows vs Linux) - this PR assumes no portability, so one should just use each shell's specific syntax. From the fixtures: ```php $env = array('FOO' => 'Foo', 'BAR' => 'Bar'); $cmd = '\\' === DIRECTORY_SEPARATOR ? 'echo !FOO! !BAR! !BAZ!' : 'echo $FOO $BAR $BAZ'; $p = new Process($cmd, null, $env); $p->run(null, array('BAR' => 'baR', 'BAZ' => 'baZ')); $this->assertSame('Foo baR baZ', rtrim($p->getOutput())); $this->assertSame($env, $p->getEnv()); ``` Commits ------- 330b61f [Process] Accept command line arrays and per-run env vars, fixing signaling and escaping
Hello,
this is an alternative proposal to #11972.
Let me summarise which issues we have and what I propose:
We need to enforce our strategy to prevent command line injections as it's already done for SQL. At the moment, the ProcessBuilder we provide lacks of support for many use case, especially:
|>>,<,>, etc...execfunctionality (it's not a command, it must not be escaped) rel [RFC] Process and chained commands in symfony 2.2 #5759, [Process] add a method to the ProcessBuilder to automatically prepend the executed script with "exec" #11335In these cases, users do not use the
ProcessBuilderas this one does not fit their needs ; they build the commandline by themselves.In #11972, I proposed to add a fluent command builder. However, thinking more about it, it will be a mess to write and maintain, it's mostly writing a bash parser and I think it's a big work to do in order to solve something that should be easy. Moreover, writing such commandline builder would not be easy to use (just read the few comment of this PR, think about explicit precedence).
Discussing about this topic with @nicolas-grekas brought us to this proposal that is fully BC and provides an easy way for developers to prepare and run commands just as in SQL.
What do you think about it?
Here's a sample of code that allows to build a safe and complex command:
would run:
It also can be used with named parameters:
would run:
It's currently working, let me know what you think about it.
I currently propose to be straight on parameters: If parameters mismatch (missing, unused), the components throws an exception.
I also propose to use
{}as default placeholder, it might be wrong, let me know if you have comments on this.Last but not least, this PR still requires:
ProcessBuilder