[Django] #24139: The response reason phrases should evaluate lazily
Django
-------------------------------+--------------------
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Motivation for this suggestion comes from a bug I found in my own code. I
traced it back to how Django handles HTTP reason phrases.
Suggestion:
If the `status_code` of an HTTP response is set outside of the
constructor, the `reason_phrase` will be "OK", the default for a 200
response. The calling code is required to take two steps to successfully
change the status and reason phrase together.
{{{
response = render_to_response(....)
response.status_code = 503 # Or any status code
# As of now, reason_phrase is still "OK"
#
# Need the line below -- or something like it -- to successfully update
the reason phrase.
response.reason_phrase = REASON_PHRASES[response.status_code]
}}}
I'm suggesting wrap `reason_phrase` as a property. Internal to the
response, `reason_phrase` remains `None` unless specified by the calling
code. Upon accessing `reason_phrase`, if the value is `None` pull from the
default `REASON_PHRASES` based on the current value of `status_code`.
I'd also suggest that any change to the `status_code` automatically sets
`reason_phrase` back to `None` to avoid surprise reason phrases.
Right now the `reason_phrase` is set once in the constructor and not
updated based on the latest `status_code`.
Pull request to follow.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* cc: jon.dufresne@… (added)
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
https://github.com/django/django/pull/3903
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:1>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* cc: cmawebsite@… (added)
* stage: Unreviewed => Accepted
Comment:
The implementation looks simple enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:2>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_docs: 0 => 1
Comment:
Is it documented that you can set `HttpResponse.status_code` outside of
the constructor?
If we're adding code to support setting this attribute, it should be
documented. If it's already documented, the change in behavior should be
documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:3>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by jdufresne):
> Is it documented that you can set HttpResponse.status_code outside of
the constructor?
Grepping for "status_code" and "status" in `docs` shows the following:
{{{
./request-response.txt:663:
.. method:: HttpResponse.__init__(content='', content_type=None,
status=200, reason=None, charset=None)
``status`` is the `HTTP status code`_ for the response.
./ref/request-response.txt:639:
.. attribute:: HttpResponse.status_code
The `HTTP status code`_ for the response.
}}}
Nothing about this indicates to me that I ''shouldn't'' change
`status_code` outside the constructor.
This was originally a problem for me as I was using `render_to_response`
in Django 1.6. This version does not accept `status` as a kwarg. I was
trying to change the `status_code` of the return value. This is when I
noticed the reason phrase was incorrect.
> If we're adding code to support setting this attribute, it should be
documented. If it's already documented, the change in behavior should be
documented.
Will do.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:4>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* needs_docs: 1 => 0
Comment:
> If we're adding code to support setting this attribute, it should be
documented. If it's already documented, the change in behavior should be
documented.
Added docs to PR. All feedback welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:5>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:6>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:7>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* needs_better_patch: 1 => 0
Comment:
Fixed PR based on feedback.
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:8>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: closed
Component: HTTP handling | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d861f95c445652e4269c9015836d14dcf8b9a587" d861f95]:
{{{
#!CommitTicketReference repository=""
revision="d861f95c445652e4269c9015836d14dcf8b9a587"
Fixed #24139 -- Changed HttpResponse.reason_phrase to evaluate based on
status_code.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24139#comment:9>