Make WordPress Core

Opened 10 years ago

Last modified 7 years ago

#28517 new defect (bug)

Logic error in WP_Rewrite flush_rules

Reported by: numis's profile numis Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: Rewrite Rules Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

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.

Attachments (4)

28517-with-tests.patch (3.7 KB) - added by numis 10 years ago.
Patch with tests
28517-without-tests.patch (1.2 KB) - added by numis 10 years ago.
Patch without tests
28517.2.with-tests.patch (3.5 KB) - added by numis 10 years ago.
Second version of patch; includes tests and documentation modifications
28517.diff (3.5 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (17)

@numis
10 years ago

Patch with tests

@numis
10 years ago

Patch without tests

#1 @numis
10 years ago

  • Keywords has-patch added

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.

#2 @numis
10 years ago

The patches still stand, but the previous assessment of the flawed code was incorrect.

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

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
INTENTIONAL SUCCESS - SOFT FLUSH PERFORMED

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

Last edited 10 years ago by numis (previous) (diff)

#3 @numis
10 years ago

Last edited 10 years ago by numis (previous) (diff)

#4 @Denis-de-Bernardy
10 years ago

  • Keywords needs-patch added; has-patch removed

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
10 years ago

Second version of patch; includes tests and documentation modifications

#5 @numis
10 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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.

#6 @helen
10 years ago

  • Version changed from trunk to 3.7

#7 follow-up: @aaroncampbell
9 years ago

  • Keywords needs-unit-tests added

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.

#8 in reply to: ↑ 7 ; follow-up: @markjaquith
9 years ago

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.

#9 in reply to: ↑ 8 @aaroncampbell
9 years ago

  • Keywords needs-testing needs-unit-tests removed

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.

#10 @chriscct7
8 years ago

  • Keywords needs-patch added; has-patch removed

@swissspidy
7 years ago

#11 @swissspidy
7 years ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.8

This ticket was mentioned in Slack in #core by flixos90. View the logs.


7 years ago

#13 @flixos90
7 years ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.