Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 17 months ago

#55633 closed defect (bug) (fixed)

First sitemap has more URLs than expected with sticky posts

Reported by: ravanh's profile RavanH Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: minor Version: 5.5
Component: Sitemaps Keywords: has-patch commit has-unit-tests
Focuses: Cc:

Description

When a site has $x sticky posts, and more posts than $max_urls (2000 by default) then the first posts sitemap will have $max_urls + $x URLs.

Not a big issue unless there are a huge number of sticky posts and the site is running on a relatively low resource server... Still, an unexpected sitemap size and it is easily fixed by setting ignore_sticky_posts :)

<?php
function wpsm_posts_query_args( $args ) {
        // Ignore stickyness.
        $args['ignore_sticky_posts'] = true;

        return $args;
}
add_filter( 'wp_sitemaps_posts_query_args', 'wpsm_posts_query_args' );

Attachments (2)

55633.diff (1003 bytes) - added by pbiron 19 months ago.
55633.2.diff (2.3 KB) - added by pbiron 18 months ago.

Download all attachments as: .zip

Change History (21)

#1 @pbiron
19 months ago

  • Version changed from 5.9.3 to 5.5

Thanx for the ticket.

I'll have to think about this a little more, but on first thought it seems to make sense to add 'ignore_sticky_posts' => true to the default args passed to wp_sitemaps_posts_query_args.

@swissspidy do you have thoughts on this?

#2 @swissspidy
19 months ago

Yeah that sounds reasonable

@pbiron
19 months ago

#3 @pbiron
19 months ago

  • Keywords has-patch needs-testing added

55633.diff adds 'ignore_sticky_posts' => true to the default args passed to wp_sitemaps_posts_query_args.

@RavanH can you please test this patch (I've tested on a small "play" site but would be good to test on a larger, more realistic site).

#4 @pbiron
19 months ago

  • Milestone changed from Awaiting Review to 6.1

#5 @RavanH
19 months ago

  • Keywords needs-testing removed

Perfect! I can confirm that the patch works on a large site. :)

#6 @pbiron
19 months ago

Thanx @RavanH !!!

@swissspidy I milestoned this for 6.1, since we're already in 6.0-beta phase. However, I then noticed that is was marked as a bug...so technically, it could still go into 6.0. I'll let you, as a commiter, make that call.

Last edited 19 months ago by pbiron (previous) (diff)

#7 follow-up: @audrasjb
19 months ago

Hello there,
The patch looks good to me, but I'm not sure we really need to add a comment since it is the default behavior of the ignore_sticky_posts argument :)

#8 in reply to: ↑ 7 ; follow-up: @pbiron
19 months ago

  • Keywords commit added

Replying to audrasjb:

Hello there,
The patch looks good to me, but I'm not sure we really need to add a comment since it is the default behavior of the ignore_sticky_posts argument :)

That's true, but the naming of that parameter can be very confusing to those who don't know it well...so I always find it useful to include such a comment :-)

This ticket was mentioned in Slack in #core by pbiron. View the logs.


19 months ago

#10 @SergeyBiryukov
19 months ago

Would it be possible to include any unit tests for this?

#11 @pbiron
19 months ago

Good idea. I'll work some up this weekend and amend the patch.

@pbiron
18 months ago

#12 @pbiron
18 months ago

@SergeyBiryukov 55633.2.diff is the same as 55633.diff but adds a unit test.

#13 in reply to: ↑ 8 @audrasjb
18 months ago

Replying to pbiron:

Replying to audrasjb:

Hello there,
The patch looks good to me, but I'm not sure we really need to add a comment since it is the default behavior of the ignore_sticky_posts argument :)

That's true, but the naming of that parameter can be very confusing to those who don't know it well...so I always find it useful to include such a comment :-)

Alright, 55633.2.diff looks good to me then.

This ticket was mentioned in PR #2834 on WordPress/wordpress-develop by audrasjb.


18 months ago
#14

  • Keywords has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/55633

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

#15 @audrasjb
18 months ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 53548:

Sitemaps: Remove duplicate sticky Posts from Sitemap Post Query.

This changeset sets the ignore_sticky_posts parameter to true in the default arguments passed to wp_sitemaps_posts_query_args.

Props RavanH, pbiron, swissspidy, audrasjb, SergeyBiryukov.
Fixes #55633.

#17 @audrasjb
18 months ago

In 53551:

Docs: Add missing @since mention in get_posts_query_args().

Follow-up to [53548].
See #55633.

#18 @SergeyBiryukov
18 months ago

In 53556:

Tests: Improve the test for sticky posts not being moved to the front in sitemaps.

  • Give the test method a most descriptive name.
  • Use a single assertion for the URL list instead of a foreach loop to provide more context in case of failure.
  • Add a failure message to each assertion, as there are multiple assertions used in the test.

Follow-up to [53548].

See #55633.

#19 @RavanH
17 months ago

I've implemented this (for now, awaiting 6.1) in the plugin https://wordpress.org/plugins/xml-sitemaps-manager/

Note: See TracTickets for help on using tickets.