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.