Code

Opened 17 months ago

Closed 13 months ago

Last modified 13 months ago

#19875 closed Cleanup/optimization (fixed)

Tutorial should have include ALLOWED_HOSTS setting

Reported by: anonymous Owned by: nobody
Component: Documentation Version: 1.4
Severity: Normal Keywords: ALLOWED_HOSTS, tutorial
Cc: timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Got bit by 500 errors when switching to DEBUG=False.

As a total newbie, this was tough because a local install on my mac wasn't sending me email by default either.

I would recommend updating the tutorial to mention setting ALLOWED_HOSTS within the initial settings.py setup, perhaps to 127.0.0.1/localhost if the user is doing a local install.

Attachments (3)

19875.diff (542 bytes) - added by Kelvin 14 months ago.
Made a patch for docs/topics/settings.txt
19875.2.diff (2.4 KB) - added by timo 14 months ago.
19875.3.diff (2.8 KB) - added by timo 13 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 17 months ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I'm hesitant to add extra verbiage at that stage of the tutorial involving something that you don't actually need until much later, whenever you try flipping DEBUG to False. I was hoping that having DEBUG and ALLOWED_HOSTS very close to each other in the template settings file, and a comment on ALLOWED_HOSTS saying "required if DEBUG is False", would be enough - but apparently it isn't. Maybe we could add another comment above the DEBUG setting in settings.py specifically reminding you to check ALLOWED_HOSTS if you flip it to False?

Accepting because I think this is a problem that should be solved; not sure yet if adding to the tutorial is the right solution.

comment:2 Changed 17 months ago by russellm

Agreed that this is a problem that needs to be solved; equally hesitant about adding it to the early tutorials.

One possible approach would be to tackle an old feature request, and write a new tutorial on deployment. This would also give us a chance to set some best practices on "how to do it right", which is something that we've historically punted on. It's sure to be a bike shed discussion, but with a couple of judicious BDFL judgements, I'm sure we could get something we can all live with.

comment:3 Changed 17 months ago by carljm

Is there an existing ticket for "deployment tutorial"? #19697 is at least closely related.

comment:4 Changed 16 months ago by timo

This was also suggested in #20139.

comment:5 Changed 16 months ago by carljm

#20171 suggests adding documentation under DEBUG noting that if you set it False you must set ALLOWED_HOSTS. It also contains a list of related tickets:

Version 0, edited 16 months ago by carljm (next)

comment:6 Changed 15 months ago by timo

Suggested again in #20239.

Changed 14 months ago by Kelvin

Made a patch for docs/topics/settings.txt

comment:7 Changed 14 months ago by timo

  • Cc timograham@… added
  • Has patch set

Current status of this:

  1. The deployment checklist has a note about ALLOWED_HOSTS.
  2. The ALLOWED_HOSTS comment in the default project template was removed (probably in Aymeric's effort to simplify it). I've added it back in the attached patch as well as other relevant warnings in a couple places.
  3. I like the idea offered in #20139: "Whenever starting the server in DEBUG = False and ALLOWED_HOSTS = [], display a warning on the console (stderr) that no request will be served because of the empty ALLOWED_HOSTS setting." If someone agrees, we can open a new ticket.

Changed 14 months ago by timo

comment:8 Changed 14 months ago by aaugustin

AFAIK it's a conscious choice not to talk about anything related to deployment (like ALLOWED_HOSTS) in the tutorial. The tutorial must stay focused on its main purpose; it mustn't be a default place for docs that don't fit anywhere else.

I strongly believe that insufficient information is worse than no information at all, and for this reason I would really like if we could keep settings.py as is (although I don't have much hope and I'm pretty sure settings.py will grow some cruft again; well, at least I'll voice my opposition).

There's tons of useful advice to give about deployment; the deployment checklist helps much more than random, piecemeal advice in settings.py. I have to admit that I don't care very much about users who can't be bothered to read the docs, too.

comment:9 Changed 14 months ago by timo

The tutorial talks about setting DEBUG to False in order to use your own 404.html, 500.html, etc. templates. The note appears right under that section so if people try it they will be aware.

I'm fine with keeping settings.py as is.

What do you think about the suggestion of modifying runserver so it won't work with a bad configuration? I feel like that should eliminate this problem for non-deployment cases.

comment:10 Changed 13 months ago by EvilDMP

I think the console should warn the user, if ALLOWED_HOSTS is empty, that it will mean the site won't work when DEBUG is False (whatever the current state of DEBUG).

It's caught me out more than once...

Changed 13 months ago by timo

comment:11 Changed 13 months ago by timo

New patch adds logic so runserver won't serve any requests if DEBUG is False and ALLOWED_HOSTS is empty (I'm unsure if this needs tests -- tests.admin_scripts.ManageRunserver mocks the run method so I don't think anything in that method is actually tested).

Also removed the note in the previous patch from settings.py.

comment:12 Changed 13 months ago by claudep

Small nitpick about self.stderr.write/exit, I think that this could be replaced by raising CommandError instead.

comment:13 Changed 13 months ago by timo

Thanks, updated in a pull request. Moved the logic to the handle() method and also added a test. https://github.com/django/django/pull/1249

comment:14 Changed 13 months ago by Tim Graham <timograham@…>

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

In 4e94c84e50b960405d5708d8d9528c44c7dabe83:

Fixed #19875 - Added warnings regarding DEBUG=False and empty ALLOWED_HOSTS

comment:15 Changed 13 months ago by Tim Graham <timograham@…>

In 46dacb5bb157ff78e21c19f1f1b109edf378ddcb:

[1.5.x] Fixed #19875 - Added warnings regarding DEBUG=False and empty ALLOWED_HOSTS

Backport of 4e94c84e50 from master

comment:16 Changed 13 months ago by Tim Graham <timograham@…>

In 96c71d423db13825869a929b25d6b83dde67c170:

Added runserver validation to detect if DEBUG=False and ALLOWED_HOSTS is empty.

Refs #19875.

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.