Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing requirements for internal file pointers #1134

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions accepted/PSR-17-http-factory-meta.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,34 @@ the time this proposal was developed allowed a string URI when creating a
`UriInterface` implementation they provided. As such, accepting a string is
expedient and follows existing semantics.

### 5.7 Why are Streams created with the read/write pointer at the beginning?

The primary use case of `StreamFactoryInterface::createStream()` and
`StreamFactoryInterface::createStreamFromFile()` is to create streams for the
body of a `ResponseInterface` instance to be emitted as an HTTP response.

An emitter can make no assumptions about streams (or their internal resource
handles) being repeatable, and therefore can't simply `rewind()` the stream.

Even in the case where a stream returns `true` for `isSeekable()`, rewinding
the stream could have unacceptable performance implications. Similarly, the
`__toString()` method results in unacceptable memory overhead for large streams.

Consequently, to efficiently emit the body stream of a `ResponseInterface`
instance as body of an HTTP response, we must be able to make an assumption
about the stream pointer being positioned at the beginning of the stream.

As for `StreamFactoryInterface::createStreamFromResource()`, the caller is
responsible for the creation of the PHP resource to use as the basis for the
stream, and therefore also assumes responsibility for the read/write pointer
state of the resource and resulting stream.

And finally, the instances created by the `RequestFactoryInterface()`,
`ResponseFactoryInterface()` and `ServerRequestFactoryInterface` factories
mindplay-dk marked this conversation as resolved.
Show resolved Hide resolved
must generate instances that return an empty body-stream in read and write
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the wording could be improved here:

must generate instances that contain an empty StreamInterface, in read/write mode, as the body

Copy link
Contributor

Choose a reason for hiding this comment

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

The same goes for other places where this wording is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementations of PSR-7/17 should defer the StreamInterface instance creation until getBody() is called for the first time, since you may otherwise run out of OS file handles. (Linux has a limit of 1000 by default.)

So it doesn't have to "contain" a stream instance, and probably shouldn't - it just has to "return" one.

I had initially written "read/write" as you suggest, but changed it to "read and write" for clarity, since the "/" could be interpreted as "or", which would be incorrect.

But I will change the wording from "body-stream" to StreamInterface everywhere.

mode, so that client-code can start writing to the body stream without
manually needing to initialize anything else.

## 6. People

