Fixed #24133 -- Replaced formatting syntax in success_url placeholders

Thanks Laurent Payot for the report, and Markus Holtermann, Tim Graham
for the reviews.

6 files changed, 94 insertions, 13 deletions

diff --git a/django/views/generic/edit.py b/django/views/generic/edit.py
index e88dd82958..ef70501bc2 100644
--- a/django/views/generic/edit.py
+++ b/django/views/generic/edit.py
@@ -1,4 +1,5 @@
 import inspect
+import re
 import warnings
 
 from django.core.exceptions import ImproperlyConfigured
@@ -162,7 +163,15 @@ class ModelFormMixin(FormMixin, SingleObjectMixin):
         Returns the supplied URL.
         """
         if self.success_url:
-            url = self.success_url % self.object.__dict__
+            if re.search(r'%\([^\)]+\)', self.success_url):
+                warnings.warn(
+                    "%()s placeholder style in success_url is deprecated. "
+                    "Please replace them by the {} Python format syntax.",
+                    RemovedInDjango20Warning, stacklevel=2
+                )
+                url = self.success_url % self.object.__dict__
+            else:
+                url = self.success_url.format(**self.object.__dict__)
         else:
             try:
                 url = self.object.get_absolute_url()
@@ -288,7 +297,15 @@ class DeletionMixin(object):
 
     def get_success_url(self):
         if self.success_url:
-            return self.success_url % self.object.__dict__
+            if re.search(r'%\([^\)]+\)', self.success_url):
+                warnings.warn(
+                    "%()s placeholder style in success_url is deprecated. "
+                    "Please replace them by the {} Python format syntax.",
+                    RemovedInDjango20Warning, stacklevel=2
+                )
+                return self.success_url % self.object.__dict__
+            else:
+                return self.success_url.format(**self.object.__dict__)
         else:
             raise ImproperlyConfigured(
                 "No URL to redirect to. Provide a success_url.")
diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt
index 4e03378f06..0a460b7739 100644
--- a/docs/internals/deprecation.txt
+++ b/docs/internals/deprecation.txt
@@ -158,6 +158,9 @@ details on these changes.
   and ``Storage.save()`` to be defined without a ``max_length`` argument will
   be removed.
 
+* Support for the legacy ``%(<foo>)s`` syntax in ``ModelFormMixin.success_url``
+  will be removed.
+
 .. _deprecation-removed-in-1.9:
 
 1.9
diff --git a/docs/ref/class-based-views/mixins-editing.txt b/docs/ref/class-based-views/mixins-editing.txt
index 3dcf28dae3..0bfbcbd3f7 100644
--- a/docs/ref/class-based-views/mixins-editing.txt
+++ b/docs/ref/class-based-views/mixins-editing.txt
@@ -160,9 +160,15 @@ ModelFormMixin
 
         ``success_url`` may contain dictionary string formatting, which
         will be interpolated against the object's field attributes. For
-        example, you could use ``success_url="/polls/%(slug)s/"`` to
+        example, you could use ``success_url="/polls/{slug}/"`` to
         redirect to a URL composed out of the ``slug`` field on a model.
 
+        .. versionchanged:: 1.8
+
+            Support for the new brace-based Python formatting syntax has been
+            added. The old ``%(slug)s`` placeholder syntax support has been
+            deprecated and will be removed in Django 2.0.
+
     .. method:: get_form_class()
 
         Retrieve the form class to instantiate. If
@@ -248,9 +254,15 @@ DeletionMixin
 
         ``success_url`` may contain dictionary string formatting, which will be
         interpolated against the object's field attributes. For example, you
-        could use ``success_url="/parent/%(parent_id)s/"`` to redirect to a URL
+        could use ``success_url="/parent/{parent_id}/"`` to redirect to a URL
         composed out of the ``parent_id`` field on a model.
 
+        .. versionchanged:: 1.8
+
+            Support for the new brace-based Python formatting syntax has been
+            added. The old ``%(slug)s`` placeholder syntax support has been
+            deprecated and will be removed in Django 2.0.
+
     .. method:: get_success_url()
 
         Returns the url to redirect to when the nominated object has been
diff --git a/docs/releases/1.8.txt b/docs/releases/1.8.txt
index a07a209347..cd9a5de682 100644
--- a/docs/releases/1.8.txt
+++ b/docs/releases/1.8.txt
@@ -388,6 +388,11 @@ Generic Views
   require a ``form_class`` to be provided anymore. If not provided ``form_class``
   defaults to :meth:`~django.views.generic.edit.FormMixin.get_form_class()`.
 
+* Placeholders in :attr:`ModelFormMixin.success_url
+  <django.views.generic.edit.ModelFormMixin.success_url>` now support the Python
+  :py:meth:`str.format()` syntax. The legacy ``%(<foo>)s`` syntax is still
+  supported but will be removed in Django 2.0.
+
 Internationalization
 ^^^^^^^^^^^^^^^^^^^^
 
@@ -1550,6 +1555,13 @@ will be removed in Django 2.0.
 Using a single equals sign with the ``{% if %}`` template tag for equality
 testing was undocumented and untested. It's now deprecated in favor of ``==``.
 
+``%(<foo>)s`` syntax in ``ModelFormMixin.success_url``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The legacy ``%(<foo>)s`` syntax in :attr:`ModelFormMixin.success_url
+ <django.views.generic.edit.ModelFormMixin.success_url>` is deprecated and
+will be removed in Django 2.0.
+
 .. removed-features-1.8:
 
 Features removed in 1.8
diff --git a/tests/generic_views/test_edit.py b/tests/generic_views/test_edit.py
index d8e0193574..b61a11086a 100644
--- a/tests/generic_views/test_edit.py
+++ b/tests/generic_views/test_edit.py
@@ -5,7 +5,7 @@ import warnings
 from django.core.exceptions import ImproperlyConfigured
 from django.core.urlresolvers import reverse
 from django import forms
-from django.test import TestCase, override_settings
+from django.test import TestCase, ignore_warnings, override_settings
 from django.test.client import RequestFactory
 from django.utils.deprecation import RemovedInDjango20Warning
 from django.views.generic.base import View
@@ -143,13 +143,24 @@ class CreateViewTests(TestCase):
         self.assertRedirects(res, 'http://testserver/edit/authors/create/')
         self.assertQuerysetEqual(Author.objects.all(), ['<Author: Randall Munroe>'])
 
+    @ignore_warnings(category=RemovedInDjango20Warning)
     def test_create_with_interpolated_redirect(self):
-        res = self.client.post('/edit/authors/create/interpolate_redirect/',
-                            {'name': 'Randall Munroe', 'slug': 'randall-munroe'})
+        res = self.client.post(
+            '/edit/authors/create/interpolate_redirect/',
+            {'name': 'Randall Munroe', 'slug': 'randall-munroe'}
+        )
         self.assertQuerysetEqual(Author.objects.all(), ['<Author: Randall Munroe>'])
         self.assertEqual(res.status_code, 302)
-        pk = Author.objects.all()[0].pk
+        pk = Author.objects.first().pk
         self.assertRedirects(res, 'http://testserver/edit/author/%d/update/' % pk)
+        # Also test with escaped chars in URL
+        res = self.client.post(
+            '/edit/authors/create/interpolate_redirect_nonascii/',
+            {'name': 'John Doe', 'slug': 'john-doe'}
+        )
+        self.assertEqual(res.status_code, 302)
+        pk = Author.objects.get(name='John Doe').pk
+        self.assertRedirects(res, 'http://testserver/%C3%A9dit/author/{}/update/'.format(pk))
 
     def test_create_with_special_properties(self):
         res = self.client.get('/edit/authors/create/special/')
@@ -272,17 +283,28 @@ class UpdateViewTests(TestCase):
         self.assertRedirects(res, 'http://testserver/edit/authors/create/')
         self.assertQuerysetEqual(Author.objects.all(), ['<Author: Randall Munroe (author of xkcd)>'])
 
+    @ignore_warnings(category=RemovedInDjango20Warning)
     def test_update_with_interpolated_redirect(self):
         a = Author.objects.create(
             name='Randall Munroe',
             slug='randall-munroe',
         )
-        res = self.client.post('/edit/author/%d/update/interpolate_redirect/' % a.pk,
-                        {'name': 'Randall Munroe (author of xkcd)', 'slug': 'randall-munroe'})
+        res = self.client.post(
+            '/edit/author/%d/update/interpolate_redirect/' % a.pk,
+            {'name': 'Randall Munroe (author of xkcd)', 'slug': 'randall-munroe'}
+        )
         self.assertQuerysetEqual(Author.objects.all(), ['<Author: Randall Munroe (author of xkcd)>'])
         self.assertEqual(res.status_code, 302)
-        pk = Author.objects.all()[0].pk
+        pk = Author.objects.first().pk
         self.assertRedirects(res, 'http://testserver/edit/author/%d/update/' % pk)
+        # Also test with escaped chars in URL
+        res = self.client.post(
+            '/edit/author/%d/update/interpolate_redirect_nonascii/' % a.pk,
+            {'name': 'John Doe', 'slug': 'john-doe'}
+        )
+        self.assertEqual(res.status_code, 302)
+        pk = Author.objects.get(name='John Doe').pk
+        self.assertRedirects(res, 'http://testserver/%C3%A9dit/author/{}/update/'.format(pk))
 
     def test_update_with_special_properties(self):
         a = Author.objects.create(
@@ -368,12 +390,18 @@ class DeleteViewTests(TestCase):
         self.assertRedirects(res, 'http://testserver/edit/authors/create/')
         self.assertQuerysetEqual(Author.objects.all(), [])
 
+    @ignore_warnings(category=RemovedInDjango20Warning)
     def test_delete_with_interpolated_redirect(self):
         a = Author.objects.create(**{'name': 'Randall Munroe', 'slug': 'randall-munroe'})
         res = self.client.post('/edit/author/%d/delete/interpolate_redirect/' % a.pk)
         self.assertEqual(res.status_code, 302)
         self.assertRedirects(res, 'http://testserver/edit/authors/create/?deleted=%d' % a.pk)
         self.assertQuerysetEqual(Author.objects.all(), [])
+        # Also test with escaped chars in URL
+        a = Author.objects.create(**{'name': 'Randall Munroe', 'slug': 'randall-munroe'})
+        res = self.client.post('/edit/author/{}/delete/interpolate_redirect_nonascii/'.format(a.pk))
+        self.assertEqual(res.status_code, 302)
+        self.assertRedirects(res, 'http://testserver/%C3%A9dit/authors/create/?deleted={}'.format(a.pk))
 
     def test_delete_with_special_properties(self):
         a = Author.objects.create(**{'name': 'Randall Munroe', 'slug': 'randall-munroe'})
diff --git a/tests/generic_views/urls.py b/tests/generic_views/urls.py
index f4965e2317..1bef0a8b77 100644
--- a/tests/generic_views/urls.py
+++ b/tests/generic_views/urls.py
@@ -1,3 +1,6 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
 from django.conf.urls import url
 from django.contrib.auth import views as auth_views
 from django.views.decorators.cache import cache_page
@@ -74,9 +77,11 @@ urlpatterns = [
         views.NaiveAuthorCreate.as_view(success_url='/edit/authors/create/')),
     url(r'^edit/authors/create/interpolate_redirect/$',
         views.NaiveAuthorCreate.as_view(success_url='/edit/author/%(id)d/update/')),
+    url(r'^edit/authors/create/interpolate_redirect_nonascii/$',
+        views.NaiveAuthorCreate.as_view(success_url='/%C3%A9dit/author/{id}/update/')),
     url(r'^edit/authors/create/restricted/$',
         views.AuthorCreateRestricted.as_view()),
-    url(r'^edit/authors/create/$',
+    url(r'^[eé]dit/authors/create/$',
         views.AuthorCreate.as_view()),
     url(r'^edit/authors/create/special/$',
         views.SpecializedAuthorCreate.as_view()),
@@ -87,7 +92,9 @@ urlpatterns = [
         views.NaiveAuthorUpdate.as_view(success_url='/edit/authors/create/')),
     url(r'^edit/author/(?P<pk>[0-9]+)/update/interpolate_redirect/$',
         views.NaiveAuthorUpdate.as_view(success_url='/edit/author/%(id)d/update/')),
-    url(r'^edit/author/(?P<pk>[0-9]+)/update/$',
+    url(r'^edit/author/(?P<pk>[0-9]+)/update/interpolate_redirect_nonascii/$',
+        views.NaiveAuthorUpdate.as_view(success_url='/%C3%A9dit/author/{id}/update/')),
+    url(r'^[eé]dit/author/(?P<pk>[0-9]+)/update/$',
         views.AuthorUpdate.as_view()),
     url(r'^edit/author/update/$',
         views.OneAuthorUpdate.as_view()),
@@ -99,6 +106,8 @@ urlpatterns = [
         views.NaiveAuthorDelete.as_view(success_url='/edit/authors/create/')),
     url(r'^edit/author/(?P<pk>[0-9]+)/delete/interpolate_redirect/$',
         views.NaiveAuthorDelete.as_view(success_url='/edit/authors/create/?deleted=%(id)s')),
+    url(r'^edit/author/(?P<pk>[0-9]+)/delete/interpolate_redirect_nonascii/$',
+        views.NaiveAuthorDelete.as_view(success_url='/%C3%A9dit/authors/create/?deleted={id}')),
     url(r'^edit/author/(?P<pk>[0-9]+)/delete/$',
         views.AuthorDelete.as_view()),
     url(r'^edit/author/(?P<pk>[0-9]+)/delete/special/$',