Fixed #29570 - Added check that MEDIA_URL isn't in STATIC_URL

[alito]

The check runs only in development mode, since that is what the warning
warns about

[timgraham]

Is there a reason to use a warning instead of ImproperlyConfigured?

[alito]

An exception seems too harsh to me. The configuration is fine when deployed (ie DEBUG=False) and the developer knows what they are doing (ie doesn't name static directories with the same name as the media directory). It's only an issue for the development server.

Not that I'd be opposed to converting it to an exception, but my first instinct was to make it a warning.

[carltongibson]

The check runs only in development mode...

As far as I can this this would get called whenever you ran collectstatic:

django/django/contrib/staticfiles/storage.py

Lines 28 to 33 in 65503ca

	def __init__(self, location=None, base_url=None, *args, **kwargs):
	if location is None:
	location = settings.STATIC_ROOT
	if base_url is None:
	base_url = settings.STATIC_URL
	check_settings(base_url)

(As well as when using runserver etc, via StaticFilesHandler)

The configuration is fine when deployed (ie DEBUG=False) and the developer knows what they are doing (ie doesn't name static directories with the same name as the media directory). It's only an issue for the development server.

OK. Given that it works we probably can't just make it error.
(Even though we want to discourage this nesting the MEDIA and STATIC folders.)

Given that, I'm not really happy with the UserWarning.
It's too fiddly to silence.

Instead could we add this as a warning to the system checks in:

django/django/contrib/staticfiles/finders.py

Line 71 in 65503ca

def check(self, **kwargs):

That way it would be easy to switch off.

(Aside for a separate ticket: I need to look into whether we could unify the
checks here all into system checks.)

[alito]

Yes, makes more sense under FileSystemFinder.check. Updated to what @carltongibson requested.

[huangsam]

This is the line that caused flake8 to fail: https://djangoci.com/job/flake8/15588/console

[alito]

Thanks @huangsam. I fixed it but now it's having issues which I think are unrelated to my changes ('Could not checkout 80bd2a6')

[timgraham]

If it works when DEBUG=False, then how about disabling the check for that case? With that change, can we make it an error?

[timgraham]

Also, I don't understand why the check should go in FileSystemFinder. Isn't it relevant no matter what static file finder is used? How about:

diff --git a/django/contrib/staticfiles/utils.py b/django/contrib/staticfiles/utils.py
index 3d28c90f47..13ceaf1a80 100644
--- a/django/contrib/staticfiles/utils.py
+++ b/django/contrib/staticfiles/utils.py
@@ -52,6 +52,11 @@ def check_settings(base_url=None):
     if settings.MEDIA_URL == base_url:
         raise ImproperlyConfigured("The MEDIA_URL and STATIC_URL "
                                    "settings must have different values")
+    elif (settings.DEBUG and settings.MEDIA_URL and settings.STATIC_URL and
+            settings.MEDIA_URL.startswith(settings.STATIC_URL)):
+        raise ImproperlyConfigured(
+            "runserver can't serve media if MEDIA_URL is within STATIC_URL.",
+        )
     if ((settings.MEDIA_ROOT and settings.STATIC_ROOT) and
             (settings.MEDIA_ROOT == settings.STATIC_ROOT)):
         raise ImproperlyConfigured("The MEDIA_ROOT and STATIC_ROOT "

[alito]

I think you are right, it'd be a problem regardless of which finder.

I think making something an error when it'd work fine with debugging off seems too harsh. It'd make developing with such configuration a much bigger pain. Maybe you do want to make that stance but then you might as well make it always an error. Also, in that case you might want to check if they are running with --insecure since that will fail as well even if DEBUG is off

[timgraham]

I don't think it'll be a problem since someone is unlikely to be intentionally developing with settings that don't work in development. In that case, they'll problem have a separate settings file for production with DEBUG = False.

[alito]

Good point.
Changed the check to be like you described and changed the test to match