Opened 2 years ago

Closed 2 years ago

Last modified 2 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 Changed 2 years 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 2 years ago by timo (previous) (diff)

comment:2 Changed 2 years ago by jimmysong

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

comment:3 Changed 2 years 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 2 years ago by jimmysong

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

comment:5 Changed 2 years 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 2 years 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 2 years ago by jimmysong (previous) (diff)

comment:7 Changed 2 years 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 2 years 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 2 years 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.

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