Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#51136 new defect (bug)

Third-party sitemap plugin returning 404 after disabling core's sitemap

Reported by: vaurdan's profile vaurdan Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.5
Component: Sitemaps Keywords: has-patch
Focuses: Cc:

Description

When using a third-party sitemap plugin (such as MSM Sitemap), sitemaps are returning HTTP status code 404, despite outputting the actual sitemap XML.

Core's sitemap is being disabled using the wp_sitemaps_enabled filter, however the 404 status code is still being sent in the response.

I believe the issue resides in WP_Sitemaps::init(), where both the rewrite rules and the template redirect are being set before the actual $this->sitemaps_enabled() check (see here).

Since both Core and msm-sitemap use sitemap parameter for the rewrite, WP_Sitemaps::render_sitemaps will still be called on a sitemap.xml request, and since Core sitemap is disabled ($this->sitemaps_enabled() is false), it will return a 404 error, despite msm-sitemap generating and outputting the correct XML:

if ( ! $this->sitemaps_enabled() ) {
        $wp_query->set_404();
        status_header( 404 );
        return;
}

I have came with the following workaround to prevent the 404 status code on sitemap requests:

<?php
// Assuming that `wp_sitemaps_enabled` was already filtered to `false`
add_action('init', function() {
        global $wp_sitemaps;
        remove_action( 'template_redirect', array( $wp_sitemaps, 'render_sitemaps' ) );
}, 100 );

I'm including a patch with a fix suggestion, that is moving the if ( ! $this->sitemaps_enabled() ) validation to the beginning of the WP_Sitemaps::init method.

Attachments (1)

patch.diff (703 bytes) - added by vaurdan 4 years ago.
Patch with a potential fix

Download all attachments as: .zip

Change History (8)

@vaurdan
4 years ago

Patch with a potential fix

#1 follow-up: @pbiron
4 years ago

Hi @vaurdan. Thanx for the report.

Can you please try to the following: after activating Metro Sitemaps, flush the rewrite rules. The easiest way to do that is to go to Settings > Permalinks and click "Save Changes" (you don't actually have to make any changes).

I think that will solve the 404 problem you're having.

#2 in reply to: ↑ 1 @vaurdan
4 years ago

Hey @pbiron! The rewrite rules have been flushed (prior to the ticket, should have mentioned that) and the problem still persists. If you take a look at my patch, you'll see that originally the rewrite rules will be loaded, even if the native sitemaps are disabled.

We have a few sites in WordPress VIP with this particular issue (using msm-sitemap and with the native sitemaps disabled by filter). The workaround I included was the only way to stop the 404's.

Replying to pbiron:

Hi @vaurdan. Thanx for the report.

Can you please try to the following: after activating Metro Sitemaps, flush the rewrite rules. The easiest way to do that is to go to Settings > Permalinks and click "Save Changes" (you don't actually have to make any changes).

I think that will solve the 404 problem you're having.

Version 1, edited 4 years ago by vaurdan (previous) (next) (diff)

#3 @pbiron
4 years ago

I have installed Metro Sitemap on a test site, without your patch applied.

When I first activated it (and ran wp msm-sitemap generate-sitemap) I was getting the 404 as you are. After flushing the rewrite rules it is working just fine, for me.

Will investigate further.

#4 @vaurdan
4 years ago

@pbiron I was able to reproduce it in a fresh WordPress install, but with a caveat: I had to use WordPress 5.4.2 initially.

Here are the steps:

  1. Install WP 5.4.2 (pre-sitemap)
  2. Install msm-sitemap (use the Git version (master), as the plugin in the Repo still doesn't disable core sitemaps with the filter) and generate a sitemap
  3. example.com/sitemap.xml will return 200
  4. Update 5.4.2 to 5.5
  5. example.com/sitemap.xml will return 404
  6. flush rewrite rules ( Settings > Permalink > Save )
  7. example.com/sitemap.xml is still returning 404
  8. apply the patch or the workaround, and it now returns 200.

I believe that the order of registering the rewrite rules (msm-sitemaps registers before core sitemaps even exists) is a factor on this issue.

Let me know if you are able to reproduce using these steps.

Last edited 4 years ago by vaurdan (previous) (diff)

#5 @vaurdan
4 years ago

@pbiron were you able to try with the steps I shared?

#6 @swissspidy
3 years ago

patch.diff will break a few things. See #50643 for history. We need the rewrite rules to always be there.

#7 @vaurdan
3 years ago

@swissspidy I think I understand the issue. However we still have a few sites with this behavior, and I believe this issue should still be addressed, as it is causing inconsistencies as well.

If I understand correctly, the issue is after the native sitemaps are disabled, wp-sitemap.xml will return a 200 with the homepage, instead of the expected 404. If we flush the rewrite rules when sitemaps are disabled and the rules are still there, would it work?

Something like:

<?
public function init() {
        if ( ! $this->sitemaps_enabled() ) {
                $this->maybe_flush_rewrites();
                return;
        }

        // These will all fire on the init hook.
        $this->register_rewrites();

        add_action( 'template_redirect', array( $this, 'render_sitemaps' ) );

        $this->register_sitemaps();

        // Add additional action callbacks.
        add_filter( 'pre_handle_404', array( $this, 'redirect_sitemapxml' ), 10, 2 );
        add_filter( 'robots_txt', array( $this, 'add_robots' ), 0, 2 );
}

private function maybe_flush_rewrites() {
        global $wp_rewrite;

        if ( ! $this->sitemaps_enabled() && array_key_exists( '^wp-sitemap\.xml$', $wp_rewrite->extra_rules_top ) ) {
                flush_rewrite_rules();
        }
}

This is a very crude solution, but it might work. Flushing will only happen once, after the native sitemaps are disabled, and should allow us to move the $this->sitemaps_enabled() check to the top of the init() method, as suggested on the patch, which will fix this bug.

Last edited 3 years ago by vaurdan (previous) (diff)
Note: See TracTickets for help on using tickets.