Skip to content

Fixed #36504 -- Clarified docs for using with FileField storage. #19725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

khosro-web
Copy link

Trac ticket number

Fixed #36504

Branch description

This PR clarifies the Django documentation regarding the usage of override_settings with FileField storage.
It explains why using a simple callable for the storage argument may not behave as expected in tests due to early evaluation at import time.
It adds a note recommending the use of a LazyObject subclass pattern to delay storage evaluation, ensuring override_settings works properly in test scenarios.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Member

@cliff688 cliff688 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this up!

I think the aim of the ticket is to remove the suggestion to use LazyObject altogether. (I don't know if there's a possible suggestion we can replace it with.)

cc'ing the folks (@AhmedNassar7 and @sarahboyce) who added the suggestion in ticket-36269 in case we might be missing something

@sarahboyce
Copy link
Contributor

I think the aim of the ticket is to remove the suggestion to use LazyObject altogether. (I don't know if there's a possible suggestion we can replace it with.)

If we are to remove it, we would need to reopen ticket-36504

Essentially what the example was hoping to do was to mimic how default_storage storage works (which is working in the test). I made a note that we needed to test my suggestion and then I assumed it was tested 🤦

@cliff688
Copy link
Member

cliff688 commented Aug 15, 2025

Essentially what the example was hoping to do was to mimic how default_storage storage works (which is working in the test).

Did a bit of digging and I figured out why it doesn't work: The signal that resets storages’ cached value when settings change only does it explicitly for default_storage and staticfiles_storage

@receiver(setting_changed)
def storages_changed(*, setting, **kwargs):
from django.contrib.staticfiles.storage import staticfiles_storage
from django.core.files.storage import default_storage, storages
if setting in (
"STORAGES",
"STATIC_ROOT",
"STATIC_URL",
):
try:
del storages.backends
except AttributeError:
pass
storages._backends = None
storages._storages = {}
default_storage._wrapped = empty
staticfiles_storage._wrapped = empty

So maybe we may close this ticket by adding a custom signal receiver to the section added in #19349? WDYT

something like:

@receiver(setting_changed)
def reset_other_storage(*, setting, **kwargs):
    if setting in ("STORAGES",):
        other_storage._wrapped = empty

@sarahboyce
Copy link
Contributor

That's a great idea 👏

@cliff688
Copy link
Member

Friendly reminder @khosro-web, when you have updated the PR per the discussions in the review, and are ready for another review, please update the ticket flags in Trac, per these docs, specifically, you need to uncheck "Patch needs improvement"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants