Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#10608 closed (fixed)

Support RequestSite along with Sites.objects.get_current() in contrib

Reported by: Henrik Vendelbo Owned by: nobody
Component: Contrib apps Version: 1.1
Severity: Keywords:
Cc: Sergey Belov Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Most contrib apps assume that the Site model from contrib.sites is used. The login function from auth does a test to see if the Site model is installed, and otherwise
resolves the site using django.contrib.sites.models.RequestSite

This patch duplicates the login function behaviour in
auth.forms, comments.feeds, contenttypes.views, gis.sitemaps.views, sitemaps, and sitemaps.views

Attachments (1)

django_requestsite.patch (6.8 KB ) - added by Henrik Vendelbo 15 years ago.

Download all attachments as: .zip

Change History (13)

by Henrik Vendelbo, 15 years ago

Attachment: django_requestsite.patch added

comment:1 by Henrik Vendelbo, 15 years ago

In admindocs views there is a similar switch which defaults to GenericSite().
It looks like it can be replaced by the same code that I added in the other spots

comment:2 by Jacob, 15 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:3 by Sergey Belov, 15 years ago

Cc: Sergey Belov added

comment:4 by Alex Gaynor, 15 years ago

Patch needs improvement: set

There are a number of places where this patch will do nothing more than raise a NameError since request isn't in scope.

comment:5 by Arthur Koziel, 15 years ago

Even if the request would be in scope, it wouldn't work correctly for some methods. For example the FlatPageSitemap method returns current_site.flatpage_set.all(). It would result in an error if the Sites framework wouldn't be installed, because the Request object doesn't have a flatpage_set.

comment:6 by veena, 15 years ago

The management of the "if statements" in applications will be annoying, don't you think?

Shouldn't there be a Site class not inherited from django.db.models.Model but provide API for getting data? Then if site app is installed it can use database as a storage for site data. If site app is not installed, it will use RequestSite class. Of course, request object is need to be passed.

It forces not to use construction like "current_site.flatpage_set.all()" but "Flatpage.objects.filter(sites=site)" and also "if statements" don't need to be used.

comment:7 by Arthur Koziel, 15 years ago

veena, please see my second patch in #10235. It adds a get_current method that will return the appropriate object.

comment:8 by James Bennett, 14 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:9 by Flo Ledermann, 14 years ago

Version: 1.01.1

I would classify this as a bug, not a feature. Without using sites the "View on site" link is generated in the admin, but fails with an error when clicked - isn't that a bug?

However this should probably be broken down into several bugs dealing with each contrib app, then the consequences of this bug could also be discussed in more detail. Too late for 1.2 anyway, probably.

comment:10 by Flo Ledermann, 14 years ago

Btw the bug for the "View on site" issue is #8960 - also still not resolved, and much older than this issue.

comment:11 by Luke Plant, 13 years ago

Resolution: fixed
Status: newclosed

(In [13980]) Fixed #14386, #8960, #10235, #10909, #10608, #13845, #14377 - standardize Site/RequestSite usage in various places.

Many thanks to gabrielhurley for putting most of this together. Also to
bmihelac, arthurk, qingfeng, hvendelbo, petr.pulc@…, Hraban for
reports and some initial patches.

The patch also contains some whitespace/PEP8 fixes.

comment:12 by Luke Plant, 13 years ago

(In [13987]) [1.2.X] Fixed #14386, #8960, #10235, #10909, #10608, #13845, #14377 - standardize Site/RequestSite usage in various places.

Many thanks to gabrielhurley for putting most of this together. Also to
bmihelac, arthurk, qingfeng, hvendelbo, petr.pulc@…, Hraban for
reports and some initial patches.

The patch also contains some whitespace/PEP8 fixes.

Backport of [13980] from trunk. Some items could be considered features
(i.e. supporting RequestSite in various contrib apps), others are definite
bugs, but it was much more robust to backport all these tightly related
changes together.

Note: See TracTickets for help on using tickets.
Back to Top