[Django] #17914: reverse() does not support namespaced view references
Django
-------------------------------------------------+--------------------
Reporter: Bradley Ayers <bradley.ayers@…> | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------------------+--------------------
It's not possible to reverse a URL by passing a function reference to
reverse()
that's hooked into the URLconf under a namespace.
Consider the trivial Django project (that doesn't work):
{{{
# urls.py
from django.conf.urls.defaults import patterns, include
from django.core.urlresolvers import reverse
from django.http import HttpResponse
def view(request):
# Return the URL to this view
return HttpResponse(reverse(view))
level2 = patterns('',
(r'^2/', view)
)
urlpatterns = patterns('',
(r'^1/', include(level2, namespace="foo"))
)
}}}
Removing ``, namespace="foo"`` will make it work.
My understanding of why this happens, is that `reverse()` traverses
`RegexURLResolver`s
*only* for strings. Function references are passed directly to the root
`RegexURLResolver.reverse`
method.:
{{{
# from django/core/urlresolvers.py ~ line 432
if not isinstance(viewname, basestring):
view = viewname
else:
# <snip> -- resolver traversal happens here to honor namespaces
# `resolver` is reassigned to last RegexURLResolver in namespace chain
return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args,
**kwargs))
}}}
Obviously traversing namespaces isn't possible for a function reference,
so what's needed is to add *all* descendent view references into each
`RegexURLResolver`'s
`reverse_dict` dictionary.
Populating `reverse_dict` is lazy and happens on first access, the work is
performed in `RegexURLResolver._populate`:
{{{
# django/core/urlresolvers.py ~ line 237
def _populate(self):
# <snip>
for pattern in reversed(self.url_patterns):
# <snip>
if isinstance(pattern, RegexURLResolver):
if pattern.namespace:
# <snip> -- tldr `self.reverse_dict` isn't populated,
`pattern` (the
# RegexURLResolver) is instead stored in
`self.namespace_dict`
else:
# <snip> -- tldr `self.reverse_dict` *is* populated with
the keys
# being the name of each URL (or the function
reference)
# defined in this level. (i.e. only children,
not all descendants)
else:
# <snip> -- tldr `self.reverse_dict` is again populated
}}}
This combination of behaviour by `RegexURLResolver._populate` and
`reverse()`
leads to the problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: 1.3
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Bradley Ayers <bradley.ayers@…>):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:1>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: 1.3
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Changes (by Bradley Ayers <bradley.ayers@…>):
* cc: bradley.ayers@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:2>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Changes (by Bradley Ayers <bradley.ayers@…>):
* version: 1.3 => SVN
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:3>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: SVN
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Comment (by Bradley Ayers <bradley.ayers@…>):
It should be noted that this patch is technically backwards incompatible.
After applying this patch, it's no longer possible to use recursive URL
patterns, e.g. consider a project named 'foo', and it had a {{{urls.py}}}
that contained:
{{{
urlpatterns = patterns('', (r'^abc/', include('foo.urls'))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:4>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: SVN
Component: Core (URLs) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by SmileyChris):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Accepting.
Like we talked about on IRC, let's just avoid circular imports when doing
your traversal rather than dropping that functionality.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:5>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Bradley Ayers <bradley.ayers@…>):
I've updated the patch to maintain support for recursive urlconfs.
https://github.com/django/django/pull/174
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:6>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mtredinnick):
Doing a quick scan of URL reversing during a sprint, the last git code
looks reasonable. Not ready to commit yet -- I haven't fully digested it,
but my intuition is that it's the right kind of thing.
Except(!) that using function objects in reverse() situations is kind of a
"don't do that" kind of situation. We permitted it initially for smooth
integration, but it's kind of a lucky happenstance. My point being, that a
patch of this size is a reasonable to fix this ticket. If it grows to
twice this size, we should maybe just add a "don't do that" note to the
documentation -- the extra complexity isn't worth it.
For now, charge ahead!
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:7>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: amirouche.boubekki@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:8>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: gwahl@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:9>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bpeschier):
Closed #21899 as a duplicate
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:10>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tonnzor):
Any news here?
Since reverse now accepts app_name -- as well as include -- we could
easily find corresponding namespace. E.g.:
{{{
from django.conf.urls import *
urlpatterns = patterns('',
url(r'^payment/', include('payment.urls', namespace='payment',
app_name='payment')),
)
reverse(view, current_app='payment') # try with current_app given
}}}
As alternative solution -- we could require passing namespace when using
with function. The we could convert function to its name, add
namespace+":" and use existing machinery.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:11>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> | Status: new
Type: Bug | Version: master
Component: Core (URLs) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by mrmachine):
#21899 was closed a duplicate of this ticket, but I'm not sure they are
the same thing. Will fixing this ticket also fix #21899? This ticket seems
to be about reversing raw callable view functions, for which we have no
way to specify a namespace. The other ticket was about reversing a dotted
path reference to a view function, specified as a string with a namespace.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:12>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* cc: apollo13 (added)
Comment:
Can someone present a usecase for using function references in reverse?
IMO this should just get closed as wontfix and names should be used
throughout instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:13>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by bpeschier):
This feels like using args and kwargs on reverses. Using either function
reverses or namespaces is fine, just don't mix them.
Maybe we should enforce it a bit more like we do with reversing with args
and kwargs?
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:14>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by guettli):
Replying to [comment:13 apollo13]:
> Can someone present a usecase for using function references in reverse?
IMO this should just get closed as wontfix and names should be used
throughout instead.
For me the view-name is redundant. We try to avoid it. Reversing by
function reference works fine (except this bug). Jumping through the code
with an IDE is much more fun if you use function references.
It would be nice if this could be solved. For our team this means: don't
use namespaced URLs :-(
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:15>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by apollo13):
Replying to [comment:15 guettli]:
> For me the view-name is redundant. We try to avoid it. Reversing by
function reference works fine (except this bug). Jumping through the code
with an IDE is much more fun if you use function references.
If I remember correctly we've been over the redundancy point on the ML
already, you can always just write your own url function which sets the
path of the function as name, not really redundant imo. Also reversing by
function reference is something which is ugly in templates etc imo… And
redundant is relative anyways, "app:view_name" is imo way nicer than
"app.views.view_name" -- especially if you have nested imports.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:16>
Django
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I'm in favor of a "won't fix" of this issue and promoting reversing by URL
name as Florian suggested. We could deprecate passing callables to
reverse, but this will probably just needlessly annoy people who are happy
with it. Instead, we could just remove it from the docs like we did with
the `@permalink` decorator.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:17>
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:18>
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* Attachment "17914.diff" added.
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
Added a documentation patch to discourage reversing by callable view.
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:19>
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by apollo13):
LGTM
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:20>
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a6acfc31837fd7a9e0e387320d995b2c85cfcfce" a6acfc31]:
{{{
#!CommitTicketReference repository=""
revision="a6acfc31837fd7a9e0e387320d995b2c85cfcfce"
Refs #17914 -- Discouraged using reverese() with callables.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:21>
Django
callable
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f32bb3adf039b72cea34347022626231d187e303" f32bb3a]:
{{{
#!CommitTicketReference repository=""
revision="f32bb3adf039b72cea34347022626231d187e303"
[1.8.x] Refs #17914 -- Discouraged using reverese() with callables.
Backport of a6acfc31837fd7a9e0e387320d995b2c85cfcfce from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:22>
Django
reverse()
-------------------------------------+-------------------------------------
Reporter: Bradley Ayers | Owner: nobody
<bradley.ayers@…> |
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/17914#comment:23>