Thanks legutierr for all your work on this ticket and patch, and everyone for the comments. I just took some time to review the patch on #15089 and had a long conversation on IRC with legutierr, and here's what I'm thinking:
It does appear that there is some code (CurrentSiteManager, for instance), that wants a Site object and simply cannot pass in the request (barring threadlocals, which is not an option for core/ contrib). I don't think the addition of multitenancy justifies breaking currently-working code. So I think we need to add support for multitenancy (which I'm using here as shorthand for a hook that takes a request and returns a Site object or something compatible) in such a way that it is explicitly enabled by setting, and calling code that fails to provide the request only fails (and in a clear way) when multitenancy has been explicitly enabled. There's also the issue of what third-party code does when it depends on contrib.sites and needs an actual Site model instance, not a RequestSite or some other replacement. 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 Site.objects.get_current() and then adds a "require_site_object" flag to get_current_site(), but this seems backwards to me. There's already a well-established indicator that you want an instance (or queryset) of a certain model, and that is to call a manager method on that model. I'd rather keep Site.objects.get_current() as the supported API for when the calling code depends on the Site model (because, e.g., it's querying the database for another model with an FK to Site), and get_current_site() as the API for when the calling code doesn't care whether contrib.sites is used or not and just wants something with a domain name. (I think this distinction is already somewhat implied by how the two methods are currently used in the Django codebase, but could be better documented. And they could probably named more clearly as well, but that ship may have sailed). In my mind, the introduction of a multitenancy hook (in the 1.4 timeframe) could then look something like this: * Introduce a SITES_BACKEND setting, which defaults to None but can be set to a dotted path to a callable that takes the request and returns a Site object (or something else entirely, which should quack like a Site/RequestSite if the dev wants to use third-party code, but isn't required to). * Add an optional "request" argument to Site.objects.get_current(). 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. * 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. * If SITE_BACKEND is not set, both Site.objects.get_current() and get_current_site() behave exactly as they do now. The result is that it's fully backwards-compatible, with no need to deprecate anything that works now. If you choose to set a SITE_BACKEND, then you must use third-party code that's been updated to either call get_current_site() or pass a request to Site.objects.get_current(). Thoughts? 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.
