Opened 11 years ago

Closed 10 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

Description

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 by anonymous, 11 years ago

Type: UncategorizedCleanup/optimization

comment:2 by alasdair, 11 years ago

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 11 years ago by alasdair (previous) (diff)

comment:3 by Lebedev Ilya, 11 years ago

Owner: changed from nobody to Lebedev Ilya
Status: newassigned

comment:5 by loic84, 11 years ago

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 by Lebedev Ilya, 11 years ago

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 by Bouke Haarsma, 11 years ago

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 by Tim Graham, 10 years ago

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 by kcphysics, 10 years ago

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

comment:10 by Raumkraut, 10 years ago

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 by gregloy, 10 years ago

Owner: changed from Lebedev Ilya to gregloy

comment:12 by gregloy, 10 years ago

Patch needs improvement: unset

comment:13 by Baptiste Mispelon, 10 years ago

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 by gregloy, 10 years ago

Owner: gregloy removed
Status: assignednew

comment:15 by Berker Peksag, 10 years ago

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

Here's a new try: https://github.com/django/django/pull/3597

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

comment:16 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 9a30acad8a1996c914351bad981d937de4db29a4:

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

comment:17 by Tim Graham, 10 years ago

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

comment:18 by Berker Peksag, 10 years ago

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.

comment:20 by Tim Graham <timograham@…>, 10 years ago

In 47789410dbf351fd17a7854492b7b5b99a66bb1c:

Corrected deprecation warnings for RedirectView; refs #21587.

comment:21 by Tim Graham, 10 years ago

Resolution: fixed
Status: newclosed

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

In 6e13c0490d67cdf210411f08feca3b78a49645ea:

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

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