Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

bug 1401246: Update to Django 1.11 #4830

Merged
merged 14 commits into from Jun 1, 2018
Merged

bug 1401246: Update to Django 1.11 #4830

merged 14 commits into from Jun 1, 2018

Conversation

jwhitlock
Copy link
Contributor

The code currently runs in Django 1.8, 1.9, 1.10, and 1.11. This drops support for everything but 1.11. This allows removing a lot of branch code to handle differences between the versions, as well as form widget monkeypatching, and the django-debreach library.

Functional tests are updated to drop support for regex URLs for tests (unused in our code), and to expect relative references (en-US/docs/Web) rather than full URLs (https://developer.mozilla.org/en-US/docs/Web)

Regex URL tests support was copied from Bedrock and not used for MDN tests.
The 404 message in debug mode now includes the locale.
With Django 1.10, redirects to the same domain now use relative
references (the part after the domain name) rather than full URLs.
The default Django logging configuration changed in several ways from
Django 1.8 to 1.11. Instead of merging the changes into our version,
copy the default and add our extensions and customizations.
Django 1.10 includes BREACH protection in CsrfViewMiddleware
On Django 1.9+, this can be replaced with a simple equality assertion.
@jwhitlock jwhitlock requested a review from escattone June 1, 2018 18:44
@escattone escattone self-assigned this Jun 1, 2018
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abosultely ❤️ this PR! Just a tiny fix in kuma/settings/common.py and a minor request regarding the import of django.utils.deprecation.MiddlewareMixin.

There is some additional clean-up that I can see:

  • Removing the Django 1.9, 1.10 ,and 1.11 tox envs in .travis.yml
  • Deleting kuma/core/tests/test_forms.py

but that can happen later, as desired.

Thanks for all of your careful work on this @jwhitlock!


if not settings.DJANGO_1_10:
from . import monkeypatch # noqa

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ This is so satisfying. It actually makes me happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I remember reading monkeypatch.py for the first time a few months ago, and thinking "this must die"

and MIDDLEWARE_CLASSES until a newer version of django-redirect-urls
is available that provides this "out of the box".
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also remove the try-except wrapper around the import of MiddlewareMixin at the top of this file as well as the top of kuma/wiki/middleware.py. So in both files replace:

 # TODO: Remove the try-except wrapper after move to Django 1.10+.
 try:
     from django.utils.deprecation import MiddlewareMixin
 except ImportError:
    MiddlewareMixin = object

with:

from django.utils.deprecation import MiddlewareMixin

@@ -1,59 +0,0 @@
from django.forms import fields, widgets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ❤️ this PR!!!

current_etag = quote_etag(current_etag)
# Django's parse_etags returns a list of quoted rather than
# un-quoted ETags starting with version 1.11.
current_etag = quote_etag(calculate_etag(doc.get_html(section_id)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping it would be resolved this way at this point, so thanks!

--hash=sha256:33d44a5cf9d333247a9a374ae1478b78b83c9b78eb316fc04adde62053b4c047
Django==1.11.13 \
--hash=sha256:18986bcffe69653a84eaf1faa1fa5a7eded32cee41cfecc77fdc65a3e046404d \
--hash=sha256:46adfe8e0abe4d1f026c1086889970b611aec492784fbdfbdaabc2457360a4a5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels so good!

--hash=sha256:1626677c47a6496c40d1794cd7264280dcccc63fc69893a86545bb84af3e0f4b \
--hash=sha256:20fef417bd12de35594ae2456c86ecf65870a24cfb604194254befb59a3dbe5c \
--hash=sha256:efefdb754fbce983302610ca3ec278fb29f86bb01ae6a7defcc573d4048fc43f

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Sorry, it's hard to resist cheering at so much of this PR!

@@ -504,25 +478,15 @@ def _get_locales():
if not MAINTENANCE_MODE:
# We don't want this in maintence mode, as it adds "Cookie"
# to the Vary header, which in turn, kills caching.

if _NEED_DEBREACH:
_MIDDLEWARE += ('debreach.middleware.CSRFCryptMiddleware',)
_MIDDLEWARE += ('django.middleware.csrf.CsrfViewMiddleware',)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed nit, should be MIDDLEWARE rather than _MIDDLEWARE.

@jwhitlock
Copy link
Contributor Author

Force-push (because of busted tests) to fix the missed _MIDDLEWARES and the MiddlewareMixin import.

@escattone
Copy link
Contributor

Looks good except missing the removal of the try-except wrapper around the import of MiddlewareMixin in kuma/wiki/middleware.py.

@jwhitlock
Copy link
Contributor Author

That will teach me to force-push. Removed try/except from kuma/wiki/middleware.py, and updated the kuma_base Dockerfile because they've removed gpg from python:2.7-slim.

* Django 1.10 introduces a new style of middleware.
* Sessions are always verified, and SessionAuthenticationMiddleware does
  nothing.
* Django 1.9 updated the ConditionalGetMiddleware. By Django 1.11, an
  issue with headers for streaming content is fixed. It also uses ETags
  by default, so USE_ETAGS is unneeded.
Django 1.10 updates widgets to use HTML5-style attributes rather than
XHTML style, so this monkeypatch is no longer needed.
The admin urlpatterns now includes a namespace, and include() is not
used to wrap it.
In Django 1.9, this changed from .mime_type to .content_type.
These settings were useful for a Django 1.8/1.9/1.10/1.11 code base, and
are no longer needed for a 1.11-only code base. Look for DJANGO_2_0 in 2019.
python:2.7-slim no longer includes gpg, and appears to include other
changes. This may be because it is not the same as
python:2.7-slim-stretch, based on Debian 9. For now, pin to
python:2.7-slim-jessie, based on Debian 8.
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @jwhitlock!

@escattone escattone merged commit 17fe129 into master Jun 1, 2018
@@ -1,4 +1,4 @@
FROM python:2.7-slim
FROM python:2.7-slim-jessie
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, python:2.7-slim moved from being the same tag as python:2.7-slim-jessie to python:2.7-slim-stretch:

https://github.com/docker-library/official-images/pull/4385/files#diff-051bb6df61b6c6e7f8c1868985011b07R43

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants