Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

added CallbackStream #90

Merged
merged 3 commits into from
Nov 24, 2015
Merged

added CallbackStream #90

merged 3 commits into from
Nov 24, 2015

Conversation

oscarotero
Copy link
Contributor

Hello.
This is a implementation of Stream that uses a callback as described in #70
Maybe should be great to add support for generators, to allow doing thinks like this:

$stream = new CallbackStream(function () {
    $command = new Command();
    $command->run();

    while ($command->isRunning()) {
        yield $command->getOutput(); //returns always a string
    }
    return false; //end of the command
});

In this case, the emitter should iterate with the stream, instead return all content at once. I mean:

while (!$stream->eof()) {
    echo $stream->read(1);
};

This behaviour is not implemented (currently $stream->read() throws a RuntimeException and the callback can be executed only once), but I can do it if you like.

@weierophinney weierophinney added this to the 1.2.0 milestone Oct 15, 2015
@weierophinney weierophinney self-assigned this Oct 15, 2015
*/
public function getSize()
{
}
Copy link
Member

Choose a reason for hiding this comment

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

This is specified as MUST return an integer. Considering we just added support to the SapiEmitter to auto-emit a Content-Length header based on the return value of this method, it's even more critical.

This would likely require that you return the strlen() of the return of getContents() — which would mean you'll likely want to have that method cache the results of calling the callback.

Doing so would also allow you to do things like read(), tell(), etc, as these could call getContents() if the content is not yet cached, and then start operating on the cached value. That would also allow better flexibility with emitters, as they would not need to typehint against this stream type to determine how to send the stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The psr interface says:

/**
 * Get the size of the stream if known.
 *
 * @return int|null Returns the size in bytes if known, or null if unknown.
 */

So, it should be perfectly valid to return null. IMHO, this stream should not behave exactly like the standard stream (like you're proposing), because its goal is different. If I want to do what you say, there's no need of this class, because it can be done more easily:

$result = $callable();
$response->getBody()->write($result);

The purpose of the callbackStream is to provide a way to send the response content in real time, so, you don't know the size or the time it will take. For example:

$body = new CallbackStream(function () {
    echo 'The process is starting. Please, wait...';
    sleep(2);
    echo 'Running the process. Be patient, please';
    sleep(8);
    echo 'The process finished successfully';
});

@weierophinney
Copy link
Member

In addition to my above comments, please run ./vendor/bin/phpcbf locally, and commit the results; you have CS errors, as reported here: https://travis-ci.org/zendframework/zend-diactoros/jobs/77199455#L175

@weierophinney weierophinney merged commit 1c50f03 into zendframework:develop Nov 24, 2015
weierophinney added a commit that referenced this pull request Nov 24, 2015
weierophinney added a commit that referenced this pull request Nov 24, 2015
Since a stream's `getSize()` **CAN** return `null`, we need to **SKIP**
injection of the `Content-Length` header when it does. The
`CallbackStream` introduced in #90 leverages this feature, so we
**MUST** support it.
weierophinney added a commit that referenced this pull request Nov 24, 2015
weierophinney added a commit that referenced this pull request Nov 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants