Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #21581 -- Fixed a number of issues with collectstatic.

When STATIC_ROOT wasn't set, collectstatic --clear would delete
every files within the current directory and its descendants.

This patch makes the following changes:

Prevent collectstatic from running if STATIC_ROOT isn't set.

Fixed an issue that prevented collectstatic from displaying the
destination directory.

Changed the warning header to notify when the command is run
in dry-run mode.
  • Loading branch information...
commit 4befb3015c26810a68cfcf57e0cd8b062f56f1c5 1 parent 4d8d76e
Loïc Bistuer loic authored timgraham committed
2  django/conf/global_settings.py
View
@@ -290,7 +290,7 @@
# Absolute path to the directory static files should be collected to.
# Example: "/var/www/example.com/static/"
-STATIC_ROOT = ''
+STATIC_ROOT = None
# URL that handles the static files served from STATIC_ROOT.
# Example: "http://example.com/static/", "http://static.example.com/"
40 django/contrib/staticfiles/management/commands/collectstatic.py
View
@@ -137,32 +137,38 @@ def collect(self):
def handle_noargs(self, **options):
self.set_options(**options)
- # Warn before doing anything more.
- if (isinstance(self.storage, FileSystemStorage) and
+
+ message = ['\n']
+ if self.dry_run:
+ message.append(
+ 'You have activated the --dry-run option so no files will be modified.\n\n'
+ )
+
+ message.append(
+ 'You have requested to collect static files at the destination\n'
+ 'location as specified in your settings'
+ )
+
+ if (isinstance(self.storage._wrapped, FileSystemStorage) and

@loic @timgraham using the _wrapped attribute breaks existing storage classes that don't have that attribute. I don't really understand the purpose of that change. Can you explain? Should storage classes inherit from something other than FileSystemStorage?

Loïc Bistuer Collaborator
loic added a note

self.storage should be a reference to a LazyObject; having a custom storage shouldn't matter since FileSystemStorage doesn't have the _wrapped attribute attribute either.

The purpose of this change was to fix the previous test which would never work since we were comparing a LazyObject to a Storage class. How do you set your custom storage? Through the STATICFILES_STORAGE setting?

Thanks. Our app was actually using a custom storage class that inherited FileSystemStorage, but we weren't setting it as the DEFAULT_FILE_STORAGE, and therefore it wasn't being lazily loaded.

Loïc Bistuer Collaborator
loic added a note

No worries, btw I'm pretty sure you need to set STATICFILES_STORAGE rather than DEFAULT_FILE_STORAGE.

@loic I know that collectstatic is getting the storage object from storage.staticfiles_storage (and that therefore it should be a LazyObject) but is it safe to assume that subclasses will as well? IMO, examining _wrapped at all breaks the abstraction of the LazyObject, but if it has to be done, wouldn't it be better to use getattr?

/cc @jezdez

Loïc Bistuer Collaborator
loic added a note

I'm not sure I understand; by "is it safe to assume that subclasses [...]", do you mean subclasses of Storage? Because LazyObject is a wrapper, not a base class, therefore whatever subclass of Storage you have should end up in storage.staticfiles_storage._wrapped. Are you trying to manually set django.contrib.staticfiles.storage.staticfiles_storage? While documented, this variable is IMO readonly.

Sorry, no. I meant subclasses of the command.

Loïc Bistuer Collaborator
loic added a note

Hum, that's a use-case I didn't think about. We could replace self.storage._wrapped by self.storage._wrapped if issubclass(self.storage, LazyObject) else self.storage.

Loïc Bistuer Collaborator
loic added a note

@matthewwithanm does this address your concerns? #2269

That should do it; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
self.storage.location):
destination_path = self.storage.location
- destination_display = ':\n\n %s' % destination_path
+ message.append(':\n\n %s\n\n' % destination_path)
else:
destination_path = None
- destination_display = '.'
+ message.append('.\n\n')
if self.clear:
- clear_display = 'This will DELETE EXISTING FILES!'
+ message.append('This will DELETE EXISTING FILES!\n')
else:
- clear_display = 'This will overwrite existing files!'
-
- if self.interactive:
- confirm = input("""
-You have requested to collect static files at the destination
-location as specified in your settings%s
+ message.append('This will overwrite existing files!\n')
-%s
-Are you sure you want to do this?
+ message.append(
+ 'Are you sure you want to do this?\n\n'
+ "Type 'yes' to continue, or 'no' to cancel: "
+ )
-Type 'yes' to continue, or 'no' to cancel: """
-% (destination_display, clear_display))
- if confirm != 'yes':
- raise CommandError("Collecting static files cancelled.")
+ if self.interactive and input(''.join(message)) != 'yes':
+ raise CommandError("Collecting static files cancelled.")
collected = self.collect()
modified_count = len(collected['modified'])
11 django/contrib/staticfiles/storage.py
View
@@ -32,16 +32,13 @@ def __init__(self, location=None, base_url=None, *args, **kwargs):
location = settings.STATIC_ROOT
if base_url is None:
base_url = settings.STATIC_URL
- check_settings(base_url)
- super(StaticFilesStorage, self).__init__(location, base_url,
- *args, **kwargs)
-
- def path(self, name):
- if not self.location:
+ if not location:
raise ImproperlyConfigured("You're using the staticfiles app "
"without having set the STATIC_ROOT "
"setting to a filesystem path.")
- return super(StaticFilesStorage, self).path(name)
+ check_settings(base_url)
+ super(StaticFilesStorage, self).__init__(location, base_url,
+ *args, **kwargs)
class CachedFilesMixin(object):
2  docs/ref/settings.txt
View
@@ -2535,7 +2535,7 @@ Settings for :mod:`django.contrib.staticfiles`.
STATIC_ROOT
-----------
-Default: ``''`` (Empty string)
+Default: ``None``
The absolute path to the directory where :djadmin:`collectstatic` will collect
static files for deployment.
5 docs/releases/1.6.2.txt
View
@@ -14,3 +14,8 @@ Bug fixes
* Fixed a crash when executing the :djadmin:`changepassword` command when the
user object representation contained non-ASCII characters (#21627).
+
+* The :djadmin:`collectstatic` command will raise an error rather than
+ default to using the current working directory if :setting:`STATIC_ROOT` is
+ not set. Combined with the ``--clear`` option, the previous behavior could
+ wipe anything below the current working directory.
11 tests/staticfiles_tests/tests.py
View
@@ -224,6 +224,17 @@ def test_all_files_less_verbose(self):
self.assertIn('apps', force_text(lines[1]))
+class TestConfiguration(StaticFilesTestCase):
+ def test_location_empty(self):
+ err = six.StringIO()
+ for root in ['', None]:
+ with override_settings(STATIC_ROOT=root):
+ with six.assertRaisesRegex(
+ self, ImproperlyConfigured,
+ 'without having set the STATIC_ROOT setting to a filesystem path'):
+ call_command('collectstatic', interactive=False, verbosity=0, stderr=err)
+
+
class TestCollection(CollectionTestCase, TestDefaults):
"""
Test ``collectstatic`` management command.
Ryan Bagwell

@loic @timgraham using the _wrapped attribute breaks existing storage classes that don't have that attribute. I don't really understand the purpose of that change. Can you explain? Should storage classes inherit from something other than FileSystemStorage?

Loïc Bistuer

self.storage should be a reference to a LazyObject; having a custom storage shouldn't matter since FileSystemStorage doesn't have the _wrapped attribute attribute either.

The purpose of this change was to fix the previous test which would never work since we were comparing a LazyObject to a Storage class. How do you set your custom storage? Through the STATICFILES_STORAGE setting?

Ryan Bagwell

Thanks. Our app was actually using a custom storage class that inherited FileSystemStorage, but we weren't setting it as the DEFAULT_FILE_STORAGE, and therefore it wasn't being lazily loaded.

Loïc Bistuer

No worries, btw I'm pretty sure you need to set STATICFILES_STORAGE rather than DEFAULT_FILE_STORAGE.

Matthew Dapena-Tretter

@loic I know that collectstatic is getting the storage object from storage.staticfiles_storage (and that therefore it should be a LazyObject) but is it safe to assume that subclasses will as well? IMO, examining _wrapped at all breaks the abstraction of the LazyObject, but if it has to be done, wouldn't it be better to use getattr?

/cc @jezdez

Loïc Bistuer

I'm not sure I understand; by "is it safe to assume that subclasses [...]", do you mean subclasses of Storage? Because LazyObject is a wrapper, not a base class, therefore whatever subclass of Storage you have should end up in storage.staticfiles_storage._wrapped. Are you trying to manually set django.contrib.staticfiles.storage.staticfiles_storage? While documented, this variable is IMO readonly.

Loïc Bistuer

Hum, that's a use-case I didn't think about. We could replace self.storage._wrapped by self.storage._wrapped if issubclass(self.storage, LazyObject) else self.storage.

Please sign in to comment.
Something went wrong with that request. Please try again.