Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#12493 closed Cleanup/optimization (fixed)

settings.configure silently allows TEMPLATE_DIRS configuration error

Reported by: mckoss Owned by: Gary Wilson
Component: Core (Other) Version: 1.1
Severity: Normal Keywords: settings, templates
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

settings.configure(TEMPLATE_DIRS="/path/to/a/single/dir")

Result:

Silently accepted, even though the code is expecting an array of path strings.

Expect:

Either a) generate an error message when invalid settings are set, or b) allow this setting to be either a single directory OR a sequence
of directories.

Note that configuration errors like this can be very painful to track down - as the site of the error is quite far from the results (in this case, the template system is unable to find the file ... but there is no indication as to why that is the case w/o debugging the Django sources - which is what I did finally to figure out my mistake).

Change History (8)

comment:1 Changed 7 years ago by Ramiro Morales

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

A workaround for this kind of user errors is already present since r213 for the case of settings read from a settings module. Should something similar also be done for the settings.configure() caseor can we close this assuming the user using such advanced scheme should make sure to read the relevant setting documentation?. Setting triage status to 'Design decision needed'.

comment:2 Changed 7 years ago by Alex Gaynor

Ugh, I'm opposed to magically fixing those settings as it is now, obviously we can't undo those (yay backwards compatibility), but I don't like extending them further.

comment:3 Changed 7 years ago by Luke Plant

I agree with Alex - fixing them automatically is pretty nasty, because it means that developer is not alerted about their error. If they set TEMPLATE_DIRS incorrectly in their settings.py, and use the value somewhere else (e.g. as part of another setting, or in some other code), the possible consequences violate the principle of least astonishment. Throwing a validation error would have been much better IMO.

I'm also not convinced the error is that inscrutable. In the normal case, you get a debug page with the error TemplateDoesNotExist. The TEMPLATE_DIRS setting, which is the obvious thing to check, is displayed at the bottom of the page, and will show a single string — and I think it ought to be fairly obvious from the plural name TEMPLATE_DIRS that the setting should be an iterable of some kind.

In the context of a settings file, you are dealing with people who might not know Python, and the need for the trailing comma in a one-tuple is quite an obscure syntax point to expect non-Python developers to know. That argument isn't so strong for normal Python code.

comment:4 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Cleanup/optimization

comment:5 Changed 5 years ago by Gary Wilson

Easy pickings: unset
Owner: changed from nobody to Gary Wilson
Status: newassigned
UI/UX: unset

Also agree that this should be raising an error instead of auto-correcting, and should perhaps be considered for removal. However, the fact remains that this is a bug since the behavior using settings.configure() is not the same as the standard behavior.

comment:6 Changed 5 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

Accepting as explained by gwilson. We'll need a deprecation path for the switch from auto-correcting to raising an exception.

comment:7 Changed 4 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: assignedclosed

In [db87016b1a92b790c4250a2b5c16ceadd737a00e]:

Fixed #12493 -- Deprecated auto-correction of TEMPLATE_DIRS

comment:8 Changed 3 years ago by Ramiro Morales <cramm0@…>

In 64cdea68e71829905da6374a066d1700375255ec:

Report wrongly-typed settings and abort, as originally planned.

Thanks Claude for the heads up. Refs #12493 and commit 5e08b792.

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