WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 months ago

#29107 closed defect (bug) (fixed)

WP_Rewrite::flush_rules() should not delete 'rewrite_rules' option

Reported by: SergeyBiryukov Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.0.1
Component: Rewrite Rules Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

If permalinks are disabled, there's an extra query on each page load:

SELECT option_value FROM trunk_options WHERE option_name = 'rewrite_rules' LIMIT 1

It comes from WP::parse_request(), which calls WP_Rewrite::wp_rewrite_rules().

Normally, it should be covered by wp_load_alloptions(), but:

  1. The rewrite_rules option is not created on installation.
  2. If you enable permalinks and disable them again, WP_Rewrite::flush_rules() deletes the option.
  3. WP_Rewrite::wp_rewrite_rules() attempts to recreate it, but fails due to the check in update_option().

I think we should replace rewrite_rules option with an empty value instead of deleting it.

Introduced in [3373].

Attachments (4)

29107.patch (1.0 KB) - added by SergeyBiryukov 5 years ago.
29107-tests.diff (730 bytes) - added by jesin 5 years ago.
Tests if rewrite_rules is an array and is not empty
29107.diff (1.7 KB) - added by voldemortensen 5 years ago.
29107.2.diff (1.5 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (14)

#2 @nacin
5 years ago

Can we confirm (perhaps with a unit test) that an empty option gets updated to a populated option when a structure is set and rules are flushed?

#3 @SergeyBiryukov
5 years ago

  • Keywords needs-unit-tests added

@jesin
5 years ago

Tests if rewrite_rules is an array and is not empty

@voldemortensen
5 years ago

#4 @voldemortensen
5 years ago

  • Keywords needs-unit-tests removed

Refreshed patch and combined unit tests.

#5 @chriscct7
4 years ago

  • Keywords needs-refresh has-unit-tests added

@swissspidy
4 years ago

#6 @swissspidy
4 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Awaiting Review to 4.5

29107.2.diff is an updated patch that applies cleanly against trunk.

In what ways does this affect #11384?

#7 @swissspidy
4 years ago

  • Keywords commit added

#8 @swissspidy
4 years ago

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

In 36254:

Rewrite: Ensure WP_Rewrite::flush_rules() does not delete the 'rewrite_rules' option.

Instead, the option gets updated to an empty string.
Adds unit tests.

Props SergeyBiryukov, jesin, voldemortensen.
Fixes #29107.

#9 @swissspidy
4 years ago

#11384 was marked as a duplicate.

#10 @johnjamesjacoby
4 months ago

update_home_siteurl() and site-info.php both continue to call delete_option( 'rewrite_rules' ).

Is it possible to confirm whether those usages may cause this issue to reappear?

Also, delete_option( 'rewrite_rules' ) has been used for a very long time as the way for plugins to easily (and maybe poorly) queue up a rewrite rule flush on the subsequent page load. A quick search on GitHub shows over 47,000 usages across all repositories of people using it, mostly for this reason.

Is that enough in the wild for core to provide a helper function (like queue_rewrite_rule_updates() or something) that everyone can change to using instead of incorrectly deleting the option and causing this extra query to reappear?

Or... instead of caring if the option is empty vs. missing, is there a different approach core can take to tuck this concern under the rug, and internally add rewrite_rules to the notoptions array before it's queried independently, or something less sinister?

Last edited 4 months ago by johnjamesjacoby (previous) (diff)
Note: See TracTickets for help on using tickets.