On Wednesday, January 19, 2011 12:01:57 PM UTC-8, Carl Meyer wrote:
>
> Contrib.flatpages is an example
> of this -- the flatpage model has an FK to Site, so it doesn't make
> sense for flatpages to call a method which might return a Site object
> or might return something else. It needs a real Site object. In my
> mind, this use case is why deprecating Site.objects.get_current() is
> not a good option.The current patch on #15089 deprecates
I'm very much in agreement here. This is what I ran into when I first tried
to replace all the calls to Site.objects.get_current_site() with
get_current_site()... there simply *are* cases where a Site object is
required, and using the object manager is the right way to do it, as you
pointed out.
> In my mind, the introduction of a multitenancy hook (in the 1.4
> timeframe) could then look something like this:
>
Also agreed on the 1.4 timeframe. While there is a valid concern for making
sure that get_current_site() (introduced in 1.3) doesn't end up getting
changed in 1.4, this whole task seems like it's snowballing a small feature
whose main purpose was to be more DRY into a large feature with a totally
different goal, and doing so very late in the game for the 1.3 release
cycle.
> * Add an optional "request" argument to Site.objects.get_current_site(). If
>
> SITES_BACKEND is set and Site.objects.get_current() is not passed the
> request, it's an error. But existing code, with no SITE_BACKEND,
> continues to work fine calling Site.objects.get_current() with no
> request.
>
I'm a little uneasy about this... I think it would be an error for
Site.objects.get_current() to ever return an object which is not a Site
instance, and if we're dealing with arbitrary callables from a SITES_BACKEND
setting being responsible for returning a Site-like object it seems like
this could break in non-obvious ways.
What that means to me is that while you might want
Site.objects.get_current_site() to have multiple ways of retrieving the
current Site object based on data in the request, I don't think those should
be conflated with the arbitrary SITES_BACKEND callables. I'd rather see
Site.objects.get_current_site() try more ways of getting the current site if
a request is passed in, and/or have a separate method on the Site manager
that can take a request and do request-based lookups. Expand the options on
the built-in manager but keep them finite.
I'd say all the SITES_BACKEND functionality should remain tied to
get_current_site() and out of the Site manager.
> * Both Site.objects.get_current() and get_current_site() call
> SITE_BACKEND, if its set, and return whatever it returns.
> Site.objects.get_current() should check this return value and error if
> its not an instance of the Site model. get_current_site() doesn't care
> what it returns.
>
See my previous comment. Raising an exception if the SITES_BACKEND callable
returns an object of the wrong type is one way to handle it, I'm just not
sure it's my favorite solution. Seems brittle given that a developer using a
custom callable might not think about how other apps (especially in contrib)
are going to interact with the return value of their callable.
That's what I've got for now. Thanks to legutierr for the work on the patch
so far.
All the best,
- Gabriel
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
