Opened 4 years ago

Closed 3 months 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: master
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 Changed 4 years ago by Danilo Bargen

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

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

Owner: Danilo Bargen deleted
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 Changed 4 years ago by Burhan Khalid

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

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

comment:5 Changed 4 years ago by Berker Peksag

Patch needs improvement: set

comment:6 Changed 4 years ago by Tim Graham

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

comment:8 Changed 2 years ago by Adam (Chainz) Johnson

Cc: me@… added

comment:9 Changed 5 months ago by Sanyam Khurana

Owner: set to Sanyam Khurana
Status: newassigned

comment:10 Changed 5 months ago by Sanyam Khurana

comment:11 Changed 5 months ago by Sanyam Khurana

Cc: Sanyam Khurana added
Patch needs improvement: unset

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

comment:12 Changed 5 months ago by Carlton Gibson

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 5 months ago by Carlton Gibson (previous) (diff)

comment:13 Changed 5 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

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

comment:14 Changed 5 months ago by Carlton Gibson

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 Changed 4 months ago by Carlton Gibson

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 Changed 4 months ago by Simon Charette

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 Changed 4 months ago by Sanyam Khurana

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 Changed 3 months ago by Tim Graham

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 Changed 3 months ago by Adam (Chainz) Johnson

+1 to your proposal Tim. Well spotted.

comment:20 Changed 3 months ago by Tim Graham <timograham@…>

In 76d31be:

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

comment:21 Changed 3 months ago by Tim Graham

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