Logic error in WP_Rewrite flush_rules

[numis]

The current logic in flush_rules of WP_Rewrite is flawed:

if ( ! $hard || ! apply_filters( 'flush_rewrite_rules_hard', true ) ) {
    return;
}

Given the four possible scenarios:

$hard has two unique values:

• true (by default)
• false

Casting apply_filters( 'flush_rewrite_rules_hard', true ) to a boolean also has two unique values:

• true (by default)
• true (a filter returns a value that evaluates to true)
• false (a filter returns a value that evaluates to false)

=============================================

If $hard is true and either no filters are added, or a filter is added that returns true:

! true  || ! true = false || false = false

UNINTENTIONAL FAILURE

=============================================

If $hard is true and a filter is added that returns false:

! true || ! false = false || true = true

UNINTENTIONAL SUCCESS

=============================================

If $hard is false and either no filters are added, or a filter is added that returns true:

! false || ! true = true || false = true

UNINTENTIONAL SUCCESS

=============================================

If $hard is false and a filter is added that returns false:

! false || ! false = true || true = true

UNINTENTIONAL FAILURE

=============================================

As seen above, 50% of the unique scenarios give an unexpected response. While the other 50% of the scenarios give the expected response, but for the wrong reason.

[numis]

Patch with tests

[numis]

Patch without tests

[numis]

Attached are two patches:

28517-with-tests.patch:

• Corrects the logic error observed above.
• Brings surrounding code up to the coding standards set forth in the PHP Coding Standards section of the WordPress Core Handbook.
• Includes modifications necessary for the tests to assert against a return value, based on the type of flush that was attempted.
• Includes 6 new tests in tests/rewrite.php:
  • test_flush_rules_hard_true_no_filters()
  • test_flush_rules_hard_false_no_filters()
  • test_flush_rules_hard_true_with_filters_true()
  • test_flush_rules_hard_true_with_filters_false()
  • test_flush_rules_hard_false_with_filters_true()
  • test_flush_rules_hard_false_with_filters_false()

28517-without-tests.patch:

• Corrects the logic error observed above.
• Brings surrounding code up to the coding standards set forth in the PHP Coding Standards section of the WordPress Core Handbook.

[numis]

The patches still stand, but the previous assessment of the flawed code was incorrect. The outcome is actually worse than previously described.

=============================================

If $hard is true and either no filters are added, or a filter is added that returns true:

! true  || ! true = false || false = false

INTENTIONAL SUCCESS - HARD FLUSH PERFORMED

=============================================

If $hard is true and a filter is added that returns false:

! true || ! false = false || true = true

UNINTENTIONAL FAILURE - SOFT FLUSH PERFORMED

=============================================

If $hard is false and either no filters are added, or a filter is added that returns true:

! false || ! true = true || false = true

UNINTENTIONAL FAILURE - SOFT FLUSH PERFORMED

=============================================

If $hard is false and a filter is added that returns false:

! false || ! false = true || true = true

UNINTENTIONAL FAILURE - SOFT FLUSH PERFORMED

=============================================

[Denis-de-Bernardy]

Besides the fact that some are actually using this filter with the current defaults and all, I see two problems with the current patch.

Firstly I disagree with this here:

If $hard is true and a filter is added that returns false:
UNINTENTIONAL FAILURE - SOFT FLUSH PERFORMED

I view this one as intentional… If I filter the value and explicitly return false, I do not care if WP would have rather have it hard flushed. I meant it. (And for good reasons, since WP fails to edit the correct htaccess file in some edge cases.)

If $hard is false and either no filters are added, or a filter is added that returns true:
UNINTENTIONAL FAILURE - SOFT FLUSH PERFORMED

This seems like a more valid bug.

Put together, methinks the correct patch would change the logic so it simply filters $hard instead of true.

Secondly, the new return value should be documented and, ideally, should be a boolean (e.g. true if a hard flush was done) rather than arbitrary strings.

[numis]

Second version of patch; includes tests and documentation modifications

[numis]

Denis, thank you for providing the feedback. I agree with both points that you made and have uploaded a second version of the patch to reflect each.

28517.2.with-tests.patch

The following scenarios would now be:

$hard	apply_filters returns	operation attempted
true (default)	true (default)	HARD
true (default)	false	SOFT
false	true	HARD
false	false (default)	SOFT

For clarity, the existing scenarios are:

$hard	apply_filters returns	operation attempted
true (default)	true (default)	HARD
true (default)	false	SOFT
false	true (default)	SOFT
false	false	SOFT

So under the default scenario for both the existing codebase and the patch, the same operation is attempted (a HARD flush). However, with the proposed patch, in the event that someone uses flush_rewrite_rules_hard to return true, a HARD flush will actually be attempted instead of the SOFT flush.

Thank you again Denis for taking the time to provide some constructive feedback.

[aaroncampbell]

I agree with the general fix here (passing $hard to the filter so it can override what's passed in), but I'm not sure about returning true and false. True and false for a function like this should denote success and failure, but that's not the case here. The patch is returning false to say "we did a soft flush, not a hard one" and true to say "we attempted a hard flush". I see that you're using the return for unit tests, but I think we need to find a better way to handle those tests.

[markjaquith]

Replying to aaroncampbell:

I agree with the general fix here (passing $hard to the filter so it can override what's passed in), but I'm not sure about returning true and false. True and false for a function like this should denote success and failure, but that's not the case here. The patch is returning false to say "we did a soft flush, not a hard one" and true to say "we attempted a hard flush". I see that you're using the return for unit tests, but I think we need to find a better way to handle those tests.

Eh. True/false doesn't necessarily need to indicate success and failure.

[aaroncampbell]

Replying to markjaquith:

Eh. True/false doesn't necessarily need to indicate success and failure.

I talked to Mark about this. You're passing in a boolean, and basically returning that boolean after it's been filtered, which actually does makes sense. Also, the tests would be too heavy/complex if we didn't have a return to look at. As long as we document the return well I'm good with it.