Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
add missing requirements for internal file pointers #1134
Changes from 3 commits
322c2d4
740fa21
983002e
79afc63
4d58ada
e2ea01a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 untilgetBody()
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.There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/MUST/SHOULD/