Deprecation of "DEFAULT_FILE_STORAGE" - Can we easen the migration?

Django Internals
21

Hi @timgraham!

Thanks for your input! As far as I understood the compatability checks, they point out changes of semantics, like

Required CSRF_TRUSTED_ORIGINS setting to include the scheme

The new check targets removed variables. That’s might be the line to separate these two (in my opinion) valid checks.

I believe warnings for removed, not deprecated (thank you, Carlton, for pointing out my mistake) setting variables do hold a great benefit. The main reason, especially the “for the rest of time” aspect is that Django focusses strongly on maintainability. This is awesome! Try upgrading React.js from one major version to the next. We should help people where we can on this topic.

Therefore, everything that helps the crowd get an upgrade done as soon as possible is a benefit from my point of view. And since we never know at what Django version people might start their upgrade journey, having support here (even since v1.0) is a plus. The other day, I got the request to take over and upgrade a 1.x Django project…

I caused data loss (yes, that’s unlucky and a little stupid from my side) by forgetting to update a variable. I work with Django for 12 years. I guess I know a lot of stuff in that area. I’d argue that if this can happen to me, it can happen to others. Especially if they are either new to Django or the given project.

I work in an agency. We’ve create A LOT of new Django projects over the years. Maintaining them is a constant challenge. Since maintenance almost never has proper customer focus, budget is tight and often devs maintain projects they haven’t built themselves. So again, I think that giving the devs more insight in what might be off is a big benefit.

True, the fellows have to keep this list in mind when removing variables but since neither Sarah nor Natalia have complained, I guess it’s not too much work. And there is a clear trigger when to do this. Furthermore, I don’t expect the list to explode to 500 entries any time soon, right? So I think the trade-off between fellow work and crowd benefit tips definitely to the benefits side.

[…] why are settings a special case that need to be checked for the rest of time?

Lastly, I would argue that settings are a must-have for any Django project. You might use Postgres - or you don’t. You might use templates - or you build something headless. But you have to have settings. Therefore we’ll have the chance to help everybody. In addition, settings are framework-specific and not that easy to grasp (the list is really long in most projects).

Sorry for the wall of text :sweat_smile: I hope I could break a lance for this feature.

Best from Cologne
Ronny

1 Like
22

I don’t understand the rationale of why removed settings are a special case that needs upgrade assistance. (Because everyone uses settings?) If we want to do more to support people upgrading Django against our best practice of one major version at a time, then why not also add back the compatibility checks that have since been removed (and agree not to remove future checks). For example:

  • The BooleanField default changed in Django 1.6.
  • Django 1.6 introduced a new default test runner which changed test discovery.
  • The checks for MIDDLEWARE_CLASSES and the various TEMPLATES_ settings had much more useful messages than “The setting was removed and its use is not recommended.”

Secondly, I remain concerned about disrupting projects using the names of removed settings. Here is a usage of LOGOUT_URL, for example. There is nothing wrong with this code, and someone coming along on a new project has no reason to avoid creating their own LOGOUT_URL setting. A new developer would be mystified at why such a name is disallowed!

Alternatively, perhaps long-lived upgrade checks would be better organized in a third-party package (“django-really-old-upgrade”) so that Django does not have to bear the performance and maintainability costs.

2 Likes
23

On the other hand, for those following the recommended one-version-at-a-time approach, if they don’t react to the deprecation warnings, it’s not clear that an extra warning would do any good.

For projects with less than 100% test coverage the compatibility check is a huge win.

I agree with Tim that breaking code that reimplements LOGOUT_URL is a problem, given that the original setting was never namespaced like CORE_DJANGO_LOGOUT_URL. I also agree that it’s not atomic enough to be forced to silence the whole warning if any of your upstream dependencies are using any of the 50 dead settings.

In terms of what to do immediately, are we considering adding the never-implemented temporary compatibility check for this one case and letting it run for about the amount of time we would have normally run it?

24

For projects with less than 100% test coverage the compatibility check is a huge win.

I agree. I wonder, @nessita, if we could define as a rule that removals always get a compatability system check?

