Opened 21 months ago

Closed 9 months ago

Last modified 8 months ago

#21587 closed Cleanup/optimization (fixed)

Make generic RedirectView default to permanent=False

Reported by: wraus@… Owned by: berkerpeksag
Component: Generic views Version: master
Severity: Normal Keywords: redirect, view
Cc: berkerpeksag 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 Changed 21 months ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Cleanup/optimization

comment:2 Changed 21 months 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 21 months ago by alasdair (previous) (diff)

comment:3 Changed 21 months ago by Melevir

  • Owner changed from nobody to Melevir
  • Status changed from new to assigned

comment:5 Changed 21 months 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 21 months ago 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?

comment:7 Changed 21 months ago by bouke

  • 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 21 months ago by timo

  • Cc timo added
  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 18 months 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 https://github.com/django/django/pull/2432

comment:10 Changed 17 months 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 17 months ago by gregloy

  • Owner changed from Melevir to gregloy

comment:12 Changed 17 months ago by gregloy

  • Patch needs improvement unset

comment:13 Changed 17 months ago by bmispelon

  • 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 17 months ago by gregloy

  • Owner gregloy deleted
  • Status changed from assigned to new

comment:15 Changed 10 months ago by berkerpeksag

  • Needs documentation unset
  • Owner set to berkerpeksag
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Version changed from 1.6 to master

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 Changed 9 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 9a30acad8a1996c914351bad981d937de4db29a4:

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

comment:17 Changed 9 months ago by timgraham

  • Cc berkerpeksag added; timo removed
  • Has patch unset
  • Resolution fixed deleted
  • Status changed from closed to new

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 Changed 9 months ago 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.

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

In 47789410dbf351fd17a7854492b7b5b99a66bb1c:

Corrected deprecation warnings for RedirectView; refs #21587.

comment:21 Changed 9 months ago by timgraham

  • Resolution set to fixed
  • Status changed from new to closed

comment:22 Changed 8 months 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