[Django] #17802: Pass the request down to the Sitemap callable
Django
----------------------------------+--------------------
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Component: contrib.sitemaps | Version: SVN
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
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'...
--
Ticket URL: <https://code.djangoproject.com/ticket/17802>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: closed
Component: contrib.sitemaps | Version: SVN
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Changes (by aaugustin):
* status: new => closed
* needs_docs: => 1
* resolution: => needsinfo
* needs_tests: => 1
* needs_better_patch: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:1>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Changes (by me@…):
* status: closed => new
* resolution: needsinfo =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:2>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (by rm_):
Replying to [comment:2 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:3>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by pdina):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
Comment:
I sent a pull request on github, the patch contains test and docs.
https://github.com/django/django/pull/2371
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:4>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:5>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: closed
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* status: new => closed
* resolution: => duplicate
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:6>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* status: closed => new
* resolution: duplicate =>
Comment:
Err. Didn't mean to close it.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:7>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by rm_):
Replying to [comment:6 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
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:8>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by rm_):
Replying to [comment:6 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. The side effect though is that i'm very much interested in the
patch [2] attached to #15089 [3] 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
[3] https://code.djangoproject.com/ticket/15089
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:9>
Django
Reporter: rm_ | Owner: nobody
Type: New feature | Status: closed
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timo):
* status: new => closed
* resolution: => duplicate
Comment:
If this ticket will be obsoleted by #15089, then I guess we can close this
as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/17802#comment:10>