Proposal: Allow contrib.sites to use the request host and fallback to a SITE_ID

[Ira Abbott]

We all learn very early in playing with django to set a site and a SITE_ID.  Once we operate as multiple sites, we either use the multiple processes, each configured with a separate SITE_ID or we can now pass a request in.  However,  I assume for backward compatibility, SITE_ID must be removed.  This allows all sites which specify SITE_ID to operate as if the addition of passing request was not added.  However, removing SITE_ID to allow get_site_by_request breaks all applications which do NOT pass request, because there is no SITE_ID.

I propose a setting, which, when set to True in addition to setting SITE_ID, causes request to take precedence over SITE_ID.  This will allow applications using both paradigms to coexist without modification - sites which pass request use get_site_by_request as if SITE_ID had not been defined and calls passing no request get SITE_ID.  When the new setting is not present, all behavior is backward compatible.

Essentially, I am proposing something like the 'change set' I typed in below - I considered that a complex and/or in the fist if could cover all of the conditions, but I think qualifying the first line with the new setting and adding the elif to cover the cleanup case of not set to True is easier to understand - especially in light of understanding the the backward compatibility aspect of the change.  The version of this change I am currently running simply defines a new SITE_DEFAULT_ID to catch the third case, but it occurred to me that some modules may expect SITE_ID directly for some reason, and MIXED_MODE has them covered too. 

I expect the documentation change to accompany the new setting would go with the Sites documentation next to SITE_ID an explanation of removing SITE_ID to use request.

This would be my first contribution, so I am unsure that it would welcome.  If this ticket is accepted, I will quite gladly prepare a pull request with an agreed upon setting name (in case mine doesn't grab ya) or other any other suggested style improvements etc.

 def get_current(self, request=None):
        """
        Return the current Site based on the SITE_ID in the project's settings.
        If SITE_ID isn't defined, return the site with domain matching
        request.get_host(). The ``Site`` object is cached the first time it's
        retrieved from the database.
        """
        from django.conf import settings
        site_id = getattr(settings, 'SITE_ID', '')
        mixed_mode = getattr(settings, 'SITE_MIXED_MODE', '')
        if site_id and not mixed_mode:
            return self._get_site_by_id(site_id)
        elif request:
            return self._get_site_by_request(request)
        elif site_id and mixed_mode == True:
            return self._get_site_by_id(site_id)

        raise ImproperlyConfigured(
            "You're using the Django \"sites framework\" without having "
            "set the SITE_ID setting. Create a site in your database and "
            "set the SITE_ID setting or pass a request to "
            "Site.objects.get_current() to fix this error."
        )

[Carlton Gibson]

Hi Ira, 

Thanks for your post. I wasn't able to follow your use-case exactly:

On Monday, 10 December 2018 13:47:54 UTC+1, Ira Abbott wrote:

We all learn very early in playing with django to set a site and a SITE_ID.  Once we operate as multiple sites, we either use the multiple processes, each configured with a separate SITE_ID or we can now pass a request in.  However,  I assume for backward compatibility, SITE_ID must be removed.  This allows all sites which specify SITE_ID to operate as if the addition of passing request was not added.  However, removing SITE_ID to allow get_site_by_request breaks all applications which do NOT pass request, because there is no SITE_ID.

Can you explain your example more fully please? 

Thanks. 

Kind Regards,

Carlton

 

[Ira Abbott]

Hi Carlton,

Thanks for taking the time to investigate.  I specifically tried to generalize for this forum as I believe the underlying behaviour for which I am proposing change is fundamental enough to warrant attention but this group.

My specific use case involves allauth and dbtemplates, two pip packages that I have run together with SITE_ID set.  I am expanding, or attempting to, to support 'ephemeral sites', which can be skinned via templates stored in dbtemplates.  So just remove SITE_ID, right?

That would be the case if dbtemplats supported request based site lookup, however the template loader portion really cannot and shouldn't be burdened with request context as it can and should be accessible without it.  So, I either 'down-shift' to SITE_ID and gear up on processes, or I look for a code solution, which is more fun for me :)  As always, there are many ways to remove the fur from a feline.

One real-world example of what I hope to be ablate achieve would be support a team or Realtors (tm) who purchase domains for each property sold, with the potential to skin each according to the domain.  I fired up dbtemplates which has a lot of good stuff, but run in to the fact that it requires SITE_ID.  Yes, I am also pursuing the 'fix dbtemplates' route, or find something else, or make it myself, but experience says that this will not be the last time I or others try to pull in pieces that conflict.  I believe this proposal would provide a means of resolution in which none of the packages need to be tweaked to get them to play nicely together when one makes valuable use of request by site and another does not allow it.

I posted (not yet approved) a companion topic to django-users for the purposes of how to work with this for existing releases, best practices for 'overloading' django and apps, etc. (running with patches / tweaks, etc).  I can tweak up dbtemplates to not call for get_current() if no SITE_ID is present, but return a configuration, I can tweak up django and run a fork.  

To bring this back around to justification for a change to such a large install base based on my current understanding:

- These two modes of operation for site managers are incompatible.

- pip is one of the top for repos and django is a major player in that space - the number of django packages is impressive. I see the rear-view roadmap as a natural progression and the addition of the request mech as a good example of 'first do no harm'.  However now that we have had two for a while, I will not be the last person to try to move from SITE_ID to request based sites and find packages that don't play together nicely.  At least I don't think I'm that unique.

- The change is designed to continue wit the 'first do no harm' strategy, so the first requirement is that all existing settings files in the install base work with exactly the same behaviour as previously.  I believe this is baked in to the suggested implementation.

I'm actually surprised that I didn't  'We decided XYZ back in ticket 123'.

I hope I am not just missing something fundamental, I am not an expert with this framework by any stretch of the imagination, mine included.  The specific behaviour does seem to be quite limited in scope, however.  You either do or do not have SITE_ID - if you have it, you get SITE_ID even if you pass in request.  If you do not have SITE_ID, then you MUST pass request, or an exception is thrown.  Never shall the twain meet, unless you have another piece of information and another check.

That much seems very isolated.  The ability to allow one portion of code to get some default site id while allowing others to get the the id based on the request seems like a reasonable enough thing to want to do when generalized.  At least that't the way it seems to me, so I thought I would offer these puny few lines of code back with gratitude for the multitudes of code that came before.

Again, thank you for your consideration and all of the work done by everyone.

Regards,

Ira