Skip to content

#10025 add pipe method to ProcessBuilder #11971

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

#10025 add pipe method to ProcessBuilder #11971

wants to merge 4 commits into from

Conversation

piotrplenik
Copy link

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10025
License MIT
Doc PR

@piotrplenik piotrplenik mentioned this pull request Sep 21, 2014
Closed
jakzal
jakzal reviewed Sep 21, 2014
View reviewed changes
src/Symfony/Component/Process/ProcessBuilder.php
$arguments = array_merge($this->prefix, $this->arguments);
$script = implode(' ', array_map(array(__NAMESPACE__.'\\ProcessUtils', 'escapeArgument'), $arguments));
$script = implode(' ', array_map(array(__NAMESPACE__ . '\\ProcessUtils', 'escapeArgument'), $arguments));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted.

@romainneutron romainneutron mentioned this pull request Sep 21, 2014
Closed
@romainneutron
Copy link
Contributor

See my proposition in #11972

@piotrplenik
Copy link
Author

@jakzal Thanks for comments. All corrected.

@fabpot fabpot added the Process label Sep 22, 2014
@romainneutron
Copy link
Contributor

Hello @jupeter and thanks for your work.
I'm definitely 👎 on this proposition:

  • Piping should not be done at Process level. Process relies on a command, working directory and is about how to run the command on system. Pipe is a word in the command
  • Piping should not take a ProcessBuilder as argument. ProcessBuilder is a factory for Process. It is semantically wrong.

I do think we should decouple Process and Command� to allow such features.
For this reason, I close this PR in favor of #11972

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@piotrplenik @romainneutron @jakzal @fabpot