Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#21587 closed Cleanup/optimization (fixed)

Make generic RedirectView default to permanent=False

Reported by: wraus@… Owned by: Berker Peksag
Component: Generic views Version: dev
Severity: Normal Keywords: redirect, view
Cc: Berker Peksag Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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.

Change History (22)

comment:1 Changed 10 years ago by anonymous

Type: UncategorizedCleanup/optimization

comment:2 Changed 10 years ago by alasdair

I think this is a good idea. Changing the default to permanent=False would be consistent with the redirect shortcut, which returns temporary redirects by default.

Last edited 10 years ago by alasdair (previous) (diff)

comment:3 Changed 10 years ago by Lebedev Ilya

Owner: changed from nobody to Lebedev Ilya
Status: newassigned

comment:5 Changed 10 years ago 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?

comment:6 Changed 10 years ago by Lebedev Ilya

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?

comment:7 Changed 10 years ago by Bouke Haarsma

Needs documentation: set

You should also make a note of the changes in the 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

comment:8 Changed 10 years ago by Tim Graham

Cc: Tim Graham added
Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

comment:9 Changed 10 years ago 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

comment:10 Changed 10 years ago 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.

comment:11 Changed 10 years ago by gregloy

Owner: changed from Lebedev Ilya to gregloy

comment:12 Changed 10 years ago by gregloy

Patch needs improvement: unset

comment:13 Changed 10 years ago by Baptiste Mispelon

Easy pickings: unset
Patch needs improvement: set

As noted in the comment on the PR, the current approach would still raise a warning in this case:

class CustomRedirectView(RedirectView):
    permanent = False

I think we could fix this with this approach:

_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.

comment:14 Changed 10 years ago by gregloy

Owner: gregloy deleted
Status: assignednew

comment:15 Changed 9 years ago by Berker Peksag

Needs documentation: unset
Owner: set to Berker Peksag
Patch needs improvement: unset
Status: newassigned
Version: 1.6master

Here's a new try:

I've added a test for the case in comment:13.

comment:16 Changed 9 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 9a30acad8a1996c914351bad981d937de4db29a4:

Fixed #21587 -- Added a warning for changing default of RedirectView.permanent.

comment:17 Changed 9 years ago by Tim Graham

Cc: Berker Peksag added; Tim Graham removed
Has patch: unset
Resolution: fixed
Status: closednew

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 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 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 test_client_regress

Since we are running with:

warnings.simplefilter("default", RemovedInDjango19Warning)
warnings.simplefilter("default", RemovedInDjango20Warning)

in, I think only the first occurrence of the warning in the test suite is output.

comment:18 Changed 9 years ago by Berker Peksag

I think the patch is wrong too. Here is a WIP version: RedirectViewDeprecationTest is failing now, I'll take a look at it.

comment:19 Changed 9 years ago by Berker Peksag

Has patch: set

comment:20 Changed 9 years ago by Tim Graham <timograham@…>

In 47789410dbf351fd17a7854492b7b5b99a66bb1c:

Corrected deprecation warnings for RedirectView; refs #21587.

comment:21 Changed 9 years ago by Tim Graham

Resolution: fixed
Status: newclosed

comment:22 Changed 9 years ago by Tim Graham <timograham@…>

In 6e13c0490d67cdf210411f08feca3b78a49645ea:

Changed RedirectView.permanent to False per deprecation timeline; refs #21587.

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