Inserting rewrite rules using the filter 'option_rewrite_rules' breaks 'WP_Rewrite::flush_rules()', causing 404 errors

[maratbn]

To reproduce:

1. Enable permalinks.
2. Subscribe to the filter option_rewrite_rules
3. Insert an additional rule inside your callback function for this filter.
4. Call flush_rewrite_rules(...) or WP_Rewrite::flush_rules(...)
5. Go to any page of your site that's not the front / home page via its permalink, and you'll get a page not found 404 error. (Because the rewrite rules will not be properly regenerated).

The rewrite rules will not get regenerated because the method WP_Rewrite::wp_rewrite_rules(), which is called internally via WP_Rewrite::flush_rules(...), is unable to do so when its current list of rewrite rules is not completely empty. You can see this on line 1475 of the current revision wp-includes/class-wp-rewrite.php ​https://github.com/WordPress/WordPress/blob/ada44f2e13cde6a2134bb9a4df628068f7c82dcb/wp-includes/class-wp-rewrite.php#L1475

There is an if-conditional there that prevents the rewrite rules from getting regenerated when the rewrite rules list is not empty. And the rewrite rules list is prevented from getting empty by that option_rewrite_rules filter callback.

This means that calling WP_Rewrite::flush_rules(...) will delete all the rewrite rules except those inserted by the option_rewrite_rules filter, which will result in 404s for most of the permalinks.

This problem is currently reproducible if running combinations of some popular plugins / themes, such as the Gravity Forms plugin with the Web API enabled together with the Jupiter Theme.

The Gravity Forms plugin is inserting its rewrite rules using this problematic filter here ​https://github.com/wp-premium/gravityforms/blob/1b22ba00f53542f94d65d285e84a91d790f5b72c/includes/webapi/webapi.php#L365 and the Jupiter Theme is calling WP_Rewrite::flush_rules(...) here ​https://gist.github.com/maratbn/af888ceaf42a1d797d0be962ec1c4c25#file-general-functions-php-L91

But the underlying crux of this problem is that using a filter in an apparently legal way is breaking the flush-rewrite-rules core functionality, which should not be happening, and what this ticket is about.

Perhaps the method WP_Rewrite::wp_rewrite_rules() could use a new optional parameter that will force it to call the method WP_Rewrite::rewrite_rules() regardless of whether get_option('rewrite_rules') returns an empty list or not, and that optional parameter should be passed-in from WP_Rewrite::flush_rules(...).

Otherwise there needs to be some advisory to not use the filter option_rewrite_rules, and perhaps use rewrite_rules_array instead.

[dd32]

The option_rewrite_rules filter really isn't the right place to add rewrite rules, and IMHO makes this an invalid approach that makes this a Gravity Forms bug more so than a core issue.

The proper way would be to filter on rewrite_rules_array or use add_rewrite_rule() (probably with the 3rd arg set totop).

[maratbn]

Replying to dd32:

The option_rewrite_rules filter really isn't the right place to add rewrite rules, and IMHO makes this an invalid approach that makes this a Gravity Forms bug more so than a core issue.

The proper way would be to filter on rewrite_rules_array or use add_rewrite_rule() (probably with the 3rd arg set totop).

Well, I absolutely agree that it isn't the right place. Now, in my communications with the Gravity Forms people I told them that, and they replied that they tried using some other filters, but that caused them some other issues and conflicts, so they switched to this one several years ago.

In any case, this bug is effecting not the Gravity Forms itself, but the WordPress users that use it in combination with some other themes / plugins, such as the Jupiter Theme. Regardless of whose fault it is, the innocent users should not have to bear the burden, and WordPress should be resilient enough to still work properly even if some theme or plugin is not coded in the most optimal way.

At the present time, the Jupiter theme people are saying that this is a Gravity Forms bug, the Gravity Forms people are saying that this is a WordPress / Jupiter theme bug (b/c the Jupiter theme is flushing rewrite rules inside page requests, likely b/c they also had some issues and conflicts doing it otherwise), and you guys are saying that this is a Gravity Forms bug. Excuses excuses. :(

[SergeyBiryukov]

Replying to maratbn:

b/c the Jupiter theme is flushing rewrite rules inside page requests

That sounds like a bad practice the ​Codex specifically warns against.

[maratbn]

Replying to SergeyBiryukov:

Replying to maratbn:

b/c the Jupiter theme is flushing rewrite rules inside page requests

That sounds like a bad practice the ​Codex specifically warns against.

Well yeah. Since they're doing it after their admin settings are updated, my guess is that doing it directly from an admin dashboard page request caused them some other issues and conflicts, so they switched to wp_loaded inside regular page requests, and to their credit they have special logic to not do it on subsequent page requests.

Perhaps WordPress could use a kind of a non-optimal code warning feature, to detect if there is a call to flush_rewrite_rules() inside an incorrect action, or if some other core operation is being performed inside an incorrect callback, and to append a warning to a log that could be displayed in the admin dashboard to warn admins that they're running themes / plugins with non-optimal logic. This way these problems would become apparent sooner, and the developers would also try harder to use the appropriate actions/filters, and to file bug reports when they are having unusual issues and conflicts using recommended callbacks.

As far as my earlier idea regarding adding a new optional parameter to wp_rewrite_rules() that will force it to call the method WP_Rewrite::rewrite_rules() regardless of whether get_option('rewrite_rules') returns an empty list or not, and that optional parameter should be passed-in from WP_Rewrite::flush_rules(...), what do you guys think about that? Here's what the patch for that looks like:

--- wp-includes/class-wp-rewrite.php--BACKUP--2016-08-02--01	2016-08-02 14:57:51.615661000 -0700
+++ wp-includes/class-wp-rewrite.php	2016-08-10 15:47:04.412482000 -0700
@@ -1468,11 +1468,23 @@
 	 * @since 1.5.0
 	 * @access public
 	 *
+	 * @param bool $flag_dont_check_cache Optional Whether to always regenerate the rewrite rules even if they already appear to be present due to phantom rules being inserted into the cache via the filter 'option_rewrite_rules'.
+	 *                                             Default false.
+	 *
 	 * @return array Rewrite rules.
 	 */
-	public function wp_rewrite_rules() {
-		$this->rules = get_option('rewrite_rules');
-		if ( empty($this->rules) ) {
+	public function wp_rewrite_rules($flag_dont_check_cache = false) {
+
+		$flag_actually_rewrite = $flag_dont_check_cache;
+
+		if (!$flag_actually_rewrite) {
+			$this->rules = get_option('rewrite_rules');
+			if ( empty($this->rules) ) {
+				$flag_actually_rewrite = true;
+			}
+		}
+
+		if ($flag_actually_rewrite) {
 			$this->matches = 'matches';
 			$this->rewrite_rules();
 			update_option('rewrite_rules', $this->rules);
@@ -1810,8 +1822,7 @@
 			unset( $do_hard_later );
 		}
 
-		update_option( 'rewrite_rules', '' );
-		$this->wp_rewrite_rules();
+		$this->wp_rewrite_rules(true);
 
 		/**
 		 * Filter whether a "hard" rewrite rule flush should be performed when requested.

Alternatively, what about not applying option_rewrite_rules when retrieving the cached rewrite rules? The patch for that looks like this:

--- wp-includes/option.php--BACKUP--2016-08-10--01	2016-08-10 15:53:47.104482000 -0700
+++ wp-includes/option.php	2016-08-10 16:11:32.744482000 -0700
@@ -25,9 +25,12 @@
  *
  * @param string $option  Name of option to retrieve. Expected to not be SQL-escaped.
  * @param mixed  $default Optional. Default value to return if the option does not exist.
+ * @param bool   $flag_dont_filter
+ *                        Optional.  Don't apply filter 'option_[option name]'.
+ *                        Default false.
  * @return mixed Value set for the option.
  */
-function get_option( $option, $default = false ) {
+function get_option( $option, $default = false, $flag_dont_filter = false ) {
 	global $wpdb;
 
 	$option = trim( $option );
@@ -120,6 +123,10 @@
 	if ( in_array( $option, array('siteurl', 'home', 'category_base', 'tag_base') ) )
 		$value = untrailingslashit( $value );
 
+	if ($flag_dont_filter) {
+		return maybe_unserialize( $value );
+	}
+
 	/**
 	 * Filter the value of an existing option.
 	 *
--- wp-includes/class-wp-rewrite.php--BACKUP--2016-08-02--01	2016-08-02 14:57:51.615661000 -0700
+++ wp-includes/class-wp-rewrite.php	2016-08-10 16:10:38.800482000 -0700
@@ -1471,7 +1471,7 @@
 	 * @return array Rewrite rules.
 	 */
 	public function wp_rewrite_rules() {
-		$this->rules = get_option('rewrite_rules');
+		$this->rules = get_option('rewrite_rules', null, true);
 		if ( empty($this->rules) ) {
 			$this->matches = 'matches';
 			$this->rewrite_rules();

[hellofromTonya]

Hello @maratbn,

Thank you for opening this ticket!

As explained:

The option_rewrite_rules filter really isn't the right place to add rewrite rules, and IMHO makes this an invalid approach that makes this a Gravity Forms bug more so than a core issue.

In review of past conversations, it seems the issue is more in how a plugin or theme is adding or changing rewrite rules, rather than following the tools available.

The proper way would be to filter on rewrite_rules_array or use add_rewrite_rule() (probably with the 3rd arg set totop).

​https://developer.wordpress.org/reference/functions/add_rewrite_rule/

​https://developer.wordpress.org/reference/hooks/rewrite_rules_array/

I'm closing this ticket as it does not seem to be a Core issue to resolve.