Make WordPress Core

Opened 10 years ago

Closed 9 months ago

Last modified 8 months ago

#30875 closed defect (bug) (maybelater)

address non-integer add_filter priority

Reported by: drrobotnik's profile drrobotnik Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: Plugins Keywords: has-patch close
Focuses: Cc:

Description

The docs say that add_filter should use int values, but there is nothing enforcing this. Currently the following is perfectly valid and works as you'd expect an array to sort keys.

add_action('wp_head', 'wrong1', '1');
add_action('wp_head', 'wrong2', '1.7');
add_action('wp_head', 'wrong3', '1.5');
add_action('wp_head', 'wrong4', 'mickeymouse');
add_action('wp_head', 'wrong5', 'donaldduck');

http://cloudup.com/cfqa31pltEE

I've attached a patch that simply returns false, because if you simply typecast to int, the function who is doing it wrong, will get priority.

Props jorbin for clearly understanding integers.

Attachments (7)

plugin.patch (595 bytes) - added by drrobotnik 10 years ago.
return false if priority provided is not an integer.
plugin.2.patch (606 bytes) - added by drrobotnik 10 years ago.
fixed patch
30875.diff (2.4 KB) - added by valendesigns 10 years ago.
30875.2.diff (2.4 KB) - added by valendesigns 10 years ago.
I removed an unnecessary check for is_empty that I was using to test with when the function returned false. It doesn't do that anymore so I removed the extra code.
30875.3.diff (3.2 KB) - added by valendesigns 10 years ago.
Added additional tests for bool, array, and object.
30875.4.diff (3.2 KB) - added by valendesigns 10 years ago.
Oops, last one. It's late. I had a typo.
30875.5.diff (3.4 KB) - added by valendesigns 10 years ago.

Download all attachments as: .zip

Change History (26)

@drrobotnik
10 years ago

return false if priority provided is not an integer.

#1 follow-up: @GaryJ
10 years ago

  • Keywords needs-refresh added

The patch should follow code-standards:

if ( ! is_int( $priority ) ) { 
	return false;
}

@drrobotnik
10 years ago

fixed patch

#2 in reply to: ↑ 1 @drrobotnik
10 years ago

Replying to GaryJ:

The patch should follow code-standards:

if ( ! is_int( $priority ) ) { 
	return false;
}

Fixed. I don't know how to delete the original attachment, so it's the newest one.

#3 @dd32
10 years ago

  • Component changed from General to Plugins
  • Keywords close added; needs-refresh removed

The problem with this, in a similar manner to #30862 is that changing this potentially breaks existing implementations.

For example, with this code, all the actions will be called (in a predictable order, although not exactly the order one would expect), but if your patch is applied only the int case will be actually added.

function _test_int() { echo __METHOD__ . "\n"; }
function _test_int_as_string() { echo __METHOD__ . "\n"; }
function _test_float() { echo __METHOD__ . "\n"; }
function _test_float_as_string() { echo __METHOD__ . "\n"; }
function _test_string() { echo __METHOD__ . "\n"; }

add_action( 'test_action', '_test_int', 10 );
add_action( 'test_action', '_test_int_as_string', '9' );
add_action( 'test_action', '_test_float', 8.1 );
add_action( 'test_action', '_test_float_as_string', '7.2' );
add_action( 'test_action', '_test_string', 'micky' );

echo '<pre>';
do_action( 'test_action' );

@valendesigns
10 years ago

#4 @valendesigns
10 years ago

  • Keywords has-patch dev-feedback added; close removed

The 30875.diff adds unit tests and solves the issue in a backwards compatible way. We can't return false since actions are expected to run no mater what you enter. However, we can normalize the priority a bit.

@valendesigns
10 years ago

I removed an unnecessary check for is_empty that I was using to test with when the function returned false. It doesn't do that anymore so I removed the extra code.

@valendesigns
10 years ago

Added additional tests for bool, array, and object.

@valendesigns
10 years ago

Oops, last one. It's late. I had a typo.

#5 follow-up: @boonebgorges
10 years ago

  • Keywords close added

30875.4.diff is not backward compatible. By rounding floats and by turning strings into 10, it creates a race condition where there was previously a determinate order (as dd32 put it, predictable though perhaps odd).

Like #30862, this is a case where we're trying to solve a problem that does not exist. The ability to use non-integer values here is perhaps not an intended feature, and perhaps it should remain undocumented, but the fact that it exists is not a bug.

#6 @valendesigns
10 years ago

Replying to boonebgorges:

30875.4.diff is not backward compatible. By rounding floats and by turning strings into 10, it creates a race condition where there was previously a determinate order (as dd32 put it, predictable though perhaps odd).

Like #30862, this is a case where we're trying to solve a problem that does not exist. The ability to use non-integer values here is perhaps not an intended feature, and perhaps it should remain undocumented, but the fact that it exists is not a bug.

Well, at the very least I've created some tests. I've uploaded a new patch that doesn't change the expected or perceived behavior in any way. it does however, return false when $priority is set to an array or object. I added that because I was getting an 'Illegal offset type' thrown in the console when I tried to assert they were false.

#7 in reply to: ↑ 5 @drrobotnik
10 years ago

Replying to boonebgorges:

30875.4.diff is not backward compatible. By rounding floats and by turning strings into 10, it creates a race condition where there was previously a determinate order (as dd32 put it, predictable though perhaps odd).

Like #30862, this is a case where we're trying to solve a problem that does not exist. The ability to use non-integer values here is perhaps not an intended feature, and perhaps it should remain undocumented, but the fact that it exists is not a bug.

