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

Sitemaps: Fix the Hook from init to wp_loaded for Sitemap URL handler. #34269

Merged

Conversation

rutviksavsani
Copy link
Contributor

@rutviksavsani rutviksavsani commented Nov 23, 2023

Fixes #

Proposed changes:

  • This PR proposes to generate the permalinks on the wp_loaded hook instead of the init hook which would allow us to get all our plugins and theme changes loaded and the modified permalinks being generated properly.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Steps to reproduce:

  1. Setup a sitemap with active Jetpack and Sitemaps active.
  2. Upload this plugin which registers a custom post type at a priority of 11 and adds it to the News Sitemap.
    Plugin gist link : https://gist.github.com/rutviksavsani/4cd931d72a981b637a2b3d745ca5c359
  3. Add a new post to the Artist post-type.
  4. Now on checking the URL of the post on the News post-type, You should be able to see a raw URL and not a final Permalink.
  5. Now when applying this PR, we should be able to see the correct URLs as we wait for all the plugins and theme actions to get registered. ( Note: Make sure to delete old Transient for new links to be showed. )

Video for reproducing:

video-22820443-4c9fe126.mp4

Screenshots:

Before:
image

After:

image

@github-actions github-actions bot added [Feature] Sitemaps [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Status] In Progress labels Nov 23, 2023
Copy link
Contributor

github-actions bot commented Nov 23, 2023

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.



Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

  • WordPress.com Simple releases happen daily.
  • WoA releases happen weekly.
  • Releases to self-hosted sites happen monthly. The next release is scheduled for February 6, 2024 (scheduled code freeze on February 5, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

@github-actions github-actions bot added the OSS Citizen This Pull Request was opened by an Open Source contributor. label Nov 23, 2023
@rutviksavsani rutviksavsani marked this pull request as ready for review November 28, 2023 11:42
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended [Pri] Normal and removed [Status] In Progress labels Nov 30, 2023
@monsieur-z
Copy link
Contributor

It looks like a fair change to me. @nbloomf, it's been a while since you worked on sitemaps but you might have some feedback to share. Any possible unwanted side effects?

Copy link
Contributor

@monsieur-z monsieur-z left a comment

Choose a reason for hiding this comment

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

Works as advertised 👍

@monsieur-z monsieur-z added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 9, 2024
@monsieur-z
Copy link
Contributor

A trunk rebase/merge is required.

@rutviksavsani
Copy link
Contributor Author

Rebase done.

@jeherve jeherve enabled auto-merge (squash) February 2, 2024 10:13
@jeherve jeherve merged commit 0905e57 into Automattic:trunk Feb 2, 2024
52 checks passed
@github-actions github-actions bot removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 2, 2024
@github-actions github-actions bot added this to the jetpack/13.1 milestone Feb 2, 2024
spsiddarthan pushed a commit that referenced this pull request Feb 15, 2024
#34269)

* Update Init hook to WP for generating sitemap URLs and not all post types are registered while this is first initiated.

* Fix hook name to wp_loaded

* Add changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sitemaps OSS Citizen This Pull Request was opened by an Open Source contributor. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants