Code

Opened 12 months ago

Closed 11 months ago

Last modified 11 months 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.

Attachments (0)

Change History (9)

comment:1 Changed 11 months ago by timo

  • Cc timograham@… added
  • Component changed from Uncategorized to HTTP handling
  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

Last edited 11 months ago by timo (previous) (diff)

comment:2 Changed 11 months ago by jimmysong

  • Owner changed from nobody to jimmysong
  • Status changed from new to assigned

comment:3 Changed 11 months ago by jimmysong

  • Triage Stage changed from Accepted to Ready for checkin

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

Tests pass under linux.

comment:4 Changed 11 months ago by jimmysong

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

comment:5 Changed 11 months ago by aaugustin

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 11 months ago by jimmysong

  • 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 months ago by jimmysong (previous) (diff)

comment:7 Changed 11 months ago by aaugustin

  • Owner changed from jimmysong to anonymous
  • Triage Stage changed from Accepted to Ready for checkin

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

I'll merge it.

comment:8 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 784377544e01322b0d09e5cf4e8f6dda159df854:

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

Thanks Jimmy Song for the patch.

comment:9 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.