Skip to content

Http redirects are not followed when using caching http client #38207

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.

Sign up for GitHub

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

Jump to bottom
Closed
dmolineus opened this issue Sep 16, 2020 · 2 comments
Closed

Http redirects are not followed when using caching http client #38207

dmolineus opened this issue Sep 16, 2020 · 2 comments
Labels
Bug HttpClient Status: Needs Review

Comments

@dmolineus
Copy link
Contributor

Symfony version(s) affected: ^4.3.0

Description

I use the http client for a GET request. By default an instance of Symfony\Component\HttpClient\HttpClient follows redirects (by default up to 20). When using the Symfony\Component\HttpClient\CachingHttpClient every redirect leads to an exception.

The issue is caused by the Symfony\Component\HttpKernel\HttpClientKernel which overrides the max_redirects option:

symfony/src/Symfony/Component/HttpKernel/HttpClientKernel.php

Lines 53 to 57 in 9c8cd08

$response = $this->client->request($request->getMethod(), $request->getUri(), [
'headers' => $headers,
'body' => $body,
'max_redirects' => 0,
] + $request->attributes->get('http_client_options', []));

How to reproduce

<?php

$client = Symfony\Component\HttpClient\HttpClient::create();
$cachingClient = new Symfony\Component\HttpClient\CachingHttpClient(
    $client,
    new Symfony\Component\HttpKernel\HttpCache\Store('/path/to/the/cache') 
);

// Work as expectede
$client->request(
    'GET', 
    // Redirect to https://api.github.com/repositories/109533757/git/matching-refs/tags/ happens here
    'https://api.github.com/repos/humbug/box/git/matching-refs/tags/', 
    ['headers' => ['Authorization' => 'token XXX']
);

// Throws Symfony\Component\HttpClient\Exception\RedirectionException
$cachingClient->request(
    'GET', 
    // Redirect to https://api.github.com/repositories/109533757/git/matching-refs/tags/ happens here
    'https://api.github.com/repos/humbug/box/git/matching-refs/tags/', 
    ['headers' => ['Authorization' => 'token XXX']
);

Additional context
@dmolineus dmolineus added the Bug label Sep 16, 2020
dmolineus added a commit to phpcq/repository-builder that referenced this issue Sep 16, 2020
@dmolineus
Follow redirects from redirect exceptions
b4d798d 
Redirects are not handled by the cached http client, see
symfony/symfony#38207
@nicolas-grekas
Copy link
Member

This line should be removed, it's a left over from BrowseKit.
PR welcome, branch 4.4 please.

@dmolineus dmolineus mentioned this issue Sep 16, 2020
Merged
@dmolineus
Copy link
Contributor Author

I checked the browser kit, it forces max_redirects=0, it seems to be save to remove it:

symfony/src/Symfony/Component/BrowserKit/HttpBrowser.php

Lines 50 to 54 in 237d91f

$response = $this->client->request($request->getMethod(), $request->getUri(), [
'headers' => array_merge($headers, $extraHeaders),
'body' => $body,
'max_redirects' => 0,
]);

@fabpot fabpot closed this as completed Sep 17, 2020
fabpot added a commit that referenced this issue Sep 17, 2020
@fabpot
bug #38212 [HttpKernel] Do not override max_redirects option in HttpC…
2f67bf3 
…lientKernel (dmolineus)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

[HttpKernel] Do not override max_redirects option in HttpClientKernel

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

As [proposed](#38207 (comment)) by @nicolas-grekas this pull request removes the `max_redirects` setting in the `\Symfony\Component\HttpKernel\HttpClientKernel::handle` method.

It solves the issue that requests made by the `\Symfony\Component\HttpClient\CachingHttpClient` wouldn't follow redirects as described in the linked issue.

Commits
-------

981a11b [HttpKernel] Do not override max_redirects option in HttpClientKernel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @nicolas-grekas @dmolineus @xabbuh @carsonbot