#8960 closed (fixed)
"View on site" does not work if contrib.sites is not installed
Reported by: | bmihelac | Owned by: | Adrian Holovaty |
---|---|---|---|
Component: | Contrib apps | Version: | 1.1 |
Severity: | Keywords: | ||
Cc: | Matthijs Kooijman, Walter Doekes | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Code in django.conf.urls.defaults.shortcut expect that contrib.sites is installed and throws error "no such table: django_sites" otherwise.
Attachments (2)
Change History (22)
by , 16 years ago
Attachment: | 8960_View_on_site_does_not_work_if_contrib_sites_is_not_installed.diff added |
---|
comment:1 by , 16 years ago
Has patch: | set |
---|
comment:2 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
Note that the patch includes a print
statement.
I'm going to review this and check it in.
by , 16 years ago
Updated version
comment:4 by , 16 years ago
I just run into the issue as well, so I'd be grateful if the patch could be committed.
I've added an updated version, since code in question was moved to a different file in r9001. Since the patch is slightly hard to read: All this does is put a big if around all the Site-related code. This increases the indent of that code a bit, making the diff so messy... If you apply the patch and run svn diff -x -b
, you can see the changes a lot more clearly (ignore space change).
comment:5 by , 16 years ago
Cc: | added |
---|
comment:8 by , 15 years ago
Cc: | added |
---|
comment:9 by , 15 years ago
I've got no intention to nag, but could somebody (adrian?) commit this rather trivial patch? It's already marked as accepted and breaks the default install for some people...
comment:10 by , 15 years ago
Component: | Core framework → Contrib apps |
---|---|
milestone: | → 1.2 |
Version: | 1.0 → 1.1 |
This is still present in 1.1.1, can we get this bug fixed in 1.2 please?
However, the patch would probably have to be rewritten using RequestSite, which was probably invented after the patch for this issue had been submitted.
comment:11 by , 15 years ago
I just looked into this - in my opinion a much simpler patch is sufficient:
--- django/contrib/contenttypes/views.py +++ django/contrib/contenttypes/views.py @@ -1,6 +1,6 @@ from django import http from django.contrib.contenttypes.models import ContentType -from django.contrib.sites.models import Site +from django.contrib.sites.models import Site, RequestSite from django.core.exceptions import ObjectDoesNotExist def shortcut(request, content_type_id, object_id): @@ -54,7 +54,10 @@ # Fall back to the current site (if possible). if object_domain is None: try: - object_domain = Site.objects.get_current().domain + if Site._meta.installed: + object_domain = Site.objects.get_current().domain + else: + current_site = RequestSite(request).domain except Site.DoesNotExist: pass
If an explicit relationship to Site objects is established in a model (this is covered by the lines above my changes) it could be argued that the method should fail if the sites app is not installed.
Unfortunately i do not have the infrastructure here to create a real patch against the trunk and run tests... I hope this helps, can somebody help to get this into 1.2?
comment:13 by , 15 years ago
Right, what's the rush for ? It's been, what, only 2 years open; other (also trivial) patches are lingering for 4 years or more...
comment:14 by , 15 years ago
I can confirm that floledermann's patch works on django trunk rev 13023. I'd love to see this in the trunk.
comment:15 by , 14 years ago
The current uploaded patch I think it more bloated than is needed and feel that floledermanns patch (http://code.djangoproject.com/ticket/8960#comment:11) would be better suited for check in. I can also confirm that floledermann's patch works on django trunk rev 13237.
comment:16 by , 14 years ago
Until the code is fixed, how about fixing the docs:
http://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.get_absolute_url should say "You will need the sites framework installed."
http://code.djangoproject.com/ticket/2319 patched the sites docs, but I think that's an odd place for that bit of info. If I discover get_absolute_url and "View on site", I have no way of knowing that I need to now go read the site docs.
comment:17 by , 14 years ago
Needs tests: | set |
---|
For people who want to see this get into 1.3, it will need tests at least.
comment:18 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → 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:19 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.
Patch is attached - it only checks if Sites application is installed before introspecting current site.