Fix react/http-client 0.4.13 response body issue #18

shulard · 2016-10-24T14:06:10Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Related tickets	fixes #17
Documentation
License	MIT

What's in this PR?

This PR has been pushed to fixed the compatibility error with react/http-client v0.4.13.

Why?

There was a break change published in v0.4.13 around response body type. Before the first response body was sent as a StreamInterface instance, now it's "just" string.

Before we are using that StreamInterface instance as $bodyStream... Now it has been updated to use the Http\Message\StreamFactory to build a valid StreamInterface instance.

If a BC Break because I've introduced a new constructor parameter to be able to inject StreamFactory instance...

Of course the Http\Discovery\StreamFactoryDiscovery object is used when parameter is empty.

With this change, I've tested on lowest dependencies and it works too.

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

To Do

Validate PR approach...
Create a release with a composer update to disallow react/http-client > 0.4.12 ?

joelwurtz · 2016-10-24T15:10:59Z

src/Client.php

     * @param ReactClient|null     $client
     */
    public function __construct(
        ResponseFactory $responseFactory = null,


This is BC Break, you should add it to the last parameter of the constructor, it would still be BC break but less given people may use the discovery system.

I know this is BC Break but seems more "logic" to add it just after the ResponseFactory... I think more persons will use a custom ResponseFactory / StreamFactory than a custom Loop or Client...

joelwurtz · 2016-10-24T15:18:53Z

If we use the StreamFactory as the last parameter of the constructor and we add php-http/message to the dependencies tree there will be no BC break, as react/http-client already include guzzle/psr7 implementation and this library has a dependency on php-http/discovery then you will always have a StreamFactory service even if it was not injected.

shulard · 2016-10-24T15:20:22Z

Ok 😅 so I'll update my code!

joelwurtz · 2016-10-24T15:38:18Z

LGTM,

@dbu @sagikazarmark Ok for you ? IMO there is no bc break here.

dbu · 2016-10-24T15:50:13Z

src/Client.php

+            $bodyStream = $this->streamFactory->createStream();
            $reactResponse->on('data', function ($data) use (&$bodyStream) {
                if ($data instanceof StreamInterface) {
-                    $bodyStream = $data;


could this have side effects? maybe on performance when we have large streams?

You're right, this mean load the whole string in memory... Maybe I can use the incoming stream as $bodyStream like before ? This is just a compatibility statement with react < 0.4.13.

Not to mention getContents reads the remainder, not the whole stream

Maybe I can use the incoming stream as $bodyStream like before ?

I think this will be very hard :/. There is no way to avoid memory problem here as react want us to work with data and not stream so the react interface block us from doing anything.

The only way to do that is to be sure that we create a stream using php://temp by example (not in memory, or at least not to much) and then read -> write by chunk of specified size (8192 is the best as it's the default chunk for PHP so there is no internal buffer).

But i don't think we should do this as the last version of react only give us data as a string and not a StreamInterface so this would be an enhancement for old version (so when people will update this lib with the enhancement they will certainly also update react/http-client which does not provide a StreamInterface anymore, so this enhancement will never be used...).

Not to mention getContents reads the remainder, not the whole stream

👍 We should only cast the stream to string $bodyStream->write((string) $data); in the same way that react has update it's library.

In fact you could remove the condition and only do :

$bodyStream->write((string) $data);

sagikazarmark · 2016-10-24T21:52:30Z

As long as the comment above is resolved OK for me

shulard · 2016-10-25T09:05:48Z

I've updated the code with string cast, much simpler 😄.

Nyholm · 2016-10-25T09:21:07Z

composer.json

        "react/stream": "^0.4.3",
-        "php-http/discovery": "^1.0"
+        "php-http/discovery": "^1.0",
+        "php-http/message": "^1.0"


Why is php-http/message added?

This library now require a StreamFactory to work which is added in the constructor, but it can be set to null and use the discovery system to work.

So if we keep it like that, this will be BC for people setting all the contrusctor parameters in order to avoid using Discovery, as the new parameter cannnot be found.

However react/http-client depends on guzzlehttp/psr7 so by adding php-http/message we are sure to found a service for the StreamFactory in all cases so there is no BC.

Okey, I've missed that discussion.
I agree. But I would like to remove that line before tagging 1.0 (and preferably before tagging 0.3.0).

So you want that I remove that line in this PR ?

No.
if this PR does not break BC then I'm happy leaving this line. But it should be removed next time we release a BC breaking change.

Nyholm · 2016-10-25T09:22:08Z

src/Client.php

-                    $bodyStream = $data;
-                } else {
-                    $bodyStream->write($data);
-                }


Code style: (string) $data

Before the first body received was a StreamInterface then a string… Now it’s always a string. StreamFactory discovery have been introduced to handle stream creation. Also require “php-http/message” as dev dependency to get StreamFactory object.

shulard · 2016-10-26T10:05:00Z

Hello !

To address #20 in the same time (and avoid adding / moving the line in two different commits) I've already moved php-http/message dependency to require-dev section.

I also added a changelog update. If everyone is OK, I'll merge the PR asap.

shulard force-pushed the issue/17 branch 2 times, most recently from 4476a7a to 7a3dc7b Compare October 24, 2016 14:12

joelwurtz reviewed Oct 24, 2016

View reviewed changes

shulard force-pushed the issue/17 branch from 7a3dc7b to 2fdcfa4 Compare October 24, 2016 15:25

dbu reviewed Oct 24, 2016

View reviewed changes

shulard force-pushed the issue/17 branch from 2fdcfa4 to 4f7aec9 Compare October 25, 2016 09:03

shulard force-pushed the issue/17 branch from 4f7aec9 to 76ee001 Compare October 25, 2016 09:06

Nyholm requested changes Oct 25, 2016

View reviewed changes

Nyholm approved these changes Oct 25, 2016

View reviewed changes

shulard force-pushed the issue/17 branch from 76ee001 to e314edf Compare October 25, 2016 11:30

shulard self-assigned this Oct 26, 2016

shulard force-pushed the issue/17 branch from e314edf to 1b7751a Compare October 26, 2016 09:58

shulard force-pushed the issue/17 branch from 1b7751a to 180fb98 Compare October 26, 2016 09:58

Add unreleased change to CHANGELOG.md

6707576

shulard merged commit 104f3c4 into php-http:master Oct 31, 2016

shulard mentioned this pull request Oct 31, 2016

Remove dependency on php-http/message #20

Closed

shulard deleted the issue/17 branch December 17, 2020 16:16

Fix react/http-client 0.4.13 response body issue #18

Fix react/http-client 0.4.13 response body issue #18

Uh oh!

Conversation

shulard commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in this PR?

Why?

Checklist

To Do

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelwurtz commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shulard commented Oct 24, 2016

Uh oh!

joelwurtz commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelwurtz Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Oct 24, 2016

Uh oh!

shulard commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelwurtz Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shulard commented Oct 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shulard commented Oct 24, 2016 •

edited

Loading

joelwurtz commented Oct 24, 2016 •

edited

Loading

joelwurtz Oct 25, 2016 •

edited

Loading

joelwurtz Oct 25, 2016 •

edited

Loading