Opened 8 years ago

Closed 6 years ago

Last modified 5 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: UI/UX:

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 8 years ago.
8960_View_on_site_does_not_work_if_contrib_sites_is_not_installed.2.diff (3.1 KB) - added by Matthijs Kooijman 8 years ago.
Updated version

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by bmihelac

Has patch: set

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

comment:2 Changed 8 years ago by Adrian Holovaty

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 Changed 8 years ago by bmihelac

print statement is left accidentally - thanks for noting it

Changed 8 years ago by Matthijs Kooijman

Updated version

comment:4 Changed 8 years ago by Matthijs Kooijman

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 Changed 8 years ago by Matthijs Kooijman

Cc: Matthijs Kooijman added

comment:6 Changed 8 years ago by Matthijs Kooijman

Any chance of adding this trivial patch?

comment:7 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 Changed 7 years ago by Walter Doekes

Cc: Walter Doekes added

comment:9 Changed 7 years ago by Matthijs Kooijman

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 Changed 7 years ago by Flo Ledermann

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 Changed 7 years ago by Flo Ledermann

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 Changed 7 years ago by Russell Keith-Magee

milestone: 1.21.3

Not critical for 1.2.

comment:13 Changed 7 years ago by anonymous

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 Changed 7 years ago by reconbot

I can confirm that floledermann's patch works on django trunk rev 13023. I'd love to see this in the trunk.

comment:15 Changed 7 years ago by Brandon M Height

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 Changed 6 years ago by CarlFK

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 Changed 6 years ago by Eric Holscher

Needs tests: set

For people who want to see this get into 1.3, it will need tests at least.

comment:18 Changed 6 years ago by Luke Plant

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 Changed 6 years ago by Luke Plant

(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 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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