Opened 4 years ago

Closed 16 months ago

Last modified 16 months 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 krzysiumed 3 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 4 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 4 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 4 years ago by aaugustin

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 3 years ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:5 Changed 3 years ago by krzysiumed

  • Owner changed from anonymous to krzysiumed
  • Status changed from assigned to new

comment:6 Changed 3 years ago by krzysiumed

  • Status changed from new to assigned

Changed 3 years ago by krzysiumed

comment:7 Changed 3 years ago by krzysiumed

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 3 years ago by charettes

  • Has patch set

comment:9 Changed 3 years ago by krzysiumed

  • Cc krzysiumed@… added

comment:10 Changed 2 years ago by aaugustin

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

comment:11 Changed 19 months ago by chrismedrela

  • Owner changed from krzysiumed to chrismedrela

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

comment:12 Changed 16 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:13 Changed 16 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 16 months ago by timo

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