WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#30875 assigned defect (bug)

enforce integers in add_filter priority

Reported by: drrobotnik Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.1
Component: Plugins Keywords: has-patch dev-feedback 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 3 years ago.
return false if priority provided is not an integer.
plugin.2.patch (606 bytes) - added by drrobotnik 3 years ago.
fixed patch
30875.diff (2.4 KB) - added by valendesigns 3 years ago.
30875.2.diff (2.4 KB) - added by valendesigns 3 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 3 years ago.
Added additional tests for bool, array, and object.
30875.4.diff (3.2 KB) - added by valendesigns 3 years ago.
Oops, last one. It's late. I had a typo.
30875.5.diff (3.4 KB) - added by valendesigns 3 years ago.

Download all attachments as: .zip

Change History (23)

@drrobotnik
3 years ago

return false if priority provided is not an integer.

#1 follow-up: @GaryJ
3 years ago

  • Keywords needs-refresh added

The patch should follow code-standards:

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

@drrobotnik
3 years ago

fixed patch

#2 in reply to: ↑ 1 @drrobotnik
3 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
3 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
3 years ago

#4 @valendesigns
3 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
3 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
3 years ago

Added additional tests for bool, array, and object.

@valendesigns
3 years ago

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

#5 follow-up: @boonebgorges
3 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.

@valendesigns
3 years ago

#6 @valendesigns
3 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
3 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
3 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
3 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
3 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
3 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 3 years ago by valendesigns (previous) (diff)

#12 @valendesigns
3 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
3 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
2 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
2 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
2 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.

Note: See TracTickets for help on using tickets.