[Django] #23829: `ping_google` uses hardcoded http protocol
Django
--------------------------------------+-----------------------------------
Reporter: jwa | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.sitemaps | Version: master
Severity: Normal | Keywords: sitemaps, ping_google
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-----------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jwa | Owner: dbrgn
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sitemaps, | Needs documentation: 1
ping_google | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by dbrgn):
* status: new => assigned
* needs_better_patch: => 0
* component: contrib.sitemaps => Documentation
* needs_tests: => 0
* owner: nobody => dbrgn
* needs_docs: => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:1>
Django
Reporter: jwa | Owner: dbrgn
Type: | Status: assigned
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sitemaps, | Needs documentation: 1
ping_google | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by burhan):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:2>
Django
Reporter: jwa | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sitemaps, | Needs documentation: 1
ping_google | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: dbrgn =>
* status: assigned => new
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:3>
Django
Reporter: jwa | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sitemaps, | Needs documentation: 1
ping_google | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by burhan):
Pull request done, and I am getting some help in writing tests as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:4>
Django
Reporter: jwa | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Component: Documentation | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: sitemaps, | Needs documentation: 1
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: jwa | Owner:
Type: | Status: new
Cleanup/optimization | Version: master
Severity: Normal | Triage Stage: Accepted
ping_google | Patch needs improvement: 1
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
* has_patch: 0 => 1
* component: Documentation => contrib.sitemaps
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by auvipy):
* owner: => auvipy
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Saifuddin Auvi
Cleanup/optimization | Status: assigned
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Adam Chainz):
* cc: me@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Asif Saifuddin Auvi):
* owner: Asif Saifuddin Auvi => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Khurana
Cleanup/optimization | Status: assigned
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Sanyam Khurana):
* owner: (none) => Sanyam Khurana
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Sanyam Khurana):
PR is up for a review: https://github.com/django/django/pull/10651
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sanyam Khurana):
* cc: Sanyam Khurana (added)
* needs_better_patch: 1 => 0
Comment:
I've addressed the reviews. Can you please take another pass?
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Ready for
ping_google | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
Bar an adjustment to AUTHORS.txt, this looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Ready for checkin => Accepted
Comment:
Adam argues on the PR that we should go through Deprecation for the
breaking change here. Pause while we sort that out.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Ready for
ping_google | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Both flags should be changed to use `-` instead of underscores for
separators to be coherent with [https://docs.djangoproject.com/en/2.1/ref
/django-admin/ the existing ones].
That means `--site-domain` and `--use-http`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sanyam Khurana):
* needs_better_patch: 1 => 0
Comment:
Just need help on this one,
https://github.com/django/django/pull/10651#issuecomment-453040019
Addressed all other reviews.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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).
[https://github.com/django/django/pull/10835 PR] for making the
ping_google command and function use https for the sitemap URL. I think
that's straightforward.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
+1 to your proposal Tim. Well spotted.
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"76d31be2d04dd6e6bcb5cb39a29ea9ed3938d0f9" 76d31be]:
{{{
#!CommitTicketReference repository=""
revision="76d31be2d04dd6e6bcb5cb39a29ea9ed3938d0f9"
Refs #23829 -- Made ping_google command/function use https for the sitemap
URL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Julian Wachholz | Owner: Sanyam
Type: | Khurana
Component: contrib.sitemaps | Version: master
Severity: Normal | Resolution: fixed
Keywords: sitemaps, | Triage Stage: Accepted
ping_google |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/23829#comment:21>