#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)
Change History (13)
by , 16 years ago
Attachment: | django_requestsite.patch added |
---|
comment:1 by , 16 years ago
comment:2 by , 16 years ago
milestone: | → 1.2 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 15 years ago
Cc: | added |
---|
comment:4 by , 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 , 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 , 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 , 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 , 15 years ago
milestone: | 1.2 |
---|
1.2 is feature-frozen, moving this feature request off the milestone.
comment:9 by , 15 years ago
Version: | 1.0 → 1.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 , 15 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 , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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 , 14 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.
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