Opened 3 years ago

Closed 3 weeks ago

#18455 closed New feature (duplicate)

Added hooks to Media for staticfiles app

Reported by: djw Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: james@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


When constructing the URLs for form media, django simply prepends the asset path with MEDIA_URL or STATIC_URL. Instead it should use staticfiles_storage.url(path).

Change History (10)

comment:1 Changed 3 years ago by djw

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I've created a patch here:

All the tests in 'forms' pass with this patch applied. However, they fail when running as part of the full test suite, and I need some advice on how to fix this.

The issue is that the tests in forms/tests/ run with overridden STATIC_URL and MEDIA_URL. When run in isolation, the static files storage backend is initialised based on these settings. However if the backend has been initialised before these tests run, it doesn't respect the new values of STATIC_URL and MEDIA_URL, so the tests fail. What's the best way to go about fixing this?

Also the absolute_path() method has an optional prefix argument — I can't see where that's used, so I dont know how to support it properly.

comment:2 Changed 3 years ago by claudep

Please try also to write a test which doesn't pass with the current code, to demonstrate this ticket's rationale.

About tests not passing in full run, try to see which global variable is the culprit and reset this variable in the test setUp method (or use the settings_change signal).

comment:3 Changed 3 years ago by djw

I've added a couple of commits to that branch which

  1. Reset the static files storage backend in setUp and tearDown, so that the full test suite passes
  2. Adds a test which fails on current master, but passes on this branch

comment:4 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design decision needed

By importing django.contrib.staticfiles in django.forms, you're adding a dependency of core on contrib. This is at best frowed upon, at worst prohibited.

if the backend has been initialised before these tests run, it doesn't respect the new values of STATIC_URL and MEDIA_URL, so the tests fail. What's the best way to go about fixing this?

We should add a listener on the setting_changed signal that resets caches that depend on STATIC_URL or MEDIA_URL.

comment:5 Changed 3 years ago by jezdez

  • Summary changed from Form media rendering doesn't respect STATICFILES_STORAGE to Added hooks to Media for staticfiles app
  • Triage Stage changed from Design decision needed to Accepted
  • Type changed from Bug to New feature

Using the staticfiles_storage in the Django core is a no-go. The staticfiles app is a contrib app and should be considered optional.

You can however easily override that by providing an own Media class, as documented. I suggest to add an own class to the staticfiles app so users can use that in their form class.

Of course this implies that we first of all clean up the Media class API in the forms library in the first place. E.g. renaming it to Static for example.

So to summarize I think this should be fixed by:

  1. add a new Static class to the form library that has hooks to implement different way to get the path of a file
  2. add a Static subclass to staticfiles that users can import in their forms modules to use the staticfiles app instead of just joining with STATIC_URL

While you're there you should also think about extending the Static class to be more flexible with regard to multiple files etc.

This ticket obviously can only handle a few of those issues, so please open new tickets for the other parts.

comment:6 Changed 5 months ago by jaylett

How should this work with 3rd party apps? contrib.admin implements its own switcher between staticfiles and the simpler static built-in template tag; perhaps staticfiles should provide such a switcher itself that anyone could use (allowing it to be used in ModelAdmin.Media and Widget.Media inner classes, for instance), and then building the staticfiles subclass of (new, putative) Static around it, meaning that re-usable apps could use that without either tying people into using staticfiles or stopping people from using it. (And preventing everyone having to re-implement the same thing independently.)

comment:7 Changed 5 months ago by jaylett

  • Cc james@… added

comment:8 Changed 5 months ago by aaugustin

There's an issue with providing the switcher in staticfiles: for Django to pick it, you have to add django.contrib.staticfiles to INSTALLED_APPS, which means the switcher loses its purprose.

comment:9 Changed 5 months ago by jaylett

I was thinking as a function, not as a templatetag (which is sometimes how it's used inside contrib.admin).

comment:10 Changed 3 weeks ago by mlorant

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

Duplicate of #21221, which has a PR pending.

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