Make sure that the rewrite rules don't get flushed on every option update

[Djennez]

Context

• We used an unsafe method to delete all the rewrite rules on every wpseo<option_group> update. This PR aims to only flush the rewrite rules when the stripcategorybase option gets updated and only via the WP core flush_rewrites function (which has its own safety in place).
• ⚠️ - Deprecation warning Please adjust the version numbers of the deprecation code according to the milestone before merging.
• Originally introduced in Flushes the rewrite rules after option update #9203
• Essentially fixes 2 bugs;
  • flushing the rewrite rules on every option update (which happens pretty often, like when doing the workouts))
  • flushing the rewrite rules in the wrong way (which may be a cause of race conditions that cause 404s)

Summary

This PR can be summarized in the following changelog entry:

• Fixes a bug where updating any Yoast options would also flush the rewrite rules.
• Fixes a rare bug where option updates may cause a race condition that would cause seemingly random 404s.

Relevant technical choices:

• This PR potentially fixes race conditions where the rewrite rules of a site would be permanently corrupted until a new rewrite_flush was executed:
  • https://yoast.atlassian.net/browse/IM-475
  • https://yoast.atlassian.net/browse/IM-1018
  • https://github.com/Yoast/wordpress-seo/issues / 13383
• Because the rewrite rules were always cleared on an options change, we may have inadvertently hidden any bugs that would occur when a Yoast option gets changed that needs a flush. So we need to have a hard think about which options would need a rewrite flush, besides the stripcategorybase option.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

• Without this PR, when you would save a Yoast option, a DELETE FROM 'wp_options' WHERE 'option_name' = 'rewrite_rules' would be performed.
  • This can only be observed with Query Monitor if persistent logging is enabled, as the page is immediately reloaded after save.
• With this PR, this should not happen anymore.
• Instead, when the "Remove the categories prefix?" option (SEO->Search Appearance->Taxonomies) is toggled, a flush_rewrite_rules should be performed on shutdown.

Test instructions for QA when the code is in the RC

• QA should use the same steps as above.

QA can test this PR by following these steps:

• Follow the steps above as well as:
  • Make sure the Strip category prefix option works as intended.
  • Think of, and test, any other Yoast options that would influence the permalinks.

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

• Permalinks and options saving

UI changes

• This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

• I have written documentation for this change.

Quality assurance

• I have tested this code to the best of my abilities
• I have added unittests to verify the code works as intended

Fixes #

[johannadevos]

CR: 🚧

We already have a dedicated watcher for the stripcategorybase option: https://github.com/Yoast/wordpress-seo/blob/trunk/src/integrations/watchers/indexable-category-permalink-watcher.php.

This watcher is doing very similar checks as the ones that you added in the option-wpseo-watcher.php. So your code should be moved here.

Regarding the changelog entry: why is it non-user-facing when it fixes a bug?

In any case, adding the label to the PR is enough, and we should not also add it between brackets ([non-user-facing]). This will mess with the RC automatization script, which will interpret [non-user-facing] as a package name.

[curtisbelt]

This PR potentially fixes race conditions where the rewrite rules of a site would be permanently corrupted until a new rewrite_flush was executed

I just want to report that I've seen this race condition in action. To track it down, I set up a mu-plugin to log PHP backtraces whenever rewrite rules were deleted or updated. Every 1-2 months on a particular customer account, their site 404s everywhere due to rewrite rules being permanently corrupted until a new rewrite_flush was executed.

Every time this has occurred, I could see in the backtrace that WPSEO_Utils::clear_rewrites() deleted the rewrite rules right before it happened. For some reason, I could not capture a backtrace for the event which wrote the incorrect rules -- but I had to move on from that once I came up with the solution below.

Until this PR can be merged, I've developed a solution using a small mu-plugin:

<?php

/*
For each of its internal options, Yoast hooks into the "update_option_{$option_name}"
action to clear the rewrite rules when their options are updated. The problem is that
their WPSEO_Utils::clear_rewrites() logic is using "delete_option('rewrite_rules')"
which can cause race conditions to occur.

Yoast has a PR to address this bug, but as of 2020-11-04 the PR is not yet merged:
https://github.com/Yoast/wordpress-seo/pull/17363

In the meantime, this plugin will surgically replace Yoast's hooks with new ones
that use the safer "flush_rewrite_rules".
*/

add_action(
  'wpseo_loaded',
  function() {
    if ( ! property_exists( 'WPSEO_Options', 'options' ) ) {
      return;
    }
    foreach( array_filter( array_keys( (array) WPSEO_Options::$options ) ) as $option_name ) {
      add_action(
        'update_option_' . $option_name,
        function() {
          remove_filter( current_filter(), [ 'WPSEO_Utils', 'clear_rewrites' ], 10 );
          add_filter( current_filter(), 'flush_rewrite_rules', 10 );
        },
        0,
        0
      );
    }
  }
);

[IreneStr]

@Djennez Is this still relevant or can it be closed? :)

[Djennez]

@IreneStr it is still relevant as far as I'm aware (I still see the support requests regarding 404s when our plugin updates). However, I don't recall it being fully finished. And when I was working on this, there were talks of reworking the options framework which would likely make these changes obsolete.

If that rework is not planned in the foreseeable future, I think it will be a good move to continue this PRs integration to see if it has the expected outcome. If it does, this could potentially fix a very big annoyance for sites that run into 404s on every one of our plugin updates.