-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Wrap fopen for PHP 8 #174
Wrap fopen for PHP 8 #174
Conversation
FYI: @GrahamCampbell did a similar PR on Guzzle: guzzle/psr7#375 |
Looks like Guzzle has always had an abstraction for I found a total of 4 instances of Looks like all tests pass, and I don’t really think we want to (or need to) add tests of our own beside the integration tests? Marking this as ready for review now. |
src/Stream.php
Outdated
@@ -61,6 +61,7 @@ private function __construct() | |||
* @param string|resource|StreamInterface $body | |||
* | |||
* @throws \InvalidArgumentException | |||
* @throws \ValueError |
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.
We should have a consistent API across all PHP versions. The internals of fopen changing should not effect the public API of this class.
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.
This can be removed, right?
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.
Theoretically \fopen('php://temp', 'rw+');
could throw a ValueError
. I have been thinking about how I might be able to use the Stream::createFromFile
for it as a refactor. Otherwise we need to wrap it in a try {} catch {}
.
I say theoretically because I would find it very counter-intuitive for PHP to throw a ValueError
on values that are internal to PHP.
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 guess we can actually make it so there are no extra fopen
s in the exact same amount of code with something like:
@@ -69,12 +69,6 @@ class Stream implements StreamInterface
return $body;
}
- if (\is_string($body)) {
- $resource = \fopen('php://temp', 'rw+');
- \fwrite($resource, $body);
- $body = $resource;
- }
-
if (\is_resource($body)) {
$new = new self();
$new->stream = $body;
@@ -86,6 +80,13 @@ class Stream implements StreamInterface
return $new;
}
+ if (\is_string($body)) {
+ $stream = self::createFromFile('php://temp', 'rw+');
+ $stream->write($body);
+
+ return $stream;
+ }
+
throw new \InvalidArgumentException('First argument to Stream::create() must be a string, resource or StreamInterface.');
}
I think it would be worth just copying Guzzle's tryFopen function: https://github.com/guzzle/psr7/blob/d7fe0a0eabc266c3dcf2f20aa12121044ff196a4/src/Utils.php#L343-L389. |
I am still divided on taking the Guzzle approach wholesale. At that point I would rather people just go and use the Guzzle implementation. That is the strength of interoperability interfaces, after all. I don’t think we should be adding utility classes/methods. Current middle of the road solution: creating a new static factory method on our Stream implementation that handles it. This also lets us minimise some code in other places and centralises us to only a single file that uses |
src/Stream.php
Outdated
* | ||
* @internal Implements Psr\Http\Message\StreamFactoryInterface::createStreamFromFile for internal use, library users should default to the actual Factory object | ||
*/ | ||
public static function createFromFile(string $filename, string $mode = 'r'): StreamInterface |
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.
If we kept this code in the factory and only surrounded the other two fopen
with try/catch
, wouldn't achieve the same thing and allow us to not have this method?
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.
This same method is used by moveTo
and getStream
from FileUpload
. Because they too create a Stream
instance from a path. Not sure wrapping all of them actually helps.
cloc src
shows that this patch only increased out code from 1007 to 1014. So probably no more or less than wrapping everything by itself. And this way we have it centralised. But I don’t feel strongly either or.
Hello guys Is something currently holding this PR back? This package is now the only one that prevents me from running/testing my app on PHP 8. |
682b9fb
to
2f4d5b5
Compare
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.
Thank you.
I added a try/catch to avoid adding a new public method.
So it turns out it was psr7-server and not this one that held me back. I updated that to 1.0.x and everything is good (semantic beta-versioning pulling pranks on me). However, 1.4.1 from this package is not on Packagist, so I was just wondering if you forgot to tag it? It's on the changelog: https://github.com/Nyholm/psr7/blob/master/CHANGELOG.md - but not here: https://github.com/Nyholm/psr7/releases |
Not forgotten. Intentionally not released yet. Pending #179 and possibly some other fixes before the release. |
fopen()
has new behaviour where it will throw an error in PHP 8. Some projects have bumped into this, e.g. Drupal and the HTTP Factory tests.This PR wraps our uses of
fopen()
to not leak theValueError
to library users.Marking as WIP as I haven’t looked at tests yet. But have a look and see if these changes seem right!