[Django] #18455: Form media rendering doesn't respect STATICFILES_STORAGE
Django
----------------------------+--------------------
Reporter: djw | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by djw):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I've created a patch here:
https://github.com/djw/django/tree/ticket_18455
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/media.py 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:1>
Django
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Comment (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).
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:2>
Django
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Comment (by djw):
I've added a couple of commits to that branch which
a. Reset the static files storage backend in setUp and tearDown, so that
the full test suite passes
b. Adds a test which fails on current master, but passes on this branch
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:3>
Django
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Design decision needed
Changes (by aaugustin):
* stage: Unreviewed => Design decision needed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:4>
Django
-----------------------------+------------------------------------
Reporter: djw | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Changes (by jezdez):
* type: Bug => New feature
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:5>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:6>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* cc: james@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:7>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:8>
Django
-----------------------------+------------------------------------
Type: New feature | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jaylett):
I was thinking as a function, not as a templatetag (which is sometimes how
it's used inside `contrib.admin`).
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:9>
Django
Reporter: djw | Owner: nobody
Type: New feature | Status: closed
Component: Forms | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mlorant):
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #21221, which has a PR pending.
--
Ticket URL: <https://code.djangoproject.com/ticket/18455#comment:10>