Make generic RedirectView default to permanent=False

[wraus@…]

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.

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

[Lebedev Ilya]

Done in ​https://github.com/django/django/pull/2064

[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?

[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?

[Bouke Haarsma]

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

[Tim Graham]

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.

[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

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

[gregloy]

New patch:
​https://github.com/django/django/pull/2564

[Baptiste Mispelon]

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.

[Berker Peksag]

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

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

[Tim Graham <timograham@…>]

In 9a30acad8a1996c914351bad981d937de4db29a4:

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

[Tim Graham]

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.

[Berker Peksag]

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.

[Berker Peksag]

​https://github.com/django/django/pull/3632

[Tim Graham <timograham@…>]

In 47789410dbf351fd17a7854492b7b5b99a66bb1c:

Corrected deprecation warnings for RedirectView; refs #21587.

[Tim Graham <timograham@…>]

In 6e13c0490d67cdf210411f08feca3b78a49645ea:

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