This PSR was produced by a FIG Working Group with the following members:
Expand Down Expand Up @@ -306,3 +334,23 @@ _**Note:** Order descending chronologically._
- [shadowhand Dependency Inversion and PSR-7 Bodies](http://shadowhand.me/dependency-inversion-and-psr-7-bodies/)
- [PHP-FIG mailing list thread discussing factories](https://groups.google.com/d/msg/php-fig/G5pgQfQ9fpA/UWeM1gm1CwAJ)
- [PHP-FIG mailing list thread feedback on proposal](https://groups.google.com/d/msg/php-fig/piRtB2Z-AZs/8UIwY1RtDgAJ)

## 9. Errata

This recommendation initially omitted the following requirements.

### 9.1 `StreamFactoryInterface::createStream()`

The state of the created temporary resource was unspecified - per section 5.7, the
requirement was added to position the temporary resource at the beginning of the stream.
Comment on lines +351 to +352
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my other comment, this should read:

Suggested change
The state of the created temporary resource was unspecified - per section 5.7, the
requirement was added to position the temporary resource at the beginning of the stream.
The state of the created temporary resource was unspecified - per section 5.7,
the recommendation was added to position the pointer consistent with an
`fwrite()` operation.


### 9.2 `StreamFactoryInterface::createStreamFromFile()`

The state of the created temporary resource was unspecified - per section 5.7, the
requirement was added to position the temporary resource at the beginning of the stream.

### 9.3 `StreamFactoryInterface::createStreamFromResource()`

The requirements did not specify whether this method might affect the state of the given
resource - per section 5.7, the requirement was added to clarify that this method MUST NOT
modify the position of the given resource.
Comment on lines +361 to +363
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The requirements did not specify whether this method might affect the state of the given
resource - per section 5.7, the requirement was added to clarify that this method MUST NOT
modify the position of the given resource.
The requirements did not specify whether this method might affect the state of the given
resource - per section 5.7, the recommendation was added to not modify the pointer position
of the given resource.

14 changes: 14 additions & 0 deletions accepted/PSR-17-http-factory.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ interface RequestFactoryInterface
}
```

The created instance MUST return an empty body-stream in read and write mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed elsewhere, the spec itself cannot be changed. We can only update the meta document with errata.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least as far as adding MUST statements go.

Perhaps it is acceptable to add SHOULD statements... @php-fig/secretaries ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the bylaws say so?

I think you should vote on an exemption here - the spec is considerably less useful without these amendments being visible where they should be. If an implementer has to dig portions of the spec out of the meta section (which isn't even in the same file) that's quite a serious problem.

If what you're saying is, this isn't an "amendment" and can't be considered "errata", then it sounds like we're back to the idea of deprecating PSR-17.

Given the fact that the current spec does not guarantee interoperable implementations, in my opinion, this is errata. The definition of errata in the bylaws is somewhat ambiguous.

As I've argued before, this isn't a "breaking change" in the sense that it's going back on something that was already specified - we aren't making changes versus the previous specification, we're adding something that was unspecified and is currently inconsistent among existing implementations.

I sincerely hope we can amend rather than putting the entire community through the painful process of replacing the entire standard with a new one?


### 2.2 ResponseFactoryInterface

Has the ability to create responses.
Expand All @@ -74,6 +76,8 @@ interface ResponseFactoryInterface
}
```

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

### 2.3 ServerRequestFactoryInterface

Has the ability to create server requests.
Expand Down Expand Up @@ -102,6 +106,8 @@ interface ServerRequestFactoryInterface
}
```

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

### 2.4 StreamFactoryInterface

Has the ability to create streams for requests and responses.
Expand All @@ -117,6 +123,9 @@ interface StreamFactoryInterface
* Create a new stream from a string.
*
* The stream SHOULD be created with a temporary resource.
*
* The stream MUST be created with the current position of the file read/write
* pointer at the beginning of the stream. (Errata 9.1)
Comment on lines +126 to +128
Copy link
Contributor

@shadowhand shadowhand Jan 26, 2022

Choose a reason for hiding this comment

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

@mindplay-dk on closer inspection, this one is not correct. Given that this is an "open + write" operation, it would be natural for the pointer to be at the end of the stream and there is no guarantee that the stream can be rewound, making it impossible to place the pointer at the beginning of the stream.

Instead, I believe this can be combined with the previous line and read:

The stream SHOULD be created with a temporary resource, and the file read/write pointer SHOULD be placed consistently with an fwrite() operation. (Errata 9.1)

Comment on lines 125 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The stream SHOULD be created with a temporary resource.
*
* The stream MUST be created with the current position of the file read/write
* pointer at the beginning of the stream. (Errata 9.1)
* The stream SHOULD be created with a temporary resource, and the file pointer SHOULD
* be placed consistently with an fwrite() operation. (Errata 9.1)

*
* @param string $content String content with which to populate the stream.
*/
Expand All @@ -130,6 +139,9 @@ interface StreamFactoryInterface
*
* The `$filename` MAY be any string supported by `fopen()`.
*
* The stream MUST be created with the current position of the file read/write
* pointer at the beginning of the stream. (Errata 9.2)
*
* @param string $filename The filename or stream URI to use as basis of stream.
* @param string $mode The mode with which to open the underlying filename/stream.
*
Expand All @@ -143,6 +155,8 @@ interface StreamFactoryInterface
*
* The stream MUST be readable and may be writable.
*
* The current read/write position of the given PHP resource MUST NOT be modified. (Errata 9.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/MUST/SHOULD/

*
* @param resource $resource The PHP resource to use as the basis for the stream.
*/
public function createStreamFromResource($resource): StreamInterface;
Expand Down