Hi Gabriel, On Jan 19, 5:55 pm, Gabriel Hurley <gab...@gmail.com> wrote: > > 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.
Yes, I agree that it's worth thinking about what we'll want to do in 1.4, and consider making necessary changes to the new get_current_site() API before 1.3. At this point I don't foresee needing backwards-incompatible API changes to get_current_site() in order to add multitenancy, so I don't think I see anything that needs to be done pre-1.3. > > * 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. Hmm. I agree that the interaction of SITES_BACKEND and Site.objects.get_current() is a relatively weak spot in this proposal, but I like this approach even less. I very much want to confine all the new behaviors to SITES_BACKEND (some common sample backends can be provided), and leave current behavior untouched if you don't define SITES_BACKEND, rather than add new builtin behaviors unrelated to SITES_BACKEND (as the current patch does). Otherwise things get more confusing, it's harder to know how to signal what behavior you want, and maintaining backwards compatibility is trickier. I also think it's important to maintain compatibility between Site.objects.get_current() and get_current_site(); just have the former be a stricter version of the latter. I think it's much better for people who define an unusual SITES_BACKEND to get explicit immediate errors if they have code calling Site.objects.get_current(), telling them in plain terms that they can't use code that depends on the Site model if their SITES_BACKEND returns something else, rather than have a situation where people define SITES_BACKEND and then it only takes effect some of the time (when get_current_site is called). The latter is less explicit, doesn't fail fast, and most importantly doesn't let you define a custom SITES_BACKEND that _does_ return a Site object, and have it take effect when Site.objects.get_current() is called, which you'd almost certainly want in that case. > > * 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. I think it's less brittle and less confusing than the alternatives. It's a pretty clear distinction: the only code that should ever use Site.objects.get_current() is code that explicitly depends on the Site model being used. If you're defining a SITES_BACKEND that returns something other than a Site model, you aren't using the Site model, and therefore you can't use code that explicitly depends on the Site model. Carl -- 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.
