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

Already on GitHub? Sign in to your account

#22477 Fix Incompatible Global Settings #2591

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

mlavin commented Apr 20, 2014

Trac Issue: https://code.djangoproject.com/ticket/22477

This changes the value of MIDDLEWARE_CLASSES in the global settings to be in line with the default INSTALLED_APPS setting. While it is not fully backwards compatible it is unlikely to impact most users since MIDDLEWARE_CLASSES is overridden by the startproject command. A compatibility check has been added to warn users which have not set MIDDLEWARE_CLASSES and rely on the defaults.

I've kept the original commits but if accepted I'm happy to squash them into one. Since this problem is exposed by the app-loading refactor I've submitted the PR against the 1.7.x branch. If that's a problem I'm happy to resubmit against master and move the docs and compatibility check forward to 1.8.

@timgraham timgraham commented on the diff Apr 21, 2014

django/conf/global_settings.py
@@ -461,12 +461,7 @@
# response phase the middleware will be applied in reverse order.
MIDDLEWARE_CLASSES = (
'django.middleware.common.CommonMiddleware',
- 'django.contrib.sessions.middleware.SessionMiddleware',
@timgraham

timgraham Apr 21, 2014

Owner

should we also remove the contrib references in TEMPLATE_CONTEXT_PROCESSORS?

@mlavin

mlavin Apr 21, 2014

Contributor

I think that's a good idea but it's far more disruptive of a change. UnlikeMIDDLEWARE_CLASSES startproject doesn't define TEMPLATE_CONTEXT_PROCESSORS by default. Many project likely override the setting to add django.core.context_processors.request but it is far more likely for projects to rely on the defaults.

@timgraham timgraham commented on the diff Apr 21, 2014

django/core/checks/compatibility/django_1_7_0.py
+ warns them about the global default changes.
+ """
+ from django.conf import settings
+
+ # MIDDLEWARE_CLASSES is overridden by default by startproject. If user's have removed
+ # this override then we'll warn them about the default changes.
+ if not settings.is_overridden('MIDDLEWARE_CLASSES'):
+ return [
+ Warning(
+ "MIDDLEWARE_CLASSES is not set.",
+ hint=("Django 1.7 changed the global defaults for the MIDDLEWARE_CLASSES."
+ "django.contrib.sessions.middleware.SessionMiddleware, "
+ "django.contrib.auth.middleware.AuthenticationMiddleware, and"
+ "django.contrib.messages.middleware.MessageMiddleware were removed from the defaults."
+ "If your project needs these middleware then you should configure this setting."
+ "See https://docs.djangoproject.com/en/dev/releases/1.7/ "
@timgraham

timgraham Apr 21, 2014

Owner

I wonder if this link is really needed. If so, it should at least link to a specific section of the release notes.

@timgraham timgraham commented on the diff Apr 21, 2014

docs/releases/1.7.txt
@@ -1110,6 +1110,22 @@ in a test class which is a subclass of
:class:`~django.test.TransactionTestCase` rather than
:class:`~django.test.TestCase`.
+Default ``MIDDLEWARE_CLASSES``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@timgraham

timgraham Apr 21, 2014

Owner

underline should be the same length as the text above it

@timgraham timgraham commented on the diff Apr 21, 2014

docs/releases/1.7.txt
@@ -1110,6 +1110,22 @@ in a test class which is a subclass of
:class:`~django.test.TransactionTestCase` rather than
:class:`~django.test.TestCase`.
+Default ``MIDDLEWARE_CLASSES``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The app-loading refactor deprecated using models from apps which are not part
+of the :setting:`INSTALLED_APPS` setting. This left an incompatiblity between
+the default :setting:`INSTALLED_APPS` and :setting:`MIDDLEWARE_CLASSES` in the
+global defaults. To bring these settings insync,
@timgraham

timgraham Apr 21, 2014

Owner

"in sync" (two words)

@timgraham timgraham commented on the diff Apr 21, 2014

django/core/checks/compatibility/django_1_7_0.py
@@ -0,0 +1,39 @@
+# -*- encoding: utf-8 -*-
+from __future__ import unicode_literals
+
+from .. import Warning, register, Tags
+
+
+@register(Tags.compatibility)
+def check_1_7_compatibility(**kwargs):
+ errors = []
+ errors.extend(_check_middleware_classes(**kwargs))
+ return errors
+
+
+def _check_middleware_classes(app_configs=None, **kwargs):
@timgraham

timgraham Apr 21, 2014

Owner

Add a test for this check?

@timgraham timgraham commented on the diff Apr 21, 2014

django/core/checks/compatibility/django_1_7_0.py
+
+@register(Tags.compatibility)
+def check_1_7_compatibility(**kwargs):
+ errors = []
+ errors.extend(_check_middleware_classes(**kwargs))
+ return errors
+
+
+def _check_middleware_classes(app_configs=None, **kwargs):
+ """
+ Checks if the user has *not* overridden the ``MIDDLEWARE_CLASSES`` setting &
+ warns them about the global default changes.
+ """
+ from django.conf import settings
+
+ # MIDDLEWARE_CLASSES is overridden by default by startproject. If user's have removed
@timgraham

timgraham Apr 21, 2014

Owner

users (no apostrophe)

Owner

timgraham commented Apr 21, 2014

Please squash the commits and send a PR against master as noted in the ticket, thanks!

@timgraham timgraham closed this Apr 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment