Opened 16 years ago

Closed 13 years ago

#8995 closed New feature (fixed)

Add support for https to sitemaps

Reported by: John Hensley Owned by: Aymeric Augustin
Component: contrib.sitemaps Version: dev
Severity: Normal Keywords: sitemaps
Cc: miracle2k, John Hensley, mmitar@…, Marcel Eyer Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I needed to put HTTPS URLs in my sitemaps, and wanted to ping search engines other than Google. The attached patch adds those enhancements.

Attachments (5)

sitemaps.diff​ (7.3 KB ) - added by John Hensley 14 years ago.
Patch against r13357 for sitemap enhancements.
search-commit-msg​ (2 bytes ) - added by John Hensley 14 years ago.
Ignore this.
https_sitemaps.diff​ (13.0 KB ) - added by John Hensley 13 years ago.
Patch to support HTTPS URLs in sitemaps
patch_1_sitemaps_https_documentation.diff​ (1.4 KB ) - added by Marco Paolini 13 years ago.
documentation
django-sitemap-protocol.diff​ (15.1 KB ) - added by John Hensley 13 years ago.
Backward-compatible patch with docs

Download all attachments as: .zip

Change History (31)

comment:1 by Adrian Holovaty, 16 years ago

Triage Stage: Unreviewed → Accepted

Sounds like a reasonable enhancement. It'll have to wait until we have a 1.0.x branch. I have not examined this patch.

comment:2 by miracle2k, 15 years ago

Cc: miracle2k added

comment:3 by miracle2k, 15 years ago

It seems yahoo is pinged twice by this patch, once with the sitemap url (super call), and then again with the base url. Any particular reason for this?

in reply to:  3 comment:4 by John Hensley, 14 years ago

Replying to miracle2k:

It seems yahoo is pinged twice by this patch, once with the sitemap url (super call), and then again with the base url. Any particular reason for this?

Well, I'd like to think it was because in trying to get Yahoo! to crawl my sites I found it necessary, but it's been so long I don't remember, and now it does seem redundant and spammy. The overridden ping method can probably be removed entirely, so just the sitemap is submitted.

comment:5 by John Hensley, 14 years ago

Cc: John Hensley added

I updated the patch to work with recent code. For now I just removed Yahoo, as the ping service seems to be broken and is always returning 403 Forbidden. It's not possible that I reached their rate limit, I tried from multiple IPs, and it seems to be reported pretty widely. Some claim you now need to get an app ID and use the update notification service instead. If that's true, Yahoo hasn't updated their docs.

At any rate, I'm afraid that's all the time I have for this at the moment, so Yahoo's out.

by John Hensley, 14 years ago

Attachment: sitemaps.diff​ added

Patch against r13357 for sitemap enhancements.

by John Hensley, 14 years ago

Attachment: search-commit-msg​ added

Ignore this.

comment:6 by Mitar, 14 years ago

Cc: mmitar@… added

comment:7 by Marcel Eyer, 14 years ago

Cc: Marcel Eyer added

comment:8 by Gabriel Hurley, 14 years ago

Component: Contrib apps → contrib.sitemaps

comment:9 by Julien Phalip, 14 years ago

Easy pickings: unset
Needs tests: set
Severity: → Normal
Type: → New feature

comment:10 by Jannis Leidel, 13 years ago

Patch needs improvement: set
UI/UX: unset

Those are really two features in one ticket, please decide which you want to cover here and open a new ticket for the other feature. Thanks.

comment:11 by John Hensley, 13 years ago

Resolution: → wontfix
Status: new → closed

Sorry, don't have time to spend on this one now. After three years, it's clearly not needed; let's just get it off the list.

comment:12 by Jannis Leidel, 13 years ago

I think the protocol attribute is definitely useful, IMO.

comment:13 by Mitar, 13 years ago

+1

comment:14 by Aymeric Augustin, 13 years ago

Resolution: wontfix
Status: closed → reopened

Let's decide this ticket is only about HTTPS URLs in sitemaps (something I've missed in the past...) and forget about Yahoo, which no longer offers a "ping" feature.

comment:15 by Jannis Leidel, 13 years ago

Summary: django.contrib.sitemaps enhancements → Add support for https to sitemaps

Changing the title..

by John Hensley, 13 years ago

Attachment: https_sitemaps.diff​ added

Patch to support HTTPS URLs in sitemaps

comment:16 by John Hensley, 13 years ago

All right, that's easier. This latest patch just adds HTTPS support.

comment:17 by vsemenov, 13 years ago

Thanks for the patch! This will fix "URLs not followed" warning in Google Webmaster Tools for HTTPS locations in sitemaps.

comment:18 by Jannis Leidel, 13 years ago

Needs tests: unset
Patch needs improvement: unset
Triage Stage: Accepted → Ready for checkin

comment:19 by Chris Beaven, 13 years ago

One question, the current patch changes the index view to use the sitemap's protocol for the site map URL locations rather than just relying on request.is_secure(). Isn't this slightly backwards incompatible?

comment:20 by Jannis Leidel, 13 years ago

That's backwards incompatible indeed, but the feature request is valid. The sitemap class should check if a protocol is specified as a class attribute and fallback to the request.is_secure check. In doubt this could all be a method called get_protocol or similar to allow the users to customize things.

comment:21 by Jannis Leidel, 13 years ago

Patch needs improvement: set
Triage Stage: Ready for checkin → Accepted

by Marco Paolini, 13 years ago

Attachment: patch_1_sitemaps_https_documentation.diff​ added

documentation

comment:22 by Marco Paolini, 13 years ago

added some documentation for this new feature

by John Hensley, 13 years ago

Attachment: django-sitemap-protocol.diff​ added

Backward-compatible patch with docs

comment:23 by John Hensley, 13 years ago

Patch needs improvement: unset

I've added a patch that includes the docs (thanks, Marco) and falls back to the request protocol if the sitemap doesn't specify it.

comment:24 by Aymeric Augustin, 13 years ago

Owner: changed from nobody to Aymeric Augustin
Status: reopened → new

comment:25 by Aymeric Augustin, 13 years ago

There are several major issues with the current patch:

  • it isn't acceptable to slap a request attribute on the Sitemap object and then magically use it within Sitemap.get_urls
  • duplicating all the tests isn't a sustainable approach
  • it seems to me that the patch doesn't actually work because the request attribute is only added in views.index, not views.sitemap

I'm currently working on a more invasive patch that should eventually result in cleaner code. I'll upload a patch when it is ready.

comment:26 by Aymeric Augustin, 13 years ago

Resolution: → fixed
Status: new → closed

In [17409]:

Fixed #8995 -- Added support for HTTPS in sitemaps.

Modularized tests and did a bit of cleanup while I was in the area.

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