Code

Opened 4 months ago

Last modified 4 days ago

#21587 new Cleanup/optimization

Make generic RedirectView default to permanent=False

Reported by: wraus@… Owned by:
Component: Generic views Version: 1.6
Severity: Normal Keywords: redirect, view
Cc: timo Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
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.

Attachments (0)

Change History (14)

comment:1 Changed 4 months ago by anonymous

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

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

comment:3 Changed 4 months ago by Melevir

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

comment:5 Changed 4 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 4 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 4 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 4 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 5 weeks 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 2 weeks 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 5 days ago by gregloy

  • Owner changed from Melevir to gregloy

comment:12 Changed 5 days ago by gregloy

  • Patch needs improvement unset

comment:13 Changed 4 days 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 4 days ago by gregloy

  • Owner gregloy deleted
  • Status changed from assigned to new

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.