#20822 closed Bug (fixed)
Content type of default error pages should be always "text/html"
Reported by: | lvella | Owned by: | anonymous |
---|---|---|---|
Component: | HTTP handling | Version: | 1.5 |
Severity: | Normal | Keywords: | |
Cc: | timograham@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
I have in my settings.py:
DEFAULT_CONTENT_TYPE = 'application/xhtml+xml'
And every view defined in the project conforms to the that. For instance, I have defined a custom 404 error page, and there is no issue, because I wrote it as valid XHTML.
The problem is that the default error pages that comes in Django are, too, being served with the content type I choose in my settings. The simple default error 500 page content "<h1>Server Error (500)</h1>" is being server as XHTML, what is obviously wrong and fails to render in the browsers.
Default error pages should use specific content type for what it its, in this case, text/html, for the case the default content type has been overridden.
Change History (9)
comment:1 by , 11 years ago
Cc: | added |
---|---|
Component: | Uncategorized → HTTP handling |
Easy pickings: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Topic branch is at: https://github.com/jimmysong-work/django/tree/ticket_20822
Tests pass under linux.
comment:4 by , 11 years ago
BTW, this is my first contribution to an open-source project so please be gentle.
comment:5 by , 11 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
Hi Jimmy, thanks for your contribution, this is an very good start.
A small point about our process: when you upload a patch, you aren't supposed to flip the "Ready for Checkin" flag yourself. We ask that another person review the patch. This "two sets of eyes" rule provides better guarantees on the quality of patches marked as RFC.
Regarding the patch itself, HttpResponse
subclasses accept a content_type
keyword argument. You don't need to create a temporary response
, you can just create the response with the appropriate content-type and return it in one line.
Does the test fail when you run it without the code changes? Given that DEFAULT_CONTENT_TYPE = "text/html"
by default I suspect it doesn't. I propose to decorate it with @override_settings(DEFAULT_CONTENT_TYPE="text/xml")
, then it should fail without the code changes and pass with them.
comment:6 by , 11 years ago
Patch needs improvement: | unset |
---|
I've made the changes as requested by aaugustin. Please let me know if there's anything else I should fix to make this patch go through.
Pull request here: https://github.com/django/django/pull/1428
comment:7 by , 11 years ago
Owner: | changed from | to
---|---|
Triage Stage: | Accepted → Ready for checkin |
Looks good (as far as code review on a mobile phone goes)!
I'll merge it.
comment:8 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
This should be fairly easy change in
django.views.defaults
with tests in
django.tests.view_tests.test_defaults
.