[Process] Added support for stdout and stderr flush (Issue #7884) #8288
Conversation
|
I've submitted the doc changes for this PR. Now working on the failed test on travis. |
|
Hey, I like the feature, but I think adding more methods is not a good idea for the api. It results in What about adding a boolean |
|
Hi, I like your suggestion, and I'll modify the PR as soon as I can ;-) Thank you! |
|
In PHP, arguments as Boolean as hard to understand when using the code: |
|
I've updated the PR addresing your suggestion, Fabien. Thank you. |
|
|
||
| $p->run(); | ||
| $p->flushErrorOutput(); | ||
| $this->assertEquals('', $p->getErrorOutput()); |
There was a problem hiding this comment.
You're right @stloyd assertEmpty is more accurate. I'll change that.
| * | ||
| * @return Process | ||
| */ | ||
| public function flushOutput() |
There was a problem hiding this comment.
@fabpot shouldn't a flush method send all the buffers content and clear the buffer afterwards, intead of only clearing it? (This is what php does when flushing a buffer, isn't it?)
There was a problem hiding this comment.
send? We don't send the buffers anyway. It's up to you to get the buffer and do whatever you want with them.
There was a problem hiding this comment.
Let me say it in another way. I would not expect that a method called flushxxx() just clears the buffer. If so, this method should be named e.g. CleanBuffer() (to be consitent with php naming)
There was a problem hiding this comment.
In this context, I don't what it can do on top of removing the data from the buffer. See http://www.catb.org/jargon/html/F/flush.html
There was a problem hiding this comment.
Taken from your link
2. [Unix/C] To force buffered I/O to disk, as with an fflush(3) call. This is not an abort or deletion as in sense 1, but a demand for early completion!
Thats what I meant.. It is not delete. Flush is more like a stop now but process everything which is already there.
Since this is not the case here, flush is not the correct term in this situation.
There was a problem hiding this comment.
I read the whole thing ;) Actually, I could link to definition 1, which is exactly what we do here, but I won't. I do understand what you mean, except that it does not apply here as the Process class never output/send the buffers, so there are no ambiguities here.
There was a problem hiding this comment.
I agree that, given functionality of ob_flush(), which sends and cleans the buffer, the methods should be named differently. See #9407.
To-do
This PR introduces flushing methods for both stdout and stderr on Process class. The new methods are:
Tests for new methods are included on the PR.