I actually don't think this behaviour should extend to reverse(). There are 
good reasons to use request.current_app in the template, but I don't 
believe this implicit behaviour is warranted outside the templates. Not 
everyone might agree, but I also believe that get_absolute_url() should 
return a single canonical url, independent of the current app. Using the 
current app wouldn't even make sense in most cases, such as the admin or 
pretty much any app other than the model's app itself. I also plan to 
change or replace get_absolute_url somewhere in the next few months, so 
that use case of thread-local storage will soon become obsolete if it's up 
to me. 

If there is a consensus that thread-local storage is the better solution, 
I'll follow suit, but I don't plan on implementing it in this patch. It is 
easily implemented as a third-party app if it suits your use cases. 

I'd like to focus on the best way to solve the backwards compatibility 
issues. 

Op maandag 8 juni 2015 02:32:58 UTC+2 schreef Tai Lee:
>
> I still think that even though `get_absolute_url` might be disliked these 
> days for various reasons, it is very common in the wild and is a perfect 
> example of a legitimate need to be able to reverse URLs in code that 
> executes in response to a request, but that doesn't have the request passed 
> to it as an argument.
>
> So I'd still like to consider the option of adding a default `current_app` 
> hint to thread local storage (which has a life cycle of one request), and 
> for `reverse()` and therefore also `{% url %}` to use that hint as a 
> default. It could still be overridden by explicitly passing `current_app` 
> to `reverse()` (and also `{% url %}`).
>
> This could be done with a middleware class, and in fact a quick search 
> reveals there are many django snippets and blog posts out where users have 
> re-implemented a `ThreadLocalRequestMiddleware` or similar. But I would 
> prefer to see it done before middleware is applied to make it non-optional, 
> since the value will be read in `reverse()` which can and is often called 
> completely outside the request/response cycle.
>
> For any such code that executes outside the request/response cycle, I 
> think we could provide a context manager that sets the `current_app` 
> default hint on `__enter__` and restores it on `__exit__`.
>
> Django appears to use thread locals already to set the current active 
> language. It's been said that is more of a wart than a design pattern to 
> follow, but clearly it can be used safely in Django, and just like the 
> current active language, having apps that are able to resolve their own 
> URLs properly and consistently is an important feature.
>
> Here's a relevant conversation from a couple years back: 
> https://groups.google.com/forum/#!topic/django-developers/VVAZMWi76nA
>
> Cheers.
> Tai.
>
>
> On Monday, June 8, 2015 at 5:29:56 AM UTC+10, Marten Kenbeek wrote:
>>
>> So 2) has been fixed (#24904 and #24906), and I got a PR for 1) (
>> https://github.com/django/django/pull/4724). 
>>
>> About #24127: I see a few options here:
>>
>>
>>    1. A context processor or similar that sets `current_app` if it isn't 
>>    set. By default `current_app` doesn't exist, so there's a good 
>> distinction 
>>    between not set and set to `None`. This would be 100% backwards 
>> compatible, 
>>    but IMO it's not the best solution in the long term. 
>>    2. Provide a new flag to {% url %} to reverse with current_app set to 
>>    request.resolver_match.namespace. This feels slightly less awkward than 
>> the 
>>    current method recommended by the docs.
>>    3. Deprecating a non-existing current_app to force an explicit choice 
>>    between the old and new behaviour, then add the new behaviour after the 
>>    deprecation period. This would be coupled with option 1 for ease-of-use. 
>>    4. A setting? A setting! Yay, another setting...
>>    5. Document the slight backwards-incompatible change? And provide an 
>>    easy way to retain the old behaviour, of course. 1 through 4 are all far 
>>    from ideal, and I don't think there's an easy, backwards-compatible fix 
>>    here. 
>>
>> Thoughts?
>>
>> Op dinsdag 2 juni 2015 14:32:42 UTC+2 schreef Marten Kenbeek:
>>>
>>> The admin proves problematic. Looking at the uses of `AdminSite.name`, 
>>> it's not easily replaced. There are quite a few places that use the name to 
>>> reverse urls, but don't have access to the request and the current 
>>> namespace. I think the best solution for now is to document that you should 
>>> pass `admin.site.urls` directly to `url()`. `include()` does a bunch of 
>>> stuff, but none of it is needed in the case of the admin urls. This allows 
>>> the new `include()` API to be as intended, but it doesn't put an 
>>> unnecessary burden of providing the "correct" namespace for an admin 
>>> instance on the end-developer. 
>>>
>>> In time I'd like to see a method on the AdminSite that returns an actual 
>>> resolver object, using the new API. The default URLconf template would 
>>> simply look something like this:
>>>
>>> urlpatterns = [
>>>     admin.site.get_resolver('^admin/'),  # or 
>>> get_resolver(RegexPattern('^admin/')) to be more explicit
>>> ]
>>>
>>> This would solve the namespace issue for the admin, but it would still 
>>> allow for only obvious way to set the instance namespace, and keep the 
>>> `include()` API clean.
>>>
>>> Op zaterdag 30 mei 2015 14:28:22 UTC+2 schreef Tai Lee:
>>>>
>>>> Right. So if I understand correctly, you can avoid problems even when 
>>>> installing two different apps into the same app namespace as long as you 
>>>> *always* supply `current_app` when reversing URLs? In which case, I don't 
>>>> think we need to worry about being able to change the app namespace to 
>>>> avoid conflicts (maybe we should even disallow it), and instead just make 
>>>> it easier to *always* supply `current_app`, which brings me to my next 
>>>> point.
>>>>
>>>
>>> Not providing an option in the API itself is the best we can do to 
>>> disallow it. But yes, always setting the current_app would avoid a lot of 
>>> problems. https://code.djangoproject.com/ticket/24127 proposes just 
>>> that. 
>>>
>>> I'd take this a step further. In this comment -- 
>>>> https://code.djangoproject.com/ticket/21927#comment:9 
>>>> <https://www.google.com/url?q=https%3A%2F%2Fcode.djangoproject.com%2Fticket%2F21927%23comment%3A9&sa=D&sntz=1&usg=AFQjCNHV0KQtjiZ8tJPhEF_AKH_JCfPInA>
>>>>  
>>>> -- I made a suggestion that Django set a `current_app` hint in thread 
>>>> local 
>>>> storage with a middleware class or even before middleware runs, and then 
>>>> have `reverse()` and `{% url %}` fallback to that default hint ONLY if no 
>>>> current app is explicitly provided via the current mechanisms, so it 
>>>> should 
>>>> be a relatively safe change.
>>>>
>>>> This should make it much more convenient and possible to reverse URLs 
>>>> correctly regardless of conflicting app namespaces, even in code that 
>>>> doesn't have access to the request object. For example, model methods that 
>>>> are executed in templates and do not have access to a `request` or 
>>>> `RequestContext` object. As far as I know, 1 thread = 1 request, so using 
>>>> thread local storage should be safe to provide such a hint for any code 
>>>> that executes in response to a request. For management commands, it would 
>>>> still need to be supplied explicitly. What do you think?
>>>>
>>>> Cheers.
>>>> Tai.
>>>>
>>>
>>> I don't think it's necessary to introduce another global state. At the 
>>> moment there are a few places in the admin that use the name but don't have 
>>> access to the request, this works fine as it is, and if need be these can 
>>> be reworked so that the current app is passed down to these methods too. 
>>> The other place where this would help is `get_absolute_url()`. This only 
>>> adds to the list of why I don't like that method, and I personally don't 
>>> find it a compelling argument for introducing a global state for the 
>>> current app.
>>>
>>> I think setting the current_app on request by default is a better 
>>> solution that works in most cases. The only apparent issue is backwards 
>>> compatibility, I'm still looking for the best way to solve this. 
>>>
>>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/dc3efc1b-c5a7-41cb-b5b0-f58698919a79%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to