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

notfound view: use HTTPTemporaryRedirect as default #3328

Merged
merged 2 commits into from Aug 23, 2018

Conversation

Projects
None yet
4 participants
@domenkozar
Member

domenkozar commented Aug 17, 2018

So that it will redirect POST requests without failing out.

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Aug 17, 2018

Member

Bleh. A mistake made in 2011 is coming back to haunt us. In 8c2a9e6, I collapsed the concept of a generic NotFound with the HTTP exception HTTPNotFound. This never should have happened, because people are going to be hella confused when they raise HTTPNotFound (302) and wind up with a 307. I'm not sure how to fix it.

Member

mcdonc commented Aug 17, 2018

Bleh. A mistake made in 2011 is coming back to haunt us. In 8c2a9e6, I collapsed the concept of a generic NotFound with the HTTP exception HTTPNotFound. This never should have happened, because people are going to be hella confused when they raise HTTPNotFound (302) and wind up with a 307. I'm not sure how to fix it.

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Aug 17, 2018

Member

I suspect we should bring back the concept of a generic NotFound and a generic Forbidden, and figure out some way to de-transition people from using the specific HTTP exceptions back to using the framework-generic ones. I don't think we should change the default notfound response code until we do this.

Member

mcdonc commented Aug 17, 2018

I suspect we should bring back the concept of a generic NotFound and a generic Forbidden, and figure out some way to de-transition people from using the specific HTTP exceptions back to using the framework-generic ones. I don't think we should change the default notfound response code until we do this.

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Aug 17, 2018

Member

Alternatively, I wrote a small documentation patch to guide readers: domenkozar@b341a9d

Member

domenkozar commented Aug 17, 2018

Alternatively, I wrote a small documentation patch to guide readers: domenkozar@b341a9d

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Aug 17, 2018

Member

Ehm... HTTPNotFound is a 404. If you raise a HTTPNotFound I am not sure how you would end up with a 307 in that code that is changed...

The change here looks to be safe and will change what used to be a 302 to a 307.

Member

bertjwregeer commented Aug 17, 2018

Ehm... HTTPNotFound is a 404. If you raise a HTTPNotFound I am not sure how you would end up with a 307 in that code that is changed...

The change here looks to be safe and will change what used to be a 302 to a 307.

Show outdated Hide outdated pyramid/config/views.py
@@ -292,7 +292,7 @@ def notfound_view(context, request): return HTTPNotFound('nope')
.. deprecated:: 1.3
"""
def __init__(self, notfound_view=None, redirect_class=HTTPFound):
def __init__(self, notfound_view=None, redirect_class=HTTPTemporaryRedirect):

This comment has been minimized.

@bertjwregeer

bertjwregeer Aug 17, 2018

Member

This change is fine and won't cause any issues.

@bertjwregeer

bertjwregeer Aug 17, 2018

Member

This change is fine and won't cause any issues.

Show outdated Hide outdated pyramid/config/views.py
Show outdated Hide outdated pyramid/config/views.py
@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Aug 17, 2018

Member

Duh. Bert's right. I still regret that 2011 commit but I retract my hold.

Member

mcdonc commented Aug 17, 2018

Duh. Bert's right. I still regret that 2011 commit but I retract my hold.

Show outdated Hide outdated pyramid/config/views.py
@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Aug 22, 2018

Member

Thanks guys, I think this is ready for seconds pass (and includes test fixes).

One not so obvious breakage here is that anyone relying in their tests on 302 status code will break. An acceptabe nuisance?

Member

domenkozar commented Aug 22, 2018

Thanks guys, I think this is ready for seconds pass (and includes test fixes).

One not so obvious breakage here is that anyone relying in their tests on 302 status code will break. An acceptabe nuisance?

@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Aug 22, 2018

Member

I would say we need to add a note to the CHANGES and to the release notes, but other than that I think this change is good to go.

Member

bertjwregeer commented Aug 22, 2018

I would say we need to add a note to the CHANGES and to the release notes, but other than that I think this change is good to go.

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Aug 23, 2018

Member

@bertjwregeer added item to CHANGES.rst, but docs/whatsnew-1.10.rst doesn't exist. Anything I missed?

Member

domenkozar commented Aug 23, 2018

@bertjwregeer added item to CHANGES.rst, but docs/whatsnew-1.10.rst doesn't exist. Anything I missed?

Show outdated Hide outdated CHANGES.rst
@bertjwregeer

This comment has been minimized.

Show comment
Hide comment
@bertjwregeer

bertjwregeer Aug 23, 2018

Member

@domenkozar docs/whatsnew-1.10.rst is created by the person doing the release, and until we are ready to release that document doesn't exist.

Member

bertjwregeer commented Aug 23, 2018

@domenkozar docs/whatsnew-1.10.rst is created by the person doing the release, and until we are ready to release that document doesn't exist.

@domenkozar domenkozar merged commit b5f4e49 into Pylons:master Aug 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment