Opened 4 years ago

Closed 4 years ago

#16129 closed Cleanup/optimization (wontfix)

FormView's "success_url" attr should accept named URLs

Reported by: renatopedigoni Owned by: nobody
Component: Generic views Version: 1.3
Severity: Normal Keywords:
Cc: renatopedigoni@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX:

Description

IMHO success_url should also accept named urls. I wrote a very simple patch that uses 'redirect' (from django.shortcuts) instead of HttpResponseRedirect.

Attachments (1)

patch.diff (1.2 KB) - added by renatopedigoni 4 years ago.

Download all attachments as: .zip

Change History (3)

Changed 4 years ago by renatopedigoni

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

I'm -1 on this patch for the following reasons:

  • that would only work for named urls that don't have any captures, since you can't pass *args or **kwargs to redirect;
  • that would allow passing a model instance as success_url, and I don't think that makes sense here;
  • redirect is not very well defined, it does more magic than necessary here, see the source: <quote> # If this doesn't "feel" like a URL, re-raise. </quote>. Ugh.

I think django.shortcuts in general, and redirect in particular, are little hacks to ease developers' lives, and not something that should be used in Django itself. Currently, django.shortcuts is only used in contrib apps, never in the core; and the only functions used are get_object_or_404 and render_to_response. Both of these are more straightforward than redirect; they don't do any magic.

I think success_url = urlresolvers.reverse(my_url_name) is explicit and works in your situation.

However, since I'm not 100% sure that we don't want this feature, I'll mark the ticket as DDN and let a core developer make the decision. If it's accepted, the patch still needs tests and documentations.

comment:2 Changed 4 years ago by lukeplant

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

Closing WONTFIX for the reasons aaugustin gave.

Note: See TracTickets for help on using tickets.
Back to Top