Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#10235 closed (fixed)

contrib.sitemaps doesn't support RequestSite

Reported by: Arthur Koziel Owned by: nobody
Component: Contrib apps Version: 1.0
Severity: Keywords: sitemap
Cc: miracle2k Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The contrib.sitemaps app will always query the database for a Site object. It doesn't support RequestSite objects (http://docs.djangoproject.com/en/dev/ref/contrib/sites/#requestsite-objects).

The attached patch solves this issue. However, it is pretty hard to test this because the runtests.py has 'django.contrib.sites' in ALWAYS_INSTALLED_APPS, so checking if the app is installed with:

    if Site._meta.installed:
        current_site = Site.objects.get_current()
    else:
        current_site = RequestSite(request)

won't work because the tests will always assume that the app is installed and query the database.

Attachments (2)

sitemaps_requestsite.diff (3.3 KB) - added by Arthur Koziel 8 years ago.
sitemaps_requestsite_2.diff (2.7 KB) - added by Arthur Koziel 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Arthur Koziel

Attachment: sitemaps_requestsite.diff added

comment:1 Changed 8 years ago by Adrian Holovaty

Triage Stage: UnreviewedAccepted

I agree that contrib.sitemaps should support RequestSite -- but before we do that, maybe we should make a helper function that does the following logic (which is repeated multiple times in this patch):

from django.contrib.sites.models import Site, RequestSite
if Site._meta.installed:
    current_site = Site.objects.get_current()
else:
    current_site = RequestSite(request)

comment:2 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:3 Changed 8 years ago by Arthur Koziel

I've revised the patch and added a helper function like you suggested.

Having the request as an attribute on the class would be cleaner than setting site.request in the view but not possible as Sitemaps can be callables as well as objects:

   sitemaps = {
    'foobar': FoobarSitemap,
    'blog': GenericSitemap(info_dict, priority=0.6),
}

FlatPageSitemap doesn't support RequestSite because the flatpages app requires the sites framework and RequestSite isn't a subclass of models.Model so flatpage_set will not be available.

The ping_google function also doesn't use RequestSite because it's impossible to get a request from the CLI. The only possibility I can think of is to specify the full sitemap url as an argument to the ping_google command e.g. "./manage.py ping_google http://foobar.com/sitemap.xml" but according to the docs this argument is already reserved for the absolute sitemap url (e.g. "/sitemap.xml") http://docs.djangoproject.com/en/dev/ref/contrib/sitemaps/#pinging-google-via-manage-py

Changed 8 years ago by Arthur Koziel

Attachment: sitemaps_requestsite_2.diff added

comment:4 Changed 7 years ago by miracle2k

Cc: miracle2k added

comment:5 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: newclosed

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

(In [14141]) Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

comment:8 Changed 6 years ago by Luke Plant

(In [14142]) [1.2.X] Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

Backport of [14141] from trunk.

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