Opened 10 years ago

Closed 8 years ago

Last modified 5 years ago

#21221 closed New feature (fixed)

Widgets and Admin's Media should use the configured staticfiles storage to create the right path to a file

Reported by: Guilherme Gondim <semente@…> Owned by: Johannes Maron
Component: contrib.staticfiles Version: 1.5
Severity: Normal Keywords: staticfiles, admin, CachedFilesMixin, media, widgets
Cc: maxime.lorant@…, info@…, cmawebsite@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Taking for example the following widget snippet:

class Editor(widgets.Textarea):   

    class Media:
        js = ('editor/editor.js',)
        css = {'all': ('editor/editor.css',)}

When using CachedFilesMixin, from the staticfiles contrib app, the right path to the media file should be the one with the hash suffix but what we got is just a concatenation of STATIC_URL and the relative path of the media file.

Change History (25)

comment:1 by Tim Graham, 10 years ago

Component: contrib.admincontrib.staticfiles
Triage Stage: UnreviewedAccepted

I haven't tested, but it seems like this should indeed work like you described.

comment:2 by Eran Rundstein, 9 years ago

Hi!
Any plan to do anything about this?
This is unfortunately still broken :(

comment:3 by Tim Graham, 9 years ago

It doesn't appear that anyone has offered a patch.

comment:4 by antonpp, 9 years ago

Owner: changed from nobody to antonpp
Status: newassigned

comment:5 by David Sanders, 8 years ago

Many of the admin forms in Django mainline get around this by manually using the django.contrib.admin.templatetag.admin_static.static template tag when specifying the filename, although this has disadvantages. See #25553 regarding the need for a lazy version of the tag since the forms may be imported during the app registry process and try to evaluate the templatetag too early (before collectstatic has run).

It's also tricky to have a subclass override one of the form assets if the filename changes, requiring it to use the static version of the filename to ensure it matches in all cases.

Off the top of my head (without thinking too deeply about it), it seems like the change would fit best in the rendering process for the Media widget? This is where the simple logic of prepending the STATIC_URL occurs so it seems logical to do the transformation here, and also leaves the filenames as their untransformed version up until the actual rendering. Specifically seems like it would fit in the absolute_path method here.

comment:6 by Maxime Lorant, 8 years ago

Cc: maxime.lorant@… added
Owner: antonpp removed
Status: assignednew

comment:7 by Maxime Lorant, 8 years ago

No patch from antonpp in seven months, so I allowed myself to set this ticket to new again. Solution proposed by dsanders seems fine, but I think we should be cautious and not re-process the URL at each page/template generated (the static URL should not changed in time, unless we call collectstatic)...

For information I got the same problem when using a third-party library (django-select2) in a project using a CDN that add a hash suffix to statics : https://github.com/applegrew/django-select2/issues/221#issuecomment-153666276

Last edited 8 years ago by Maxime Lorant (previous) (diff)

comment:8 by Johannes Maron, 8 years ago

Cc: info@… added

I agree this is an issue. It is ether poorly documented that form media behaves different than static files or the behaviour is just wrong.

I'd very much like to clarify that, before making any changes to django-select2. If this is in deed wrong behaviour and not a documentation issue, I can work on this on the DUTH sprints.

comment:9 by Claude Paroz, 8 years ago

I did some experiments with this, leading to https://github.com/claudep/django/commit/2f5c6892523e80d57f9b50bb7bc2e5dcfc07426b
But I'm afraid this suffers from the re-processing load mentioned in comment:7.

comment:10 by Johannes Maron, 8 years ago

Owner: set to Johannes Maron
Status: newassigned

I agree. I'll look into it during the DUTH sprints.

comment:11 by Collin Anderson, 8 years ago

Cc: cmawebsite@… added

comment:12 by Johannes Maron, 8 years ago

Has patch: set

comment:13 by Collin Anderson, 8 years ago

(If we don't like the contrib import from core, and alternative could be to move the necessary code into core #25689). In this case I think it's ok.

comment:14 by Claude Paroz, 8 years ago

Should we lru_cache the static tag results?

comment:15 by Johannes Maron, 8 years ago

I wouldn't. I don't see a big performance opportunity, but the risk that someone wants to manipulate the storage during runtime.

We could rather cache the storage.url method.

I added a second commit that should clean up a little.

Last edited 8 years ago by Johannes Maron (previous) (diff)

comment:16 by Tim Graham, 8 years ago

Patch needs improvement: set

Left some comments for improvement on the pull request.

comment:17 by Johannes Maron, 8 years ago

Patch needs improvement: unset

I hope I caught them all. Thanks Tim

comment:18 by Tim Graham, 8 years ago

Patch needs improvement: set

Left some more comments.

comment:19 by Johannes Maron, 8 years ago

Patch needs improvement: unset

Updated my PR

comment:20 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin
Type: BugNew feature

Looks good pending incorporation of my edits.

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

Resolution: fixed
Status: assignedclosed

In cf546e1:

Fixed #21221 -- Made form Media and static template tag use staticfiles if installed.

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

In 20e2b22:

Refs #21221 -- Added test for legacy static usage in form Media.

Before cf546e1, static files in form or widget Media were usually
wrapped with contrib.staticfiles.templatetags.staticfiles.static.
This test ensures compatibility with third-party code that's still
using this pattern.

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

In a85e842:

Refs #21221 -- Prevented {% static %} tests from using contrib.staticfiles.

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

In 7d607127:

Refs #21221 -- Deprecated staticfiles and admin_static template tag libraries.

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

In 92d4d085:

Refs #21221 -- Removed staticfiles and admin_static template tag libraries.

Per deprecation timeline.

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