Opened 3 months ago

Last modified 2 weeks ago

#35674 assigned New feature

Provide a check for settings removed (post deprecation)

Reported by: Serafeim Papastefanos Owned by: Ronny Vedrilla
Component: Core (System checks) Version: 5.1
Severity: Normal Keywords:
Cc: Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Natalia Bidart)

This ticket was repurposed following the original report and a related forum conversation to provide a check for "outdated settings", meaning those settings that went thru the usual and expected deprecation process and have been removed from the code base. These settings are effectively pointless to have defined, as they no longer have any effect, and even in some cases, having them defined might lead project maintainers to mistakenly believe they are still in effect.

Original report:

Hello friends, I had a DEFAULT_FILE_STORAGE = "storages.backends.s3.S3Storage" in my Django 5.0.8 project. When I upgraded it the project worked but the user-uploaded content was uploaded to the filesystem instead of S3. This happened because my DEFAULT_FILE_STORAGE setting was ignored so it fall back to the STORAGES default (https://docs.djangoproject.com/en/5.0/ref/settings/#storages):

{
    "default": {
        "BACKEND": "django.core.files.storage.FileSystemStorage",
    },
    "staticfiles": {
        "BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage",
    },
}

I know that the release notes mention that these settings are removed but since I fell for it there may be also other users that experience the same problems.

My recommendation is to provide a patch in 5.1 that uses the checks framewortk (https://docs.djangoproject.com/en/5.0/topics/checks/) to make sure that there is no DEFAULT_FILE_STORAGE (and probably a STATICFILES_STORAGE) setting and do not allow starting the project unless the user fixes it.

Change History (22)

comment:1 by Natalia Bidart, 3 months ago

Resolution: wontfix
Status: newclosed

Hello Serafeim Papastefanos, thank you for taking the time to create this report. While I do understand your situation, please note that Django has a strict deprecation policy described in this doc link, where it indicates that a deprecation should last for at least 2 feature releases, including the raising of deprecation warnings accordingly.

There is also a doc describing in detail the recommended steps to upgrade Django to a newer version, which explains how to use the -Wall Python flag to catch and fix deprecations early.

Specifically, in Django 4.2, when running any Django command with this -Wall Python flag, the following deprecation message was being shown:

RemovedInDjango51Warning: The DEFAULT_FILE_STORAGE setting is deprecated. Use STORAGES instead.

Because of the above, and because Django can't possibly add a check for every possible misconfiguration of a Django project, I'll be closing this ticket accordingly.
If you disagree, you can consider starting a new conversation on the Django Forum, where you'll reach a wider audience and likely get extra feedback. More information in the documented guidelines for requesting features.

comment:2 by Serafeim Papastefanos, 3 months ago

Hello Natalia Bidart, I understand and will try to integrate -Wall to my management commands from now on.

Kind regards,
Serafeim

comment:3 by Natalia Bidart, 3 months ago

There is an on-going conversation about a related topic in the Forum so we may re-open this ticket. Further details: https://forum.djangoproject.com/t/deprecation-of-default-file-storage-can-we-easen-the-migration/34284

comment:4 by Tim Graham, 3 months ago

Component: Error reportingCore (System checks)
Easy pickings: unset

comment:5 by Natalia Bidart, 3 months ago

Description: modified (diff)
Resolution: wontfix
Status: closednew
Summary: Provide a check for `DEFAULT_FILE_STORAGE`Provide a check for settings removed (post deprecation)
Triage Stage: UnreviewedAccepted

Edited the description to match what was agreed on the referenced forum topic.

comment:6 by Adam Johnson, 3 months ago

I did a quick grep for setting.*removed in docs/releases and found the below. It’s likely incomplete but a good starting place. I think we shouldn’t have a cut off and should feature all these settings in the check. Projects can be very long-lived...

5.1

  • DEFAULT_FILE_STORAGE
  • STATICFILES_STORAGE

5.0

  • DATABASES->name->TEST->SERIALIZE
  • USE_L10N
  • USE_DEPRECATED_PYTZ
  • CSRF_COOKIE_MASKED

4.1

  • CSRF_COOKIE_MASKED

4.0

  • SECURE_BROWSER_XSS_FILTER
  • PASSWORD_RESET_TIMEOUT_DAYS
  • DEFAULT_HASHING_ALGORITHM

3.1

  • FILE_CHARSET

3.0

  • DEFAULT_CONTENT_TYPE

2.1

  • USE_ETAGS

1.10

  • LOGOUT_URL
  • ALLOWED_INCLUDE_ROOTS
  • TEMPLATE_CONTEXT_PROCESSORS
  • TEMPLATE_DEBUG
  • TEMPLATE_DIRS
  • TEMPLATE_LOADERS
  • TEMPLATE_STRING_IF_INVALID

1.8

  • SEND_BROKEN_LINK_EMAILS
  • CACHE_MIDDLEWARE_ANONYMOUS_ONLY

comment:7 by Natalia Bidart, 3 months ago

Adding to the above, one obvious candidate I see missing is the set of DATABASE_* settings. I think another key file to inspect is docs/internals/deprecation.txt.

comment:8 by Derek Bantel, 2 months ago

In the forum discussion for this ticket, it seems to have someone working on it. If that is not true could this ticket be assigned to me?

comment:9 by Tim Graham, 2 months ago

While I see some benefit of the proposed check, it's also possible for users to define their own custom settings. Thus, I'm not sure it's reasonable to disallow these names for the rest of time.

comment:10 by Ronny Vedrilla, 7 weeks ago

Hi there! I created the first PR, thx Adam for your list. I asked GPT as well and came up with a list, which we have do double-check. Furthermore, it might be a good idea to go back until v1? After all, it's Django, right?

comment:11 by Ronny Vedrilla, 7 weeks ago

@Tim: You are right and we talked about that. But since maintenance is a huge thing in a softwares lifecycle, all the help you can get is a huge plus IMHO.

Therefore I argue in favour of somehow "disencouraging" variables that were already used by the framework. If someone really insists on naming a variable this way (why should you have such a strong opinion about it?), you can still disable the check, right?

comment:12 by Clifford Gama, 7 weeks ago

Owner: set to Ronny Vedrilla
Status: newassigned

comment:13 by Ronny Vedrilla, 7 weeks ago

@Derek Bantel: I've started working on it but need some support. If you want to join, please, feel free to do so 🙂

in reply to:  13 comment:14 by Derek Bantel, 7 weeks ago

Replying to Ronny Vedrilla:

@Derek Bantel: I've started working on it but need some support. If you want to join, please, feel free to do so 🙂

Will do thanks, Ronny.

comment:15 by Ronny Vedrilla, 7 weeks ago

We came up with a first version. Would be happy about some more thoughts/reviews 🙂

comment:16 by Ronny Vedrilla, 7 weeks ago

Has patch: set

comment:17 by Sarah Boyce, 6 weeks ago

Needs documentation: set

comment:18 by Ronny Vedrilla, 5 weeks ago

Needs documentation: unset

Added new feature to 5.2 release notes

comment:19 by Tim Graham, 5 weeks ago

I remain unconvinced that this is a wise change. Are we going to document an ever growing list of a disallowed settings names? Disallowing something so generic as LOGOUT_URL seems like it will cause unneeded annoyance.

Although the check could be disabled (usage of one disallowed setting requires disabling the check completely), this is more of a backward-incompatible change than a new feature.

We have had temporary compatibility checks to point out deprecated settings. I guess that may have been more visible than a deprecation warning in the case of DEFAULT_FILE_STORAGE.

in reply to:  19 comment:20 by Natalia Bidart, 5 weeks ago

Replying to Tim Graham:

I remain unconvinced that this is a wise change. Are we going to document an ever growing list of a disallowed settings names? Disallowing something so generic as LOGOUT_URL seems like it will cause unneeded annoyance.

Although the check could be disabled (usage of one disallowed setting requires disabling the check completely), this is more of a backward-incompatible change than a new feature.

We have had temporary compatibility checks to point out deprecated settings. I guess that may have been more visible than a deprecation warning in the case of DEFAULT_FILE_STORAGE.

Hey Tim, thank you for sharing your thoughts. I appreciate your engagement. However, I want to clarify that the change was discussed and agreed upon in the Django Forum, which is where these conversations typically take place. The audience for ticket comments is smaller, so I encourage you to continue the discussion in the forum. This way, we can follow the usual process and involve more people in the conversation.

Personally, I believe practicality beats purity, and in this case, the benefits of letting users know outweigh any potential issues with someone who chose to use a setting name that was deprecated by Django. Having said that, I agree that we should consider a way to ignore a subset of deprecated setting names. I'll consider this when reviewing the PR.

comment:21 by Natalia Bidart, 5 weeks ago

Patch needs improvement: set

comment:22 by Ülgen Sarıkavak, 2 weeks ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top