Opened 6 years ago

Closed 6 years ago

#29570 closed Cleanup/optimization (fixed)

Add check that MEDIA_URL is not inside STATIC_URL.

Reported by: Alejandro Dubrovsky Owned by: nobody
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: Herbert Fortes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Setting a MEDIA_URL that is within STATIC_URL, eg


STATIC_URL = '/static/'
MEDIA_URL = os.path.join(STATIC_URL, 'upped') + '/'

leads to the development server refusing to serve media files when staticfiles is installed even if a URL route is specified since the staticfiles.handlers.StaticFilesHandler takes over.

While there is a check in staticfiles warning that MEDIA_URL should not be equals to STATIC_URL, it does not warn about the cases where STATIC_URL is a prefix of MEDIA_URL. This also does not seem to be documented anywhere that I've seen.

The suggestion to allow this seems to have been closed wontfix in https://code.djangoproject.com/ticket/15199 so I won't attempt a patch. Adding a check for this case does seem easy though, and I've attached a patch that adds a warning to this ticket. The documentation could also mention that adding an explicit url pattern for static and running python manage.py runserver --nostatic can be used as a workaround.

Attachments (1)

staticwarning.patch (813 bytes ) - added by Alejandro Dubrovsky 6 years ago.

Download all attachments as: .zip

Change History (7)

by Alejandro Dubrovsky, 6 years ago

Attachment: staticwarning.patch added

comment:1 by Carlton Gibson, 6 years ago

Component: DocumentationCore (System checks)
Summary: Document that MEDIA_URL inside STATIC_URL leads to unexpected behaviour in the development serverAdd check that MEDIA_URL is not inside STATIC_URL.
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 2.0master

The documentation could also mention that adding an explicit url pattern for static and running python manage.py runserver --nostatic can be used as a workaround.

The conclusion from #15199 was that having MEDIA_URL inside STATIC_URL (MEDIA_ROOT inside STATIC_ROOT?) is a pattern that we want to avoid, so I don't think documenting a workaround is consistent with that.

Rather, strengthening the system check seems reasonable. (The patch checks media is not inside static. Is it worth doing the opposite too?)

The MEDIA_ROOT docs already say:

MEDIA_ROOT and STATIC_ROOT must have different values. ...

With the extra check this may be enough (I've always read it that way) but if an alternative phrasing that says "not inside each other too" that is both short and precise can be found then maybe we can update that as well.

comment:2 by Carlton Gibson, 6 years ago

Needs tests: set

Alejandro, could you create a Pull Request on GitHub with your patch, adding at least a test case, so we can review? Thanks.

comment:3 by Alejandro Dubrovsky, 6 years ago

Done (update): https://github.com/django/django/pull/10201

As mentioned in the pull request, the check only runs when staticfiles takes over (ie in DEBUG mode when staticfiles is in the INSTALLED_APPS), since that is when the problem occurs and that's what the warning warns about.

Also, I've fixed the isort failure and most of the flake8 failure but the two remaining are 'E128 continuation line under-indented for visual indent' which seems to be due to the recommended style by Django, and the related 'E125 continuation line with same indent as next logical line'.

Could someone look at that bit of code and tell me which format would be acceptable for the project please?
ie

    with self.assertWarnsMessage(UserWarning,
            'The contrib.staticfiles app will not serve media if MEDIA_URL is within STATIC_URL'):
Last edited 6 years ago by Alejandro Dubrovsky (previous) (diff)

comment:4 by Herbert Fortes, 6 years ago

Cc: Herbert Fortes added

Hi,

I ran the tests (./runtests.py) without a problem.

I have one minor suggestion. Instead of \ use () to break a line:

if (settings.MEDIA_URL and settings.STATIC_URL and
                settings.MEDIA_URL.startswith(settings.STATIC_URL)):

comment:5 by Alejandro Dubrovsky, 6 years ago

Modified to use parenthesis instead of a continuation marker to break up the line as per Herbert's suggestion.

comment:6 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In 108c04f:

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

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