Opened 4 years ago

Last modified 13 hours ago

#23829 assigned Cleanup/optimization

Allow customizing the `ping_google` 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 (13)

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 days ago by Sanyam Khurana

Owner: set to Sanyam Khurana
Status: newassigned

comment:10 Changed 4 days ago by Sanyam Khurana

comment:11 Changed 3 days 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 35 hours 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 30 hours ago by Carlton Gibson (previous) (diff)

comment:13 Changed 29 hours ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

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

comment:14 Changed 13 hours 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.

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