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 all 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
55 changes: 55 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,41 @@ 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 specified read/write pointer positions?

The `StreamFactoryInterface::createStreamFromFile()` method implementation
typically maps directly to an `fopen()` call and, in terms of the read/write
pointer position, should behave consistently with `fopen()` with regards to
the `$mode` argument, as covered by the
[PHP manual page](http://php.net/manual/en/function.fopen.php).

The primary use case for `StreamFactoryInterface::createStream()` is to
create streams for the body of a `ResponseInterface` instance to be emitted
as an HTTP response. It can also be used to create an empty stream, which
can subsequently be written to.

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
must generate instances that return an empty `StreamInterface` instance in
read and write 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 +341,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 resource consistently with `fopen()`.
Comment on lines +356 to +357
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 state of the created temporary resource was unspecified - per section 5.7, the
requirement was added to position the resource consistently with `fopen()`.
The state of the created temporary resource was unspecified - per section 5.7,
the recommendation was added to position the pointer of the resource consistently
with `fopen()`.


### 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.

15 changes: 15 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 `StreamInterface` instance in read and write mode.

### 2.2 ResponseFactoryInterface

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

The created instance MUST return an empty `StreamInterface` instance 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 `StreamInterface` instance 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,10 @@ 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 positioned consistently with that of `fopen()` with regards to
* the `$mode` flag. (Errata 9.2)
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 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 +156,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