Opened 10 years ago

Closed 6 years ago

#23829 closed Cleanup/optimization (fixed)

Make ping_google use https for the sitemap URL

Reported by: Julian Wachholz Owned by: Sanyam Khurana
Component: contrib.sitemaps Version: dev
Severity: Normal Keywords: sitemaps, ping_google
Cc: me@…, Sanyam Khurana Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The django.contrib.sitemaps.ping_google function currently does not allow modification of the protocol and will always send Google a "http://" link.

Maybe we should also add a way to supply an absolute URL (including a protocol) to the function so we can run this from CLI without having contrib.sites installed.

Change History (20)

comment:1 by Danilo Bargen, 10 years ago

Component: contrib.sitemapsDocumentation
Needs documentation: set
Owner: changed from nobody to Danilo Bargen
Status: newassigned
Triage Stage: UnreviewedAccepted

Why would you want to change the protocol?

Anyways, there actually is a way to change the ping URL: https://github.com/django/django/blob/master/django/contrib/sitemaps/__init__.py#L17 It probably needs a doc update though.

comment:2 by Burhan Khalid, 10 years ago

This does not change the scheme, it just changes the URL. The scheme is hard coded (see https://github.com/django/django/blob/master/django/contrib/sitemaps/__init__.py#L42). In addition, this relies on django.contrib.sites, which is not enabled by default.

I would like to propose a fix to this which adds two optional commands to the ping_google method:

  • is_secure, which will flag for https:// (defaults to False)
  • site_domain which can be used to pass in the domain if django.contrib.sites is not used.

Here are my proposed changes: https://github.com/burhan/django/tree/ticket_23829

comment:3 by Danilo Bargen, 10 years ago

Owner: Danilo Bargen removed
Status: assignednew

Ah, you're talking about the scheme of the sitemap URL... Would you mind creating a pull request against master? That would make review easier :)

Here's my (now incomplete) pull request with doc updates: https://github.com/django/django/pull/3527

comment:4 by Burhan Khalid, 10 years ago

Pull request done, and I am getting some help in writing tests as well. See https://github.com/django/django/pull/3528

Last edited 10 years ago by Burhan Khalid (previous) (diff)

comment:5 by Berker Peksag, 10 years ago

Patch needs improvement: set

comment:6 by Tim Graham, 10 years ago

Component: Documentationcontrib.sitemaps
Has patch: set
Needs documentation: unset
Summary: `ping_google` uses hardcoded http protocolAllow customizing the `ping_google` URL

comment:8 by Adam Johnson, 8 years ago

Cc: me@… added

comment:9 by Sanyam Khurana, 6 years ago

Owner: set to Sanyam Khurana
Status: newassigned

comment:10 by Sanyam Khurana, 6 years ago

comment:11 by Sanyam Khurana, 6 years ago

Cc: Sanyam Khurana added
Patch needs improvement: unset

I've addressed the reviews. Can you please take another pass?

comment:12 by Carlton Gibson, 6 years ago

Reviewing, the PR looks almost ready to go.

Posting here for input: it's late-2018, I propose the is_secure flag default to True, i.e. we default to https://....

  • This would be a slight breaking change for anyone using this to send http URLs — they'd need to pass the is_secure flag.
  • But, presumably the vast majority (and best practice now) wouldn't need to set the flag at all. (On balance less key-strokes)

Thoughts/objections?

Update: After discussion on the PR, defaulting to https and having a --use-http flag was implemented. Unless there are objections we'll go with this.

Last edited 6 years ago by Carlton Gibson (previous) (diff)

comment:13 by Carlton Gibson, 6 years ago

Triage Stage: AcceptedReady for checkin

Bar an adjustment to AUTHORS.txt, this looks good to me.

comment:14 by Carlton Gibson, 6 years ago

Triage Stage: Ready for checkinAccepted

Adam argues on the PR that we should go through Deprecation for the breaking change here. Pause while we sort that out.

comment:15 by Carlton Gibson, 6 years ago

Triage Stage: AcceptedReady for checkin

For me, calling out the need to add the --use_http flag is enough. (Some people will need to add it sure, but everyone benefits from the changes.) I'll mark it RFC so we can ensure it's at least considered for v2.2. 

comment:16 by Simon Charette, 6 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Both flags should be changed to use - instead of underscores for separators to be coherent with the existing ones.

That means --site-domain and --use-http.

comment:17 by Sanyam Khurana, 6 years ago

Patch needs improvement: unset

Just need help on this one, https://github.com/django/django/pull/10651#issuecomment-453040019

Addressed all other reviews.

comment:18 by Tim Graham, 6 years ago

Summary: Allow customizing the `ping_google` URLMake ping_google use https for the sitemap URL

As I noted on the PR, the ticket reporter talks about "run ping_google from CLI without having contrib.sites installed." but you can't run a management command without it's app installed. Unless someone has a use case for it, I'm in favor of deferring the site domain changes for a separate ticket (if at all).

PR for making the ping_google command and function use https for the sitemap URL. I think that's straightforward.

comment:19 by Adam Johnson, 6 years ago

+1 to your proposal Tim. Well spotted.

comment:20 by Tim Graham <timograham@…>, 6 years ago

In 76d31be:

Refs #23829 -- Made ping_google command/function use https for the sitemap URL.

comment:21 by Tim Graham, 6 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top