Fixed #26029 -- Added STORAGES setting

[jwygoda]

I'd like to introduce a file storage registry similar to BaseConnectionHandler (django/utils/connection.py) and EngineHandler (django/template/utils.py).

Example settings.py snippet:

STORAGES = {  # rename to FILE_STORAGES to make it more explictit?
    'example': {
        'BACKEND': 'django.core.files.storage.FileSystemStorage',
        'OPTIONS': {
            'location': '/example',
            'base_url': '/example/',
        },
    },
}

Changes introduced by this pr are backward compatible. Users can still use existing settings to configure static and media storages.

Currently storages can be retrieved from the following objects:

django/core/files/storage.py:

• get_storage_class
• DefaultStorage
• default_storage

django/contrib/staticfiles/storage.py:

• ConfiguredStorage
• staticfiles_storage

What do you think about deprecating them?

I'll write tests and docs if this approach is acceptable.

[jwygoda]

I wrote some tests and docs. Should I deprecate stuff used for retrieving storages backends?

[jwygoda]

@felixxm Thanks for the review! I've deprecated get_storage_class. Both DefaultStorage and ConfiguredStorage use this method so we'll get a deprecation warning from there. Should I deprecate these classes separately with custom deprecation messages?

[felixxm]

Both DefaultStorage and ConfiguredStorage use this method so we'll get a deprecation warning from there. Should I deprecate these classes separately with custom deprecation messages?

Yes we should deprecate them separately. Also, they shouldn't use deprecated settings.

[jwygoda]

@felixxm DefaultStorage and ConfiguredStorage need to stay after all because default_storage and staticfiles_storage depend on settings and need to be lazy. I've deprecated STATICFILES_STORAGE and DEFAULT_FILE_STORAGE, wrote tests and docs. We may consider deprecating MEDIA_ROOT, MEDIA_URL, FILE_UPLOAD_PERMISSIONS, FILE_UPLOAD_DIRECTORY_PERMISSIONS, STATIC_ROOT and STATIC_URL because they can be confiured with OPTIONS in STORAGES.

[jwygoda]

@felixxm I've applied requested changes, it's ready for review.

[jwygoda]

@carltongibson Can we tackle the storage argument to FileField in a separate ticket?

Which shortcut do you mean? Is it settings.STORAGES["file_storage"]? If so see https://github.com/django/django/pull/15610/files#diff-1d5b04b0d2298a5675c19aeca9f82d3b5c08870a515f9c5063b77294279365beR2654

[carltongibson]

Hi @jwygoda — sorry for the slow reply. I was ill.

Can we tackle the storage argument to FileField in a separate ticket?

I think 🤔 it's just a docs example — no other changes needed no? — so it would be good to include if you've not totally run out of steam 🙂 — If you have then, we can follow-up yes.

Which shortcut do you mean? Is it settings.STORAGES["file_storage"]?

No. See the comment:1 on the ticket, where have this: FileField(storage='myapp.test.a') with ...if myapp.test.a ... were in the configuration dictionary... — I'm not sure this additional API is needed, but we should decide that before leaving it (as otherwise we miss the window.)

Thanks for the effort here!

[jwygoda]

No worries @carltongibson, hope you're feeling better now. I've updated the docs with an example usage of storages in FileField. I didn't add tests because it relies on the existing functionality of using callable in the storage kwarg. I'd prefer using a key from the STORAGES setting to specify the storage. I'd implement it by adding following to FileField.__init__:

from django.core.files import storages

self.storage = storages.get(self.storage, self.storage)

The only problem I have with this approach is that STORAGES keys can have any type that is immutable but it'll usually be a string and strings are prone to typos. Say someone has STORAGES with default key and a typo in FileField's storage kwarg, e.g. dfault. They wouldn't get an error until the first operation on the storage object, a string in this case. We could mitigate this problem by only allowing strings in STORAGES keys or requiring FileField.storage to be a subclass of Storage. Please let me know what you think about this, maybe I'm overthinking.

[jwygoda]

Is there anything else needed to get this merged? @carltongibson @felixxm

[carltongibson]

Hi @jwygoda — thanks for rebasing.

There's the failure you'll see...

ImportError: cannot import name 'StorageHandler' from 'django.core.files.storage' (/Users/carlton/Projects/Django/django/django/core/files/storage/init.py

Is there anything else needed to get this merged?

The ticket-26029 was marked as Patch needs improvement and Needs tests, so it wouldn't show up on the review queue. I've unchecked those now, so if you can fix the test failures I can take a look again next week or so.

[jwygoda]

Hi @carltongibson, It's fixed now.

[carltongibson]

Great thanks. You're on my list. 🥳

[bhavya-tech]

Hey @jwygoda, I wanted to know if this PR is stable to be merged. I am planning to use the file storage registry for my contribution #15848.

[pauloxnet]

Hey @jwygoda, I wanted to know if this PR is stable to be merged. I am planning to use the file storage registry for my contribution #15848.

I think you can help the process of merge reviewing this PR 🙂

[jwygoda]

@bhavya-tech This pr is ready for review.

[jwygoda]

Yeah, I've struggled for a while with override_settings signal but it seems to be working now. EngineHandler works similarly to BaseConnectionHandler (reseted in clear_cache_handlers signal) and EngineHandler (reset_template_engines) so take a closer look at these. Take your time, better save than sorry.

[felixxm]

I pushed final edits and added release notes.