Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#17005 closed New feature (fixed)

Add CurrentSiteMiddleware to set the current site on the request

Reported by: jordan@… Owned by: chrismedrela
Component: contrib.sites Version:
Severity: Normal Keywords:
Cc: krzysiumed@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I end up doing something like the following all over my code:

from django.contrib.sites.models import Site

def my_view(request, template_name="foo.html"):
    site = Site.objects.get_current()
    ...
    return render(request, template_name, locals())

Seems like it would save time (and possibly reduce database requests, depending on where else in the application the Site object is being used) if django.contrib.sites.middleware.SiteMiddleware was added, which either set request.site to Site.objects.get_current() or added site or current_site to the RequestContext

I just did a search for "Site.objects.get_current" in one of my web apps and came up with 26 hits. There's really no reason to have to write out that code so many times. It should just be present as a variable somewhere.

Attachments (1)

17005.diff (3.7 KB) - added by Christopher Medrela 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

I think the cleanest solution is to set request.site = Site.objects.get_current(). I wouldn't use get_current_site() because this middleware doesn't make sense when the sites app isn't enabled. The current site is cached, so the performance impact is negligible.

This middleware will be trivial, but it could be useful, and I don't see any reason not to include it in Django. It's similar to the static and media context processors.

comment:2 Changed 5 years ago by jordan@…

I'd be happy to be involved with the coding of it.

It's similar to the static and media context processors."

So do you think it should be made available via a context processor, or as middleware?

comment:2 Changed 5 years ago by jordan@…

I'd be happy to be involved with the coding of it.

It's similar to the static and media context processors."

So do you think it should be made available via a context processor, or as middleware?

comment:3 Changed 5 years ago by Aymeric Augustin

A middleware is more appropriate — I expect the current site object to be used in the view rather than in the template.

The comparison with the static and media context processors was just about the triviality of the code.

comment:4 Changed 5 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned

comment:5 Changed 5 years ago by Christopher Medrela

Owner: changed from anonymous to Christopher Medrela
Status: assignednew

comment:6 Changed 5 years ago by Christopher Medrela

Status: newassigned

Changed 5 years ago by Christopher Medrela

Attachment: 17005.diff added

comment:7 Changed 5 years ago by Christopher Medrela

I'd written a patch. It contains docs and tests (I believe they are not necessary, because the middleware is very simple). Sorry for my poor English in docs.

comment:8 Changed 5 years ago by Simon Charette

Has patch: set

comment:9 Changed 5 years ago by Christopher Medrela

Cc: krzysiumed@… added

comment:10 Changed 4 years ago by Aymeric Augustin

#13559 is about adding a context processor for the same purpose in templates.

comment:11 Changed 3 years ago by chrismedrela

Owner: changed from Christopher Medrela to chrismedrela

I've updated the patch: https://github.com/django/django/pull/1939. BTW, krzysiumed is my old nick.

comment:12 Changed 3 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

The code looks good to me. Can someone review the docs?

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In b22d6c47a7e4c7ab26a8b7b033d11fa6743aae86:

Fixed #17005 -- Added CurrentSiteMiddleware to set the current site on each request.

Thanks jordan at aace.org for the suggestion.

comment:14 Changed 3 years ago by Tim Graham

Summary: Adding django.contrib.site.middleware ?Add CurrentSiteMiddleware to set the current site on the request
Note: See TracTickets for help on using tickets.
Back to Top