Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#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 Tim Graham, 11 years ago

Cc: timograham@… added
Component: UncategorizedHTTP handling
Easy pickings: set
Triage Stage: UnreviewedAccepted

This should be fairly easy change in django.views.defaults with tests in django.tests.view_tests.test_defaults.

Version 0, edited 11 years ago by Tim Graham (next)

comment:2 by jimmysong, 11 years ago

Owner: changed from nobody to jimmysong
Status: newassigned

comment:3 by jimmysong, 11 years ago

Triage Stage: AcceptedReady for checkin

Topic branch is at: https://github.com/jimmysong-work/django/tree/ticket_20822

Tests pass under linux.

comment:4 by jimmysong, 11 years ago

BTW, this is my first contribution to an open-source project so please be gentle.

comment:5 by Aymeric Augustin, 11 years ago

Has patch: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 jimmysong, 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

Last edited 11 years ago by jimmysong (previous) (diff)

comment:7 by Aymeric Augustin, 11 years ago

Owner: changed from jimmysong to anonymous
Triage Stage: AcceptedReady for checkin

Looks good (as far as code review on a mobile phone goes)!

I'll merge it.

comment:8 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 784377544e01322b0d09e5cf4e8f6dda159df854:

Fixed #20822 -- Set content type of default error pages to 'text/html'.

Thanks Jimmy Song for the patch.

comment:9 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 2eac9899856d0e19c8e2df02256f2e8c4bdb5344:

[1.6.x] Fixed #20822 -- Set content type of default error pages to 'text/html'.

Thanks Jimmy Song for the patch.

Backport of 7843775 from master.

Note: See TracTickets for help on using tickets.
Back to Top