enforce integers in add_filter priority

[drrobotnik]

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.

[drrobotnik]

return false if priority provided is not an integer.

[GaryJ]

The patch should follow code-standards:

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

[drrobotnik]

fixed patch

[drrobotnik]

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.

[dd32]

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]

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]

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]

Added additional tests for bool, array, and object.

[valendesigns]

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

[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]

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.

[drrobotnik]

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.

[boonebgorges]

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.

[valendesigns]

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.

[dd32]

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.

[valendesigns]

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.

[valendesigns]

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.

[dd32]

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.

[wonderboymusic]

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

[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.

[DrewAPicture]

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.