Opened 16 years ago

Closed 14 years ago

Last modified 13 years ago

#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)

8960_View_on_site_does_not_work_if_contrib_sites_is_not_installed.diff (3.0 KB ) - added by bmihelac 16 years ago.
8960_View_on_site_does_not_work_if_contrib_sites_is_not_installed.2.diff (3.1 KB ) - added by Matthijs Kooijman 16 years ago.
Updated version

Download all attachments as: .zip

Change History (22)

comment:1 by bmihelac, 16 years ago

Has patch: set

Patch is attached - it only checks if Sites application is installed before introspecting current site.

comment:2 by Adrian Holovaty, 16 years ago

Owner: changed from nobody to Adrian Holovaty
Status: newassigned
Triage Stage: UnreviewedAccepted

Note that the patch includes a print statement.

I'm going to review this and check it in.

comment:3 by bmihelac, 16 years ago

print statement is left accidentally - thanks for noting it

by Matthijs Kooijman, 16 years ago

Updated version

comment:4 by Matthijs Kooijman, 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 Matthijs Kooijman, 16 years ago

Cc: Matthijs Kooijman added

comment:6 by Matthijs Kooijman, 15 years ago

Any chance of adding this trivial patch?

comment:7 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 by Walter Doekes, 15 years ago

Cc: Walter Doekes added

comment:9 by Matthijs Kooijman, 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 Flo Ledermann, 14 years ago

Component: Core frameworkContrib apps
milestone: 1.2
Version: 1.01.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 Flo Ledermann, 14 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:12 by Russell Keith-Magee, 14 years ago

milestone: 1.21.3

Not critical for 1.2.

comment:13 by anonymous, 14 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 reconbot, 14 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 Brandon M Height, 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 CarlFK, 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 Eric Holscher, 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 Luke Plant, 14 years ago

Resolution: fixed
Status: assignedclosed

(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 Luke Plant, 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.

comment:11 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

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