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

Support 308 Permanent Redirect HTTP redirect #1794

Merged
merged 3 commits into from Jul 27, 2019

Conversation

@ingoogni
Copy link
Contributor

commented Jul 26, 2019

Recognize raise cherrypy.HTTPRedirect('/new_uri', 308) as a legitimate redirect.

Refs:

What kind of change does this PR introduce?

  • bug fix
  • feature
  • docs update
  • tests/coverage improvement
  • refactoring
  • other

What is the related issue number (starting with #)

N/A

What is the current behavior? (You can also link to an open issue here)

raise cherrypy.HTTPRedirect('/new_uri', 308) is not recognized as a redirect and causes a ValueError

What is the new behavior (if this is a feature change)?

raise cherrypy.HTTPRedirect('/new_uri', 308) works the same way as other HTTP codes for redirection.

Other information:

N/A

Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences
@webknjaz

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Thanks for your contribution @ingoogni!

FTR I've just restarted a failed job on AppVeyor (and it's green now).
I'll probably need to improve the commit message before merging.

May I ask you to add a test case covering this change, please?

@codecov

This comment was marked as outdated.

Copy link

commented Jul 26, 2019

Codecov Report

Merging #1794 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1794      +/-   ##
==========================================
- Coverage   80.67%   80.55%   -0.12%     
==========================================
  Files         104      104              
  Lines       13252    13255       +3     
==========================================
- Hits        10691    10678      -13     
- Misses       2561     2577      +16

@webknjaz webknjaz changed the title Update _cperror.py Support 308 Moved Permanently HTTP status redirect Jul 26, 2019

@webknjaz
Copy link
Member

left a comment

Should the default_status property be also updated?

@webknjaz webknjaz requested a review from jaraco Jul 26, 2019

@ingoogni

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Thank @webknjaz Regarding default_status, for what I found 308 is either implemented properly in browsers / clients or not at all. I wouldn't add 308 to the default_redirect. If 308 is not implemented the client should fall back on their unknown status procedure and just render the message.
I'll look into a test case.

@webknjaz webknjaz changed the title Support 308 Moved Permanently HTTP status redirect Support 308 Permanent Redirect HTTP redirect Jul 27, 2019

@webknjaz

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Apparently, the status text is Permanent Redirect: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/308#Status

I've also checked the client behavior and found out that there's no user confirmation on the browser side so I've corrected that too.

ingoogni and others added some commits Jul 25, 2019

@webknjaz webknjaz merged commit 9eac624 into cherrypy:master Jul 27, 2019

5 of 6 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
WIP Ready for review
Details
codecov/patch 100% of diff hit (target 80.67%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@webknjaz

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Thanks @ingoogni!

@ingoogni

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Thanks @webknjaz

@ingoogni ingoogni deleted the ingoogni:ingoogni-redirect-308 branch Jul 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.