@valendesigns, thanks for updating the patch.

I'm having trouble accepting the backwards compatibility argument on this issue. At a certain point you're choosing backwards compatibility over the documentation and intended usage.

Accept that anyone that's using this outside of an integer is doing it wrong purposefully. They'll need to take a moment to adjust it in for future releases or we automatically normalize their priority. The issue does exist and we're pre-empting it from becoming an issue by closing it here.

#8 @boonebgorges
10 years ago

Adherence to "intended usage" is not a sufficient reason to make breaking changes. We'll only make changes that break backward compatibility if the current usage patterns - even if those patterns are "unintended" - are causing practical problems. Aside from the mild confusion that might result from PHP's sorting algorithms, I can't think of any actual bugs that would come out of the current behavior. We should not make breaking changes for academic reasons.

If the problem is that the actual behavior doesn't match the docs, then we should update the docs.

#9 @valendesigns
10 years ago

And that's why my last patch is basically just unit tests. If it can't be changed then we need to at least document exactly what's meant to happen, even if there's no changes made to the filter. I just want to fix what we can and test for what we can't.

#10 follow-up: @dd32
10 years ago

it does however, return false when $priority is set to an array or object. I added that because I was getting an 'Illegal offset type' thrown in the console when I tried to assert they were false.

This is the type of error that we'd normally leave in place, as a development warning to the developer that they're passing something in that doesn't evaluate in a manner that works. That might sound counter intuitive given the concerns raised here, but as long as a warning isn't something that should be generated in the normal runtime and correct use of a function, we generally leave it there for the dev to pick up on.

Realistically I agree with adding a unit test that covers it, but I do question if unit testing the addition of filters is worthwhile, and rather that a test on the order of execution might be more useful.

#11 in reply to: ↑ 10 @valendesigns
10 years ago

Replying to dd32:

Realistically I agree with adding a unit test that covers it, but I do question if unit testing the addition of filters is worthwhile, and rather that a test on the order of execution might be more useful.

That sounds reasonable to me. I'll rewrite the tests to cover the order of execution and leave the filter untouched.

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

#12 @valendesigns
10 years ago

So I found some odd results that can cause string based priorities to be removed completely and never fire when using true as a priority.

Also, if you enter the priorities from above you get back array( '7.2', 'micky', 8, 9, 10 ), but if you add an integer below 7.2 like 1. You get back array( 'micky', 1, '7.2', 8, 9, 10 ).

I know you guys are against changing the filter, but it should still be sane. As it stands there doesn't appear to be a predictable pattern that can be tested for when you start throwing in wildcards.

#13 @dd32
10 years ago

In that case, I'd just leave non-numeric strings out of it entirely, but still don't believe it'd be something worth documenting or preventing.

The odd ordering you're coming up against is, I think, caused by PHP's typecasting, comparing 1 to '2' will typecast the numeric-string to an int, but comparing 7.2' to 'micky' won't, and it'll perform a string comparison, the order in which this happens, plus the type of sort PHP uses, would explain the ordering you're seeing.

#14 @wonderboymusic
9 years ago

  • Owner set to dd32
  • Status changed from new to assigned

@dd32 what do we want to do here - let it ride? close it out?

#15 follow-up: @dd32
9 years ago

  • Owner dd32 deleted

@dd32 what do we want to do here - let it ride? close it out?

We won't be enforcing integers, however as the comments suggest, adding a unit test showing and confirming order of execution makes sense to me.

#16 in reply to: ↑ 15 @DrewAPicture
9 years ago

Replying to dd32:

@dd32 what do we want to do here - let it ride? close it out?

We won't be enforcing integers, however as the comments suggest, adding a unit test showing and confirming order of execution makes sense to me.

I wonder if the tests on #17817 cover order of execution. I'd have to look.

#17 @hellofromTonya
9 months ago

  • Keywords dev-feedback removed
  • Resolution set to maybelater
  • Status changed from assigned to closed

Following up on older close candidate tickets.

The priority is clearly documented as requiring an integer data type. Passing it anything other than an int is doing it wrong. That said, requiring only an int could and likely will cause unexpected behaviors/results for users. There's an argument for back-compat concerns due to how essential (dare say mission critical) hooks are in WordPress.

I'm not convinced there's enough benefit to integer enforcement (input validation) to overcome the risks to users and extenders.

If data type strictness is truly needed for the priority (for example, to ensure its compatible with a future PHP version that will impact it), then an intentional slow campaign is warranted. For example, first inform through deprecation or notice with a long runway before raising the error level to enforce the integer data type and take action (such as bailing out or setting a default priority value).

What about tests that validate the current priority call order? There are limited tests now. More would be appreciated and helpful. But I think that can be handled in a separate ticket to avoid confusion of what actions were taken in this ticket.

Actions:

  • Opening a new ticket for more tests to be added for testing happy and unhappy paths for actions/filters priority call orders.
  • Closing this ticket as maybelater conditioned on the premise that later = if needed for compatibility with a future PHP version that has a severe enough impact on this area of the code.

#18 @hellofromTonya
9 months ago

#60193 is now open for expanding the priority call order tests.

#19 @drrobotnik
8 months ago

  • Summary changed from enforce integers in add_filter priority to address non-integer add_filter priority

When I originally opened this issue I was chaffing up against clever plugin/theme developers using negative numbers and strings to subvert the documented hook/filter priority. The solution seemed to be to do the thing I was chaffing against. That said, 9 years later I think a long term PHP deprecation and/or "you're doing it wrong" notice would be acceptable. @hellofromTonya, thanks for re-opening the conversation.

Note: See TracTickets for help on using tickets.