Skip to content

[Process] Add support for streams as input #10934

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

Closed
wants to merge 2 commits into from

Conversation

romainneutron
Copy link
Contributor

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

Process::setStdin now accepts a stream as input.
I've also split windows and unix pipes management.

array('file', '/dev/tty', 'w'),
array('file', '/dev/tty', 'w'),
);
} elseif ($this->ptyMode && Process::isPtySupported()) {
Copy link
Member

Choose a reason for hiding this comment

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

just if as the previous if returns

@romainneutron
Copy link
Contributor Author

Updated, addressed @stof comments

* @param bool $blocking Whether to use blocking calls or not.
* @param bool $close Whether to close pipes if they've reached EOF.
*
* @return string An array of read data indexed by their fd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is string correct? the description tells otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

@romainneutron Can you fix this before I merge?

@romainneutron
Copy link
Contributor Author

PR has been updated, comments have been addressed

@fabpot
Copy link
Member

fabpot commented Jun 14, 2014

👍

@lyrixx
Copy link
Member

lyrixx commented Jun 15, 2014

Love that 👍

@stof
Copy link
Member

stof commented Jun 16, 2014

👍

I think the PipesInterface should be tagged as @internal though

@fabpot
Copy link
Member

fabpot commented Jun 16, 2014

@romainneutron Can you tag all internal classes to make it clear that BC rules do no apply to these classes?

@romainneutron
Copy link
Contributor Author

Classes have been tagged, PR has been rebased

@fabpot
Copy link
Member

fabpot commented Jun 17, 2014

Thanks @romainneutron.

@fabpot fabpot closed this in 3b4afe7 Jun 17, 2014
@stof
Copy link
Member

stof commented Jun 17, 2014

@romainneutron this should be documented (you omitted the "Doc PR" line in your summary btw)

@romainneutron
Copy link
Contributor Author

@stof I've opened an issue for this, see symfony/symfony-docs#3955

@romainneutron romainneutron deleted the process-pipes branch June 17, 2014 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants