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

Implement a layer around dynamic rewrite rules and use them for sitemaps #10802

Closed
wants to merge 6 commits into from

Conversation

felixarntz
Copy link
Contributor

Summary

This PR can be summarized in the following changelog entry:

  • Makes sitemap rewrite rules more reliable by applying them dynamically.

Relevant technical choices:

  • A class Yoast_Dynamic_Rewrites is introduced as a layer to handle dynamic rewrites.
  • Dynamic rewrites can be added with its add_rule() method, that has the same signature as core's WP_Rewrite::add_rule() (and its alias add_rewrite_rule()).
  • The class filters the rewrite_rules option, adding all dynamic rewrite rules in there.
  • The class still calls core's WP_Rewrite::add_rule(). This needs to happen so that rewrite rules are also in place when they are not fetched from the option, but being regenerated "live".
  • In order to keep the dynamic rewrite rules out of the actual option (they are added via a filter and thus shouldn't redundantly pollute the option value), the class also filters sanitizing the rewrite_rules option, stripping those dynamic rules out of it.
  • The class is instantiated on setup_theme since this is the first action where WP_Rewrite is available.

Please read the issue #10801 for more background information.

Test instructions

This PR can be tested by following these steps:

  • Deactivate the plugin.
  • Go to the Settings > Permalinks screen and just hit Save (you don't need to make any changes). This will flush rewrite rules (just to make sure Yoast SEO rewrite rules are not included).
  • In the _wpseo_activate() function in wp-seo-main.php, temporarily comment out the if-else construct that flushes rewrite rules (starts with is_multisite() && ms_is_switched()).
  • Activate the plugin again.
  • Access sitemap_index.xml and ensure it appears, not giving a 404.
  • From there, also access the other sitemaps, and ensure they appear.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes #10801

@felixarntz
Copy link
Contributor Author

I'll look into tests once we have a decision on whether/how to proceed with this.

@omarreiss
Copy link
Contributor

Closing this as we are moving towards a different sitemap implementation.

@omarreiss omarreiss closed this May 26, 2020
@IreneStr IreneStr deleted the idea/dynamic-rewrites branch February 3, 2021 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Dynamic Rewrite Rules
2 participants