I agree with Tim that breaking code that reimplements LOGOUT_URL is a problem, given that the original setting was never namespaced like CORE_DJANGO_LOGOUT_URL. I also agree that it’s not atomic enough to be forced to silence the whole warning if any of your upstream dependencies are using any of the 50 dead settings.

Would this concern be lessend if we could silence warnings one-by-one? Currently, all warnings are settings.W001. I’m not really happy with that, too and have a couple of things in mind how we could fix that.

25

There’s precedent for this sort of version migration aid as a separate package in (e.g.,) jquery-migrate. (Though even that requires working through multiple steps when doing big version jumps.)

26

Hi there!

I took some time to think about all the headwind that my idea got and I’m going to try to propose a compromise.

Immediate cure for future deprecations

I suggest that the fellows ensure that every dropped variable gets a system check. This check will be around for a couple of versions and is then removed. This will ensure that it’s visible to everyone and that bad things won’t happen to people like me.

I already talked to @sarahboyce about it and she seemed fine with it. Sarah, please correct me if I’m wrong here :sweat_smile:

Maintenance support

I think it’s settled that we want to do something helpful.

It was agreed here initially that trying to do something was worthwhile: these kind of things come up almost every time we remove anything. It would be a long-term win if we were to find a good solution here.

I’m honestly not sure about this - IMHO academical issue - that people might use this one variable for different purposes. But since we’re not where we should be in terms of silencing system checks more precisely, I do see the concerns about “I have to silence everything when I only want to silence one thing.”

I agree with Carlton on having a permanent list of all the variables that were removed. It’s not that the fellows deprecate 200 variables per version. This list will grow very slowly and it could help quite a bunch of people.

Reading the PR: The list of removed settings isn’t uninteresting… […]

To bring together the points of a too broad system check and the wish to help the crowd more, I think I like Carltons idea of an optional check:

I wonder if running this only via a flag --removals (similar to --deploy) which we’d do on upgrading resolves the issue? If the settings were grouped by release, I could pass a --since-version option to limit the check to just recent versions (after 4.2 if I’m updating from the LTS next April, say). (I guess I could maintain an --ignore list if I had shadowed an old setting.)

IF I really am going crazy and reuse a bunch of old variables, at least it won’t be as intrusive as a system check that fires on every dev-server reload. On the other hand, it gives people like me a neat tool at hand when taking over maintaining old projects.

What’s the general feeling about that proposal? I’d be very happy to get this stuff in in 5.2 since it’s a LTS version and will therefore help a lot of people.

Best from Cologne
Ronny

27

  1. I think there need not be a hard and fast rule about deprecating settings in a certain way – not every setting is equal – but where appropriate, I’d certainly encourage using the the system check framework as has been done in the past. For DEFAULT_FILE_STORAGE, it seems a check would be useful, and I wouldn’t veto adding a check to Django 5.1 at this point since continued use of the setting can lead to data loss.

  2. I don’t think we should go down the road of adding upgrade tips to the check command unless there is consensus to revisit the supported versions policy and the recommended advice to upgrade Django one major version at a time. A philosophical change where Django caters to upgrades from arbitrarily old Django versions would impose an additional maintenance cost that I don’t think the project should bear. As I’ve suggested before, interested developers could produce their own “django-really-old-upgrade” package.

1 Like
28

For DEFAULT_FILE_STORAGE, it seems a check would be useful, and I wouldn’t veto adding a check to Django 5.1 at this point since continued use of the setting can lead to data loss.

:ok_hand:

As I’ve suggested before, interested developers could produce their own “django-really-old-upgrade” package.

I’ve moved the code into a package and added some extra tweaks that weren’t possible in the core setup: GitHub - ambient-innovation/django-removals: Package to check for known Django removals and deprecations

Would be happy about some feedback from the invested crowd :slight_smile:

3 Likes
29

Thanks @GitRon — I shared it on Mastodon too

2 Likes
30

The underlying problem hasn’t gone away here. When we have energy we should review what steps we can make to improve the DX here.

Experienced devs silencing a system check doesn’t seem a big lift.

3 Likes