[PSR 17] Define how the stream is created in http messages

[Oscar Otero]

Hi.

In this example:

$response = $responseFactory->createResponse(200);

$body = $response->getBody();
$body2 = $streamFactory->createStream();

I've always assumed that $body and $body2 were empty writable streams, so it was safe to write in them. But PSR-17 says nothing about how the streams must be created in new http messages (request, serverRequest and responses).

This problem appeared here https://github.com/middlewares/error-handler/pull/5/files#r227117082 and forces to always require a streamFactory along with a responseFactory any time that you have to create a response in a middleware.

I think this makes the responseFactory less useful and reliable, forcing to require more dependencies just in case.

So I propose to include in the spec a text indicating that the stream created in the responseFactory, requestFactory and serverRequestFactory should be the same than if they were created with streamFactory->createStream()

What do you think?

Oscar Otero.

[Martijn van der Ven]

So I propose to include in the spec a text indicating that the stream created in the responseFactory, requestFactory and serverRequestFactory should be the same than if they were created with streamFactory->createStream()

Somehow I missed this thread completely, but made the same proposal as part of the other ongoing StreamFactory thread.

I think it makes sense to codify this as part of a PSR, as I have seen it used in many different projects. It may even be necessary to specify that StreamFactoryInterface::createStream() leads to a writeable Stream, currently it is not described as such.

Cheers,

Martijn

[Rasmus Schultz]

I think this problem can be traced back to PSR-7, which doesn't specify anything about constructors - e.g. doesn't specify anything about the initial state of the models for which it defines abstractions.

You could argue PSR-7 shouldn't deal with creation and should only define the behavior of the methods it defines.

I suppose I'd tend to agree - we have PSR-17 that defines details about creation.

The conundrum, however, is that PSR-7 existed long before PSR-17, and still exists as an independent spec - which, if it doesn't deal with creation, arguably defines something that isn't really meaningful or complete in itself.

Anyhow, if we can let go of the past (and ignore reality for a bit) and pretend that PSR-7 is just sort of a sub-component of a more complete PSR-17 standard, I guess I'd say we need to further amend PSR-17 to address this.

Not sure if you saw my current PR here:

https://github.com/php-fig/fig-standards/compare/master...mindplay-dk:stream-pointer-state

I guess I'd propose the following addition to each of Section 2.1, 2.2 and 2.3:

"Implementations of this interface MUST initialize the body stream with an empty temp-stream in read/write mode, e.g. fopen('php://temp', 'rw+')"

And then of course expanding on the errata section with a short rationale.

Thoughts?

[Lcfvs]

Le lundi 28 janvier 2019 12:57:49 UTC+1, Rasmus Schultz a écrit :

"Implementations of this interface MUST initialize the body stream with an empty temp-stream in read/write mode, e.g. fopen('php://temp', 'rw+')"

 Like I already said on this group, open a stream, by default, for everything like this increases the risk to reach the "Fatal Error - Too many open files".

I strongly disagree that, please don't make it a requirement/recommandation!

[Rasmus Schultz]

Yikes, I didn't know about this issue.

To others who don't know what this is about, this thread has some information:

https://github.com/guzzle/guzzle/issues/1927

TL;DR Linux has a default limit of 1024 open file handles at the OS level.

Okay, but...

This can't become a choice between an incomplete model or a model that can't scale. Both are unacceptable.

The only sensible recourse I can think of, is we need special TempStream implementation that doesn't open an actual file handle until you write to it.

I think we could put this as a recommendation in the PSR? It's an easy work around.

Another possible work around is a custom stream implementation that uses a simple string as buffer. 

Of course, this approach only works up to a certain file size, but you could further optimize by switching to a temp-stream when the buffer hits a certain size limit or somebody tries to detach the resource handle.

Yet another option is to lazy initialize in getBody() ...

So there are ways to work around this.

I don't think returning an incomplete, unusable model an acceptable solution.

For one, the type-hint of getBody() is StreamInterface, it's not StreamInterface|null - so you have to return an instance or you're not complying with the original spec in the first place.

This is a lot of information to put in an amendment though 🤔

--
You received this message because you are subscribed to a topic in the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/php-fig/mv9-MWDVVBM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/7123beae-3ac4-4062-b4dc-bb29effa90fa%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Rasmus Schultz]

How about this wording?

"The created instance MUST return a body an empty body-stream in read/write mode (e.g. `fopen('php://temp', 'rw+')`) and SHOULD defer the creation of body-streams to conserve system resources."

Under all three sections.

And then a short section in the meta-document summarizing why and how to defer the creation of file-system handles.

Sound good?

[Oscar Otero]

Hi. To me, this is a problem of PSR-17, not PSR-7.

What I'm asking is being sure that whenever I create a new response, I can write the body:

$response = $factory->createResponse();

$response->getBody()->write('Ok');

Errors like "Fatal Error - Too many open files" are out of scope because it's an implementation detail. We can create an implementation not using streams at all (just use a variable containing the string). The spec should focus only in the fact that, any new stream created with PSR-17 (directly using $streamFactory->createStream() or though a request or response message) must be empty and writable.

You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.

To post to this group, send email to php...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CADqTB_i8MZoNLDBhLQcmHpT4h6HbRyks%3D227h9BdiHKjstVrhQ%40mail.gmail.com.

[Rasmus Schultz]

> The spec should focus only in the fact that, any new stream created with PSR-17 (directly using $streamFactory->createStream() or though a request or response message) must be empty and writable.  

Okay, so just this then?

"The created instance MUST return an empty body-stream in read/write mode."

(please suggest exact wording if you disagree.)

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CALrFU9WaL-jXvguq8Fu0Hh%3DtuwKN%3DDfK4tt2_64JEwWc6%3DNhrQ%40mail.gmail.com.

[Oscar Otero]

Yes, I think that expresses it very well.

To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/CADqTB_g3TRuJk28CYiDJTBJS3z5FJvMyAS464VGqYjPqk%2BRODw%40mail.gmail.com.

[Rasmus Schultz]

I've updated my PR with this commit:

https://github.com/php-fig/fig-standards/pull/1134/commits/983002eb09fb961bccb0c77250e39af498c313be

Thanks for bringing this up, Oscar! Glad you caught this :-)