Opened 18 months ago

Closed 16 months ago

Last modified 16 months ago

#22477 closed Cleanup/optimization (fixed)

Remove contrib middleware from MIDDLEWARE_CLASSES global defaults

Reported by: mlavin Owned by: mlavin
Component: Core (Other) Version: 1.7-beta-2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The app-loading refactor has deprecated importing a model which is not in the installed apps #21680. This leads to some incompatible defaults in the global settings defaults. In the defaults INSTALLED_APPS is empty but MIDDLEWARE_CLASSES contains 'django.contrib.sessions.middleware.SessionMiddleware' and 'django.contrib.auth.middleware.AuthenticationMiddleware' which rely on the related apps being in the INSTALLED_APPS. Since most users create their initial settings via startproject this isn't an issue but it still makes for a poor default. It also implies a coupling or dependency between Django core and contrib.auth and contrib.sessions which doesn't actually exist.

My recommendation would be to remove any contrib middleware from the MIDDLEWARE_CLASSES in the global settings but continue to set them in the startproject template which also sets an appropriate INSTALLED_APPS setting. Since this setting is created by default in startproject I don't think this raises any major backwards incompatibility issues.

Change History (8)

comment:1 Changed 18 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Indeed, this is inconsistent, but it should hardly ever crop up in practice.

Fixing it technically backwards-incompatible for people who:

  • have removed the declaration from their settings because they're using the global default
  • are importing the global default and tweaking it, for instance by appending some middleware to the list.

I believe the global settings could use a much more fundamental overhaul and stop pretending they're the documentation for settings -- we have ref/settings.txt for this purpose.

I don't have a strong opinion on how to deal with this. I usually prefer tackling problems globally rather than piecewise. But changing a large number of defaults could be unpopular!

comment:2 Changed 18 months ago by mlavin

  • Owner changed from nobody to mlavin
  • Status changed from new to assigned

The first case of people who have removed the declaration could be handled through the system checks similar to the warning for the change to the TEST_RUNNER setting:

I would personally rather fix them piecewise at least on the ticket basis. I know there are larger movements within the community to try to remove or simplify the settings (there are currently 6 settings just for the CSRF cookie) but sweeping changes are difficult to move through the process. Waiting on a more general solution to global settings is letting better be the enemy of done. To me this is a clear inconsistency which is simple to fix and document, it's unlikely to impact many users and some (though not all) of the rare users who might be bitten by this change can be warned through the system checks.

I'll put together a PR.

comment:3 Changed 18 months ago by mlavin

  • Has patch set

Added a PR against 1.7.x If you feel like this shouldn't be back-ported I'll fix to submit vs master.

comment:4 Changed 18 months ago by timo

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

It's a bit late in the 1.7 release cycle to make this change, so I suggest we do it in 1.8. Note that PRs should always be against master anyway; we then backport from there.

comment:5 Changed 16 months ago by timo

  • Component changed from Uncategorized to Core (Other)
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Summary changed from Incompatible Global Settings to Remove contrib middleware from MIDDLEWARE_CLASSES global defaults
  • Version changed from master to 1.7-beta-2

The fact that this results in deprecation warnings when testing reuseable apps with minimal settings came up in IRC so I think we should consider including it in 1.7 after all. Updated PR.

comment:6 Changed 16 months ago by mlavin

Yes if you have a for a reusable app which doesn't include contrib.auth or contrib.sessions in the INSTALLED_APPS and doesn't change the MIDDLEWARE_CLASSES but does hit a view by running through the middleware stack (i.e with the test client) you'll see these deprecation warnings. With this change you'll instead see the system check which for 1.7 at least is louder then the RemovedInDjango19Warning. The fix for it in either way is to be explicit about the required set of MIDDLEWARE_CLASSES for running the tests and if you need the auth or session middleware then those should be in the INSTALLED_APPS as well.

comment:7 Changed 16 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In d94de802d34c858805a01d3c699799aebc247ec3:

[1.7.x] Fixed #22477 -- Removed contrib middleware from the global settings defaults.

Also added a compatibility check for changed middleware defaults.

comment:8 Changed 16 months ago by Tim Graham <timograham@…>

In 4696cd9671243958fd4a596631d75b3cbca325c3:

Fixed #22477 -- Removed contrib middleware from the global settings defaults.

Also added a compatibility check for changed middleware defaults.

Forwardport of d94de802d3 from stable/1.7.x

Note: See TracTickets for help on using tickets.
Back to Top