search

Nyholm / psr7

A super lightweight PSR-7 implementation
MIT License
1.16k stars 75 forks source link

Wrap fopen for PHP 8 #174

Closed Zegnat closed 3 years ago

Zegnat commented 3 years ago

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 the ValueError 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!

Nyholm commented 3 years ago

FYI: @GrahamCampbell did a similar PR on Guzzle: https://github.com/guzzle/psr7/pull/375

Zegnat commented 3 years ago

Looks like Guzzle has always had an abstraction for fopen() while we use it immediately. They always caught errors already, while we supress them and instead count on false being returned. I am not sure what the best way to go is.

I found a total of 4 instances of fopen() in our code, and would prefer to wrap them on a case-by-case basis as I have done in this PR, rather than abstracting the whole function. Especially as one of the four is inside Stream::create, not part of the PSR-7 specification, and is allowed to throw a ValueError. Leaves only three cases to address.

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.

GrahamCampbell commented 3 years ago

I think it would be worth just copying Guzzle's tryFopen function: https://github.com/guzzle/psr7/blob/d7fe0a0eabc266c3dcf2f20aa12121044ff196a4/src/Utils.php#L343-L389.

Zegnat commented 3 years ago

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 \fopen. I have also marked this method as @internal to steer end-users towards the actual factories.

nickdnk commented 3 years ago

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.

nickdnk commented 3 years ago

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

GrahamCampbell commented 3 years ago

Not forgotten. Intentionally not released yet. Pending https://github.com/Nyholm/psr7/pull/179 and possibly some other fixes before the release.