Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#21750 closed Bug (fixed)

STATIC_ROOT is required in 1.7

Reported by: aaugustin Owned by: nobody
Component: contrib.staticfiles Version: 1.6
Severity: Release blocker Keywords:
Cc: loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In Django 1.7, STATIC_ROOT is required to use the staticfiles app, while it wasn't in Django 1.6 and earlier.

For instance I'm encountering the following error when running the debug toolbar's test suite.

ImproperlyConfigured: You're using the staticfiles app without having set the STATIC_ROOT setting to a filesystem path.

If this was done on purpose, it should be documented in the release notes.

Change History (9)

comment:1 Changed 20 months ago by timo

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

This is due to ticket #21581 which was also backported to 1.6.x but unreleased at this time. Possibly the patch was too invasive. How does the test suite use static files?

comment:2 Changed 20 months ago by aaugustin

That may be triggered by the Selenium tests. You can reproduce the issue simply by checking out the Django Debug Toolbar repository, removing STATIC_ROOT from tests/settings.py, putting Django 1.7 on PYTHONPATH and running "make test".

Like runserver, LiveServerTestCase doesn't require collectstatic; files can be served from their original locations. STATIC_ROOT isn't needed until one runs collectstatic. That's why the tests used to work correctly without STATIC_ROOT.

comment:3 Changed 20 months ago by loic84

  • Cc loic@… added

@timo suggested "perhaps move the validation of STATIC_ROOT to collectstatic" on #21581. That's an option but not a pretty one since collectstatic should work with backends that don't rely on STATIC_ROOT; we'd be leaking StaticFilesStorage implementation details on the generic management command.

It's tricky because there is no need for STATIC_ROOT until collectstatic is run, but on the other hand, with STATIC_ROOT out of sight in the default template, you hit an issue the first time you run the management command, which is the finality of the staticfiles subsystem.

I lean towards a release notes patch and adding STATIC_ROOT=os.path.join(BASE_DIR, 'static') to the default template, but I'll do as instructed.

comment:4 Changed 20 months ago by mjtamlyn

The existing patch to #21581 seems to be a major inconvenience to me, and is certainly unreleasable on 1.6.x. - we can't make projects which run (or test suites which run) suddenly start breaking. I certainly have projects where the STORAGE_BACKEND is deliberately configured different during the tests (and sometimes even for local dev) to avoid the possibility of overwriting the files on S3.

It seems reasonable that collectstatic should do the check, but should do so by asking the STORAGE_BACKEND whether it is properly configured before running the command - thus not leaking the implementation details.

comment:5 Changed 20 months ago by loic84

This should address the backward compatibility issues while preserving the fix for #21581: https://github.com/loic/django/compare/ticket21750.

comment:6 Changed 20 months ago by loic84

I'm reasonably confident that this is the best way to deal with the immediate issue, so I turned it into a PR: https://github.com/django/django/pull/2156.

On IRC, @jezdez, @mjtamlyn and myself discussed a Storage.check_configured method that would raise ImperperlyConfigured if the storage is missing some settings or dependencies. I think the idea has merit as a feature of its own but that wouldn't help to solve this problem which boils down to: "validate STATIC_ROOT when STATIC_ROOT is actually used", as opposed to "validate STATIC_ROOT when the storage is used".

comment:7 Changed 20 months ago by loic84

  • Has patch set

comment:8 Changed 20 months ago by Tim Graham <timograham@…>

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

In 1e9e7351f88a59261da6e616934c8283a6e3e565:

Fixed #21750 -- Fixed regression introduced by 4befb30.

Validating STATIC_ROOT in StaticFilesStorage.init turned out to be
problematic - especially with tests - because the storage refuses to work even
if there are no actual interactions with the file system, which is backward
incompatible.

Originally the validation happened in the StaticFilesStorage.path method, but
that didn't work as expected because the call to FileSystemStorage.init
replaced the empty value by a valid path. The new approach is to move back the
check to the StaticFilesStorage.path method, but ensure that the location
attribute remains None after the call to super.

Refs #21581.

comment:9 Changed 20 months ago by Tim Graham <timograham@…>

In 6728f159f060e536f1543b4afecb602b6693babb:

[1.6.x] Fixed #21750 -- Fixed regression introduced by 4befb30.

Validating STATIC_ROOT in StaticFilesStorage.init turned out to be
problematic - especially with tests - because the storage refuses to work even
if there are no actual interactions with the file system, which is backward
incompatible.

Originally the validation happened in the StaticFilesStorage.path method, but
that didn't work as expected because the call to FileSystemStorage.init
replaced the empty value by a valid path. The new approach is to move back the
check to the StaticFilesStorage.path method, but ensure that the location
attribute remains None after the call to super.

Refs #21581.

Backport of 1e9e7351f8 from master

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