Skip to content
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

[Process] Process Builder support for pipe command lines #10025

Closed
cordoval opened this issue Jan 15, 2014 · 23 comments
Closed

[Process] Process Builder support for pipe command lines #10025

cordoval opened this issue Jan 15, 2014 · 23 comments
Labels

Comments

@cordoval
Copy link
Contributor

Creating an issue here in accordance with alchemy-fr/BinaryDriver#2 . @romainneutron could you please tell me how may we implement this, do i push a failing test for this first so we can pin point the problem? Thanks you rock man!

@romainneutron
Copy link
Contributor

We should know if we want to add support for pipe (|) and redirections operators (>, <, 2>&1 among others ...) in the ProcessBuilder.

I think it's a nice feature, I don't know if we should parse user input or provide methods :

ProcessBuilder::create(['cat', 'error.log'])->pipe(ProcessBuilder::create(['grep', 'proton']));
ProcessBuilder::create(['grep', '-lR', 'proton', '/tmp'])->redirect('>', ProcessBuilder::create('output.log'));

VS

ProcessBuilder::create(['cat', 'error.log', '|', 'grep', 'proton']);
ProcessBuilder::create(['grep', '-lR', 'proton', '/tmp', '>', 'output.log']);

@lyrixx
Copy link
Member

lyrixx commented Jan 15, 2014

I like the first approach. And I think it's easier to implement.

@stof
Copy link
Member

stof commented Jan 15, 2014

I would vote for the pipe method, as this would be BC and fits better with an OO builder.
If we start parsing |, it would break BC as it is currently escaped as an argument.

@fabpot
Copy link
Member

fabpot commented Jan 15, 2014

definitely the first approach.

@cordoval
Copy link
Contributor Author

ping @jakzal could you please label this issue as [Process]?

@cordoval
Copy link
Contributor Author

thanks @jakzal

@cryptiklemur
Copy link
Contributor

Any way to do this now?

@cordoval
Copy link
Contributor Author

cordoval commented May 4, 2014

@stof @lyrixx @romainneutron @fabpot do you think when this will happen? it is kind of blocking, right now we are working with @aequasi hard to integrate Gush and Bldr.io bldr-io/gush-block#2 to create macro flows and if we have this feature we can do more dynamic things 💃

Thanks a lot guyz /o/ :+1: :baby:

@cryptiklemur
Copy link
Contributor

I'm sure there are also a lot of other people that would benefit from this :)

@mattjanssen
Copy link
Contributor

👍 Would love to see this. I can't use the builder as often without it.

@gbirke
Copy link

gbirke commented Sep 9, 2014

I'd love to have this. The taskphp Process plugin would also benefit from that because it does no longer need to collect the whole output.

@piotrplenik
Copy link

In PR #11971 I've suggest implementation of pipe ("|") functionality.

@romainneutron
Copy link
Contributor

This feature request makes sense

@romainneutron romainneutron reopened this Dec 15, 2014
@cryptiklemur
Copy link
Contributor

I think he closed it because it gets resolved by #11972

fabpot added a commit that referenced this issue Feb 8, 2017
…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
@dshiryaev-plesk
Copy link

Is there a way to do this now ?

@sstok
Copy link
Contributor

sstok commented Sep 12, 2017

You can use \Symfony\Component\Process\ProcessUtils::escapeArgument to escape the arguments, but that's deprecated 😐 OK...

You can always use a string, but you will need to manually escape the arguments.
https://gist.github.com/Zenexer/40d02da5e07f151adeaeeaa11af9ab36#sanitizing-input-for-safe-shell-parsing
https://github.com/johnstevenson/winbox-args

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 12, 2017

@sstok NOOOoooo :)
Do never manually escape args, that's a very bad practice.
Just use "prepared" command lines (>=3.3):

$p = new Process('cmd1 "$VAR1" | cmd2 "$VAR2"');
$p->run(null, array('VAR1' => 'value1', 'VAR2' => 'value2'));

@dshiryaev-plesk
Copy link

Thank you!

@sstok
Copy link
Contributor

sstok commented Sep 12, 2017

@nicolas-grekas Thanks for the tip 👍

@weitzman
Copy link

weitzman commented Dec 9, 2018

At least in 3.4, the 2nd argument to run() is an array of env variables, not replacements. Has the recommended approach changed?

@nicolas-grekas
Copy link
Member

Nope, replacement has always been preformed by the shell, using env vars yes.

@staabm
Copy link
Contributor

staabm commented May 10, 2019

@sstok NOOOoooo :)
Do never manually escape args, that's a very bad practice.
Just use "prepared" command lines (>=3.3):

$p = new Process('cmd1 "$VAR1" | cmd2 "$VAR2"');
$p->run(null, array('VAR1' => 'value1', 'VAR2' => 'value2'));

@nicolas-grekas this no longer works on symfony/process 4.x. strings are not longer allowed for the command - arrays are expected. how should those args be passed instead then?

@lyrixx
Copy link
Member

lyrixx commented May 10, 2019

$process = Process::fromShellCommandline('my_command "$MY_VAR"');
$process->run(null, ['MY_VAR' => $theValue]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests