Opened 4 years ago

Closed 16 months ago

#17802 closed New feature (duplicate)

Pass the request down to the Sitemap callable

Reported by: rm_ Owned by: nobody
Component: contrib.sitemaps Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It would be handy if the Sitemap would know a bit more about the environment so pass down to it the request. This is useful for me because i serve the same app under different domains and i'd like to serve a bit different sitemap. This could be useful too if you want to serve a different sitemap depending on the REMOTE_ADDR or HTTP_USER_AGENT.

Diffed against r17606.

Tests looks ok:

$ PYTHONPATH=. tests/runtests.py --settings=test_sqlite
Creating test database for alias 'default'...
Creating test database for alias 'other'...
[snip]


Ran 4678 tests in 627.803s

OK (skipped=94, expected failures=2)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

Attachments (1)

pass-the-request-down-to-the-sitemap-callable.diff (1.9 KB) - added by rm_ 4 years ago.
Diffed against r17606

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by rm_

Diffed against r17606

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

This patch doesn't change anything to the public API. It only makes it easier for developers to write subclasses of Sitemap (which isn't officially supported).

If we decided to make this change, it would have to be tested. Otherwise, it could be refactored away by mistake. We could also document more parts of Sitemap, making them public APIs.


However, I'm not sure this patch is the best solution to the original problem. If you "serve the same app under different domains", you must be using different sites, which means different settings.SITE_ID. IMO you should just change the output of your sitemaps based on the current site (for the "different domains" part), or on another setting.

Regarding:

This could be useful too if you want to serve a different sitemap depending on the REMOTE_ADDR or HTTP_USER_AGENT.

nobody's asked for that yet, I invoke the YAGNI principle.

Tentatively closing "needsinfo", please reopen if that solution doesn't work.

Last edited 3 years ago by aaugustin (previous) (diff)

comment:2 follow-up: Changed 20 months ago by me@…

  • Resolution needsinfo deleted
  • Status changed from closed to new

Bump.

I'm using an own set request variable; request.LANGUAGE (based on TLD) to select the content on my site, for example Foo.objects.filter(language=request.LANGUAGE).

Sure this could be set with the Sites framework and use a Site for the lookup instead, however this means I need to run a wsgi process for each language (with different SITE_ID), if I were to add 20 languages for my site with different content I then need to run 20 instances of django.

I'd prefer to add the request object to the Sitemap object instead.

I'm reopening, please consider this again.

comment:3 in reply to: ↑ 2 Changed 20 months ago by rm_

Replying to me@…:

I'm using an own set request variable; request.LANGUAGE (based on TLD) to select the content on my site, for example Foo.objects.filter(language=request.LANGUAGE).

Sure this could be set with the Sites framework and use a Site for the lookup instead, however this means I need to run a wsgi process for each language (with different SITE_ID), if I were to add 20 languages for my site with different content I then need to run 20 instances of django.

Share the same sentiments about SITE_ID

I'm reopening, please consider this again.

Care to respin the patch with the tests aaugustin asked for?

comment:4 Changed 18 months ago by pdina

  • Needs documentation unset
  • Needs tests unset

I sent a pull request on github, the patch contains test and docs.

https://github.com/django/django/pull/2371

comment:5 Changed 18 months ago by aaugustin

Comments 2 and 3 look like they belong to #15089. The problem here isn't sitemaps, it's multi-tenancy -- and it's a hard problem.

comment:6 follow-ups: Changed 18 months ago by aaugustin

  • Resolution set to duplicate
  • Status changed from new to closed

I can't say what the proper solution is, but I'm afraid hardcoding a sitemap-specific solution will only make proper multi-tenancy harder. I think this requires a discussion on the DevelopersMailingList. See also #22231.

comment:7 Changed 18 months ago by aaugustin

  • Resolution duplicate deleted
  • Status changed from closed to new

Err. Didn't mean to close it.

comment:8 in reply to: ↑ 6 Changed 18 months ago by rm_

Replying to aaugustin:

I can't say what the proper solution is, but I'm afraid hardcoding a sitemap-specific solution will only make proper multi-tenancy harder. I think this requires a discussion on the DevelopersMailingList. See also #22231.

Thanks for your comment Aymeric. Proper multi tenancy in django would be very cool (at the time liked a lot alex gaynor's talk [1] about getting the application state from the request) but just adding an attribute to a class it's fixing the issue for at least a couple of us in quite an unobtrusive way.

[1] https://speakerdeck.com/alex/take-2-if-i-got-to-do-django-all-over-again

comment:9 in reply to: ↑ 6 Changed 17 months ago by rm_

Replying to aaugustin:

I can't say what the proper solution is, but I'm afraid hardcoding a sitemap-specific solution will only make proper multi-tenancy harder. I think this requires a discussion on the DevelopersMailingList. See also #22231.

So tried to open a discussion on mailing list few weeks ago without much success (1). The side effect though is that i'm very much interested in the patch (2) attached to #15089 which make this #17802 ticket obsolete. Aymeric, any chance you can review the patch?

  1. https://groups.google.com/forum/#!topic/django-developers/ORo_28B_VIA
  2. https://code.djangoproject.com/attachment/ticket/15089/0001-Make-settings.SITE_ID-optional-for-django.contrib.si.patch
Last edited 17 months ago by rm_ (previous) (diff)

comment:10 Changed 16 months ago by timo

  • Resolution set to duplicate
  • Status changed from new to closed

If this ticket will be obsoleted by #15089, then I guess we can close this as a duplicate.

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