I would suggest checking if mb_strlen exists before to use it, as some hosts have it disabled.

On further inspection mb_strlen was the wrong thing to use here anyway- it specifically counts characters instead of bytes, which is what we need. I changed this to use just strlen instead, although reading some documentation I'm concerned that this will not do the right thing either as some hosts alias strlen to mb_strlen.

It might be good to check your use of spaces and yoda conditions across your changes, to match WordPress' coding standards>

You can use Code Sniffer rules to make that process easier.

Did you deprecate this filter? If so, you can add it to the list here.

You can probably remove this comment now.

This would need a textdomain so it can be translated.

You'll probably need to take the jetpack_sitemap_location filter into account, since some people may use the filter to define a custom sitemap location.

The jetpack_sitemap_location filter is now respected. The way it's used did change a bit though: long story short, instead of setting the location as /path/to/the/sitemap.xml we only set the location to /path/to/the/.

Can you explain the long story? :) Why is it important that jetpack_sitemap_location changed behavior? Is it possible to implement this without changing the behavior?

It'd be nice to keep that filter around somehow, it's useful for some site owners.

The jetpack_sitemap_skip_post filter is now respected. As far as I can tell there is no change in behavior compared to the old version.

Some site owners already use that filter, so I think it would be nice to keep it.

As of 7afe5e7 the jetpack_print_sitemap filter is respected.

I'd be interested in seeing what this is used for. If this filter is kept there probably need to be analogous filters for image sitemaps and sitemap indices.

Another filter to add to the list of deprecated hooks.

This call uses 3 parameters but query_posts_after_id only accepts 2.

I fixed this by making the logger object a class member, rather than passing one around.

The selectors #description, #content, .odd, #footer, img.thumbnail are never used.

#description, .odd, #content, #footer, img.thumbnail

In the browser sitemap.xml is transformed by an XML stylesheet, these are used in the transformed HTML. You can see the stylesheet at sitemap.xsl.

What's the advantage of creating this inline function, against a foreach that goes through array elements defined as

(
    'jp_sitemap_master',
    'Sitemap Master',
    'jetpack-sitemap-master'
)

and calls register_post_type directly?

I can't think of one. :) The latest commit uses an array with foreach.

Scratch that- sitemaps are now stored in a custom database table, so this code is not relevant anymore.

How can we handle default permalinks and PATHINFO permalinks?

Does the URL of robots.txt depend on the permalink settings? (Edit: I just checked and omg it does. I don't know what's real anymore :))

This is doable, though I have to confess I'm not sure what the benefit is given that sitemaps are meant for robots. But then so is robots.txt.

Edit again- this module hijacks the usual URL routing by looking at $_SERVER['REQUEST_URI'] directly. Does this cause problems with any of the permalink schemes?

The latest commit handles plain and PATHINFO permalinks. Plain permalinks are served at /?jetpack-sitemap=sitemap.xml.

Is it possible to register only the post type that is needed at a certain time? So for example if I'm seeing the image sitemap, I can't skip registering this?

Why did you choose a custom post type to model, well, other post types? What is the advantage of this vs rows in a custom database table?

What's really needed is a way to store (1) the generated sitemap xml, with (2) a name, (3) a unique numeric ID, and (4) a timestamp, which are needed during the generation phase. Using a custom post type seemed like a natural fit for this. (Also this is how msm-sitemap does it, if I understand correctly.)

But also I did not realize that using a custom database table was an option. That might be a better solution, because sitemaps are sort of "out of band" compared to posts. It should be easy to switch, because the actual database queries are abstracted behind the query_ methods. I will have to look into this.

As for registering the post types, it's possible that this is not necessary at all. What exactly does register_post_type do and when is it required? Can we store custom post types in the database without having to register them? Because the sitemap post types are not needed in the admin panel.

The latest commit uses a custom database table to store the sitemap data.

Maybe this can only be instantiated when WP_DEBUG is defined and set to true? It's better not to load this every time during production.

This file and the class instantiation in line 87 should be done only when WP_DEBUG is defined and set to true.

This inline doc for the filter must be moved one line down, right before apply_filters.

The same mistake was in two more places- all are fixed now.

Conditional can be simplified to return 1 === $num_rows_deleted;

Missing @since 4.5.0 instead of @module sitemaps

Every method should now have a @since annotation.

Jetpack_Sitemap_Stylist::sanitize_with_links strips out the em tag. Just remove the em.

Missing @return.

It turns out there were lots of missing @returns- I checked again and every function that returns something should now have a @return annotation.

Will this benefit from using Jetpack_Options::get_option_and_ensure_autoload( 'jetpack_sitemap_location', '' )?
How about others get_option calls in this PR?

Reading through the source of get_option, it looks like a bunch of stuff happens when get_option is called on a nonexistent name- stuff that is unnecessary when we provide a default value. So I imagine there will be some benefit to storing the default, especially for the calls that happen many times (which is all of them).