Opened 13 years ago
Closed 11 years ago
#17802 closed New feature (duplicate)
Pass the request down to the Sitemap callable
Reported by: | rm_ | Owned by: | nobody |
---|---|---|---|
Component: | contrib.sitemaps | Version: | dev |
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)
Change History (11)
by , 13 years ago
Attachment: | pass-the-request-down-to-the-sitemap-callable.diff added |
---|
comment:1 by , 13 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Resolution: | → needsinfo |
Status: | new → 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.
Tentatively closing "needsinfo", please reopen if that solution doesn't work.
follow-up: 3 comment:2 by , 11 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → 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 by , 11 years ago
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 by , 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
I sent a pull request on github, the patch contains test and docs.
comment:5 by , 11 years ago
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.
follow-ups: 8 9 comment:6 by , 11 years ago
Resolution: | → duplicate |
---|---|
Status: | new → 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 by , 11 years ago
Resolution: | duplicate |
---|---|
Status: | closed → new |
Err. Didn't mean to close it.
comment:8 by , 11 years ago
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 by , 11 years ago
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?
comment:10 by , 11 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
If this ticket will be obsoleted by #15089, then I guess we can close this as a duplicate.
Diffed against r17606