Code

Opened 3 years ago

Closed 3 years ago

#15094 closed (fixed)

Convert STATICFILES_DIRS into a tuple if set as string

Reported by: oxy Owned by: nobody
Component: contrib.staticfiles Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Hi

If you mistakenly set STATICFILES_DIRS as a string instead of a tuple (e.g. forgot the comma), findstatic et al raise an exception like

ValueError: The joined path (/css/main.css) is located outside of the base path component (/)

which is not very enlightening in this case.

Django core solves this for INSTALLED_APPS and TEMPLATE_DIRS nicely in django/conf/__init__.py line 93ff.

Attachments (2)

staticfiles_dirs_to_tuple.diff (839 bytes) - added by oxy 3 years ago.
convert STATICFILES_DIRS into tuple if set as string
0001-Raise-ImproperlyConfigured-if-STATICFILES_DIRS-is-no.patch (2.2 KB) - added by oxy 3 years ago.
Second attempt to fix the unexpected behavior by raising an exception.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by jezdez

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

This can't be easily achieved since STATICFILES_DIRS can itself contain tuples (prefx, location pairs), so a check or automagic change would make this rather pointless.

comment:2 Changed 3 years ago by oxy

  • Has patch set

Sure, you can't determine if STATICFILES_DIRS is set properly if you know it's of type tuple. But you can determine that it's definitely wrong if it's a string (STATICFILES_DIRS has to be a tuple).

Therefore if it's mistakenly entered as string converting it into a tuple should be safe.

As this is a simple fix I created a patch, I hope it's good?!

Changed 3 years ago by oxy

convert STATICFILES_DIRS into tuple if set as string

comment:3 follow-up: Changed 3 years ago by lukeplant

Automagically fixing these settings, the way that INSTALLED_APPS and TEMPLATE_DIRS does, is a bad idea IMO. Validation that throws an exception is acceptable, but automatically fixing like that only leads to further confusion, and leads to other code having to make the same fixes so that it can cope with everything that Django copes with. This often shows up with backwards compatibility or interop concerns.

In this case, there will be confusion over why switching from a string to a tuple pair causes breakage.

To give a different example, the mess in http://code.djangoproject.com/browser/django/trunk/django/views/decorators/cache.py#L11 was caused by requiring backwards compatibility with code that gave the 'convenience' of being able to do these:

@cache_page
def foo(request):
    ...

# or
foo = cache_page(foo)

# or
@cache_page(123)
def foo(request):
    ...

# or
foo = cache_page(foo, 123)

instead of the slightly uglier but consistent:

@cache_page()
def foo(request):
    ...

# or
foo = cache_page()(foo)

# or
@cache_page(123)
def foo(request):
    ...

# or
foo = cache_page(123)(foo)

I'd be strongly in favour of changing the INSTALLED_APPS handling to raise a ConfigurationError instead, but that might require a deprecation path.

comment:4 in reply to: ↑ 3 Changed 3 years ago by jezdez

Replying to lukeplant:

Automagically fixing these settings, the way that INSTALLED_APPS and TEMPLATE_DIRS does, is a bad idea IMO. Validation that throws an exception is acceptable, but automatically fixing like that only leads to further confusion, and leads to other code having to make the same fixes so that it can cope with everything that Django copes with. This often shows up with backwards compatibility or interop concerns.

I'd be strongly in favour of changing the INSTALLED_APPS handling to raise a ConfigurationError instead, but that might require a deprecation path.

Couldn't agree more, although we already have ImproperlyConfigured which I believe could be used for this without having to require a deprecation path.

comment:5 Changed 3 years ago by oxy

Your points are true.

I just think the currently raised exception does not point to the real cause. At least in my case it took me some time to find I just forgot to append the comma.

My suggestion was based on the behaviour in django core, but now an exception like "you entered a string, but a tuple is expected" seems more reasonable to me.

comment:6 Changed 3 years ago by lukeplant

Yes, a patch like that would be accepted, since it is always going to be an error to pass a string. Remember not use the the 'if then else' expression that is not available in Python 2.4

Changed 3 years ago by oxy

Second attempt to fix the unexpected behavior by raising an exception.

comment:7 Changed 3 years ago by oxy

According to your suggestions I created a second patch and added a test.

comment:8 Changed 3 years ago by jezdez

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

comment:9 Changed 3 years ago by jezdez

  • Component changed from Contrib apps to django.contrib.staticfiles

comment:10 Changed 3 years ago by carljm

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

In [15386]:

Fixed #15094 - Added check for forgetting trailing comma in STATICFILES_DIRS tuple. Also reorganized staticfiles settings-checks for better consistency.

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.