WPcom specific rewrite rules logic should be removed

[WPprodigy]

1) Flushing rewrite rules from wp-admin forces them to essentially flush twice.

This bit of code in rri_wpcom_flush_rules () should no longer be necessary and should be removed: https://github.com/Automattic/vip-go-mu-plugins/blob/master/rewrite-rules-inspector.php#L57-L69

The call to flush_rewrite_rules( false ); that the plugin makes before this hook is called is sufficient on VIP Go: https://github.com/Automattic/Rewrite-Rules-Inspector/blob/master/rewrite-rules-inspector.php#L282-L283

flush_rewrite_rules( false ); boils down to this (which you'll notice is essentially the same thing we duplicate in our addon plugin):

// In wp_rewrite->flush_rules()
update_option( 'rewrite_rules', '' );
$this->wp_rewrite_rules();

// In wp_rewrite->wp_rewrite_rules()
$this->rules = get_option('rewrite_rules');
if ( empty($this->rules) ) {
  $this->matches = 'matches';
  $this->rewrite_rules(); // Reconstructs rules from scratch. Remember this is what the plugin does to find missing rules.
  update_option('rewrite_rules', $this->rules);
}

2) Flushing rewrites can behave differently when a theme is using the older WPcom ways of declaring permalink structures.

If a site is using wpcom_vip_load_permastruct(), wpcom_vip_load_tag_base(), etc, then you'll see in the same function as above (rri_wpcom_flush_rules()) that it calls wpcom_vip_refresh_wp_rewrite().

It's in this function that I believe we have some error-some logic that might be causing some hard to catch bugs. This portion in particular feels like it should not be needed:

vip-go-mu-plugins/vip-helpers/vip-permastructs.php

Lines 145 to 158 in da7fcd2

	// Reconstruct WP_Rewrite and make sure we persist any custom endpoints, etc.
	$old_values = array();
	$custom_rules = array(
	'extra_rules',
	'non_wp_rules',
	'endpoints',
	);
	foreach( $custom_rules as $key ) {
	$old_values[$key] = $wp_rewrite->$key;
	}
	$wp_rewrite->init();
	foreach( $custom_rules as $key ) {
	$wp_rewrite->$key = array_merge( $old_values[$key], $wp_rewrite->$key );
	}

I believe the $wp_rewrite->init(); might be interferring with things. Though I'm not 100% sure how this all works so a second opinion here would be very much appreciated.

[sboisvert]

I'm pretty sure you're right about point 1. Point 2 I'm less sure, but one thing that came to mind is, why are we still supporting this? Why do we want to have this dark magic that we copy pasted from wpcom? Does it help our clients?
I can see one argument for it makes sites easier to cut and paste to Go. But I think long term this is just more technical debt and we should instead remove all this special handling and have it be just like core does.

I think this warrants a bigger discussion with the platform team about how they view this. ( I think the same conversation could be had about the special way we handle roles on wpcom and how we've cut and pasted that here)

[mjangda]

But I think long term this is just more technical debt and we should instead remove all this special handling and have it be just like core does.

This. Any special considerations that need to be made for WP.com migrations should happen via the compat plugin. mu-plugins should remain as close to core as possible.

[WPprodigy]

Also noting here that I've seen a few tickets where missing rewrite rules would not be added until I ran wp cache flush.

[WPprodigy]

Ah, think I figured out some of what goes wrong here. The request to flush_rewrite_rules() is being interrupted by a second request (any frontend page view). Example:

1. Admin Request One: Empties out the rewrite_rules option here.
2. Secondary Visitor Request: WP runs parse_request very early and fetches rewrite_rules here. This then has to rebuild them since they are empty from step 1 above. But notably, the rri_flush_rules action is not triggered and it's possible this request isn't yet recognizing new rules from step 1.
3. Admin Request Two: Back in our first admin request, we hit this line but step 2 already re-populated the option so it doesn't bother re-building.

Ideally, core could have a "force flush" type option to prevent this type of issue I'm thinking.

[WPprodigy]

Found a bug with the wpcom_vip_refresh_wp_rewrite() logic.

If a site uses only wpcom_vip_load_category_base() for example, every flush via the plugin will result in permalink_base being reset to this:

vip-go-mu-plugins/vip-helpers/vip-permastructs.php

Line 121 in 7bf6b09

'permalink_structure' => '/%year%/%monthnum%/%day%/%postname%/',

It seems to require an all-or-nothing approach to using those functions.

[WPprodigy]

Still in favor of deprecating/removing this extra bit of wpcom functionality. But we'll need an ideal system to switch to.

This seems fine:

add_action( 'init', function() {
	global $wp_rewrite;
	$wp_rewrite->set_category_base( 'category' );
	$wp_rewrite->set_tag_base( 'terms' );
} );

But is susceptible to the same problem we see with user roles - it's possible it triggers a stampede of FE writes should something go wrong/change. Could suggest this:

add_filter( 'pre_option_tag_base', function() {
	return 'terms';
}, 99 );

But it's more-or-less exactly what we're already doing with the custom wpcom functionality (and has possible issues with how early/late it's registered).

[github-actions]

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.

[GaryJones]

@WPprodigy Still value in pursuing this one?

[WPprodigy]

I'd like to, yes :). One day...

[github-actions]

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.

[github-actions]

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.

[github-actions]

This issue has been marked stale because it has been open for 60 days with no activity. If there is no activity within 7 days, it will be closed.

This is an automation to keep issues manageable and actionable and is not a comment on the quality of this issue nor on the work done so far. Closed issues are still valuable to the project and are available to be searched.

[WPprodigy]

https://core.trac.wordpress.org/ticket/58998 helps out a bit here, notably with the inconsistent flushes.

Long-term, still planning to deprecate/replace this "API".