Code

Opened 20 months ago

Closed 9 months ago

#19277 closed New feature (fixed)

LocaleMiddleware permanent redirects

Reported by: ppetrid@… Owned by: Tim Graham <timograham@…>
Component: Internationalization Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

the process_response() method of django.middleware.locale.LocaleMiddleware redirects urls as follows when using i18n_patterns:
http://example.com/whatever/ becomes http://example.com/en/whatever/

These sorts of redirects must be permanent (301) and not temporary (302). The reasoning behind this: 302 redirects suggest that the requested page does have a content of its own (therefore it is a perfectly valid permalink) but for some reason we are temporarily redirected to another page. This is not true in our case, /whatever/ is a duplicate of /en/whatever/ and users will always get redirected as long as the middleware is enabled. Current behavior can be confusing to web crawlers as well.

The HttpResponseRedirect response should become HttpResponsePermanentRedirect to fix this. I'll provide the patch if ticket gets accepted.

Attachments (0)

Change History (14)

comment:1 Changed 20 months ago by claudep

  • Component changed from Translations to Internationalization
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to New feature
  • Version changed from 1.4 to master

I don't think that a permanent redirect is always wanted. Depending on the user agent, the redirected URL will change. You talk about web crawlers, but I guess you'd better find that www.yourdomain.com is kept in the search index instead of www.yourdomain.com/en/.

Google itself is using 302 redirects to redirect to country specific interfaces.

I'm afraid there are no true right answer to this. We could even imagine using the 300 code. I'd be in favour of setting a status_code class attribute for LocaleMiddleware, so it is easy for subclasses to customize it. Accepting on this base.

comment:2 Changed 20 months ago by anonymous

Agreed on the setting. Normaly you would't want both / and /en/ indexed since it will split your backlinks, pagerank etc (and they'll both get indexed unless the developer takes further action). The agent is indeed an issue and when it comes to that 302 redirects are more suitable. As for google, I quit using them as an example the day I found google is the first to break their own guidelines.. :)

comment:3 Changed 20 months ago by EmilStenstrom

  • Owner changed from nobody to EmilStenstrom

Hi!

We're sprinting (Stockholm) at the moment and plan to solve this bug like this:

Instead of setting a status_code attribute, we would like to use a redirect_class. The reason for this is that HttpResponseRedirectBase has built in protection against unsafe protocol redirections. If we use a HttpResponse object and let the user supply their own status code they will probably miss that security issue, which would be a shame.

comment:4 Changed 20 months ago by ppetrid@…

Hello, just my 2 cents: HttpResponseRedirectBase subclasses must be used no matter what. I favor the redirect_class attribute, setting the class through an attribute is a common practice in many aspects over the framework. Besides that, one might use custom HttpResponseRedirects.

comment:5 Changed 20 months ago by EmilStenstrom

Here's a pull request with the changes we suggest: https://github.com/django/django/pull/522

comment:6 Changed 20 months ago by EmilStenstrom

  • Has patch set

comment:7 Changed 20 months ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 20 months ago by ptone

  • Needs documentation set
  • Triage Stage changed from Ready for checkin to Accepted

I agree that a redirect_class is good architecture for this - but the only point of adding this is as some form of API - and if the decision is being made to add API - it needs documentation.

Perhaps as a note in https://docs.djangoproject.com/en/dev/topics/i18n/translation/#how-django-discovers-language-preference

comment:9 Changed 17 months ago by aaugustin

I think it makes sense to make the implementation more subclassing-friendly even without committing to support a given API. We've done that before.

comment:10 Changed 17 months ago by aaugustin

  • Easy pickings unset
  • Patch needs improvement set

It'd be nice to make the implementation, tests (and docs if we decide to document this API) consistent between the locale and redirects middleware (#19321).

The pull request is out of date.

comment:11 Changed 17 months ago by EmilStenstrom

I'll leave this one to a more experienced developer.

Edit: How do I remove myself as owner?

Last edited 17 months ago by EmilStenstrom (previous) (diff)

comment:12 Changed 17 months ago by EmilStenstrom

  • Status changed from new to assigned

comment:13 Changed 17 months ago by EmilStenstrom

  • Owner EmilStenstrom deleted
  • Status changed from assigned to new

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

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 7a97df190cb993056aa097befb3813100cfc286e:

Fixed #19277 -- Added LocaleMiddleware.response_redirect_class

Thanks ppetrid at yawd.eu for the suggestion.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.