Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10235 closed (fixed)

contrib.sitemaps doesn't support RequestSite

Reported by: arthurk 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 arthurk 6 years ago.
sitemaps_requestsite_2.diff (2.7 KB) - added by arthurk 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by arthurk

comment:1 Changed 6 years ago by adrian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:3 Changed 6 years ago by arthurk

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

comment:4 Changed 6 years ago by miracle2k

  • Cc miracle2k added

comment:5 Changed 5 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new 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:6 Changed 5 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:7 Changed 5 years ago by lukeplant

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

(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