[Django] #21587: Make generic RedirectView default to permanent=False
Django
-------------------------------+----------------------------
Reporter: wraus@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Generic views | Version: 1.6
Severity: Normal | Keywords: redirect, view
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+----------------------------
Having been bitten by this, and seeing some other reports of it, it seems
very unintuitive that RedirectView defaults to permanent.
Permanent redirects are cached on the browser, so the ramifications of
accidentally making a permanent redirect away from an URL you are
currently using or intend to use in the future are fairly serious. On the
flip side, using a non-permanent redirect when you meant to use a
permanent one has little real impact outside of taking slightly more time
to redirect.
Having RedirectView default the permanent argument to False wouldn't
actually break existing sites, since it will still redirect, so there's no
serious backwards compatibility issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: wraus@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by anonymous):
* needs_better_patch: => 0
* type: Uncategorized => Cleanup/optimization
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:1>
Django
Reporter: wraus@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by alasdair):
I think this is a good idea.. Changing the default to permanent=False
would be consistent with the
[https://docs.djangoproject.com/en/dev/topics/http/shortcuts/#redirect
redirect shortcut], which returns temporary redirects by default.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:2>
Django
Reporter: wraus@… | Owner: Melevir
Type: | Status: assigned
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Melevir):
* owner: nobody => Melevir
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:3>
Django
Reporter: wraus@… | Owner: Melevir
Type: | Status: assigned
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Melevir):
Done in https://github.com/django/django/pull/2064
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:4>
Django
Reporter: wraus@… | Owner: Melevir
Type: | Status: assigned
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
This is very much backward incompatible; changing the redirect code can
have consequences, especially with SEO.
How are you planning to tackle the deprecation cycle?
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:5>
Django
Reporter: wraus@… | Owner: Melevir
Type: | Status: assigned
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Melevir):
May be my changes was done too fast.
Good idea here looks like to show deprecation warnings first and actually
changing default behavior later.
So, solution for now is to show warnings? Am I right?
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:6>
Django
Reporter: wraus@… | Owner: Melevir
Type: | Status: assigned
Cleanup/optimization | Version: 1.6
Component: Generic views | Resolution:
Severity: Normal | Triage Stage:
Keywords: redirect, view | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bouke):
* needs_docs: 0 => 1
Comment:
You should also make a note of the changes in the
[[https://docs.djangoproject.com/en/dev/ref/class-based-
views/base/#redirectview|documenation]].
One way to tackle this issue is to have RedirectView use a permanent
redirect and some sort of 'future package' containing the new RedirectView
with temporary redirect. e.g. `from django.views.generic.base.future
import RedirectView`
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:7>
Django
Reporter: wraus@… | Owner: Melevir
Type: Cleanup/optimization | Status: assigned
Component: Generic views | Version: 1.6
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by timo):
* cc: timo (added)
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Russ and I discussed this on IRC.
From him: "Given the way that browsers hard cache permanent redirects, I
think there's potential for a footgun in having permanent the default. So
yes, I think [making this change is] worth it. As for migration strategy -
Our process would dictate a full 2 release deprecation seems excessive for
this. We've made exceptions for some small things in the past (e.g.,
changes to the profane words list) and given that we have the checks
command as well, a 1 release warning cycle seems appropriate to me. The
full "from future" approach seems excessive.
me: so if a subclass doesn't specify the permanent flag, we'd warn now,
then change the default in 1.8.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:8>
Django
Reporter: wraus@… | Owner: Melevir
Type: Cleanup/optimization | Status: assigned
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by kcphysics):
Added deprecation warning for default change from True to False in the
upcoming versions.
Added documentation about the change to the docs.
Added test to make sure that warnings are issued.
Done in https://github.com/django/django/pull/2432
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:9>
Django
Reporter: wraus@… | Owner: Melevir
Type: Cleanup/optimization | Status: assigned
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by Raumkraut):
Should a similar change to the default also be implemented for the
`contrib.redirects` app? It would make sense to me to change any other
occurrences of 301-as-default at the same time.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:10>
Django
Reporter: wraus@… | Owner: gregloy
Type: Cleanup/optimization | Status: assigned
Component: Generic views | Version: 1.6
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by gregloy):
* owner: Melevir => gregloy
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:11>
Django
Reporter: wraus@… | Owner: gregloy
Type: Cleanup/optimization | Status: assigned
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 1 | UI/UX: 0
Changes (by gregloy):
* needs_better_patch: 1 => 0
Comment:
New patch:
https://github.com/django/django/pull/2564
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:12>
Django
Reporter: wraus@… | Owner: gregloy
Type: Cleanup/optimization | Status: assigned
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* easy: 1 => 0
Comment:
As noted in the comment on the PR, the current approach would still raise
a warning in this case:
{{{#!python
class CustomRedirectView(RedirectView):
permanent = False
}}}
I think we could fix this with this approach:
{{{#!python
_sentinel = object()
class RedirectView(View):
permanent = _sentinel # empty objects evaluate to True
...
def __init__(self, **kwargs):
if 'permanent' not in kwargs and self.permanent is _sentinel:
# raise deprecation warning
...
}}}
The tricky part then becomes writing a check for this (which is necessary
at this point since we're making a subtle change to a default behavior)
and I'm not sure exactly how to approach this.
In any case, I'm removing the `easy pickings` flag.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:13>
Django
Reporter: wraus@… | Owner:
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.6
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => new
* owner: gregloy =>
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:14>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: redirect, view | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* status: new => assigned
* needs_docs: 1 => 0
* version: 1.6 => master
* owner: => berkerpeksag
* needs_better_patch: 1 => 0
Comment:
Here's a new try: https://github.com/django/django/pull/3597
I've added a test for the case in comment:13.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:15>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Component: Generic views | Version: master
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9a30acad8a1996c914351bad981d937de4db29a4"]:
{{{
#!CommitTicketReference repository=""
revision="9a30acad8a1996c914351bad981d937de4db29a4"
Fixed #21587 -- Added a warning for changing default of
RedirectView.permanent.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:16>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: timo (removed)
* cc: berkerpeksag (added)
* status: closed => new
* has_patch: 1 => 0
* resolution: fixed =>
Comment:
I think the strategy for silencing deprecation warnings is faulty. We need
to silence when they are used in URLconfs similar to how is done in the
[https://github.com/django/django/blob/e9d1f1182aaccaa8b60cf6a3491f0103d2bb0229/tests/urlpatterns_reverse/urls.py#L17-L18
url tests] although I think it needs to be:
{{{
with warnings.catch_warnings(record=True):
warnings.filterwarnings('ignore', category=RemovedInDjango20Warning)
}}}
(at least the `module='...'` approach doesn't seem to work in
[https://github.com/django/django/pull/3626 PR 3626], although I am not
sure why.
Right now the ordering of tests affects whether or not you will see
errors. For example, run the following and you should get a warning:
{{{
python -Wall runtests.py test_client_regress
}}}
Since we are running with:
{{{
warnings.simplefilter("default", RemovedInDjango19Warning)
warnings.simplefilter("default", RemovedInDjango20Warning)
}}}
in `runtests.py`, I think only the first occurrence of the warning in the
test suite is output.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:17>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: new
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by berkerpeksag):
I think the patch is wrong too. Here is a WIP version:
https://gist.github.com/berkerpeksag/4b6d3843a1941593741c
`RedirectViewDeprecationTest` is failing now, I'll take a look at it.
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:18>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/3632
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:19>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: new
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"47789410dbf351fd17a7854492b7b5b99a66bb1c"]:
{{{
#!CommitTicketReference repository=""
revision="47789410dbf351fd17a7854492b7b5b99a66bb1c"
Corrected deprecation warnings for RedirectView; refs #21587.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:20>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Component: Generic views | Version: master
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:21>
Django
Reporter: wraus@… | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: closed
Component: Generic views | Version: master
Severity: Normal | Resolution: fixed
Keywords: redirect, view | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"6e13c0490d67cdf210411f08feca3b78a49645ea"]:
{{{
#!CommitTicketReference repository=""
revision="6e13c0490d67cdf210411f08feca3b78a49645ea"
Changed RedirectView.permanent to False per deprecation timeline; refs
#21587.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21587#comment:22>