Fixed #33206 -- Add redirect type choice to contrib.redirects #15002
Conversation
|
Hello @niccolomineo! Thank you for your contribution 💪 As it's your first contribution be sure to check out the patch review checklist. If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket! If you have any design or process questions then you can ask in the Django forum. Welcome aboard ⛵️! |
There was a problem hiding this comment.
@niccolomineo Thanks for this patch 👍 IMO we should limit this patch only to allow redirect type to be changed (301, 302, 307, 308) per a redirect instance. I would revert swapping the redirect model and a new behavior for 410 ("Gone") redirects.
Moreover, we need to have a clear and backward compatible migration path for exisiting users. There are many questions in this area:
- How to migrate current rows?
- What's a reasonable
default? - How to migrate users with a custom
response_redirect_class?
I'm not sure if this is worth the additional complexity and backward compatibility concerns 🤔
| response_redirect_types = { | ||
| 301: HttpResponsePermanentRedirect, | ||
| 302: HttpResponseRedirect, | ||
| 410: HttpResponseGone | ||
| } |
There was a problem hiding this comment.
Do we need this mapping? We could redirect to a HttpResponse with the status_code, e.g. HttpResponse(status_code=r.redirect_type).
There was a problem hiding this comment.
Hi thank you for all the feedback.
Re the mapping, of course we can simplify like that.
As far as migrating is concerned, I think we could assume 301 is the default, but you're right those with a custom response_redirect_class will be hindered by this change - and there's perhaps little we can do about it. We could just put an alert in the docs like we're doing for similar breaking changes.
Also, why would you prefer not to have the response "Gone" as a separate choice? Isn't the current behaviour, however logical, kind of implicit?
In general, I am not familiar with the decision path. When exactly am I advised to make the suggested changes?
Because that's another change that is backward incompatible.
This change is backward incompatible and against our stability policy. We also don't have a clear migration path for existing users. I'm going to mark this ticket as "wontfix", we should go back to the mailing list and reach a strong consensus before moving it forward. As a last resort, we can request a vote of the Technical board if discussion will fail to reach a consensus. Personally, I don't think it's worth the additional complexity and backward compatibility concerns. |
|
May I suggest to consider the importance of allowing the choice between permanent and temporary redirects? A single setting (the middleware attribute) for both permanent and temporary redirects really feels arbitrary in the first place, a design choice which does not reflect the way redirects are used by the end user, therefore begging for an override. Allowing the user to choose the redirect type can help serve Django's purpose to "build better apps more quickly", as I was requested to implement this feature from the very ones managing the content publishing unit at the company I work for. Let me know how I can improve this patch, I would be glad to help. |
It's not about the importance of the feature, but the importance of not breaking anything for the ~thousands of installs already using This PR is very large. If the selection of redirect type can be implemented with less impact, whilst supporting all existing logic, it would be more convincing. As Mariusz says this would be best to show and discuss on the mailing list. |
|
I wrote a new proposal here, taking into account what we said: https://groups.google.com/g/django-developers/c/c_ghDNzAN50 Please let me know. |
Original ticket: https://code.djangoproject.com/ticket/33206