django icon indicating copy to clipboard operation
django copied to clipboard

Fixed #35335 -- Updated "Enabling the sites framework" docs to reiterate usage of get_current_site.

Open sdarwin opened this issue 2 years ago • 4 comments

Hi, When learning the sites framework, a few questions came to mind and needed to be researched. The idea with this pull request is to address those topics. Feedback is welcome.

  • When considering adding "sites" to our Django site in order to enable a feature of Allauth, it was not clear if the typical implementation of "sites" could be done with one application server (ideally) or must be scaled across multiple servers, each with their own SITE_ID (which would be problematic). It seems the original design would require that. Thus, the sentences about "multiple app servers" to explain the point. I think it's interesting information.

  • added the word "optional" to the directive that you must add a SITE_ID in the settings.

  • added a mention about what is "recommended". If you believe that is going too far, and it's not recommended, it could be edited.

sdarwin avatar Mar 14 '24 17:03 sdarwin

Yes. I like your point

mahiuddin-dev avatar Mar 17 '24 09:03 mahiuddin-dev

As mentioned in the ticket, I think the changes should focus only on updating the "Enabling the sites framework" to reiterate the ability to use get_current_site.

nessita avatar Mar 27 '24 21:03 nessita

Hi Nessita,

I see that you are taking issue with the word "server".

To revisit that particular point, in the original implementation of "sites", where you would hard-code the SITE_ID in the settings file, then in that case... imagine if you would like to have varying behavior for multiple so-called "sites". One nginx vhost, in a docker container, isn't enough. Horizontally scaled replicas of that same container with the same files isn't enough. You really have to do something "extra". And for that I am calling them "servers", to emphasis the point. Most features of Django will "just work" on your one server, when you update the python code. But not this. Not a static SITE_ID. So perhaps the wording could be altered, and I am open to suggestions. "server" conveyed the idea. Perhaps "instances" or "vhosts" or "deployment" instead. In reality, in the world of k8s, you'd probably have multiple deployments. Or multiple VMs.

By the way, pytest is failing on the Boost website if SITE_ID is missing. :-) But per this very issue here, it should be ok , to not set that variable.

sdarwin avatar Mar 27 '24 21:03 sdarwin

After having completed a "sites" framework deployment, and thinking about the situation further, I see that it's more involved and nuanced than just recommending get_current_site(request). Even though get_current_site(request) has more features the most common installations of Django will not use that functionality or multiple domains. They are better off coding a fixed SITE_ID.

With that in mind, I have updated the PR.

Also, the words "server" and "recommended" have been removed.

One way to propose an update, although not required, is to copy-paste a whole section into this github conversation, and modify the text directly. That makes it clear what the modification is.

sdarwin avatar Apr 10 '24 15:04 sdarwin