Opened 5 years ago

Closed 3 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: 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_ 5 years ago.
Diffed against r17606

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by rm_

Diffed against r17606

comment:1 Changed 5 years ago by Aymeric Augustin

Needs documentation: set
Needs tests: set
Resolution: needsinfo
Status: newclosed

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 5 years ago by Aymeric Augustin (previous) (diff)

comment:2 Changed 3 years ago by me@…

Resolution: needsinfo
Status: closednew

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 3 years 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 3 years 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 3 years ago by Aymeric Augustin

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 Changed 3 years ago by Aymeric Augustin

Resolution: duplicate
Status: newclosed

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 3 years ago by Aymeric Augustin

Resolution: duplicate
Status: closednew

Err. Didn't mean to close it.

comment:8 in reply to:  6 Changed 3 years 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 3 years 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 3 years ago by rm_ (previous) (diff)

comment:10 Changed 3 years ago by Tim Graham

Resolution: duplicate
Status: newclosed

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