oscarotero / psr7-middlewares

[DEPRECATED] Collection of PSR-7 middlewares
MIT License
668 stars 56 forks source link

Pass the stream you're replacing to the `createStream` method. #42

Closed jasny closed 7 years ago

jasny commented 7 years ago

This method will try to use the same stream resource uri and will fall back to php://temp. The stream that is being replaced is also passed to the factory as third argument. This will allow the factory to create a new stream with the same class (or affect it in some other way).

oscarotero commented 7 years ago

Hi. Thank you for the contribution, but I don't understand what problem are your trying to fix with this. The reason of creating new streams is, precisely, to have a clean isolate stream. Creating a new stream using the same source makes no sense, but maybe I'm missing something.

jasny commented 7 years ago

I've choose to use a php://memory stream, use the middleware it becomes a php://temp. Usually that okay, but it's unexpected behavior.

You might also choose to stream to a file, rather than to php://temp in order to generate a static website.

$request = new ServerRequest();
$response = new Response(fopen($request->getUri()->getPath(), 'r+'));

Also a stream might provide additional functionality above just being a wrapper around a PHP stream. Checkout the Guzzle streams for instance. For the application to maintain it's basic behavior regardless of middleware, the factory needs to know which stream it's replacing.

Point is, this change provides enough information to the factory method, to let the application decide what kind of stream to create, rather than it being enforced by the middleware.

oscarotero commented 7 years ago

It's recommended php://temp instead php://memory because the first one uses temporary files if the amount of data hits a predefined size (usually 2Mb), so it's better to handle big files without hit the memory limit. more info

Anyway, the reasons to create new empty streams in these middlewares are:

In other words: creating new streams for each body modification keeps the body inmutable, avoiding accidental modifications in the original file. There're the ReadResponse and SaveResponse middlewares to read/save these streams into files.

jasny commented 7 years ago

I've removed the part where it automatically uses the same stream as the replacing stream and uses php://temp instead. I left the part that Stream that is replaced is passed as third argument to the factory, so you can use it if needed.

I hope this change addresses your concerns enough to accept the PR.

oscarotero commented 7 years ago

Ok, but just for curiosity, I still not understanding what's the point of this change. I mean, why I would want to do this?:

//Create a stream using the same url
$stream2 = new Stream($stream1->getMetadata('url'), 'r+');

//Write something
$stream2->write('foo');

If I could do simply this?:

//Rewind the stream to begining
$stream1->rewind();

//Write something
$stream1->write('foo');

Is there any use case in which I need two streams pointing to the same file?

jasny commented 7 years ago

Currently the middleware factory doesn't allow either, regardless of which is better or if it's a good thing in the first place. With this change, it would be possible to do either

Middleware::setStreamFactory(function ($file, $mode, Stream $replacing) {
    if ($replacing->getMetadata('url') === $file) {
        $replacing->rewind();
        return $replacing;
    }

    return new Stream($file, $mode);
});

Another use case is for introducing PSR-7 with into an old application that uses echo by using output buffer. For instance when moving from Slim 2 to Slim 3. You might do new Stream('php://output') and use the ob_*() functions in the factory.

oscarotero commented 7 years ago

Well, I'm not sure about this. No middleware provided here needs this, and the type of stream factory needed shouldn't be decided by the factory but by the middleware. Here's the includeResponse to include old applications into a middleware and capture the output to insert in the response. And I don't think is a good idea to use php://output inside a middleware. This should be handled by the emitter (like Diactoros does).

Anyway, if this does not change the behaviour of the current middlewares (I guess this is for a specific need), I'm willing to merge this.

jasny commented 7 years ago

great, thanks