[wp-trac] [WordPress Trac] #30875: address non-integer add_filter priority

[WordPress Trac]

#30875: address non-integer add_filter priority
-----------------------------+-------------------------
 Reporter:  drrobotnik       |       Owner:  (none)
     Type:  defect (bug)     |      Status:  closed
 Priority:  normal           |   Milestone:
Component:  Plugins          |     Version:  4.1
 Severity:  normal           |  Resolution:  maybelater
 Keywords:  has-patch close  |     Focuses:
-----------------------------+-------------------------

Comment (by kkmuffme):

 @hellofromTonya I'd like to reopen this, since this can have unexpected
 security consequences.

 - by default PHP will internally cast int-like strings to int when used as
 array key https://3v4l.org/gsojJ so there is some type-casting happening
 already anyway

 - given that the docs state the priority must be int, I see various
 (mostly security) plugins use code like:

 {{{#!php
 add_filter( 'some_hook', 'must_run_last', PHP_INT_MAX );


 or the paranoid ones:
 function one_before_last() {
     add_filter( 'some_hook', 'must_run_last', PHP_INT_MAX );
 }
 add_filter( 'some_hook', 'one_before_last', PHP_INT_MAX - 1, 0 );
 }}}

 But since WP core accepts any type for the priority - unlike documented -
 you can just do this:

 {{{#!php
 add_filter( 'some_hook', 'actually_i_will_run_last', (string) PHP_INT_MAX
 . 0, 0 );
 }}}

 (example see: https://3v4l.org/7SBeg)

 and circumvent this (naturally also works in reverse with "run first")

 Then additionally, with PHP<8 (since those issues are fixed in 8.0.0) it
 can lead to really inconsistent results where the order the callbacks are
 executed is different depending on the PHP version (this issue also partly
 exists with "int" in PHP<8 but it's much more consistent). e.g.

 {{{#!php
 $array = array( PHP_INT_MAX => 'MAX', PHP_INT_MIN => 'MIN',
 "-9223372036854775809" => "SMALLER THAN MIN" );

 ksort( $array, SORT_NUMERIC );
 var_dump( $array );
 }}}

 Results in different output in PHP 7.4 and PHP 8+

 - another issue is when you want to conditionally run stuff after an
 existing filter.

 {{{#!php
 add_filter( 'foo', 'bar', '1O' );

 $priority = has_filter( 'foo', 'bar' );
 add_filter( 'foo', 'run_after_bar', $priority + 1 );
 }}}

 In PHP7.1+ it will result in "A non well formed numeric value encountered"
 and from 8 onwards in "Warning: A non-numeric value encountered"

 Now if you try the same but with:

 {{{#!php
 add_filter( 'foo', 'bar', 'SOME TEXT' );
 }}}

 It will result in a fatal error from PHP 8+
 Fatal error: Uncaught TypeError: Unsupported operand types: string + int


 - PHP - expect the unexpected :-) Would you expect 3 do be negative?

 {{{#!php
 $priority_1 = '9223372036854775806';
 $priority_2 = $priority_1 + 1;
 $priority_3 = $priority_2 + 1;

 var_dump(
         array(
                 $priority_1 => 'first',
                 $priority_2 => 'second',
                 $priority_3 => 'third',
         )
 );
 }}}




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

 This has long happened. PHP < 8.1 does not even receive security patches
 anymore, and WP still has "compatibility with exceptions" support for it.
 In php-src there has long been a push for stricter types and static
 analysis, but WP somehow slept on that. It's been 4 years since PHP 8
 release.

 Adding a "doing_it_wrong" for cases where !is_int( $priority ) is simple
 enough and not breaking and should have shipped ages ago.

 If you merge it, I will create a PR so it can be shipped with WP 6.9 at
 least.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/30875#comment:20>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform