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

Retain safe request method when following redirects #2047

Merged
merged 2 commits into from Jun 27, 2020
Merged

Retain safe request method when following redirects #2047

merged 2 commits into from Jun 27, 2020

Conversation

webignition
Copy link
Contributor

TLDR
A solution to #2046, ensures HEAD request method is retained when following 301 redirects.

Overview
RetryMiddleware::modifyRequest() modifies the method when following 301 redirects. In most cases, the method is changed to GET.

Safe methods (GET, HEAD, OPTIONS) can be retained safely when following redirects.

This change ensures that requests issued when following 301 redirects will retain the original request method if that method is safe.

@Chuvisco88
Copy link

Ran into the same issue (HEAD Request gets a 302 redirect and follows up with a GET Request).
This would fix it!
Why is it still open?! :(

@webignition
Copy link
Contributor Author

@Chuvisco88 I'm not a maintainer and so can't merge. Maybe follow this up with @sagikazarmark?

@Nyholm
Copy link
Member

Nyholm commented Dec 8, 2019

This looks alright.

Thank you @Chuvisco88 for the review.

Thank you @webignition for the PR. Can you rebase this on master?

@Nyholm Nyholm added this to the 7.1.0 milestone Dec 8, 2019
@webignition
Copy link
Contributor Author

Yep, can rebase, no problem. I'll do so in about a week when I'm back from vacation.

@Nyholm
Copy link
Member

Nyholm commented Jun 13, 2020

Sorry for the long delay. Could you please rebase this PR?

@webignition
Copy link
Contributor Author

@Nyholm Apologies also for not getting round to doing so years ago!

Rebased now, and as a bonus all the checks are passing 😄

@Nyholm Nyholm requested a review from gmponos June 27, 2020 08:55
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@Nyholm Nyholm merged commit 4781b26 into guzzle:master Jun 27, 2020
Copy link
Member

@gmponos gmponos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi.. sorry for the delay.. I was 👍 on this

@albig
Copy link

albig commented Aug 27, 2020

@Nyholm, we would appreciate if this fix goes into a 6.5 bugfix release one day.

We use TYPO3 which has as composer requirement even in current master "guzzlehttp/guzzle: ^6.3.0"...

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.

None yet

5 participants