replace hard locking to Guzzle Stream with PSR StreamInterface - #581
Conversation
Guzzle::streamFor() already accepts StreamInterface and does not require Guzzle Stream.
|
|
|
First, thanks for taking time to improve this library Jan-Sverre Riksfjord (@jasverix)! 🎉 A pointer: However, we can still discuss the issue you're facing here... The getters currently return a Psr StreamInterface impl (albeit Guzzle Stream) in all cases I've thought of i.e. GuzzleHttp\Psr7\Utils::streamFor() returns a Guzzle Stream which is Psr compliant Only issue I see is calling the setter with a Nyholm stream and calling the getter returning a Guzzle stream. Is there any other way that using the Nyholm stream implementation doesn't work with this library? I agree with changing our PHPDocs to StreamInterface (both setters and getters) to make things clearer and agree with the if-statement type check changing to StreamInterface |
|
Hi, I might have overreacted here. My problem is the attachBody method in the GraphRequest class. I can update this PR and keep only that change. |
This reverts commit a3d4925
|
I have now fixed the branch. I just did a big replace operation, but my problem is the |
Philip Gichuhi (Ndiritu)
left a comment
Philip Gichuhi (Ndiritu)
left a comment
There was a problem hiding this comment.
Looks good.
| "guzzlehttp/guzzle": "^6.0 || ^7.0", | ||
| "ext-json": "*" | ||
| "ext-json": "*", | ||
| "psr/http-client": "^1.0" |
There was a problem hiding this comment.
StreamInterface is defined in psr/http-message
Can we opt to let guzzlehttp/psr7 that is included implicitly with guzzlehttp/guzzle determine the version of psr/http-message that is included?
There was a problem hiding this comment.
Changed to correct package now.
How can we let guzzle determine the version? Right now, we have the exact same version query as guzzle (^1.0).
There was a problem hiding this comment.
I can use a string, as you did before, instead of the ::class constant, and remove the package requirement, if you want.
I just, in general, personally like code better than strings, but it will do no big difference in this case.
There was a problem hiding this comment.
I can use a string, as you did before, instead of the
::classconstant, and remove the package requirement, if you want.I just, in general, personally like code better than strings, but it will do no big difference in this case.
I also prefer the ::class constant. We'll be removing the string in v2.
There was a problem hiding this comment.
Changed to correct package now.
How can we let guzzle determine the version? Right now, we have the exact same version query as guzzle (^1.0).
Thanks for making the change. I agree with setting psr/http-message as a direct dependency instead of letting guzzle define this.
We've had a running discussion on whether to explicitly set guzzlehttp/psr7 & in this case psr/http-message as our direct dependency.
Inviting thoughts from silaskenneth (@SilasKenneth) Michael Mainer (@MIchaelMainer) Mustafa Zengin (@zengin)
3 participants
Guzzle::streamFor() already accepts StreamInterface and does not require Guzzle Stream.
We are using another provider (nyholm) for streams in our project and the requirement for Guzzle Stream is really a blocking issue for us. And we cannot see any reason for that, because Guzzle itself only requires StreamInterface.
Microsoft Reviewers: Open in CodeFlow