Opened 7 years ago

Closed 3 years ago

#8995 closed New feature (fixed)

Add support for https to sitemaps

Reported by: john Owned by: aaugustin
Component: contrib.sitemaps Version: master
Severity: Normal Keywords: sitemaps
Cc: miracle2k, john, mmitar@…, maersu 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 5 years ago.
Patch against r13357 for sitemap enhancements.
search-commit-msg (2 bytes) - added by john 5 years ago.
Ignore this.
https_sitemaps.diff (13.0 KB) - added by john 4 years ago.
Patch to support HTTPS URLs in sitemaps
patch_1_sitemaps_https_documentation.diff (1.4 KB) - added by mpaolini 4 years ago.
documentation
django-sitemap-protocol.diff (15.1 KB) - added by john 4 years ago.
Backward-compatible patch with docs

Download all attachments as: .zip

Change History (31)

comment:1 Changed 7 years ago by adrian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 Changed 5 years ago by miracle2k

  • Cc miracle2k added

comment:3 follow-up: Changed 5 years ago by 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?

comment:4 in reply to: ↑ 3 Changed 5 years ago by john

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 Changed 5 years ago by john

  • Cc john 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.

Changed 5 years ago by john

Patch against r13357 for sitemap enhancements.

Changed 5 years ago by john

Ignore this.

comment:6 Changed 5 years ago by mitar

  • Cc mmitar@… added

comment:7 Changed 4 years ago by maersu

  • Cc maersu added

comment:8 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.sitemaps

comment:9 Changed 4 years ago by julien

  • Easy pickings unset
  • Needs tests set
  • Severity set to Normal
  • Type set to New feature

comment:10 Changed 4 years ago by jezdez

  • 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 Changed 4 years ago by john

  • Resolution set to wontfix
  • Status changed from new to 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 Changed 4 years ago by jezdez

I think the protocol attribute is definitely useful, IMO.

comment:13 Changed 4 years ago by mitar

+1

comment:14 Changed 4 years ago by aaugustin

  • Resolution wontfix deleted
  • Status changed from closed to 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 Changed 4 years ago by jezdez

  • Summary changed from django.contrib.sitemaps enhancements to Add support for https to sitemaps

Changing the title..

Changed 4 years ago by john

Patch to support HTTPS URLs in sitemaps

comment:16 Changed 4 years ago by john

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

comment:17 Changed 4 years ago by vsemenov

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

comment:18 Changed 4 years ago by jezdez

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:19 Changed 4 years ago by SmileyChris

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 Changed 4 years ago by jezdez

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 Changed 4 years ago by jezdez

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Changed 4 years ago by mpaolini

documentation

comment:22 Changed 4 years ago by mpaolini

added some documentation for this new feature

Changed 4 years ago by john

Backward-compatible patch with docs

comment:23 Changed 4 years ago by john

  • 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 Changed 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from reopened to new

comment:25 Changed 3 years ago by aaugustin

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

  • Resolution set to fixed
  • Status changed from new to 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