Skip to content

Enforce post objects to WP_Post in class WPSEO_Post_Type_Sitemap_Provider#16313

Merged
IreneStr merged 1 commit intotrunkfrom
sitemap-enforce-wp_post
Nov 9, 2020
Merged

Enforce post objects to WP_Post in class WPSEO_Post_Type_Sitemap_Provider#16313
IreneStr merged 1 commit intotrunkfrom
sitemap-enforce-wp_post

Conversation

@stodorovic
Copy link
Copy Markdown
Contributor

@stodorovic stodorovic commented Nov 7, 2020

Context

Summary

This PR can be summarized in the following changelog entry:

  • Fixes a bug where post-sitemap.xml would cause a fatal error when the filter post_link required a WP_Post object. Props to stodorovic.

Relevant technical choices:

  • Use sanitize_post instead of casting to int and enforce WP_Post object instead of default stdClass.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Test instructions for QA when the code is in the RC

  1. Add following PHP code:
add_filter( 'post_link', function( $permalink, WP_Post $post ){ 
    return $permalink;
}, 10, 2 );
  1. Check post-sitemap.xml. It should work without PHP fatal error.
  • QA should use the same steps as above.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • It affects all post_type sitemaps (post, page, CPTs).

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

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 #16308

@stodorovic stodorovic added the changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog label Nov 7, 2020
@jdevalk
Copy link
Copy Markdown
Contributor

jdevalk commented Nov 9, 2020

CR: 👍

@Djennez
Copy link
Copy Markdown
Member

Djennez commented Nov 9, 2020

Acceptance: ✅

Things I noticed:

  • It looks like the wpdb query, just a few lines before these changes, now requests data that is no longer utilized (like the post author).
  • There are differences in data size and values between the 2 objects:
    image

@stodorovic
Copy link
Copy Markdown
Contributor Author

The class WP_Post contains some default properties ( eg. $comment_status, $comment_count, ... ). I think that's good for better compatibility and it doesn't affect on the code ( memory usage isn't the problem, maybe few MB more).

Also, we should keep post_author because someone can check it via the filter wpseo_sitemap_entry (3rd argument ).

@jdevalk
Copy link
Copy Markdown
Contributor

jdevalk commented Nov 9, 2020

I'm of a mind to rebuild XML sitemaps on top of indexables anyway, so this fixes the bug, which is fine. Merging!

@IreneStr IreneStr merged commit ed7ae98 into trunk Nov 9, 2020
@IreneStr IreneStr deleted the sitemap-enforce-wp_post branch November 9, 2020 12:11
@IreneStr IreneStr added this to the 15.4 milestone Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: bugfix Needs to be included in the 'Bugfixes' category in the changelog community-patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong parameter in get_permalink - class WPSEO_Post_Type_Sitemap_Provider

5 participants