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

Add filter to limit the sitemap date range #147

Merged
merged 7 commits into from
Feb 21, 2019
Merged

Add filter to limit the sitemap date range #147

merged 7 commits into from
Feb 21, 2019

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Jan 22, 2019

Fixes #146.

  • Introduce the msm_sitemap_index filter which allows removing daily sitemaps from the root sitemap index.

@kasparsd kasparsd changed the title [WIP] Add filter to limit the sitemap date range Add filter to limit the sitemap date range Jan 29, 2019
README.md Outdated Show resolved Hide resolved
@kasparsd
Copy link
Contributor Author

kasparsd commented Feb 7, 2019

This is ready for code review.

@GaryJones
Copy link
Contributor

How about a test to demonstrate and prove how this new filter works in practice?

@kasparsd
Copy link
Contributor Author

kasparsd commented Feb 8, 2019

@GaryJones Are you thinking of a unit test for the filter? I'm not sure exactly what to test there since it is only a pass-through for a variable and the result depends purely on the filter implementation. In a pure unit test way it would just check if apply_filters() was called during build_root_sitemap_xml().

We've already added an example to the readme to illustrate a sample usage.

Did you have any other ideas in mind?

@GaryJones
Copy link
Contributor

In a pure unit test way it would just check if apply_filters() was called during build_root_sitemap_xml().

Right - we're not testing how apply_filters() works, only that build_root_sitemap_xml() has the behaviour / logic such that the value is filterable.

@lgedeon
Copy link
Contributor

lgedeon commented Feb 9, 2019

I don't see any tests for build_root_sitemap_xml, but it appears that to test this I would need to:

  • Create two posts on different dates using create_dummy_post
  • Call MSM_SiteMap_Test::build_sitemaps
  • Assert that the return value of build_root_sitemap_xml contains two date nodes
  • Add a filter to remove everything before a date that falls between the two dates from the first step.
  • Assert that the return value now contains one date node

@GaryJones, does this sound right? Also should this be added to test-sitemap-functions.php, test-sitemap-filter.php or somewhere else?

msm-sitemap.php Outdated
@@ -674,6 +674,9 @@ public static function build_root_sitemap_xml( $year = false ) {
// Sometimes duplicate sitemaps exist, lets make sure so they are not output
$sitemaps = array_unique( $sitemaps );

// Filter daily sitemaps from the index by date.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be properly documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GaryJones This is documented now. What is next on the path to merger?

@lgedeon lgedeon mentioned this pull request Feb 21, 2019
@GaryJones GaryJones merged commit 14ba9b2 into Automattic:master Feb 21, 2019
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.

3 participants