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.

Sign up for GitHub

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

add missing requirements for internal file pointers #1134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
322c2d4
add missing requirements for internal file pointers
mindplay-dk Jan 11, 2019
740fa21
add errata section
mindplay-dk Jan 13, 2019
983002e
add missing requirements for body-streams in generated Message instan…
mindplay-dk Jan 30, 2019
79afc63
"body stream" -> `StreamInterface` instance
mindplay-dk Jan 30, 2019
4d58ada
`Interface()` -> `Interface`
mindplay-dk Jan 31, 2019
e2ea01a
`createStreamFromFile()` should behave like `fopen()`
mindplay-dk Feb 20, 2019
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
6 changes: 6 additions & 0 deletions accepted/PSR-17-http-factory-meta.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,12 @@ 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
6 changes: 6 additions & 0 deletions accepted/PSR-17-http-factory.md
View file
Open in desktop
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 Down