Skip to content
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

[HttpClient] Fix seeking in not-yet-initialized requests #47808

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Reported on Slack by @drupol

@drupol
Copy link
Contributor

drupol commented Oct 7, 2022

Oh ! Going to test this ASAP ! Thanks @nicolas-grekas !

@drupol
Copy link
Contributor

drupol commented Oct 7, 2022

I tested it and it works, however, there is still an issue in our app.

This is because of PsrHttpFactory.php#L130. I have the feeling that it also need some adaptations.

@drupol
Copy link
Contributor

drupol commented Oct 7, 2022

I made further test, but I'm still unable to get the body of a response twice. I'm also unable to reproduce the issue in a few lines of code.

I made a screenshot after adding some dump and dd in Psr18Client::sendRequest(), without changing anything else, as such:

   /**
     * {@inheritdoc}
     */
    public function sendRequest(RequestInterface $request): ResponseInterface
    {
        try {
            $body = $request->getBody();

            if ($body->isSeekable()) {
                $body->seek(0);
            }

            $response = $this->client->request($request->getMethod(), (string) $request->getUri(), [
                'headers' => $request->getHeaders(),
                'body' => $body->getContents(),
                'http_version' => '1.0' === $request->getProtocolVersion() ? '1.0' : null,
            ]);

            $psrResponse = $this->responseFactory->createResponse($response->getStatusCode());

            foreach ($response->getHeaders(false) as $name => $values) {
                foreach ($values as $value) {
                    try {
                        $psrResponse = $psrResponse->withAddedHeader($name, $value);
                    } catch (\InvalidArgumentException $e) {
                        // ignore invalid header
                    }
                }
            }

            if ($response instanceof StreamableInterface) {
                dump('Response is StreamableInterface', $response);
                $body = $response->toStream(false);
            } else {
                dump('Response is not StreamableInterface', $response);
                $body = StreamWrapper::createResource($response, $this->client);
            }

            $body = $this->streamFactory->createStreamFromResource($body);

            $r = $psrResponse->withBody($body);

            dump((string)$r->getBody());
            dd((string)$r->getBody());

            return $r;
        } catch (TransportExceptionInterface $e) {
            if ($e instanceof \InvalidArgumentException) {
                throw new Psr18RequestException($e, $request);
            }

            throw new Psr18NetworkException($e, $request);
        }
    }

And here's the result. Sorry for the pixelisation of the information, but they are not relevant here.

screenshot-127 0 0 1_8000-2022 10 07-23_45_48

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 7, 2022 via email

@drupol
Copy link
Contributor

drupol commented Oct 7, 2022

I've spent the whole evening trying to reproduce it, without luck. I'll try again this weekend.

@drupol
Copy link
Contributor

drupol commented Oct 9, 2022

What I propose is to merge this PR, then I'll open another issue for the rest. WDYT ?

@nicolas-grekas nicolas-grekas merged commit aea3499 into symfony:4.4 Oct 11, 2022
@nicolas-grekas nicolas-grekas deleted the hc-stream-seek branch October 17, 2022 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants