Code

Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#8960 closed (fixed)

"View on site" does not work if contrib.sites is not installed

Reported by: bmihelac Owned by: adrian
Component: Contrib apps Version: 1.1
Severity: Keywords:
Cc: matthijs, wdoekes 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)

Change History (22)

comment:1 Changed 6 years ago by bmihelac

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 6 years ago by adrian

  • Owner changed from nobody to adrian
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

Note that the patch includes a print statement.

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

comment:3 Changed 6 years ago by bmihelac

print statement is left accidentally - thanks for noting it

comment:4 Changed 5 years ago by matthijs

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 5 years ago by matthijs

  • Cc matthijs added

comment:6 Changed 5 years ago by matthijs

Any chance of adding this trivial patch?

comment:7 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:8 Changed 5 years ago by wdoekes

  • Cc wdoekes added

comment:9 Changed 5 years ago by matthijs

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 4 years ago by floledermann

  • Component changed from Core framework to Contrib apps
  • milestone set to 1.2
  • Version changed from 1.0 to 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 Changed 4 years ago by floledermann

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 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2.

comment:13 Changed 4 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 4 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 4 years ago by lasko

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 4 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 4 years ago by ericholscher

  • Needs tests set

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

comment:18 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to 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 Changed 4 years ago by lukeplant

(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 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.