[Process] Add InputStream to seamlessly feed running processes #18386
Conversation
88f45f5 to
936cc1a
Compare
|
|
||
| public function __construct(callable $onEmpty = null) | ||
| { | ||
| $this->onEmpty($onEmpty); |
There was a problem hiding this comment.
Why calling method instead of directly the variable?
There was a problem hiding this comment.
because the class could be extended
There was a problem hiding this comment.
in fact, I removed the constructor
87cc4f8 to
547862c
Compare
| } | ||
|
|
||
| /** | ||
| * Tells is write buffer is closed or not. |
547862c to
dd04b23
Compare
|
comments addressed |
dd04b23 to
5255acc
Compare
b28901d to
fd984ef
Compare
|
This looks awesome and the implementation is quite elegant, I hope this will be merged soon! |
|
👍 However, it definitely needs a documentation PR associated |
|
@romainneutron doc PR linked |
| } | ||
|
|
||
| /** | ||
| * Appends an input to the write buffer. |
There was a problem hiding this comment.
missing phpdoc for the argument type
There was a problem hiding this comment.
fd984ef to
40f0f10
Compare
| if (!is_scalar($input)) { | ||
| throw new InvalidArgumentException(sprintf('%s yielded a value of type "%s", but only scalars and stream resources are supported', get_class($this->input), gettype($input))); | ||
| } | ||
| $input = (string) $input; |
There was a problem hiding this comment.
I'm not too familiar with streams, but the exception makes me believe that a stream will be casted to a string here.
$ php -r '$f = fopen("composer.json", "r"); echo (string) $f;'
Resource id #5
Is this what it's intended to to or am I missing something?
There was a problem hiding this comment.
you're missing #18386 (diff)
There was a problem hiding this comment.
Ah right, that explains the "resource" part of the exception, gotcha!
40f0f10 to
3d20b6c
Compare
|
Thank you @nicolas-grekas. |
…rocesses (nicolas-grekas) This PR was merged into the 3.1-dev branch. Discussion ---------- [Process] Add InputStream to seamlessly feed running processes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18262 | License | MIT | Doc PR | symfony/symfony-docs#6424 Look at the tests, beautiful, isn't it? Commits ------- 3d20b6c [Process] Add InputStream to seamlessly feed running processes
… (nicolas-grekas) This PR was merged into the 3.1-dev branch. Discussion ---------- [Process] Implement IteratorAggregate to stream output | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#6424 Sibling to #18386 for iterating of the output streams. Commits ------- 5947f5d [Process] Implement IteratorAggregate to stream output
Look at the tests, beautiful, isn't it?