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 helper function for getting all sitemaps for the index. #103

Open
joemcgill opened this issue Jan 17, 2020 · 0 comments
Open

Add helper function for getting all sitemaps for the index. #103

joemcgill opened this issue Jan 17, 2020 · 0 comments

Comments

@joemcgill
Copy link
Collaborator

@joemcgill joemcgill commented Jan 17, 2020

Description

Which feature is your enhancement request related to?
This is a follow-up to #102.

Describe the solution you'd like
The implementation for gathering all sitemap entries for the sitemap index is implemented procedurally within the Core_Sitemaps_Index::render_index() method, and repeated in the _get_sitemap_entries() helper method in the PHPUnit tests.

At present, we've not abstracted this into a separate functional method since it's not reused anywhere else in the codebase, nor would we expect it to be needed for implementations extending or modifying the core functionality, and would introduce backwards compatibility concerns. However, as described in #102 (comment), abstracting this important functionality would make it easier to test and prevent possible future bugs from being introduced, like #102, during code refactoring.

Additional context
I would suggest an implementation, similar to the following...

function core_sitemaps_get_sitemap_entries() {
	$sitemaps  = array();
	$providers = core_sitemaps_get_sitemaps();

	foreach ( $providers as $provider ) {
		// Using array_push is more efficient than array_merge in a loop.
		array_push( $sitemaps, ...$provider->get_sitemap_entries() );
	}

	return apply_filters( 'filter_name', $sitemaps );
}

Note that the use of the splat operator makes this functionality only compatible for PHP 5.6+. To extend compatibility to earlier versions, this could be modified to use call_user_func_array() on an array of sitemap entry arrays. See https://github.com/dseguy/clearPHP/blob/master/rules/no-array_merge-in-loop.md for more context.

Acceptance Criteria

Add the relevant acceptance criteria here.

  • A publicly accessible function/method exists that encapsulates the functionality for gathering all sitemap entries for the index, returning all available sitemap pages data.
  • The new method is covered by new PHPUnit tests.
  • The current places in the codebase where this functionality exists has been updated to make use of the abstracted method.
@joemcgill joemcgill added this to Review in progress in Execution Jan 20, 2020
@joemcgill joemcgill moved this from Review in progress to To do in Execution Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Execution
  
To do
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.