Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #34688 -- Removed contrib.sitemaps.ping_google() and ping_google management command. #17039

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

anorthall
Copy link
Contributor

@anorthall anorthall commented Jul 2, 2023

Ticket

https://code.djangoproject.com/ticket/34688

Forum thread

https://forum.djangoproject.com/t/consensus-required-on-pr-to-remove-contrib-sitemaps-ping-google/22042

Implementation details

Google have announced that the sitemaps ping API is being deprecated "in 6 months" from June 26th 2023, which is right around the release time of Django 5.0 (Jan 2024). As such, I have not only deprecated contrib.sitemaps.ping_google() and the ping_google management command but also made them non-operational and replaced them with placeholder methods which perform no action. The placeholder methods will ensure backwards compatibility in code whilst avoiding making API calls to an endpoint which will no longer exist at the release of Django 5.0.

The following changes have been made:

  • contrib.sitemaps.ping_google() replaced with a noop method and deprecated.
  • contrib.sitemaps._get_sitemap_full_url(), a private method used only by contrib.sitemaps.ping_google(), has been removed.
  • Tests for the above two methods have been removed.
  • sitemap_only.py, index_only.py and empty.py in tests/sitemaps_tests/urls have been removed. These were test urlpatterns used only by the tests for ping_google().
  • The ping_google management command has been replaced with a noop replica which issues a deprecation warning.
  • Tests have been added for the deprecation warnings of the ping_google management command and the ping_google() method.
  • All appropriate documentation updated - deprecation log, release notes, sitemaps docs, django-admin command docs.
  • django-admin man page updated.

I hope you agree this is a sensible way to proceed in light of the API being deprecated at the same time as the Django 5.0 release. Cheers!

Edited: PR updated to fully remove contrib.sitemaps.ping_google() and the ping_google management command as per consensus on the forum and below.

@felixxm
Copy link
Member

felixxm commented Jul 3, 2023

@anorthall Thanks for this patch 👍 TBH, I see no reason to keep the management command and function as no-ops. This can be confusing to end users and has no practical value 🤷 I'd remove them and document this in as a backward incompatible change. After all, I would not consider it breaking the stability of our API, as they were wrappers for a Google endpoint that will no longer exist.

You can start a topic on the Django Forum to reach a wider audience and see what other think.

@anorthall
Copy link
Contributor Author

Thanks @felixxm, I agree it is confusing but didn't realise making a backward incompatible change was an option. I will make a thread on the forum to gather consensus as you suggest.

Copy link
Contributor

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job ! A few comments.

docs/ref/contrib/sitemaps.txt Outdated Show resolved Hide resolved
docs/releases/5.0.txt Outdated Show resolved Hide resolved
@ngnpope
Copy link
Member

ngnpope commented Jul 4, 2023

I'd remove them and document this in as a backward incompatible change.

I want to +1 this. I think that we may as well just remove this as a backward incompatible change. Silently doing nothing for another 24 months after the removal of the API is rather pointless. (Most people would just end up removing it anyway as soon as they saw the deprecation warning.)

In fact, for anyone using this on 4.2 or earlier, they're likely going to have this crashing or producing the wrong response and, as such, will likely remove it before they even upgrade to 5.0 because of that.

@ewjoachim
Copy link
Contributor

they're likely going to have this crashing or producing the wrong response

No, google said the endpoint will return a 404 and Django doesn't check the status code (and doesn't do anything with the response) so I believe it will be completely silent (though I may be wrong)

@anorthall anorthall changed the title Fixed #34688 -- Deprecate (and make noop) contrib.sitemaps.ping_google() Fixed #34688 -- Remove contrib.sitemaps.ping_google() and ping_google management command Jul 8, 2023
@anorthall
Copy link
Contributor Author

PR updated to fully remove contrib.sitemaps.ping_google() and the ping_google management command without prior deprecation as per consensus on the Django forum.

docs/man/django-admin.1 Outdated Show resolved Hide resolved
docs/releases/5.0.txt Outdated Show resolved Hide resolved
@anorthall
Copy link
Contributor Author

@felixxm requested changes made! thanks

@felixxm felixxm changed the title Fixed #34688 -- Remove contrib.sitemaps.ping_google() and ping_google management command Fixed #34688 -- Removed contrib.sitemaps.ping_google() and ping_google management command. Jul 10, 2023
…e management command.

Thanks Joachim Jablon for the report.

Google has deprecated the sitemap ping endpoint, and will be removing
it in 6 months ~January 2024.
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anorthall Thanks 👍 I pushed small edits.

django/contrib/sitemaps/__init__.py Outdated Show resolved Hide resolved
docs/ref/django-admin.txt Outdated Show resolved Hide resolved
docs/releases/5.0.txt Outdated Show resolved Hide resolved
@felixxm felixxm merged commit 6d42728 into django:main Jul 10, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants