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 filters to handle custom post statuses when generating sitemap #12075

Merged
merged 2 commits into from
Mar 21, 2019

Conversation

tolnem
Copy link

@tolnem tolnem commented Jan 21, 2019

Summary

This PR can be summarized in the following changelog entry:

  • Add filters to handle custom post statuses when generating sitemap

Test instructions

This PR can be tested by following these steps:

  1. Create a custom post type
  2. Enable Edit Flow plugin or otherwise create custom post status for the custom post type
  3. Create one or more posts with the custom post status - see that they do not appear in the sitemap.xml created by Yoast SEO
  4. Add a filter for 'wpseo_sitemap_poststatus' and 'wpseo_sitemap_poststatus_lastmodified' to add the custom post status to the array of post status names.
  5. See that the posts are now included in the sitemap.xml

UI changes

  • This PR does not change the UI in the plugin.

Quality assurance

  • I have tested this code to the best of my abilities

Fixes #12074

Copy link
Contributor

@abotteram abotteram left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to Yoast SEO!

I have contacted our SEO team to review this PR. In the meanwhile please address the provided code review feedback.

inc/sitemaps/class-post-type-sitemap-provider.php Outdated Show resolved Hide resolved
inc/sitemaps/class-sitemaps.php Outdated Show resolved Hide resolved
@abotteram abotteram self-assigned this Mar 14, 2019
@tolnem tolnem force-pushed the 12074-custom-post-status-in-sitemap branch from e2a9453 to 6649aa0 Compare March 14, 2019 18:09
@abotteram
Copy link
Contributor

Acceptance 👍

Thank you for your contribution. This feature will be included in our 10.2 release on April 9th.

@abotteram abotteram merged commit 6f7a967 into Yoast:trunk Mar 21, 2019
@abotteram abotteram added this to the 10.2 milestone Mar 21, 2019
@stodorovic
Copy link
Contributor

stodorovic commented Mar 25, 2019

@xyfi I think that we should little postpone the milestone. I've found couple issues:

  • There is still query which isn't covered with the filter:

    $sql = "
    SELECT post_modified_gmt
    FROM ( SELECT @rownum:=0 ) init
    JOIN {$wpdb->posts} USE INDEX( type_status_date )
    WHERE post_status IN ( 'publish', 'inherit' )
    AND post_type = %s
    AND ( @rownum:=@rownum+1 ) %% %d = 0
    ORDER BY post_modified_gmt ASC
    ";

    I think that we should use unique filter for all queries which will be more clear for users (and prevent possible differences between the root sitemap, CPT sitemaps and tax sitemaps). I'm preparing PR which will fix it.

  • There is hard-coded "publish" in post object.

    $post->post_type = $post_type;
    $post->post_status = 'publish';
    $post->filter = 'sample';
    $post->ID = (int) $post->ID;

It could confuse users which uses filter wpseo_sitemap_entry.

  • Attachments still uses only 'publish' status for parent posts.

  • We should test performance if there are 100K posts because we've changed sensitive SQL queries (I don't expect differences, but we should test it).. It's related to Performance fix for Yoast sitemaps #12161

@stodorovic
Copy link
Contributor

I just found new possible issue:

$sql = "
SELECT MAX(p.post_modified_gmt) AS lastmod
FROM $wpdb->posts AS p
INNER JOIN $wpdb->term_relationships AS term_rel
ON term_rel.object_id = p.ID
INNER JOIN $wpdb->term_taxonomy AS term_tax
ON term_tax.term_taxonomy_id = term_rel.term_taxonomy_id
AND term_tax.taxonomy = %s
AND term_tax.term_id = %d
WHERE p.post_status IN ('publish','inherit')
AND p.post_password = ''
";

Also, I created PR #12511, but it needs some modifications (eg. taxonomy).

@abotteram
Copy link
Contributor

@tolnem I'm sorry, but based on @stodorovic 's findings we decided to revert this PR. As it looks now the 10.2 release has been pushed back by a week (to April 16th). If you would like to help you could review and test #12511 so we can try to get this into 10.2.

@moorscode moorscode removed this from the 11.0 milestone Apr 11, 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.

5 participants